The more a class is broken down to the simplest core the easier it is to understand what role in plays in the entire script, not to mention making more portable. A good example of this is your pdo connection for that looks like it’s utilizing dependency interjection making it easier to swap that particular class out for another. Though to me the class doesn’t look that busy and maybe with some code optimization that goal could be achieved relatively easily.
Not sure what DI is but I do have autoloading (for classes) in place. Is that relevant at all?
Dependency Injection is what you already did with the PDO connection. The connection object is a dependency of your login class (i.e. your class needs it in order to function) and instead of instantiating it inside your class (which creates a tight coupling between the two) you’re passing it in (injecting it) through the constructor.
Ah ok that makes sense. I still don’t see the point of breaking my class down further. I understand it’s to make it more readable/portable but technically the functions are all UserServices (login/logout, and soon registering.)
I suppose though when I do add registering, this will get to be a pretty big class. Eh, I guess I could make it into a Login / Logout / Register class (3 separate.) Not sure I agree with breaking up the “Login” class though since all the functions have purposes for logging in.
Check out SOLID principles. Having different classes to break down your problem into “things that needs changes for the same reasons” or “one reason for change” follow the “S” for SRP or Single Responsibility Principle.
It is a bit long winded from Uncle Bob, but watch the video.
This video is a bit more PHP related.
Now answer this question:
Would sessions and cookies have different “consumers” in your application? If yes, then they should take on their own responsibilities, because they would need to change for different reasons than your checkCredentials class.
I hope that makes sense. 
Scott
1 Like
No - they should be the same person. If I’m understanding you right.
The cookie will only be activated if the user logging in has correct credentials, and if they selected the remember me feature. The cookie is populated with the authkey associated with each user account.
Upon logging out though, they get a new authkey in the database for security purposes.
I’ll watch those videos but I can’t right now. I’ll put them on my list of videos to watch. Promise I’ll get to it ASAP.
Edit- @cpradio . Just got an error upon posting “Error occured” - IIRC…Bounced back in like 1 second. Tried refreshing. Post wasn’t there. Tried submitting but it said “body was too similar to what was previously posted”. So I edited this comment on, submitted it, and it went through.
Consumer doesn’t mean the same person (although Uncle Bob does use that analogy), but rather parts of your application consuming those “things”. The better question is, will no other part of your application set or read data from a cookie (and not even the same cookie but A cookie)? Will no other part of your application need information from the session or even set information in the session?
Scott
There might be other places I set session information, but I doubt it. I just can’t confirm or deny due to not thinking about it. I think it will all come from databases. I’d bet a small amount of money on it. I don’t think I will be storing anything else into sessions.
I am guessing that if I will need to set sessions elsewhere, then I’ll need to restructure my code.
Likewise, I do not believe I will be setting any other cookies. I only did it here since it’s the best way to do “remember me”.
Edit-I started watching that CodeAcademy Single Responsibility video and my eyes are now open. I understand it and I see how my class has multiple responsibilities. Now I feel dirty. I’ll need to clean this up.
1 Like
After just a quick look through your class, I have identified a pretty big security issue you’ll want to address. If the login attempt is not successful, you are returning to the user the reason the attempt failed. In the AJAX response, the reason will be the second element of the result array, while if standard POST, you add the reason as a GET parameter on the redirect. This could indicate to a potential hacker if they’ve identified a valid username. Ideally, if the attempt is unsuccessful, the only thing the user will know is just that. You should not output anything beyond the success status for the attempt.
The first thing I noticed in your code was the lack of comments. Even though this is just a personal project right now, if you do plan to implement this at some point in the future, the comments will be a big help. It’s my opinion that for dynamic languages like PHP, you should at least provide a comment for every method of a class that details the purpose of the function, expected types of it’s arguments (when you can’t type-hint, e.g. scalar values) and the return data type.
Additionally, you may want to look in to how to implement the idea of Gateway classes:
http://martinfowler.com/eaaCatalog/tableDataGateway.html
A Gateway class abstracts the retrieval of data into and out of your persistence mechanism away from your application logic. A Gateway class would remove the need for your UserServices class to understand the database schema related to the Users and Attempted_Logins tables. An example UsersGateway class might look like this:
<?php
class UsersGateway {
private $pdo;
public function __construct(PDO $pdo) {
$this->pdo = $pdo;
}
public function selectUserByUsername($username) {
// Query and return details from the database related to the provided username
}
public function updateAuthKey($username) {
// Query to update the authkey value for the provided username
}
}
The dependencies within your UserServices class then update. Instead of needing a PDO object, your class now has dependencies of the UserGateway and AttemptedLoginsGateway.
The value of converting your code to using Gateways will not be apparent at first, but when you can re-use these Gateway classes (everywhere you need access to user information, for example) the benefit of implementing Gateways is easy to see. BONUS: if you move to using some other persistence mechanism (NoSQL or other), you can be assured that the only classes you’ll have to modify are named *Gateway.
Finally, this is also how relying on another service to provide your cookie functionality can be helpful (as mentioned by @s_molinari). The action of setting a cookie is a persistence activity (albeit on the client side), so can thus benefit from using a Gateway mechanism.
One more quick note: blocking access to your application by IP address is almost always ONLY going to inhibit valid users. The IP Address is very easy to spoof, and if you’re worried about a million hits per minute, you’d need to be worried about thousands of IP addresses. A more appropriate security mechanism would probably be locking out attempted usernames.
3 Likes
Very good point! Eventually they could keep trying until they found a username that is found. Very very nice find.
Yeah, this will eventually be open to the public and I will comment, but I just haven’t gotten around to it yet since it’s only been myself working on it. I will definitely make sure I do thorough comments in the future. As you can tell throughout this thread, I’ve been changing my code as per suggestions so for right now, I feel like I need to hold off on commenting until I have the solid foundation that won’t change all that much.
The ultimate goal is to give the hacker less time chopping away at my users table. Between 2s / 5 attempts max per IP, that will significantly slow down hackers since otherwise their attempt isn’t even considered when logging in.
I do like the idea of “per username” and am making note of it. You’ve been of great help. Thank you.
Edit-“Sorry, an error just occured” when I posted @cpradio
This is a good point. Any serious hacker will be brute forcing through a variety of IPs (think botnet). Blocking by IP will probably be ineffective.
However, locking the username would be even worse. That would allow an attacker to effectively disable someone’s account by constantly sending bad login attempts, keeping the account permanently locked.
So what’s the solution then to blocking people? How do I identify them aside from IP?
Sounds a lot like a repository, actually. I might have to re-read those parts of the book to discovered what the nuanced differences are.
Table Data Gateway: Holds all the SQL for accessing a single table or view: selects, inserts, updates, and deletes.
Repository: Encapsulates the set of objects persisted in a data store and the operations performed over them, providing a more object-oriented view of the persistence layer.
Wait! You believed him but you didn’t believe me? LOL! Thanks a lot for that! 
Scott
I’m sorry - I must have missed your comment about that 
.
Jeff did just discredit it though so I guess it’s good I missed it.
Locking a username poses no additional security threats. It’s true that this would be inconvenient to the user whose account has been locked, but if the hacker has access to a valid username, half of a valid authentication combination has already been compromised. If the user causes the lockout of their own account, ideally, you’d be able to provide them a way to verify they are the owner of the account to have it unlocked (sending an unlock link to the email address on file for that account, etc). If the lockout was caused by a hacker, your system could notify the user that their account has been locked and they can communicate back to you that there is malicious activity going on with the application.
Your videos distracted me! I missed this. Sorry.
I’m going to hold off on adding this feature to my list until you all debate this.
Now I sorta see this as an advantage to do this by username.
You’re right on with this, but the queries being performed in his class are only acting on a single table, so referencing the class as a Gateway is probably appropriate. The concept to grasp though, is the importance of abstracting the persistence away from the logic.