Login system in PHP using AJAX

Question

I am beginning the cycle of creating my role based access control into my framework. I now want to log the user into my application and my _user table looks like this:

CREATE TABLE `_users` (
 `id` int(11) NOT NULL AUTO_INCREMENT,
 `username` varchar(255) NOT NULL,
 `hash` varchar(255) NOT NULL,
 `session` varchar(255) DEFAULT NULL,
 PRIMARY KEY (`id`)
)

I create my user like so in my model, where bin2hex() is my IV / salt.

Database::getInstance()->Prepare( 'INSERT INTO _users (username, hash, session) VALUES (?, ?, ?)' )
 ->execute( array(
 'Test',
 password_hash( 'Test', PASSWORD_BCRYPT ),
 md5( time() . bin2hex( random_bytes( 32 ) ) )
 ) );

To then log into the user, I use a method. The method I use in the request is POST because my application has built in CRSF tokens. If the CRSF token is passed, then a xauth_protected aliased class is instanced which ensures that X-Auth header exists and is of md5(session_id()) to ensure that no cross-site-origin requests can be made.

public function login( Request $request )
{
 $this->middleware( 'json_response', $request );
 $this->middleware( 'xauth_protected', $request );
 
 if( !$request->has( 'username' ) || !$request->has( 'password') )
 {
 echo json_encode( array( 
 'status' => FALSE,
 'reason' => 'Please fill in the required fields...',
 ) );
 return;
 }
 
 $stmt = Database::getInstance()->Prepare( 'SELECT id, hash, session FROM _users WHERE username = ? LIMIT 1' );
 $stmt->execute( array( $request->username ) );
 $row = (object) $stmt->fetch();
 
 if( isset( $row->id ) )
 {
 if( password_verify( $request->password, $row->hash ) )
 {
 // $_SESSION['oath'] = $row->session
 // This is used to find the user that is logged in, if it is set
 $request->setSession( 'oauth', $row->session );
 
 echo json_encode( array( 
 'status' => TRUE,
 ) );
 
 return;
 }
 
 echo json_encode( array( 
 'status' => FALSE,
 'reason' => 'Invalid credentials...',
 ) );
 
 return;
 }
 
 echo json_encode( array( 
 'status' => FALSE,
 'reason' => 'Sorry, that username was not found in our records...',
 ) );
}

To then test this method, I can use:

const formData = new FormData();
 
formData.append( 'crsf_token', sessionStorage.getItem( 'token' ) ); // CRSF token is set elsewhere
formData.append( 'username', document.getElementById( 'username' ).value ); // Example input field
formData.append( 'password', document.getElementById( 'password' ).value ); // Example input field
fetch( App.__viewFactory.homepage.login, { // This is a prestored route that looks like /oauth/login
 method: 'POST',
 headers: {
 'X-Auth': sessionStorage.getItem( 'session' ) // md5( session_id() ) which is set elsewhere
 },
 body: formData
} )
.then( response => response.json() )
.then( data => {
 if( data.result ) {
 window.location.href = App.__viewFactory.dashboard.view; // prestored route to the dashboard
 return;
 }
 
 document.getElementById( 'login-error' ).innerHTML = data.reason;
} )
.catch(error => {
 document.getElementById( 'login-error' ).innerHTML = 'Oh no! Something went wrong.';
});

I have incorporated CRSF protected, custom headers using the session_id() to prevent cross-origin requests (since my framework rewrites all requests to my index page, this then turns on CORS). Is everything I am doing on the database side as secure? Is there something I could be doing better?

• Mast: "Is everything I am doing on the database side as secure?" Secure against what/who?
• Your Common Sense: what is session?
• Jaquarh: session is just the output of md5(session_id()) - it probably has absolutely no adversary effect but deception is a key defence principle even if so. These are set on a .tpl file which my page executes. I can show that if needed @YourCommonSense
• Your Common Sense: I am sorry, what?
• Jaquarh: I appreciate you want me to expand on 'secure' but I am trying to generalise the noun. Am I doing everything correctly is what I am trying to ask, what is best practice, am I generating my sessions correctly etc etc @Mast

Answer 1

Your querying practices look secure to me. Your are using a prepared statement with bound parameters and using password_hash() and password_verify(). It really can be as simple as doing those basic/essential things.

As for other refinements, I recommend writing the failure branches before successful ones, doing early exit()s, and only passing back an empty or populated reason to reduce the data structure to its vital value.

• I will assume/hope that the username is a UNIQUE table column, so LIMIT 1 provides no value.
• If you want the result set / row to be an object, just tell pdo that that is what you want -- fetch(PDO::FETCH_OBJ).
• The result set will either be an object or false, so just check for a falsey result set. For this reason, you can remove id from the SELECT.
• I don't like to give too much specificity when giving failed login responses. I would tell the user that the credentials generally failed without spelling out which field was the problem.

Recommendation:

public function login(Request $request)
{
 $this->middleware('json_response', $request);
 $this->middleware('xauth_protected', $request);
 
 if(!$request->has('username') || !$request->has('password')) {
 exit(json_encode(['reason' => 'Please fill in the required fields']);
 }
 
 $stmt = Database::getInstance()->Prepare(
 "SELECT hash, session FROM _users WHERE username = ?"
 );
 $stmt->execute([$request->username]);
 $rowObject = $stmt->fetch(PDO::FETCH_OBJ);
 
 if (!$rowObject || !password_verify($request->password, $rowObject->hash)) {
 exit(json_encode(['reason' => 'Invalid credentials']));
 }
 $request->setSession('oauth', $rowObject->session);
 exit(json_encode(['reason' => null]));
}

Then in your js, you can use this:

.then(data => {
 if (data.reason) {
 document.getElementById('login-error').innerHTML = data.reason;
 } else {
 window.location.href = App.__viewFactory.dashboard.view;
 }
})

• Jaquarh: It is nice to see that, even though there may be no direct server security issues, that the thought of brute forcing ties in with the ability of giving the user a specific output rather than generalising it. I think I should perhaps integrate an "attempts" into it also. I appreciate your time in writing this, its nice to know I can use fetch(PDO::FETCH_OBJ) without having to cast the array to an object. The reason I do not use exit() is because the middleware in my script runs functions after the route has executed which could be things like re-generating the CRSF token for POST requests
• Jaquarh: It would be nice to add you as a collaborator to see your opinion of the framework so far
• mickmackusa: Sorry, but due to my 9 to 5 dev job that is rarely only 9 to 5, my Stack Exchange addiction (can't argue that it is anything other than an addiction because my wife is a neuroscientist and mental wellness professional), my family, and what is left of my sleep schedule -- there isn't much left over to devote to new endeavours.
• mickmackusa: The drawback of using an attempts count to prevent brute force attacks, is that it is even simpler for an attacker to lockout your users by smashing out N number of attempts for their rainbow list of usernames. I suppose though, if you want to allow the lesser evil, it would be better that your users get locked out for a while versus your system being infiltrated.
• Jaquarh: Could you not lock the client IP rather than the account itself? Granted they could use a VPN or packet switch network but eventually they'll run out of IP addresses to use limiting their brute force & most "free" brute forces don't have the functionality of "remembering" where it stopped in a words list - but again, maybe its worth building but can be turned on or off via the configuration?