PHP Simple login with PDO

Question

This is my Controller class login check section

case 'checkLogin':

        $username   = isset($_REQUEST['username']) ? trim($_REQUEST['username']) : "";
        $password   = isset($_REQUEST['password']) ? trim($_REQUEST['password']) : "";


        try{

            $login = $user->login($username,$password);

            if ($login === false) {
                throw new Exception("username or password is wrong");
            }else {

                $_SESSION['id'] =  $login;
                header('Location: index.php');

            }

        }
        catch(Exception $ex){

            $errMsg = $ex->getMessage();

            $view->render('view/login.php', array('errMsg' => $errMsg ));
        }   

        break;

UserModel Function

This is my user model function for checking username and password.

public function login($username,$password){

    $username = strip_tags(stripslashes(mysql_real_escape_string($username)));

    $password = strip_tags(stripslashes(mysql_real_escape_string($password)));


    $stmt = $this->db->con->prepare("SELECT `password`, `id` FROM `user` WHERE `username` = ?");
    $stmt->bindValue(1, $username);

    try{

        $stmt->execute();
        $data               = $stmt->fetch();

        $stored_password    = $data['password'];
        $id                 = $data['id'];


        if($stored_password === md5($password)){
            return $id; 
        }else{
            return false;   
        }

    }catch(PDOException $e){
        echo $e->getMessage();
    }

}

Please tell me this is right.This code is working for me. I'm using this to implement basic MVC pattern login.

I've got some code from here http://www.sunnytuts.com/article/login-and-registration-with-object-oriented-php-and-pdo

Some tips: The MySQL extension is outdated. Remove the function calls to `mysql_real_escape_string()`, `strip_tags()` and `stripslashes()`. They are unnecessary and also restrict a user's password! You're using PDO with prepared statements anyway, so you don't need escaping. MD5 shouldn't be used for password hashing. Switch to bcrypt or scrypt. You should use a salt, too. — ComFreek, Oct 02 '13 at 17:25
This has nothing to do with MVC. Aside from problem that were alreasy listed by @ComFreek, you also are completely breaking [SoC](https://en.wikipedia.org/wiki/Separation_of_concerns), especially regarding divide between presentation layer and model layer. And the separation between user request an response handling withing the presentation layer. — tereško, Oct 02 '13 at 17:26
Regarding "how to login user in MVC", maybe [this](http://stackoverflow.com/a/16650437/727208) or [this](http://stackoverflow.com/a/16997200/727208) might shed some light. — tereško, Oct 02 '13 at 17:35

Your Common Sense · Answer 1 · 2013-10-02T17:41:20.317

1

If get rid of all the useless and wrong stuff, login() become like this

public function login($username, $password)
{
    $stmt = $this->db->prepare("SELECT password, id FROM user WHERE username = ?");
    $stmt->execute(array($username));
    $row = $stmt->fetch();
    if(crypt($password, $row['password']) == $row['password'])
    {
        return $id; 
    }
}

Note the better password hashing algorithm

something similar have to be done to other code part:

    $login = $user->login($_POST['username'], $_POST['username']);
    if ($login)
    {
            $_SESSION['id'] =  $login;
            header('Location: index.php');
            exit;
    }
    $view->render('view/login.php', array('errMsg' => "Wrong credentials" ));
    break;

And yes, it have very little to do with MVC

edited Oct 02 '13 at 17:41

answered Oct 02 '13 at 17:31

Your Common Sense

156,878
40
214
345

1

People should stop treating code in SO as *copy-paste-able* pieces. The code you see in this site is usually for illustrating an **EXAMPLE** and should never be used in production. – tereško Oct 02 '13 at 17:39
@tereško don't get me wrong. *My* code is copy-and-paste-ready even for the production. – Your Common Sense Oct 02 '13 at 17:42
Then what would you call the try/catch block for PDOExceptions? – Sammitch Oct 02 '13 at 17:46
@Sammitch Good question. I'd call it one of worst PHP world superstitions. Most of PHP folks have no idea what exceptions are and what they are for. And the code in question makes an excellent example of it. – Your Common Sense Oct 02 '13 at 17:48
@Sammitch I see [you are moving the right way](http://stackoverflow.com/a/13518823/285587) (this answer of yours bears such a rare mark as an *upvote* from me). If it was not just mindless recital of some mantra, eventually you will understand the *proper* error handling. – Your Common Sense Oct 02 '13 at 18:12

score -1 · Answer 2 · answered Oct 02 '13 at 17:40

This is a good initial framework for MVC, but your code needs work. A couple suggestions.

I'd use a better encryption method on the password, MD5 is a bit outdated these days.
You are making a framework so continue to think in a modular way, try to condense and simplify your code so that it basically takes in a request and returns a result, handle the extraneous stuff elsewhere. For example, by the time you are at "case checklogin" your application should have already handled the post and checked whether or not the post was set.

I'm not posting code because I get the idea that you are teaching yourself, like I once did.. and the last thing we were looking for is cut and paste solutions.

'Encryption' is not the correct term here. It's called 'hashing'. — ComFreek, Oct 02 '13 at 17:52

PHP Simple login with PDO

2 Answers2