Password-Lock a Single Page with PHP - How did I do?

Question

I just wrote some PHP to let a client lock off a their one-page site from the public using a password. No users and no database. Just checking the password the user enters and testing it against the chosen phrase. This should be simple (right?) but I havn't built this sort of thing before. Let me know what you think, and whether it could be done better. Thanks!

includes/session.php

class Session {

private $logged_in;

function __construct() {
    session_start();
    $this->check_login();
}

// Return whether they are logged in.
public function is_logged_in() {
    return $this->logged_in;
}

// When initialized, check to see if the user is logged in.
private function check_login() {
    if (isset($_SESSION['logged_in'])) {
        // If logged in, take this action
        $this->logged_in = true;
    } else {
        // If not logged in, take this action
        $this->logged_in = false;
    }
}

// Set the session. User will remain logged in.
public function login() {
    $_SESSION['logged_in'] = 1;
    $this->logged_in = true;
}

// Log out. Unset session and destroy it. 
public function logout() {
    unset($_SESSION['logged_in']);
    $this->logged_in = false;
    session_destroy();
}

public function check_pass($pass) {
    $pass = @strip_tags($pass);
    $pass = @stripslashes($pass);

    if ($pass !== 'TheChosenPassword') return false;

    return true;
}
}

login.php:

require_once('includes/session.php');

// If already logged in just send them to index.php
if ($session->is_logged_in()) {
    header('Location: index.php');
    exit;
}

// Create the var in the global scope.
$msg = 'This page is private.<br> Please enter the password.';

// If the form was submitted ... 
if (isset($_POST['submit'])) {

    // Make sure they entered a pass
    if (!$_POST['pass']) {
        $msg = "Please enter a password.";
    } else {

        // If they entered a pass, grab it and trim it.
        $pass = trim($_POST['pass']);

        // Now check the pass
        if ($session->check_pass($pass)) {
            $session->login();
            header('Location: index.php');
            exit;
        } else {
            $msg = "Sorry, that password is incorrect.";
        } // Endif checkpass
    } // endif isset(pass)

} // Endif isset(post)

?> <!-- The form is bellow -->

index.php

require_once('includes/session.php');
if (!$session->is_logged_in()) {
    header('Location: login.php');
    exit;
}

You can probably guess from the code that the form just has an <input> with a name of 'pass' and a submit button with a name of 'submit'. Also a <small> tag in which i echo $msg. So, any thoughts on how this could be improved? Did I make any glaring mistakes? Thanks for the help!

Answer 1

A few things, mostly around logged_in:

private function check_login() {
    if (isset($_SESSION['logged_in'])) {
        // If logged in, take this action
        $this->logged_in = true;
    } else {
        // If not logged in, take this action
        $this->logged_in = false;
    }
}

Simply:

$this->logged_in = isset( $_SESSION['logged_in'] );

Also, the accessor method is_logged_in is being used quite well, yet there is a lack of a corresponding set_logged_in. Add a set accessor, even if private, and use it exclusively.

Directly assigning the value of an instance variable in more than one place violates the DRY principle. That is, the assignment of $this->logged_in = <<value>> should exist only once: within the accessor. For example:

private function set_logged_in( $logged_in ) {
  $this->logged_in = $logged_in;
}

Once the accessor is in place, use it:

private function check_login() {
  $this->set_logged_in( isset($_SESSION['logged_in']) );
}

After the logged in status is using accessors exclusively, you can remove the logged_in variable completely and rely only on the $_SESSION['logged_in'] value. Think about what this implies for the check_login() method.

Next:

if ($pass !== 'TheChosenPassword') return false;

return true;

Simply:

return $pass === 'TheChosenPassword';

Answer 2

I don't see anything in there which forces the use of TLS (HTTPS). It could be that you're certain that the page can only be accessed over TLS, but even so it's good practice to double-check.

The reason for using TLS on login pages is that it prevents the password from being sent in plaintext (e.g. readable by anyone on the same wifi network). If TLS just isn't an option, you can at least use HTTP digest authentication. MD5 isn't considered particularly strong any more, but it's better than nothing.

See Apache's mod_auth_digest. As a bonus, you can replace your session include file and login page with a trivial password file and a couple of lines of Apache config.