2
\$\begingroup\$

I am currently making a very big blogging system for my Website where I want really neat code! So, I was wondering: is this method safe/does it have any problems with security? Yes, I'll be encrypting the password anyway. I also want to know if there is a better way to achieve the same idea. Also any tips on implementing high security and high performance?

<?php
class Users {
    protected $db;
    public $id;
    public $username;
    public $password;
    public $lastlogin;
    public $email;
    public $rank;
    public $creationdate;
    public $country;
    public $status;

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

    public function login($username, $password) {
        $query = $this->db->prepare("SELECT username, password FROM blog_users WHERE username = ? AND password = ? LIMIT 1");
        $query->bind_param('ss', $username, $password);
        $query->execute();
        $query->bind_result($username, $password);
        $query->store_result();
        if($query->num_rows == 1) {
            while($query->fetch()) {
                $userData = getData($username);

                $this->id             =   $this->userData['id'];
                $this->username       =   $username;
                $this->password       =   $password;
                $this->lastlogin      =   time();
                $this->email          =   $this->userData['email'];
                $this->rank           =   $this->userData['rank'];
                $this->creationdate   =   $this->userData['creationdate'];
                $this->country        =   $this->userData['country'];
                $this->status         =   $this->userData['status'];

                $_SESSION['loggedin'] = 1;
                return true;
            }
        } else {
            return false;
        }
        $query->close();
        $query->free_result();
    }

    public function getData($username) {
        $query = $this->db->prepare("SELECT id, email, rank, creationdate, country, status FROM blog_users WHERE username = ?");
        $query->bind_param('s', $username);
        $query->execute();
        $query->bind_result($id, $email, $rank, $creationdate, $country, $status);
        $query->store_result();
        $userData = array(
            'id'           => $id,
            'email'        => $email,
            'rank'         => $rank,
            'creationdate' => $creationdate,
            'country'      => $country,
            'status'       => $status
        );
        return $userData;
        $query->close();
        $query->free_result;
    }

}
?>
\$\endgroup\$
  • 2
    \$\begingroup\$ Please read the help section of this site: you are asked to post actual, working code... the constructor you've posted can't possibly work!. $this->db = $db; when $db is not in scope, typo's: (like eles instead of else). Debug your code first, before you can get it reviewed \$\endgroup\$ – Elias Van Ootegem May 12 '14 at 13:39
  • 1
    \$\begingroup\$ I've seen the $db, but still: the code, as posted here, doesn't work. You are required to post working code to get a review here. That was my point \$\endgroup\$ – Elias Van Ootegem May 12 '14 at 13:41
  • 1
    \$\begingroup\$ Nope, it can't be working as you expect it to: in your login method, there's a call to the getData method, but you wrote getData($username); when it should've been $this->userName \$\endgroup\$ – Elias Van Ootegem May 12 '14 at 13:50
  • 1
    \$\begingroup\$ Why are you so harsh? :l Won't work.. won't work.. won't work... You can always point out errors. Actually, I thought CodeReview at StackExchange was actually good, but CodeReview is also filled with big headed people who think they're the best. \$\endgroup\$ – Abandoned Account May 12 '14 at 13:59
  • 3
    \$\begingroup\$ Well, that's because, IMO codereview has to be tough, to be good. I've explained my viewpoints on the matter here. It's important to keep in mind that, though critiques can be harsh, the only thing we (the people reviewing your code) are trying to do, is to help you, not to hurt you \$\endgroup\$ – Elias Van Ootegem May 12 '14 at 14:01
4
\$\begingroup\$

Ok, just a few quick thoughts, will be adding to this answer later today (ie after work):

One class, One task
Chances are, you've already heard of the SOLID principles. Well, here they are again: your code is in violation of the SRP (Single Responsability principle). A class has one task. No more, no less. Your class does 4 things: it stores data (all those public properties), messes with sessions, connects to the DB and performs queries.
The Database connection and queries are all things that should be dealt with in another class, that simply connects to the DB and executes queries.

This other class could then have a method like this:

public function login(User $user)
{
    $query = $this->db->prepare("SELECT username, password FROM blog_users WHERE username = ? AND password = ? LIMIT 1");
    $query->bind_param('ss', $user->username, $user->password);
    $query->execute();
    $query->bind_result($user->username, $user->password);
    $query->store_result();
    if($query->num_rows == 1)
    {
        $query->fetch();
        $query->close();
        return $this->getUserData($user);//changed name of method
    }
    $query->close();
    throw new RuntimeException('User can not login');//error
}

Even this isn't close to efficient code, because it still calls a second method to, basically, repeat the same query. Why would you do so, why not simply write:

public function login(User $user)
{
    $query = $this->db->prepare(
        "SELECT id, email, rank, creationdate,
            country, status, username, password
        FROM blog_users
        WHERE username = ? AND password = ? LIMIT 1"
    );
    $query->bind_param('ss', $user->username, $user->password);
    $query->execute();
    $query->bind_result(
        $user->id,
        $user->email,
        $user->rank,
        $user->creationdate,
        $user->country,
        $user->status,
        $user->username,
        $user->password
    );
    $query->store_result();
    if($query->num_rows == 1)
    {
        $query->fetch();
        $query->free_result();//call free_result...
        $query->close();//and close here, too: your code doesn't do this
        return $user;
    }
    $query->free_result();
    $query->close();
    throw new RuntimeException('User can not login');//error
}

Isn't that a lot shorter, clearer and doesn't this also do exactly what you are doing? K.I.S.S!

Classes with a dependency, should be open to injection
Your current code is hard to test: the constructor connects to the DB, there is no way for me (the user), or you (when testing) to mock the db connection, or use a bogus testing table.
The best way to make your class easier to test it to allow the user to pass a specific connection to the constructor, and then he can use your class/functionality to work on the db of his choice:

public function __construct(mysqli $connection = null)
{//default null means the argument is optional
    if ($connection === null)
        $connection = new mysqli();//connect to default
    $this->db = $connection;//use connection we've created, or the one the user passed through to the constructor
}

And of course: a DB connection that is opened, should be closed: when your class dies (is destructed), the DB connection should die, too:

public function __destruct()
{
    $this->db->close();
    $this->db = null;
}

That should be added to your code, too. If you don't then the connection will be kept open until your script has terminated, which could be longer than you want. If performance and cleanliness matter to you, then this should be a habit.

Standards, coding style
Yes, I complain/nag a lot about coding style and standards but if you are, like you say, creating a sizeable code-base, then you should know that coding style and standards are insanely important.
The unofficial PHP standards can be found here. Though unofficial, all major players (ie Zend, Symfony, Cake, Doctrine, ...) all subscribe to them.

Other niggles:
Your class has tons of properties, and it's those properties that you are assigning in the methods. Why, then aren't you assigning $username as soon as you can, so you don't have to pass it to the getData method as an argument?

Another remark I have is simply this:
If a script contains only PHP code, then that script should not have a closing ?> tag. Get rid of it.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ @HassanAlthaf: No, the other way around: the User class should be a simple data container. That data can be passed around from the core (the model layer, or the place where your logic resides... whatever you call it), all the way through to the view. The DB connection should not be passed to the view. Ever. This DB class should have methods like findUser(User $user), to which you pass this Data object, and it's that class that queries and returns new User instances \$\endgroup\$ – Elias Van Ootegem May 12 '14 at 14:27
  • \$\begingroup\$ Are you talking about Model View Controller? And I'm seriously impressed on the Namespaces, can you give me a good beginner tutorial on them? I think they'll be seriously useful for me. \$\endgroup\$ – Abandoned Account May 12 '14 at 14:29
  • \$\begingroup\$ @HassanAlthaf: I'm not really talking about MVC (not as such, anyways), just common sense: User represents a User's data, so it shouldn't perform queries. Either way, though unrelated, Here's an answer on IoC and DI I've posted a while back. If you want to learn about namespaces, then this is all you really have to know, and then experiment, because they can indeed be useful in larger projects \$\endgroup\$ – Elias Van Ootegem May 12 '14 at 14:33
  • \$\begingroup\$ Yes, my project is going to be a large one! Would you like to stay on touch with me via Skype? \$\endgroup\$ – Abandoned Account May 12 '14 at 14:37

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.