5
\$\begingroup\$

I am relatively new to programming and OOP in PHP. I tried to create a Simple Login Register Script using my basic knowledge of OOP. I'm sure my code can be better in a lot of way. I'm trying to code better and learn new things.

Be harsh, please find out as many small noob mistakes as you can. Suggestions and Feedbacks are always welcomed !

Config.php

<?php 
class dbConfig {
   public $host;
   public $username;
   public $password;
   public $dab;
   public $conn;

public function dbConnect() {
    $this->conn = mysqli_connect($this->host,$this->username,$this->password);

    if (!$this->conn) {
        die("Connection failed: " . mysqli_connect_error());
    }
    else{
        echo "Connected successfully to server";
    }

    $db_selected = mysqli_select_db($this->conn, $this->dab);

    if (!$db_selected) {
        // if the given database doesn't exists
        // creates new database with that name
        $db_sql = 'CREATE DATABASE chatapp';

        // verify the database is created
        if (mysqli_query($this->conn, $db_sql)){
            echo "Database chatapp already exists or created successfully\n";
        } else {
            echo 'Error creating database: ' . mysqli_error() . "\n";
        }
    }

    // creating tables
    $table_sql = "CREATE TABLE IF NOT EXISTS users (".
            "uid INT PRIMARY KEY AUTO_INCREMENT,".
            "username VARCHAR(30) UNIQUE,".
            "password VARCHAR(50),".
            "name VARCHAR(100),".
            "email VARCHAR(70) UNIQUE); ";

    // verify the table is created
        if (mysqli_query($this->conn, $table_sql)) {
            echo "Table: users already exists or created successfully\n";
        } else {
            echo 'Error creating table: ' . mysqli_error($table_sql) . "\n";
        }
}
}

$obj = new dbConfig();

$obj->host = 'localhost';
$obj->username = 'root';
$obj->password = '';
$obj->dab = 'chatapp';
$obj->dbConnect();

login.php

<?php
include('config.php');
session_start();

if($_SERVER["REQUEST_METHOD"] == "POST")
{
// username and password sent from Form
$emailusername = mysqli_real_escape_string($obj->conn,$_POST['emailusername']); 
$password = mysqli_real_escape_string($obj->conn,$_POST['password']); 
$password = md5($password);

$sql="SELECT uid FROM users WHERE username='$emailusername' or email = '$emailusername' and password='$password'";
$result=mysqli_query($obj->conn,$sql);
$row=mysqli_fetch_array($result,MYSQLI_ASSOC);
$active=$row['active'];
$count=mysqli_num_rows($result);


// If result matched $username and $username, table row must be 1 row
if($count==1)
{
$_SESSION['login_user'] = $emailusername;
header("location: welcome.php");
}
else 
{
$error="Your Login Name or Password is invalid";
}
}
?>
<<!DOCTYPE html>
<html>
<head>
    <title>Login</title>
</head>
<body>
    <form action="login.php" method="post">
        <label>UserName or Email:</label>
        <input type="text" name="emailusername"/><br />
        <label>Password :</label>
        <input type="password" name="password"/><br/>
        <input type="submit" value=" Submit "/><br />
    </form>
</body>
</html>

register.php

    <?php
include('config.php');
if(isset($login_session))
{
header("Location: login.php");
}
if ($_SERVER["REQUEST_METHOD"] == "POST") 
{
$username = mysqli_real_escape_string($obj->conn,$_POST['username']); 
$password = mysqli_real_escape_string($obj->conn,$_POST['password']); 
$name     = mysqli_real_escape_string($obj->conn,$_POST['name']); 
$email    = mysqli_real_escape_string($obj->conn,$_POST['email']); 

$password = md5($password);

$sql ="SELECT uid from users WHERE username = '$username' or email = '$email'";
$register_user = mysqli_query($obj->conn,$sql) or die(mysqli_error($sql));
$no_rows = mysqli_num_rows($register_user);

if($no_rows == 0)
{
    $sql2 = "INSERT INTO users(username, password, name, email) values ('$username', '$password', '$name', '$email')";
    $result = mysqli_query($obj->conn, $sql2) or die(mysqli_error($sql2));
    echo "Registration Successfull!";
}
else{
    echo "Registration Failed.";
}
}
?>
<!DOCTYPE html>
<html>
<head>
    <title>Register</title>
</head>
<body>
    <form action="register.php" method="post">
    <label>UserName or Email:</label>
    <input type="text" name="username" required/><br />
    <label>Password :</label>
    <input type="password" name="password" required/><br/>
    <label>Full Name :</label>
    <input type="text" name="name" required/><br/>
    <label>Email :</label>
    <input type="email" name="email" required/><br/>
    <input type="submit" value=" Submit "/><br />
    </form>
</body>
</html>

logout.php

<?php
session_start();
if(session_destroy())
{
header("Location: login.php");
}
?>

welcome.php

<?php
include('lock.php');
?>
<html>
<head><title>Home</title>
</head>
<body>
<h1>Welcome <?php echo $login_session; ?></h1>
</body>
</html>

lock.php

<?php
include('config.php');
session_start();
$user_check=$_SESSION['login_user'];

$ses_sql=mysqli_query($obj->conn,"SELECT username FROM users WHERE username='$user_check' ");

$row=mysqli_fetch_array($ses_sql,MYSQLI_ASSOC);

$login_session=$row['username'];

if(!isset($login_session))
{
header("Location: login.php");
}
?>

While copy-pasting the code here, I was getting feeling that I made the code a bit more lengthier for such a small task. Please let me know how to minify the code and other various aspects of my code.

\$\endgroup\$
5
\$\begingroup\$

At first glance

After a quick glance at your code, it reminded me of this review I posted a while back. Much like that piece of code, a couple of (broadly similar) issues jumped out:

  • Wrapping procedural style code in a class with some methods does not qualify as OOP.
  • Your one and only method (dbConnect) is fundamentally flawed and unsafe. Not in the least because it relies on the user (the code that creates an instance of your class) to set the properties manually before calling the method. What if these properties aren't set? create a __construct method (constructor) that requires the user to pass the connection params to the class when an instance is created, much in the same way that mysqli or PDO require these params to be passed.
  • There is some sanitation of values used in your query, but you really ought to look into prepared statements
  • Methods don't echo, and they certainly don't die. If they encounter a problem, they throw new RuntimeException, or some other exception that accurately describes the problem (InvalidArgumentException, BadMethodCallException, ...).
  • Your dbConnect method just does too much. Its name suggests it merely connects, when in reality, it echoes, creates tables and never closes the DB connection. Calling dbConnect twice is possible, your code doesn't take that into account.
  • Coding standards are important. PHP doesn't have any official standards (yet), but the PHP-FIG standards are adopted by all major players (Symfony, Zend, Apple, Composer, CakePHP, ...). You'd do well following them, too
  • md5, as a hash, is no longer secure. It first started showing weaknesses back in 1998, was deemed unsafe about ten years ago, and back in 2010, a single-block collision was found. At the very least, use sha1, but even so: PHP supports blowfish, sha256 and sha512 just as easily.
  • When hashing passwords, you'd also do well using salt. Don't just hash the password as-is. Add some random string that only you know. Even so, PHP has a very useful, easy to use and helpful function called password_hash. Use it.
  • You seem to be confused as to the point of OOP. In PHP, and many other languages, OOP isn't used to reduce the size of your code base (look at Java, or even C++). It's about making a sizable code base more manageable, more structured. Yes, when you first start writing OO code, you'll find yourself writing a lot more code than you were used to. The benefits are that, after a while, you'll find yourself re-using bits of code that you had written for another project. OOP enable (or at least facilitates) abstraction, separation of concerns and force developers to think more about what task should go where in the application. If you want to write OO code, don't go looking for ways to reduce the amount of code you write, look for ways to improve the overall structure of your project, and optimize what component has access to what functionality at any given time.

I'll be going into all of these issues in detail, each time updating this answer, but for now, I'd urge you to check out the links I provided here, and google some OOP terms like the SOLID principles

If you want to avoid lengthy code at all costs, then perhaps you should consider learning another language than PHP. Perl, for example, allows you to write obfuscated code a lot easier. Mind you, PHP has a couple of tricks up its sleeve, as this "Hello world" demonstrates (source):

<?=${[${[${[${[${[${[${[${[${${![]}.=[]}.=${![]}{!![]}]}.=${!![${[${[${[${[${[${[${[]}++]}
++]}++]}++]}++]}++]}++]}{![]+![]+![]}]}.=${[${[${[${[]}++]}++]}++]}{![]+![]}
.${![]}{![]+![]+![]}]}.=${[${![]}=${![]}{!![]}]}{!![${!![${!![${![]}++]}++]}++]}^
${!![${[${[${[]}++]}++]}++]};

Update 1
As you requested, some more details on the second issue I mentioned (about the dbConnect method being flawed). Your code, as it stands now expects the user to write something like:

$obj = new dbConfig();
$obj->host = '127.0.0.1';
$obj->username = 'user';
$obj->password = 'pass';
$obj->dab = 'dbname';
$obj->dbConnect();

But what if, because most devs are lazy, someone writes:

$db = new DbConfig();//classes should start with an UpperCase
//some code
someFunction($db);
//where someFunction looks like:
function someFunction(DbConfig $db)
{
    $db->dbConnect();//<== ERROR! host, user, pass... nothing is set
}

The instance could be created in a different file as the line that is calling dbConnect, so debugging this kind of code is hellish. You could re-write each piece of code that calls dbConnect to make sure all of the required properties have been set correctly, but what if they aren't? And what is "correct"? That's something the instantiating code should know, not the code that calls dbConnect. Luckily, you can use a constructor to ensure all parameters are known:

class DbConnect
{
    /**
     * @var string
     */
    protected $host = null;

    /**
     * @var string
     */
    protected $username = null;

    /**
     * @var string
     */
    protected $pass = null;//null for no pass

    /**
     * @var string
     */
    protected $dbName = null;

    /**
     * @var mysqli
     */
    protected $conn = null;

    public function __construct($host, $user, $pass = null, $db = null)
    {
        $this->host = $host;
        $this->user = $user;
        $this->pass = $pass
        $this->dbName = $db;
    }

}

Now, whenever an instance is created, the host and user must be passed to the constructor, if not, PHP will throw up a fatal error. So you'll have to use the class like so:

$db = new DbConnect('127.0.0.1', 'root', '', 'chatapp');

This reduces the users' code (where your class is being used), and makes the instances behave more predictably.

You may have noticed that I've changed the visibility from public to protected. That's because public properties can be reassigned on the go, without any validation on their new value whatsoever. That's dangerous. Instead, I suggest you add some getters and setters for the properties that you want to expose (like the database):

public function setDbName($name)
{
    if (!is_string($name))
    {//dbName MUST be a string, of course, so if it isn't, notify the user
        throw new InvalidArgumentException(
            sprintf(
                '%s expects name argument to be a string, instead saw "%s"',
                __METHOD__,
                gettype($name)
            )
        );
    }
    $this->dbName = $name;
    return $this;
}

Using this method, you can also check if the instance is currently holding an active db connection, clean up whatever needs to be cleaned up (closing cursors, freeing results, ...) and (re)connect or select the new db on that connection.
On the user side, using these setters is as simple as:

$instance->setDbName('foobar');
//but:
$instance->setDbName(array());
//InvalidArgumentException
//message: DbConnect::setDbName expects name argument to be a string, instead saw "array"

Given that your class clearly tries to abstract the nitty-gritty of the rather messy mysqli API away from the user (which I can understand), it's also important that the user needn't worry about closing the connection in time. When your instance of DbConnect goes, then so must the DB connection itself go. For that, a simple desctructor would do the job just fine:

public function __destruct()
{
    if ($this->conn instanceof mysqli)
        this->conn->close();//or mysqli_close($this->conn);
    $this->conn = null;//optional
}

Now you can rest assured that, in case someone writes this:

$db = new DbConnect('127.0.0.1', 'root');
$db->dbConnect();
//...code here
$db = null;//or unset($db), or $db just goes out of scope

The connection will be closed automatically. Naturally, for all of these changes to take effect, you'll have to refactor your dbConnect method somewhat, but that's something I'll try to cover in the next update.

\$\endgroup\$
3
  • \$\begingroup\$ Yes, I was really confused of OOP. Thank you for your comment. Well to be honest, I wasn't aware of the real objective of OO code.I would love to hear more from you. for now can you explain 2nd point a bit more ? Thank you for your time. \$\endgroup\$ – Rajeeb Dec 30 '14 at 11:47
  • \$\begingroup\$ @RajeebTheGreat: You're welcome. I will update this answer, and focus on your code a bit more. When I find the time (later today probably), I'll post an update explaining the 2nd point a bit more, and providing some suggestions as to how to approach things \$\endgroup\$ – Elias Van Ootegem Dec 30 '14 at 12:09
  • \$\begingroup\$ @RajeebTheGreat: Added the first of, probably, many updates just now, focusing mainly on the second issue I pointed out \$\endgroup\$ – Elias Van Ootegem Dec 30 '14 at 15:38
2
\$\begingroup\$

@Elias Van Ootegem already covered quite a lot, so just a couple of points from me:

Security: Login as any user

Right now, an attacker does not need to know the password of a user to login, just the username. This is because of the precedence of and over or. Your query is executed as:

$sql="SELECT uid FROM users WHERE username='$emailusername' or (email = '$emailusername' and password='$password')";

But what you want is:

$sql="SELECT uid FROM users WHERE (username='$emailusername' or email = '$emailusername') and password='$password'";

Creating Database and Table

Creating the database and tables should not be part of the database connection/configuration class. You don't want this to execute on every connection, but just once when you set up your website. Create a separate setup script.

This has the additional advantage of simplifying your dbConfig class. Right now, it stores the database configuration, connects to the database, and sets up tables (in addition to echoing and dying, which no class should do). That's too much responsibility for one class, so it's good to extract code from it.

Misc

  • try to use correct indentation, it makes your code a lot easier to read.
  • be consistent with your formatting (eg spacing before and after =, etc).
  • use prepared statements instead of mysqli_real_escape_string, as it's a lot easier to use correctly (no forgotten single quotes for numerical values, etc).
\$\endgroup\$
1
  • \$\begingroup\$ I don't know how I made that silly mistake. Thanks a ton for pointing out that. I'm on the way to refactor dbConfig class. \$\endgroup\$ – Rajeeb Jan 1 '15 at 9:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.