HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Login script for an application

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scriptloginforapplication

Problem

I am currently developing a login script for my application. The login will use SSL and all required resources will be served through this. It is not protecting anything like a bank however I would like to know what is right and wrong especially for learning purposes

I would love some feedback on my class that I have developed. I have been reading various sources on the net and a lot seems to be contradictory. I would love to know if there is anything else I can do with regard to this code and also the login process as a whole

Areas I feel need improvement:

  • Use something stronger than sha1 for storing passwords.



  • Maintaining login - currently it times out after 20 minutes.



```
class User extends Model{

private $logLocation;
private $loginLog;

public function __construct(){
$this->logLocation = 'system/logs/';
$this->loginLog = "logins";
}

/**
*
* Add User
* @param array $data An array of data that will get added to User table.
*/
public function add($data){
$db = Database::getInstance();

$salt = substr(md5(uniqid(rand(), true)),0,3);

$query = 'INSERT INTO user( user_id, user_username, user_password, user_salt, user_forename, user_lastname, user_email, user_attempts)
VALUES( :user_id, :user_username, sha1(:user_password), :user_salt, :user_forename, :user_lastname, :user_email, 0)';
$args = array(
':user_id' => $data['user_id'],
':user_username' => $data['user_username'],
':user_password' => $data['user_password'].$salt,
':user_salt' => $salt,
':user_forename' => $data['user_forename'],
':user_lastname' => $data['user_lastname'],
':user_email' => $data['user_email']);
$db->query($query, $args);

SessionRegistry::instance()->addFeedback('user Saved Successfully');
return true;
}

public function getUserId($username){
$db = Database::getInstance();
//Check to see if the username exists
$query = "SELECT user_id FROM user WHERE user_usernam

Solution

Like Fanis said in his comment, you might want to put error checking on both your getUserId() and getUsername().

Good code overall (except maybe the singleton for your database object), few minor things like replacing rand() with mt_rand(). If you're worried about the security, you could check out crypt() too.

Although getting user IP's is never 100% reliable you shoulud to look into using something like this function for getting the IP of a user.

Context

StackExchange Code Review Q#3308, answer score: 2

Revisions (0)

No revisions yet.