help on user authorization

[eluser]Johnny Freeman[/eluser]
Hello, I am fairly new to CI and want to custom build a user authorization section for the backend section of a site that i am making. I already know that there are already plugins built for this task. However, I have chosen to do it myself.

I am seeking feedback on the following code about how I could improve on security AND most importantly I want to make sure i am doing it properly according to CI standards. I have already tested the code so far and it works just fine. Also, keep in mind that this is far from being complete, just curious to see what the community thinks and make sure I am on the right track.

This is the login controller function:

Code:

function login()
    {
        $this->load->model('user');

        $user = $this->input->post('username');
        $pass = $this->input->post('password');
        $rmbr = $this->input->post('rememberme');
        
        if ( $this->user->doesexist($user) && $this->user->passwordiscorrect($user, $pass) ) :
            echo "true";  
        else :
            echo "false";
        endif;
            
    }

And, this is the user model:

Code:

function doesexist($user)
    {
        ############# This function checks ONLY to see if a user exists #############
        
        /*********************************
         * function to retrieve the user
         *********************************/
        
        $query = $this->db->get_where('users', array('username' => $user ) );
        
        /*********************************
         * if the user exists, return TRUE
         *********************************/
        
        if ( $query->num_rows() > 0 )
         { return TRUE; }
        
        /*********************************
         * if not, return FALSE
         *********************************/
        
        return FALSE;
    }
    
    #############################################################################################
    
    function passwordiscorrect($user, $pass)
    {
        ############# This function checks if the given password matches the given username #############
        
        /*********************************
         * function to retrieve the user's data
         *********************************/
        
        $query = $this->db->get_where('users', array('username' => $user ) );
        
        /*********************************
         * encrypt the given password so we can match it to the real password
         *********************************/
        
         $pass = md5($pass);
        
         /*********************************
         * if the password matches, return TRUE
         *********************************/
        
        $row = $query->row_array();

        if ( $pass == $row['password'] )
        {
            return TRUE;
        }

        /*********************************
         * if not, return FALSE
         *********************************/
        
        return FALSE;
    }

I appreciate all of your comments and I look foward to this new learning experience!

[eluser]Colin Williams[/eluser]
Well, right away it is clear that you are running two identical queries, something you should never have to do. Consider caching results in a property of your model. This means you'll always check your cache property first, then run the query if you don't find it there. You'll also need to remember to update your cache when the data is manipulated (per request only.)

Prototype:

Code:

class User extends Model {
  var $users = array();

  function get($uid)
  {
    if (!isset($this->users[$uid]))
    {
      // 1) Query the db: $user = $this->db->get() ...
      // 2) Add result to $this->users cache
    }
    else
    {
      // $user = $this->users[$uid]
    }
    return $user;
  {
}

Also, I see no need to run two checks to do the login. You could simply have an authenticate() method that returns false on the conditions a) there is no such user in the database, or b) the password for the user is wrong.

[eluser]Bramme[/eluser]
Dunno if this is of any use for you, but I've always used a query like

Code:

if(num_rows(mysql_query(SELECT * FROM users WHERE username = $username AND password = $password)) == 1) {
return true;
};

Much faster if you ask me... (should do a benchmark though)

[eluser]Johnny Freeman[/eluser]
Colin:
Thanks for the pointer about caching although I have to admit that (slapping myself on the hand) I don't fully understand exactly how it's all done. I will do some research on it. Also, the reason that I created two seperate functions for the check is so that I could also use them later. Example: When a user is registering for a new acount i can use the $this->user->doesexist() function to see if the username is availible or not.

However, would it be bad practice to do something like this:

Code:

function isauthorized($user, $pass)
    {
    
        if ( $this->doesexist($user) && $this->passwordiscorrect($user, $pass) )
        {
        
            return TRUE;
        
        }
        
        return FALSE;
    
    }

And, if this is acceptable should I put it in the controller and make a _private_funtion() or just leave it in the model.

Bramme:
Your suggestion would be perfect if i were coding with simplicity in mind. I'm not sure if it is faster as far as loading time goes, but it's definitely faster for typing the code. So thanks.

To All:
Thank you again for your suggestions and I am excited to hear more from you.

[eluser]loathsome[/eluser]
Why would you check if the user exist AND if the password is correct at the same time? Just running "passwordiscorrect" would be sufficient enough.

[eluser]Johnny Freeman[/eluser]
You're absolutely correct, Thank you.

[eluser]Unknown[/eluser]
Hi All,

I am new to CI and want to create a user login module, I will be using the code above. Now My question is that is this a right way or there are other ways around also :question:


Thanks,

Nauman

[eluser]Johnny Freeman[/eluser]
I'm sure there are alternative ways of achieving this goal. But the code above will safice. I used it for a while and eventually rewrote it into a library. Have fun!

Johnny