Login script (OO PHP)

[php-beginner]

Hello everyone,

 

The last few weeks I've asked a few questions. From the answers given, I've finished my login script.

But, I am a noob at oop php and I have also no clue if there are any security holes.

 

So my question to you guys is:

 

• What have i done wrong?
  
• What can i do better?
  
• And what's missing?
  

 

I also have a one basic question:

 

• I have't declared any variable to public, protected or private. Is it better to declare every variabe? or only a few?
  

 

Here is my code:

 

Index.php:

 

<?php

if ($_SERVER['REQUEST_METHOD'] == 'POST')

{

require('classes/class_lib.php');

if(isset($_POST['username'])){
$username = $_POST['username'];
}
if(isset($_POST['password'])){
$password = $_POST['password'];
}

try{
$user = new User;
$user->login($username, $password);
}
catch(MysqlException $error){
echo $error->getError();
}

catch(LoginException $error){
echo $error->getError();
}

}

?>

// form etc.

 

And my class_lib.php:

 

<?php

class MysqlException extends Exception{

public function getError(){
	$errorMessage = 'Er is een fout opgetreden in '.$this->getFile().' op regel '.$this->getLine().'<br />';
	$errorMessage .= 'Foutmelding: <i>'.$this->getMessage().'</i><br />';

	return $errorMessage;
}
}

class LoginException extends Exception{

public function getError(){
	$errorMessage = $this->getMessage();

	return $errorMessage;
}
}

class Mysql{

public function __construct(){
	$this->db = new mysqli('localhost','root','','login');

	if($this->db->connect_error){
		throw new MysqlException('Kan geen verbinding maken.');
	}
}
public function escapeString($string){
	$this->string = $this->db->real_escape_string($string);

	return $string;
}
}

class Query extends Mysql{

public function runQuery($query){
	$this->result = $this->db->query($query);

	if(!$this->result){
		throw new MysqlException('Er is iets fout gegaan tijdens het uitvoeren van de query.');
	}
}
public function returnQuery(){
	return $this->result->num_rows;

	if(!$this->result){
		throw new MysqlException('Er is iets fout gegaan tijdens het ophalen van de resultaten.');
	}
}
}

class User{

public function __construct(){
	$this->mysql = new Mysql;
	$this->query = new Query;
}
public function login($username, $password){
	$this->username = $this->mysql->escapeString($username);
	$this->password = $this->mysql->escapeString($password);

	$this->setQuery = "SELECT gebruikerid FROM gebruikers WHERE gebruikersnaam='" . $this->username . "' AND wachtwoord='" . $this->password . "'";
	$this->query->runQuery($this->setQuery);

	if($this->query->returnQuery() > 0){
		return true;
	}else{
		if(empty($username) || empty($password)){
			throw new LoginException('U moet alle velden invullen.');
		}else{
			throw new LoginException('Uw logingegevens kloppen niet.');
		}
	}
}
}

?>

[gizmola]

My suggestions:

 

- Put every class in its own class file, named using a convention where the file name matches the class.  One convention might be:  classname.class.php, so class Login ... would be in the login.class.php script.  With a convention like that you can implement a simple autoloader if you don't want to manually include every dependency.

 

-Yes declare all your class variables and scope them as needed.  There is very little reason to have variables ever be public -- you have private and protected not to mention static and even constants that you can declare. 

 

-Don't overuse exceptions.  Exceptions should be for issues that are truly unusual, like database errors.  Someone supplying a login form that is missing the password or username is not what I would code in an exception.  The frameworks for the most part have form classes, with widgets and validators that give structure to that type of code, and a login form is no different than any other type of form.  Those are rules that should be built into a validation routine for the form.

 

Just to nitpick further, where you put the LoginException in your routine is ill advised, because you already attempted to query before even checking those conditions.  A long time ago I was a developer for a system that had its own custom database engine, and the entire operation was at a standstill because the system seemed to be broken.  As it turned out, there was code very similar to what you were doing, where a query would occur with blank or empty values, and what I eventually uncovered with a debugger, was that a blank/empty row had gotten inserted into the database.  When the queries would occur, any blank column would cause the blank record to be retrieved, so everyone thought the system was corrupted and broken, because they were always getting a blank screen when they searched.

 

[php-beginner]

Thankyou for your suggestions. I've read that there are no rules for using exceptions or which error handling is the best. But I agree that it doesn't feel right to use it this way for the login.

 

The frameworks for the most part have form classes, with widgets and validators that give structure to that type of code, and a login form is no different than any other type of form.  Those are rules that should be built into a validation routine for the form.

 

Could you give me an example?

[zenlord]

Example of PHP-framework?

 

The best-known are Zend, CodeIgniter, CakePHP etc. 'Recently' there are a few new competitors that aim to be more lightweight: Kohana, Yii etc.

 

I'll be diving into Kohana the moment I feel at ease using objects. I am now struggling somewhat to make the change...

 

If you want a validation routine, then I guess you should look into the official documentation for PHP Filters. They contain pre-built functions to validate and even sanitize different data formats (strings, numeric, email, url, etc). The examples that are given in that section of the documentation are very good...

[php-beginner]

No, I mean a form class. But why should I use a framework? I mean, I'm new to OOP, is it better to concentrate on frameworks?

[gizmola]

Doing a really blown out and sophisticated form, with data cleansing, validation rule checking, error message integration and a mix of complicated form elements, some of which might be connected, is one of the more interesting challenges in basic web development.  These frameworks each recognize that and provide form classes and child objects that give you a way of doing forms efficiently with a high degree of sophistication and usability.  You could put a toolkit together that would have a lot of these same capabilities yourself, but there's a good deal to it.

 

Here's 2 manual pages to browse.  You'll probably notice that there are a lot of similarities and use of design pattern jargon to describe what they are doing (filter, decorator, validator) etc.

 

http://www.symfony-project.org/gentle-introduction/1_4/en/10-Forms

http://framework.zend.com/manual/en/zend.form.forms.html

 

In particular, they offer things like validation chaining.  So you might have a basic rule that validates the length of a textbox in characters, but then it also might only be valid if it has some text, and even a minimum number of characters, and it also might require that the input have the name of at least one fruit in it.  When you start writing this code yourself, big function that is also going to have to associate an errror message with each individual problem, not to mention that it would be better if the form showed all the errors for that column as well as any other columns that they needed to change, rather than only displaying one error. 

 

That's the tip of the iceberg in terms of talking about why people use frameworks and in this particular case, form classes.  These classes can also be based on the actual database strucure, so that the form knows in advance how to pull data out of the model layer (from the database) and to enforce basic schema level validations without you having to know anything.  Symfony really makes this easy because it will generate base form classes from the schema file, and you can derive from those classes.  The big win is that you have a built-in form->save() method that already knows how to take a form with some changes, and save it back to the original table, even when that table has a number of related tables.

 

ZF has hooks to do the same things, but again it requires you set a lot of this up yourself via configuration.  I should probably say for the record that the current phpfreaks main site, was written using ZF, with the data being pulled from mysql using doctrine integration.

 

 

 

 

[php-beginner]

Thankyou for your explanation. Do you think it is wise to use a framework for a newb like me. I mean, three weeks ago I started learning OO in php.

 

And, why isn't it wise to use exception for wrong login input. I found an article that says that there are no rules for that. Or am I wrong?

[gizmola]

I don't think it's a problem to be new and use a framework. In a lot of ways it's a great idea because you'll learn a lot of the best practices from working within the framework, and emulating the structure and design they use. 

 

The question of exceptions, like many things is a judgement call, but in my experience you use a try ... catch block because you are doing something that has a flow, and you expect that in most cases it will complete successfully, but there are a variety of unexpected conditions usually embedded within routines called in the flow, that could go wrong, and you want to be able to deal with those "exceptions" gracefully. 

 

In my opinion, you're processing a login form, and that's a basic expectation of the task -- that logins fail, and a failed login should in no way require the equivalent of an application interrupt, that drops you out of the application flow and into the exception handling flow.

 

 

[ignace]

Like gizmola already pointed out error/exception handling and input handling are 2 different things. You don't have to crash your application because someone typed in wrong credentials. Instead, if using a Form class you could add the message to the form telling the user what went wrong or use some general messaging class to inform the user of your application's state: "wrong credentials", "profile updated", ...

 

If you are using an MVC approach you can add it directly to your view:

 

$view->showMessage('username/password match does not exist.');

[php-beginner]

Thankyou for your reactions.

 

I think I will continue this script without the use of a framework. Maybe I'll use it in the future.

About the general message class: I could make a message class where I bound messages to a variable.

But, then I have to echo them in other classes right? And echo shouldn't be used in classes. Or am I wrong?

 

Like:

 

if(login){then true} else{echo $this->error->message;}

[ignace]

Whether you use a framework or not. Your code should always have a clear separation between retrieving and transforming and displaying information. Your application should transform the display to inform the user of it's state which could be as something simple as:

 

class MyHTMLPage {
    public function showDialog($title, $message) {
        // do stuff
    }
}

 

$html->showDialog('No such username.');

[php-beginner]

class Message{

private $message;

public function showDialog($message){
	$this->message = $message;
	return $this->message;
}
}

 

if(empty($username) || empty($password)){
		$this->message->showDialog('epic failure');
	}

 

If I echo, it works. But that's not allowed.

[ignace]

Why wouldn't you be allowed to use echo in your view or UI-layer? An application is divided up into 4 layers: Presentation, Application, Model, and Infrastructure.

 

Presentation is your UI (HTML/CSS and the PHP to echo the passed variables or your template).

Application knows about the Model and Presentation layer and makes the decisions regarding your application flow (like: performing file upload after form submission)

The Model is the one who will actually perform the work (validating the upload, storing it in some directory, etc..)

Infrastructure is your DB/Frameworks, everything that does not belong in the above 3 layers

 

If you view an application as 4 different layers you'll be able to better determine what you can or "can't" do. It wouldn't make sense to echo stuff in your Application layer because why would you otherwise need the Presentation layer?

[php-beginner]

Thankyou for your explanation.

 

From what I've read, return is used inside the class and echo is used outside the class. So I'm a bit confused now ^^

[ignace]

You should use return because it's possible you may want to modify the data before it's send to the browser. That said, a class can echo stuff (eg Zend_View "renders" the HTML by calling echo)