patternphpMinor
Login script for an application
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:
```
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
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
Good code overall (except maybe the singleton for your database object), few minor things like replacing
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.
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.