I’m writing a login repository class to handle all aspects of logging in - error messages to be returned, delaying attempts to a few seconds, maxing attempts out, etc etc etc.
What makes a class, good? I’m looking over my code and I know DRY is good and being able to make it reusable. I think my code is at a point where as long as the database structure (name/fields) are the same, then this can be reused on other systems. Is that how it should be? Are there factors I’m missing?
To get an idea how the class is set up, my construct initiates the PDO, and then i call the checkCredentials function which goes through and calls mini functions to do other tasks, and ultimately returns true/false to the program to allow login. Is this how it should be? What am I missing? Thank you.
From the detail provided it sounds reasonably tightly focussed, which is a good thing. Acknowledging the Single Resposibility Principle will help keep your code structured nicely.
One thing that I can point out here though is that this login processor probably shouldn’t be instantiating it’s own PDO object based on the assumption that you’ll need a database connection elsewhere in your application.
If the login processor needs a database connection to work properly (presumably yes!!) then you can consider that PDO instance as a dependency. That being the case, you should consider instantiating that database connection elsewhere and injecting it as a constructor param. ie
class Loginator
{
private $db;
public function __construct(PDO $db)
{
$this->db = $db;
}
... // rest of the class
}
This will give you a nice, loosely coupled design and not tie you a hard-coded PDO set up routine.
<?php
//javascript give prpr errr messages / php too
//remember me feature. 1 week? datetime
class LoginRepository
{
private $pdo;
public function __construct(PDO $pdo)
{
$this->pdo=$pdo;
}
public function checkCredentials($email, $pass)
{
Ignore the comment at the top; that’s what I need to accomplish still (I assume custom error messages in this application will require changing the return values to actual strings which I parse .)
Perhaps I explained it wrong, since I seem to already have that design pattern?
Sorry, but I personally consider using $_SERVER or $_SESSION bad design to begin with or probably a better way of saying insecure. I personally avoid them myself and I’m starting to have a lot of reusable classes that I can transfer from one project to the other without. It just requires using a tight file and directory structure majority of the time. The biggest gripe that I personally having now is with CSS styling it would be nice to be able to easily change styling that the classes use, but this is going off topic.
In which case, the login class looks fine (although whether it should be called a repository is another matter, since a repository is something that “stores stuff for later retrieval” )
What’s in this file? An array? There’s no clear indication of where the content of this file is actually used since the PDO is set up beneath the require with hard coded values.
Also, you’d be well advised against using “naked” require_once statements as they’ll only bring you trouble later on (see other threads) - at the very least provide a custom loader function that’s globally available or better yet, go for an autoloader. Being able to fail gracefully when a filename fails the is_readable() test is important.
You’re likely to want the PDO instance in other locations too, so this probably shouldn’t be instantiated here inside an if() for a login page.
Your if() would benefit from being changed to a test to see if the email and password variables are actually available in the POST array - currently you’re just assuming that they are as the request method was ‘POST’.
Validate that these values are available before requiring the config and connecting to the database and you’ll save yourself a few processor cycles.
But you weren’t asking about the login page, I know! The class looks good, from what I can see of it .
Yeah I’ve been following that Tony thread about auto loaders but I have no idea how to implement it. I’ll put that on my list to do.
Should I put this outside the if statement for server request method? I mean PDO is reusable right? Declare once and you’re good? The first step for this program is to login so this PDO should be automatically set. Why do I move it?
Ah good point. I’ll add that in.
I’ll post the final result for critique. Although nothing I have coded will be wrong since it’s my interpretation of OOP principles (joking) .
Just wanted to make sure fundamental OOP/class principles weren’t really blatant. I’ve gotten good feedback though!
This thread is more for …theory really. What sort of class structure is “good” .
After a successful login, you redirect the user to their dashboard. That’s a whole new request. If the dashboard needs a db connection, you’ll be duplicating the PDO setup code (since the one in this login page won’t be accessible.)
You could move the PDO setup code to a file called “init.php” or “bootstrap.php” and get that loaded in at the start of a request. At least then you’ll have centralised the config and db set up stuff.
$pdo = new PDO('mysql:dbname=codefund_cadeui', 'codefund', 'asdfasdf!');
$login = new LoginRepository($pdo);
To a separate file and where that code is in my page, replace that with the include to include that file.
Now on all other pages that need this PDO, I simply include that same file and I automatically still have the db connection? Any need to include db-config (my loginrepository class?)
Edit-Well, I mean includes for now until I get the autoloader crap working .
If one or both variables are missing the login check would fail and redirect the user back to the form anyway so it’s not really a problem in this situation. Obviously when you’re planning to do something with the submitted data (eg. save to the DB) then you’d want to validate the input.
I know this will sound harsh but: The best way of staying insecure is being ignorant of how the system works in the first place.
$_SESSION vars are written by the PHP script itself for its own retrieval later. The client has no direct access to it. If tainted code gets into a $_SESSION variable it’s the fault of the script for putting it there.
$_SERVER vars are mixed. Some are part of the client request and can indeed be modified by the client to make an attack attempt. All $_SERVER variables stating with HTTP_ are this way. But there there a good many $_SERVER variables that are cannot be tainted this way, such as $_SERVER[‘REQUEST_TIME’]. When working with $_SERVER directly its important to know which of these variables can be tainted and which cannot. Also, in my opinion $_SERVER should be regarded as a read only variable.
The real problem with direct referencing these variables is not security, it’s testability (Admittedly, lack of good testing can cause security holes). Most PHP Unit tests are ran from the command line, and since $_SERVER and $_SESSION are superglobal monkeying with their contents poses the possibility of tainting other tests. Further I don’t think $_SESSION is available when PHP is in the CLI sapi where most unit tests are run.
As a result of this, if you have a class that works with these variables, take them as an argument like so
class LoginRepository
{
private $pdo;
protected $session;
public function __construct(PDO $pdo, array $session = [])
{
$this->pdo=$pdo;
$this->session = count($session) > 0 ? $session : $_SESSION;
}
This approach allows the unit test to pass a mock session array into the object to perform functional testing without tampering with the superglobal state.
My last point of advice is this - barring wanting to learn about the process of writing a login script (after all, log in pages are such a rite of passage they’re referenced in John Coulton’s “Code Monkey” song) why are you writing this? I’d recommend looking into the login management classes from Symfony 2 (which are shared by Laravel I believe) rather than trying to reinvent the wheel.
I sanitize it in the login class now. I figure just in case, just in case I forget (or someone else) just put the sanitize crap in the login class that does all the work. Besides I am using prepared statements also and echoing anything out in htmlspecialchars().
Should I return it back outside of the class? Or does it not matter that I just check isset() now like you see?
I want to do it myself - practice + I learn better when not relying on other frameworks / libraries. I’ve found it to be effective to learn languages. I “know” PHP but nothing terribly advanced. Sort of “I’ll look it up and hack it together” sorta knowledge. I’m trying to cement it. I spend so much of my free time / work on HTML/CSS I need this practice to get JS/PHP/SQL back to comfortable levels. Plus I’m bored and have no other things to work on .
Thanks for the advice on server/sessions. Those explanations are helpful .
Eventually in the commercial space you will need to rely on them - that said knowing how they work by building one of your own is invaluable, so I’m not going to dissuade you from this exercise. I would however encourage you to, after completing your class, compare it against Symfony’s login management objects. You’ll likely find places where you came to the same solution, and places where they did something quite different.
Keep in mind much of the complexity of large frameworks arises out of their need to address the idiosyncrasies of multiple server environments, something custom code rarely needs to concern itself with.
If nothing else, look at the public API’s of their classes, compare and contrast.
Sorry, I’d started to type something in that post, changed my mind, but forgot to remove the text! I’ve updated it to say what I intended.
Your ‘controllers’ should be the only part of your app that know about where the user input comes from (eg. $_POST, $_GET, command line arguments etc) and for most validation/santizing you’d want to use a separate class (as it’s a separate responsibility). In this situation things are relatively simple and using filter_input avoids an isset check before attempting to get the contents of a $_POST variable.
Oh of course - I’m also going to post it here to critique; perhaps get suggestions on what I could have done differently, etc.
I’m well aware of reinventing the wheel and while this application may probably be open to the public, I’m purely doing this for education. It’ll jsut be open to the public just in the spirit of contribution. I don’t want / expect anyone to use it .
[quote=“Michael_Morris, post:17, topic:154532”]
Keep in mind much of the complexity of large frameworks arises out of their need to address the idiosyncrasies of multiple server environments, something custom code rarely needs to concern itself with.
[/quote]Will do.
Once I’m finished I’ll open them up. Thanks.
So if I’m understanding you right, to keep the layers separate here (MVC), I need to avoid the isset and just simply filter_input it?
Can I still keep the filter sanitize in there just in case? I don’t like the idea of (one time) forgetting to validate the input / do something to protect myself (I’m aware sanitizing is different than validating - I just want SOMETHING JUST in case.)
Can I have both? Sanitize/filter_input? I assume that’s fine since that’s not what you’re addressing, but just asking.
If I can, then I’ll add filter_input and remove issets .
.
.