Determining what functions should be implemented in this user-authentication class

Question

Long story short, I would like to use OOP for my new PHP project. It has a "login" requirement (i.e. user will first need to enter the username and password (login.php), if it's correct, it will be redirected to index.php and then fetch some products information). Also there will be a settings page that allows user to change the password, and email if necessary.

In summary:

• When user attempts to login, the program will check if the username, password entered is correct or not. If it's correct, username (or maybe the whole User object) will be saved in the $_SESSION.
• When the user settings page is loaded, it will display all user related fields on screen (i.e. by using $_SESSION).
• The user can make changes in the user settings pages and data should be saved to the database.

I have this User class:

class User {
     private $username;
     private $email;
     private $hashed_password;
     private $dob;
...

     public __construct($user) {
          $this->username = $user;
     }

     public function isPasswordCorrect($userenteredpassword) {
           $this->loadFromDatabase();
           //hashed userenteredpassword, compare it against $this->hashed_password
           //if it matches, return true, otherwise return false.
     } 

     public function saveToDatabase() {
          //save info to database
     }

     public function loadFromDatabase() {
         $row = select fields from user table where username = $this->username limit 1
         if ($row is found) {
              $this->hashed_password = $row['hashed_password'];
              $this->dob = $row['dob'];
              ...
         }
     }
}//class

However, I was told that the function isPasswordCorrect shouldn't be implemented inside class User, as it is not its job to determine whether the login is valid or not. Instead, I should create an authService helper and use it to determine whether the login is good or not.

Now, this makes me wonder if the rest of my User class design is okay or not. If I shouldn't put the password checking function in the User class, then is it okay to implement the database related functions in User? How do I determine which function should be implemented in the class? Should I follow Single Responsibility principle? Wouldn't it be too extreme?

Answer 1

Now, this makes me wonder if the rest of my User class design is okay or not.

That depends entirely on your viewpoint. You certainly don't have to have a UserEntity, UserRepository, UserFactory, UserFinder, Email, Username and Password Value Objects, AuthenticationService AuthenticationAdapter and implementations. But you could have.

Also, we could (no, we should) argue, whether storing the password hash in the user is a good idea. You'll only need it to authenticate and then never again, so that's one time in the application. After that, you can use a token or set a flag in the Session. There is no need to store the password then, regardless of where you do the authentication (a separate component sounds fine to me. You could inject that to the User and then delegate the call).

The most important thing is that the code does what the enduser thinks it should do. However, it should also be implemented in a way that won't come back to haunt you, should your ever need to change the application. So whether it's "okay" or not pretty much depends on the scope of your project. Ask yourself: "is it good enough?"

If I shouldn't put the password checking function in User class, then I wonder if it's okay to implement the database related functions in User???

That gets us right back to the first question. If we assume User to be a class holding business logic, then technically, putting the db access into the User is a violation of SRP. However, when the impedance mismatch is small or doesn't exist, then using an ActiveRecord'ish pattern might be practical. So check how much impedance mismatch you have. When you notice your User to turn into a ORM, consider using an ORM instead.

The other option would be that your User doesnt have any business logic and is really just a Gateway to the storage engine. Then it's okay to have db logic in it. It just shouldn't be called a User then though.

How do I determine which function should be implemented in the class?

A good general set of rules to follow is GRASP.

Another easy test is to look at what the class does and then check that the name matches that what it does. Your User apparently loads things from the database and checks passwords. Not exactly what I'd expect from a User.

should I follow Single Responsibility principle?

Yes, always. Except for when you can reasonably justify not to. Following SRP will make your code easier to maintain in the long run and will increase reuse possibilities. Once you assign multiple responsibilities, you will have to have the same set of responsibilities in another project if you want to reuse the class. And the chances for that are smaller than for single responsibilities.

Wouldn't it be too extreme??

No. Having many small classes is perfectly fine. It's a matter of appropriateness though again. I found it helps to keep things separated and small. However, it also gets harder to visualize the code flow in your head then the more classes you add.

Answer 2

The Single Responsibility Principle is a good thing to aim for, for many classes. It doesn't always mean that each class only does one thing (though it can), but that it is responsible for a small amount of tightly-related functionality.

That said, the job of some classes is to collect together a number of responsibilities. User may well be one of these, but the individual responsibilities can still be delegated to other classes - e.g. you could always give User an instance of a Credentials class that deals with the password checking.

You almost certainly don't want your database-related functions in User as if you write any tests for User, many of them are likely to have a need to hit the database when they run.

Answer 3

The reason why you were told, that isPasswordCorrect() should not be implemented in User class is because someone is pretty bad at explaining stuff.

It's OK for User instance to validate the provided credentials. The real problem there is the SQL. Your User instance should not care about the existence of database or even be aware that such a thing exists.

A much better API would be:

$user = new User;
$user->setUsername( $username );
$mapper = new UserMapper( $pdo );

$mapper->fetch( $user );
if ( $user->isPasswordCorrect($password) )
{
   // blah blah .. 
}

The terms that cover this are domain object and data mapper.

Answer 4

When situations like this come up, it's a good idea to step away from the code and think about how the real-world operates. Objects in programming mimic those in the real-world, so it can be useful to just look at a use case you see elsewhere.

For example, let's say you're a US Citizen, and you went to another country for vacation. On your way back into the US, you must prove who you are so that they know you're not on some black list somewhere. You, the user, provide your credentials: primarily your passport, but you might also be asked for your drivers license or any other identifying documents. You hand those documents over, and then it's someone else who determines whether your papers are a forgery or not. It is not your responsibility to tell the officers that you're not a threat. It is their responsibility to determine that.

Taking this back to computer authentication: The user provides their username and password, and then someone else(the system) determines if they are authorized to access the system or not.

Taking this to the coding level: it doesn't make much sense to have a user authenticate themselves. The code can be written to do that, but it really isn't the user's job to say he's authorized to have access. Someone else in the system needs to determine that - like the authService your colleague mentioned.

On the topic of database access...like others have said: that's somewhat up to you. I always like to have one object that provides access to data resources(not necessarily to mean one thread, but one object that determines which operations are allowed concurrently and when to lock the resource for writes/updates). Alternatively, many objects can access a data resource but they're synchronized in some way(mutex, semaphore, etc).

This way, the concurrency issues will be located within that object(s) instead of scattered in mysterious locations that make debugging harder. So, if my program is running and I'm noticing wonky things going to/from the database, it's pretty straight forward to drill down to where the problem exists. If I had a bunch of users who were in their own thread and doing whatever they wanted with the database, it might not be immediately obvious that the users are the problem. The symptom is the database looks wrong, and the reason it's wrong is because users are changing the database without respecting the changes the others are making.

An example of that case would be source control, which is a tool used to allow multiple developers to work on the same project at the same time without worrying about who is writing in which file. Now, take away the source control. If two developers write to the same file without knowledge of the other, one of the developer's code is going to get erased completely, and when that developer tries to watch his functionality work...it simply won't be there because someone wrote over it. Then the question becomes...who wrote over it, when, and is there a copy of the feature that was implemented, or does it need to be done again? These kinds of things happen on the order of minutes, hours, or even days...but in a program that uses a database...this will be happening hundreds of times a second. It's better to give control of a resource like that to someone who can safely manage it(source control, database handler, etc).