MVC'd login form

I've written this small MVC login application as an example for someone, would anyone let me know what they think of it?

index

include("login_view.php");
include("login_model.php");
include("login_controller.php");

Login_view::show_login_form();
$Login_controller = new Login_controller();
$Login_controller->validate($conn);

login_view

class Login_view{
    static function show_login_form(){
        echo 
        "
            Username:  
            Password:  
            
        ";
    }

    static function show_message($message){
        echo $message;
   }

}

login_controller

class Login_controller{
    function validate($conn){
        if( isset($_GET["username"]) && isset($_GET["password"]) ){
            if( !empty($_GET["username"]) and !empty($_GET["password"]) ){
                $Login_model = new Login_model($conn);
                if($Login_model->check_user_and_pass()){
                    session_start();
                    $_SESSION["username"] = "logged_in";
                    Login_view::show_message("logged in");
                }
            }else{
                Login_view::show_message("empty");
            }
        }
    }
}

login_model

class Login_model{
    function __construct($conn){
        $this->conn = $conn;
    }
    function check_user_and_pass(){
        $select_user_pass_query = $this->conn->prepare("SELECT username, password FROM users WHERE username = :username, password = :password");

        if($select_user_pass_query->execute(array(':username'=>$_GET["username"], ':password'=>$_GET["password"]))){
            $details = $select_user_pass_query->fetch(PDO::FETCH_NUM);
            if(!empty($details)){
                return true;
            }else{
                Login_view::show_message("Wrong user or password");
            }
        }else{
            Login_view::show_message("Error occured");
        }
    }
}

MVC

• the model shouldn't care where user input comes from, so it shouldn't work on POST, that's the job of the controller. To fix this, add arguments to check_user_and_pass which you pass from the controller. This makes your code more flexible and easier to read (GET, POST are only handled in the controller, not all over the place).

• the entry point should be the controller. It should decide what the view shows. If you always want to show the form, just add Login_view::show_login_form(); to the top of validate. If it should only be displayed when not logged in, put it in the else clauses.

• I would make the view non-static. It doesn't really matter all that much, as it doesn't have a state, but the controller doesn't either, and it is non-static, so just for consistency it makes sense (also, it might make writing tests easier).

Security

• $_SERVER["PHP_SELF"] is user controller, and thus echoing it creates an XSS vulnerability. Either handle this a different way (empty action, etc), or use htmlspecialchars.

• use POST for logins instead of GET. It might happen that a user shares a link with their password still in the URL, a URL might be bookmarked or saved in the browser history, etc. It's also the correct way of handling it, because GET should only retrieve resources.

Misc

• all that escaping in the form makes it hard to read. If you use ' instead of the outer ", you don't need to do it (be careful when doing it the other way around - " for the outer quotes, and ' for the inner once - as htmlspecialchars doesn't encode ' per default).

• variable names should be lower-case

• I would use && instead of and in the PHP code and AND instead of , in SQL.

• class names are generally written in CamelCase.

• if you use guard clauses checking the arguments, you can safe one level of nesting and produce more readable code. Eg in validate, you might negate the outer if and return: if( !isset($_GET["username"]) || !isset($_GET["password"]) ) { return; }.

• having a session value called username which doesn't contain the username is a bit confusing. I would probably change it to isLoggedIn or something and then set it to true.

StackExchange Code Review Q#87640, answer score: 2