4

I'm writing a user account system in PHP, with an emphasis on security, but I'm stuck on refactoring it into something cleaner and more usable.

The problem is trying to group the user account functionality together, but in separate classes. The way I'm doing it now is, there are a bunch of classes with public static methods that all take $username as the first parameter, and they use other static methods, passing the same $username as the first parameter as well. Obviously, OOP is a better way to go, especially since each method must strtolower the username in order to make DB queries, and must handle the case where the username provided doesn't exist at all.

The problem with putting everything in a "User" class is that it would be huge, and there would be a lot of completely unrelated code in the same file. For example, the Change Password code doesn't need to call methods related to validating the user's email or changing the user's (unrelated) settings.

I could have two classes: User and AuthenticatedUser, where AuthenticatedUser inherits User. User would implement all the functionality that is possible without the user being logged in, and AuthenticatedUser would be all the functionality that requires a login -- such as accessing the user's encrypted data. Maintenance code could use objects of User, and GUI code would get an AuthenticatedUser object after the user has logged in. But I don't want to cram all of the functionality into User.

Here's the list of some of operations that are related to a user account, just to show why they won't all fit in one class:

  • Login
  • Lockout user if >=X attempts in past Y minutes (includes methods for determining whether a user is locked out, adding a tick to the attempt count, etc).
  • Bypass the lockout with an email loop
  • Change password
  • Administrator change password (force)
  • Password reset (email loop) (incl. methods for initiating the reset, validating the email token, etc)
  • Set/get user data stored in plaintext
  • Set/get encrypted user data
  • Set/get account settings (password reset is allowed?, lock out the account on password failures?, is bypassing lockout with email loop allowed?, etc) Ideally these settings should be set/got near the code whose behavior depends on them.
  • Get the user's username in the proper case (as they specified it when creating account)
  • Email validation
  • A lot of functionality only used by specific code. E.g. get user "salt" used for key derivation.
  • A bunch more...

I was thinking I could do something like have a PasswordChanger class that inherits User, and implements the password changing functionality. So it would look like:

$proper = $user->getProperName();
...
$passChanger = $user->getPasswordChanger();
$result = $passChanger->changePassword($oldPass, $newPass);
...
$userLockout = $user->getUserLockout();
$result = $userLockout->isUserLockedOut();
...
$userEV = $user->getEmailValidation();
$result = $userEV->tryValidateEmail($token);
...

That's the best solution I've come up with so far. It lets me split up related functionality into it's own file, and saves from having to pass around the username. But it seems really weird -- I've never seen code like that before. It forces the superclass to know about all of its subclasses, which is bad design. Any ideas?

Edit: An alternative avoiding inheritance would be to have a PasswordChanger that has a a User. Like:

$passChanger = new PasswordChanger($user);
$passChanger->changePassword($old, $new); // uses $user->getUsername() 
...
$emailValidator = new EmailValdiator($user);
$emailValidator->tryValidate($token);

"PasswordChanger has a User" vs. "PasswordChanger is a User." The former actually makes sense, so I like this way a little better.

user1219582
  • 41
  • 2
  • Well thought out. But avoid using inheritance for the password changer. What does password changer exactly do and why create an own class for it? – busypeoples Feb 19 '12 at 21:01

3 Answers3

2

Well, you've made a good start and you're asking the right questions. There is no single answer though. Design is an art rather than a science. It does sound like you are trying to re-design rather than refactor. You might find it easier if you start to refactor your code with only a loose idea of where your design might end up (a really good resource for refactoring is this book by Michael Feathers. As you refactor more and more you should find that a design emerges from the darkness! And often that design is not what you thought you'd end up with when you started out.

Oh, and btw inheritance is one of the most over-used aspects of OO. If you are thinking of inheritance, stop and think again. If you still think you need inheritance, stop and think some more. If you still think you need inheritance, it's possible that you do...

liquorvicar
  • 6,081
  • 1
  • 16
  • 21
1

PasswordChanger and User have nothing in common so you should avoid to inherit them.

What I do in this case is something like:

class User {
   var $pswChanger;  //> -> Object of PasswordChanger

}

Basically put all the objects that you will need with User class within its attribute

You can of course access them with something like $this->pswChanger->method();

dynamic
  • 46,985
  • 55
  • 154
  • 231
1

You might try using a decorator pattern. You can extend the UserDecorator by adding a validateDecorator, a notifyDecorator etc.

class User {

    private $name;
    private $password;


    public function __construct($name, $pw) {
      $this->name = $name ;
      $this->password  = $pw;
    }

    public function getUsername() {
      return $this->name;
    }

    public function getPassword() {
      return $this->password;
    }

}

class UserDecorator {

    protected $user;
    protected $name;
    protected $password;

    public function __construct(User $user) {
      $this->user= $user;
      $this->setValues();
    }

    public function setValues() {
      $this->name = $this->user->getUsername();
      $this->password = $this->user->getPassword();
    }

    public function getUsername() {
      return $this->name;
    }

    public function getPassword() {
      return $this->password;
    }


}

class PasswordChangeDecorator extends UserDecorator {

    private $userdecorator;

    public function __construct(UserDecorator $user) {
      $this->userdecorator = $user;
    }

    public function changePassWord($newPw) {
      $this->userdecorator->password = $newPw;
    }
}

  $user = new User("some random user name", "oldpw");

  $userdecorator = new UserDecorator($user);

  $pwdecorator = new PasswordChangeDecorator($userdecorator);

  print $userdecorator->getPassword() . "\n";
  // set the new password 
  $pwdecorator->changePassWord("newpw");
    // test the outcome
  print $userdecorator->getPassword();
busypeoples
  • 737
  • 3
  • 6