A simple PHP login system

Question

I'm super new to PHP and I'm trying to get a functional login system together. This is the code I have:

class/user.php

<?php
 class Users {
 public $username = null;
 public $password = null;
 public $salt = "Zo4rU5Z1YyKJAASY0PT6EUg7BBYdlEhPaNLuxAwU8lqu1ElzHv0Ri7EM6irpx5w";
 public function __construct( $data = array() ) {
 if( isset( $data['username'] ) ) $this->username = stripslashes( strip_tags( $data['username'] ) );
 if( isset( $data['password'] ) ) $this->password = stripslashes( strip_tags( $data['password'] ) );
 }
 public function storeFormValues( $params ) {
 //store the parameters
 $this->__construct( $params ); 
 }
 public function userLogin() {
 $success = false;
 try{
 $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD ); 
 $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
 $sql = "SELECT * FROM users WHERE username = :username AND password = :password LIMIT 1";
 $stmt = $con->prepare( $sql );
 $stmt->bindValue( "username", $this->username, PDO::PARAM_STR );
 $stmt->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
 $stmt->execute();
 $valid = $stmt->fetchColumn();
 if( $valid ) {
 $success = true;
 }
 $con = null;
 return $success;
 }catch (PDOException $e) {
 echo $e->getMessage();
 return $success;
 }
 }
 public function register() {
 $correct = false;
 try {
 $con = new PDO( DB_DSN, DB_USERNAME, DB_PASSWORD );
 $con->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
 $sql = "INSERT INTO users(username, password) VALUES(:username, :password)";
 $stmt = $con->prepare( $sql );
 $stmt->bindValue( "username", $this->username, PDO::PARAM_STR );
 $stmt->bindValue( "password", hash("sha256", $this->password . $this->salt), PDO::PARAM_STR );
 $stmt->execute();
 return "Registration Successful <br/> <a href='index.php'>Login Now</a>";
 }catch( PDOException $e ) {
 return $e->getMessage();
 }
 }
 }
?>

login.php

<?php 
 include_once("config.php");
?>
<?php if( !(isset( $_POST['login'] ) ) ) { ?>
 <form method="post" action="">
 <input type="text" maxlength="30" required autofocus name="username" />
 <input type="password" maxlength="30" required name="password" />
 <input type="submit" name="login" value="Log me in" />
 </form>
<?php 
} else {
 $usr = new Users;
 $usr->storeFormValues( $_POST );
 if( $usr->userLogin() ) {
 echo "Welcome"; 
 } else {
 echo "Incorrect Username/Password"; 
 }
}
?>

register.php

<?php 
 include_once("config.php");
?>
<?php if( !(isset( $_POST['register'] ) ) ) { ?>
 <form method="post">
 <input type="text" id="usn" maxlength="30" required autofocus name="username" />
 <input type="password" id="passwd" maxlength="30" required name="password" />
 <input type="password" id="conpasswd" maxlength="30" required name="conpassword" />
 <input type="submit" name="register" value="Register" />
 </form>
<?php 
} else {
 $usr = new Users;
 $usr->storeFormValues( $_POST );
 if( $_POST['password'] == $_POST['conpassword'] ) {
 echo $usr->register($_POST); 
 } else {
 echo "Password and Confirm password not match"; 
 }
}
?>

I am really keen to know how this code is structured, as I used information from several different tutorials combined to create it. Any input would be appreciated.

Answer 1

Data Handling & Security

You use prepared statements, which protects you from SQL injection, which is good. However, the way you handle passwords could be improved to increase security.

It's not a good idea to semi-randomly apply sanitation functions to user input as it results in dirty data.

stripslashes for example really only makes sense when magic quotes is enabled, so you should check that first (and then only strip the slashes in some init file, not every time you access GET). For example, if a user has a password like this: a\'b\\c\'d, stripping slashes reduces the password length from 9 to 6 characters.

Same problem with strip_tags, only much worse. If my password is a<super$secure!password, now my password is a.

You also don't need strip_tags. As XSS protection it's not sufficient anyways (and definitely not needed for passwords, which aren't echoed); You should HTML-encode data when echoing it instead.

There's also really no need to have a length restriction on the password, especially not one that is shorter than what your hashing algorithm allows.

Hashing

Simple sha is really not good enough anymore, as it's too fast. Use bcrypt instead.

Error Handling

Don't echo database error messages directly to the enduser. They won't know what to do with it, and it may provide attackers with information or enable some forms of attacks (such as error based SQL injection).

It's also not a good idea to echo anything in a function, as side-effects like that make it harder to reuse.

Structure

I think your structure is pretty clean, and your code easy to read. Just a couple of small points:

storeFormValues doesn't seem to have any purpose. I wouldn't call the constructor inside a function of the same class, but just use it directly.

I would also not pass a magic array to the constructor, as it's hard to use and reuse (I have to guess how the array must be build and what it must contain, and I have to use the pre-defined array keys).

Instead, just change your constructor to __construct($username, $password). This increases clarity and reduces future bugs.

Misc

• remove unused variables such as $correct.
• comments such as //store the parameters are not really needed as they don't add any information.
• same things should be named in the same way to avoid confusion. For example, registering and logging in are quite similar. But the names userLogin and register are constructed differently. Just use login and register.

• Jimmy: Thank you very much for your feedback! I really appreciate it

Answer 2

I wouldn't talk too much on on the rest of your code but your userLogin() and Register() function does not belong to your user class..

In short note a user class should be something like..

<?php
 class user(){
 //user property/profile
 protected $username;
 protected Lang = lang;
 //user behaviour
 public change_password();
 public change_lang();
 private setup();
 protected addLang();
 public getLang;
 };

Object are supposed to be used for namespacing and dealing with data. Your above class contains functions not related to data for users class.

register() primarily uses data supplied data users form input which is irrelevant to user class that gets it data from database using session, username from login etc.

Better approach is

<?php
 class Register{
 //register data gotten from input
 //validate input
 //submit to database and send.
 //email verification messages
 }
?>

Do same thing for user login.... Infact User login do not need to be a class nor a function but you could create a Validator class instead, I advise you should use procedural.

Before you do OOP or build class. Think of the data you wil need to work on... Ask what are it behaviours.

• Your Common Sense: why such an exception for the lang? doesn't change_lang(); depend on the input as well? Why there is no recommendation for the change_lang class as well? and why login is neither a class or a method but a function? How come a method that deals with the user name and password is "not related to data for user"? this answer is inconsistent and misleading. Let me advise you to provide only answers on the matter you have a firm understanding yourself.
• Stanley Aloh: @YourCommonSense change_lang() would affect data in the user field in database that belongs to user. i agree that user could supply such data at registration too but the point I am driving home is that his User class is too responsible.
• Your Common Sense: register would affect the data in the user field in the database. so what's the difference
• Stanley Aloh: @YourCommonSense user class fetch data from database and mimick user behaviour while register class fetch data from user input, validate and store. Furthermore, user class should hold state of existing user while registration help update the database with new user info.. This responsibility are totally different and should not be handled by one class