A simple framework code review request

[cryptapus]

Greetings!

 

I am by no means a php expert, but I've attempted to make a simple php/sqlite user framework and would appreciate some critical feedback. Especially on the authentication technique. My goal with this project was to make it simple and light-weight for light-duty/personal use.

 

The project can be found here:

 

https://github.com/cryptapus/phpsqliteuser

 

I currently have two different login mechanisms in lib/class.user.php. A user->login() that uses POST username and password, and a user->login_cookie() that uses a "Remember Me" cookie from the client.

 

Thank you in advance for any constructive feedback. Pull requests are also welcome.

 

Kind Regards,

[scootstah]

At a glance, your classes have waaaay too much responsibility, and are way too tightly coupled. For example your User class is creating a new database connection, when it should be given an instantiated database object with dependency injection. Your User class construct is also doing way way too many things. A construct should be used to set up the class, not do a bunch of logic. Your User class should not be responsible for creating database tables and inserting initial records. Likewise, your database class should not be responsible for creating database tables either.

 

Since your classes are very tightly coupled, this library is not very portable. It relies on the fact that a database connection is to be made, that specific tables must be created with specific columns, that data must be inserted in a specific way, that certain constants are required to exist, that things like $_SESSION are available, and that that is the way the session is accessed, etc.

 

I recommend you do some more research on the principles of OOP. In particular, check out the Single Responsibility Principle, and Dependency Injection.

 

Props for using password_hash() and password_verify() though.

[cryptapus]

Thanks for the feedback, I will do some more reading on the topics you suggest.

 

I think I understand your concerns, except for your statement of the database class should not be responsible for creating database tables. Is it that you mean that the database class should strictly be meant as a connection to the database? If one were interested in abstracting the database calls in the interest of making it database independent, wouldn't it be best for the database class to handle all SQLite/MySQL/PostgreSQL translations?

 

One more item, I've done some reading on different implementations of a "Remember Me" cookie and I think I'm safe, but here's the process:

 

1) User logs into the site with "Rember Me" checkbox checked.

2) Site does a Username/Password validation, then sets a cookie for 30 days that contains a Username and a SHA256 hash of rand(), (Cookietok) which is stored in the database.

3) User leaves, then comes back and presents the Username and Cookietok cookies. If a match, a new Cookietok is calculated and set as the Cookietok cookie and database is updated rewriting the old value with the new. (User is allowed to have multiple Cookietoks for different devices but are treated the same way).

4) On logout, all Cookietok's are removed for the user.

 

Thanks for your review.

 

Kind Regards,

[scootstah]

I think I understand your concerns, except for your statement of the database class should not be responsible for creating database tables. Is it that you mean that the database class should strictly be meant as a connection to the database?

Your database class should not be tightly coupled to any particular database schema. Doing so prevents your library from being portable, because now everyone's project needs to exactly match your database schema or it will not work.

 

If one were interested in abstracting the database calls in the interest of making it database independent, wouldn't it be best for the database class to handle all SQLite/MySQL/PostgreSQL translations?

It is not necessary for you to build something like that. PHP already has a built in database driver called PDO which is already compatible with a whole bunch of database vendors using a single abstracted API. There is absolutely no reason you should attempt to build your own version of PDO.

 

EDIT: But remember, that is another external dependency. Your classes should not make definitive decisions about dependencies if at all possible. For example instead of just depending on PDO throughout your library, create a new database interface that it depends on instead. Then you can implement many kinds of drivers that use that interface (PDO, MySQLI, etc). That way people are free to use whatever infrastructure they want to, and your library won't care.

 

2) Site does a Username/Password validation, then sets a cookie for 30 days that contains a Username and a SHA256 hash of rand(), (Cookietok) which is stored in the database.

No. Bad. rand() is not meant to create cryptographically random and unique tokens, which is what you want for such a system. You want openssl_random_pseudo_bytes() instead. Also, you don't need to store the username in the cookie - just store the token.

Edited August 3, 2015 by scootstah

[cryptapus]

Excellent tips. Thank you very much.

[ignace]

Don't reinvent the wheel. It's okay to create your own little framework but be sure to utilize available libraries. Like Doctrine for example as your ORM or DB Abstraction Layer. Something like Silex as a way to utilize a CMS. Build on the shoulders of giants. your far better off this way.

[cryptapus]

Don't reinvent the wheel. It's okay to create your own little framework but be sure to utilize available libraries. Like Doctrine for example as your ORM or DB Abstraction Layer. Something like Silex as a way to utilize a CMS. Build on the shoulders of giants. your far better off this way.

Generally, I agree with your statement. I guess I had a couple of goals for this project in order of importance:

 

1) Education, learning about php and how to apply it.

2) Simplicity, I didn't want anything like a registration scheme, etc., limiting attack surface area.

3) I didn't want to have to track another php project for security. I felt like if I coded it I would have more control in how/what was vulnerable (of course I recognize the can of worms that opens). If something simple is available in debian stable (to get security support) I would be interested, but I didn't want something super big like zend. I didn't see anything obvious... Suggestions are welcome.

 

Thanks for the advice, I think it's very good and will do some thinking on what I can apply that's out there...

[scootstah]

1) Education, learning about php and how to apply it.

I absolutely encourage this behavior. For me, the best way to learn how some system works is to build it myself and see all of what goes into it. But, if you were to need something in a production environment I would suggest something that already exists and is more polished. At the very least extend something that already exists - then it can be custom, built the way you want, but it's still solid.

 

3) I didn't want to have to track another php project for security. I felt like if I coded it I would have more control in how/what was vulnerable (of course I recognize the can of worms that opens).

That's not really your burden. Remember that popular open source libraries are community-driven, and lots of people maintain, test, and fix the code. Depending on popularity, usually security concerns are found and addressed quickly. On the other hand, if you are the sole maintainer, how will you even know if you have a security problem? Attackers could be exploiting vulnerabilities for a long time before you stumble upon them.

[ignace]

3) I didn't want to have to track another php project for security. I felt like if I coded it I would have more control in how/what was vulnerable (of course I recognize the can of worms that opens). If something simple is available in debian stable (to get security support) I would be interested, but I didn't want something super big like zend. I didn't see anything obvious... Suggestions are welcome.

Like scootsah already mentioned. Now you are the only one guarding the security whereas otherwise thousands. I also didn't say you need to go and download the entire Zend framework. I don't even like Zend. I tried Apigility (by Zend on ZF), yet a simple composer update breaks the entire thing (it could not find it's own core tools ZFTool for some reason)...

 

I proposed simple libraries that do ONE thing and do it WELL. The unix philosophy.

Edited August 7, 2015 by ignace