Laravel - Authentication Controller

Question

I'm developing a Social Engineering Awareness Training Application. This is the focus of my thesis for my undergraduate degree. This will be a multi-part review request, however, if you want to see the entire application, it can be found on GitHub. For this request, I'm looking to see how my AuthController is set up and how effective you think it might be. I open to any and all suggestions about any facet of the code.

One question I do have, though, is what benefit is there to my application being 90% static?

Keep in mind that this application is nearly to testing, however, there are a few pieces still left to do (a few templates, a few views, documentation).

AuthController

<?php

namespace App\Http\Controllers;

use App\Libraries\RandomObjectGeneration;
use App\Models\Two_Factor;
use App\Models\User;
use App\Models\User_Permissions;
use Illuminate\Http\Request;

class AuthController extends Controller
{
/**
 * create
 * Create a new user instance after a valid registration.
 *
 * @param   Request         $request
 * @return  User
 */
public static function create(Request $request) {
    $users = User::all();
    $username = $request->input('usernameText');
    foreach($users as $user) {
        if($user->Username == $username) {
            return redirect()->route('register');
        }
    }
    if($request->input('emailText') != $request->input('confirmEmailText')) {
        return redirect()->route('register');
    }
    $email = $request->input('emailText');
    $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%&';
    $password = RandomObjectGeneration::random_str(getenv('DEFAULT_LENGTH_PASSWORDS'),$keyspace);
    $user = User::create([
        'Username' => $username,
        'Email' => $email,
        'FirstName' => $request->input('firstNameText'),
        'LastName' => $request->input('lastNameText'),
        'MiddleInitial' => $request->input('initialText'),
        'Password' => password_hash($password,PASSWORD_DEFAULT),
        '2FA' => 0,
    ]);
    EmailController::sendNewAccountEmail($user,$password);
    return redirect()->route('users');
}

/**
 * authenticate
 * Authenticates the user against the user's database object. Submits to 2FA if they have
 * the option enabled, otherwise logs the user in.
 *
 * @param   Request         $request
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function authenticate(Request $request) {
    $user = User::where('Username',$request->input('usernameText'))->first();
    if(password_verify($request->input('passwordText'),$user->Password)) {
        if($user->getAttribute('2FA') == 1) {
            $twoFactor = Two_Factor::where([
                'UserId' => $user->Id, 'Ip' => $_SERVER['REMOTE_ADDR']
            ])->first();
            if(count($twoFactor)) {
                $twoFactor->delete();
            }
            $code = RandomObjectGeneration::random_str(6, '1234567890');
            Two_Factor::create([
                'UserID' => $user->Id,
                'Ip' => $_SERVER['REMOTE_ADDR'],
                'Code' => password_hash($code,PASSWORD_DEFAULT)
            ]);

            EmailController::sendTwoFactorEmail($user,$code);
            \Session::put('2faUser',$user);
            return redirect()->route('2fa');
        }
        \Session::put('authUser',$user);
        \Session::put('authIp',$_SERVER['REMOTE_ADDR']);
        if($user->UserType == 1) {
            \Session::put('adminUser',$user);
        }
        $intended = \Session::get('intended');
        if($intended) {
            return redirect()->to($intended);
        }
        return redirect()->route('authHome');
    }
    return redirect()->route('login');
}

/**
 * generateTwoFactorPage
 * Route for generating the 2FA page.
 *
 * @return \Illuminate\Http\RedirectResponse | \Illuminate\View\View
 */
public static function generateTwoFactorPage() {
    if(\Session::has('2faUser')) {
        return view('auth.2fa');
    }
    return redirect()->to('login');
}

/**
 * twoFactorVerify
 * Validates the 2FA code to authenticate the user.
 *
 * @param   Request         $request
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function twoFactorVerify(Request $request) {
    if(\Session::has('2faUser')) {
        $user = \Session::get('2faUser');
        $twoFactor = Two_Factor::where([
            'UserId' => $user->Id, 'Ip' => $_SERVER['REMOTE_ADDR']
        ])->first();
        if(password_verify($request->input('codeText'),$twoFactor->Code)) {
            \Session::put('authUser',$user);
            \Session::put('authIp',$_SERVER['REMOTE_ADDR']);
            if($user->UserType == 1) {
                \Session::put('adminUser',$user);
            }
            \Session::forget('2faUser');
            $twoFactor->delete();
            $intended = \Session::get('intended');
            if($intended) {
                return redirect()->to($intended);
            }
            return redirect()->route('authHome');
        }
        return redirect()->route('2fa');
    }
    return redirect()->route('login');
}

/**
 * resend2FA
 * Generates and sends a new 2FA code.
 *
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function resend2FA() {
    if(\Session::has('2faUser')) {
        $user = \Session::get('2faUser');
        $twoFactor = Two_Factor::where([
            'UserId' => $user->Id, 'Ip' => $_SERVER['REMOTE_ADDR']
        ])->first();
        if(count($twoFactor)) {
            $twoFactor->delete();
        }
        $code = RandomObjectGeneration::random_str(6, '1234567890');
        Two_Factor::create([
            'UserID' => $user->Id,
            'Ip' => $_SERVER['REMOTE_ADDR'],
            'Code' => password_hash($code,PASSWORD_DEFAULT)
        ]);

        EmailController::sendTwoFactorEmail($user,$code);
        return redirect()->route('2fa');
    }
    return redirect()->route('login');
}

/**
 * check
 * Validates if the user is authenticated on this IP Address.
 *
 * @return  bool
 */
public static function check() {
    return \Session::get('authUser') && \Session::get('authIp') == $_SERVER['REMOTE_ADDR'];
}

public static function adminCheck() {
    return \Session::has('adminUser');
}

/**
 * logout
 * Removes session variables storing the authenticated account.
 *
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function logout() {
    \Session::forget('authUser');
    \Session::forget('authIp');
    \Session::forget('adminUser');
    \Session::forget('intended');
    return redirect()->route('login');
}

public static function generateLogin() {
    if(self::check()) {
        return redirect()->route('authHome');
    }
    return view('auth.login');
}

public static function generateRegister() {
    if(self::adminCheck()) {
        $permissions = User_Permissions::all();
        $variables = array('permissions'=>$permissions);
        return view('auth.register')->with($variables);
    }
    return redirect()->route('e401');
}
}

User Model

<?php

namespace App\Models;

use Illuminate\Auth\Authenticatable;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Auth\Passwords\CanResetPassword;
use Illuminate\Contracts\Auth\Authenticatable as AuthenticatableContract;
use Illuminate\Contracts\Auth\CanResetPassword as CanResetPasswordContract;
use Illuminate\Support\Facades\DB;

class User extends Model implements AuthenticatableContract, CanResetPasswordContract {

use Authenticatable, CanResetPassword;


protected $table = 'users';

public $timestamps = false;

protected $primaryKey = 'Id';

/**
 * The attributes that are mass assignable.
 *
 * @var array
 */
protected $fillable =
    ['Username',
    'Email',
    'FirstName',
    'LastName',
    'MiddleInitial',
    'Password',
    '2FA',
    'UserType'];

public static function updateUser($user, $email, $password, $twoFactor, $userType = '') {
    $query = User::query();
    $query->where('Id',$user->Id);
    $update = array();

    if(!empty($email)) {
        $update['Email'] = $email;
    }
    if(!empty($password)) {
        $update['Password'] = $password;
    }
    if(!empty($twoFactor)) {
        if($twoFactor) {
            $update['2FA'] = 1;
        } else {
            $update['2FA'] = 0;
        }
    }
    if(!empty($userType)) {
        $update['UserType'] = $userType;
    }

    $query->update($update);
    return $query->get();
}

public static function queryUsers() {
    $users = DB::table('users')
         ->leftJoin('user_permissions','users.UserType','user_permissions.Id')
        ->select('users.Id','users.Username','users.Email','users.FirstName',
            'users.LastName','users.MiddleInitial','user_permissions.PermissionType')
        ->orderBy('users.Id', 'asc')
        ->get();
    $user = \Session::get('authUser');
    for($i = 0; $i < count($users); $i++) {
        if($users[$i]->Id == $user->Id) {
            unset($users[$i]);
            break;
        }
    }
    return $users;
}
}

User Permissions Model

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class User_Permissions extends Model
{
protected $table = 'user_permissions';

protected $primaryKey = 'ID';

public $timestamps = false;

protected $fillable = ['PermissionType'];
}

Two Factor Authentication Model

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class Two_Factor extends Model
{
protected $table = 'two_factor_codes';

protected $primaryKey = 'ID';

protected $fillable = ['UserID',
    'Ip',
    'Code'];
}

RandomObjectGeneration Library

/**
 * const::KEYSPACE
 * Alphanumeric Keyspace for string randomization.
 */
const KEYSPACE = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';

/**
 * random_str
 * Generates a random string.
 *
 * @param   int                         $length         Length of string to be returned
 * @param   string                      $keyspace       Allowed characters to be used in string
 * @throws  InvalidArgumentException
 * @return  string
 */
public static function random_str($length, $keyspace = RandomObjectGeneration::KEYSPACE)
{
    $str = '';
    if(is_null($length) || !is_int($length) || (is_int($length) && $length < 0)) {
        $str .= 'random_str: Length is invalid. Length must be a positive integer. Value Provided: ' .
            var_export($length) . PHP_EOL;
    }
    if(strlen($keyspace) == 0) {
        $str .= 'random_str: Keyspace cannot be of length 0. Length must be a positive integer.' . PHP_EOL;
    }
    if(!empty($str)) {
        throw new InvalidArgumentException($str);
    }
    $max = mb_strlen($keyspace) - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}

Answer 1

I think comments I made on your other open review around style, error handling, data validation, happy path, etc. are applicable here as well, so I won't re-hash them here.

---

$users = User::all();
$username = $request->input('usernameText');
foreach($users as $user) {
    if($user->Username == $username) {
        return redirect()->route('register');
    }
}

Are you really marshalling user objects for every user in your system to compare against provided username string one by one? This is really wasteful of resources and could actually bring your application server to it's knees with memory consumption as your user base grows.

At a minimum, you should query against the username field in your database (which should have a unique index on it) to specifically see if that user name exists (like you are doing in your authenticate() method). However, you should be thinking about going directly to insert (after validating input data of course) and simply checking to see whether the insert failed based on a unique index constraint on the username field. That is generally the most performant pattern for performing a unique index insert when you expect that there will be a low collision rate of input against existing records. This is because, with this approach, you always have exactly one query per execution as compared to a select-then-insert approach which would give you two individual queries against the database for most executions (with a single query in cases where there is a collision).

---

$keyspace =
'0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%&';

This looks to me like a value that should be a class constant that is currently hidden away inside this method.

---

if(password_verify($request->input('passwordText'),$user->Password)) {

Invert this conditional so you can exit the method early on password mismatch.

---

Typically, one would need to regenerate session ID's at login/logout events. I admit I am not familiar with Laravel as as framework, so maybe that is happening here, but it does not seem to be.

I was unable to find documentation as to what \Session is referring to in Laravel, but it may be worthwhile exploring whether this class properly handles obsolescence of session data. Please see PHP session documentation below for really good information about how sessions should be handled securely at the lowest level in PHP.

http://php.net/manual/en/features.session.security.management.php

---

use Authenticatable, CanResetPassword;

Not sure if you have control over the names of these traits or not. If you do, you should ideally call them *Trait.

---

for($i = 0; $i < count($users); $i++) {
    if($users[$i]->Id == $user->Id) {
        unset($users[$i]);
        break;
    }
}

Again, rather than filtering out user in for loop, it seems like this should be filtered out in the query itself.

Even if you were to manually operate against this array of objects like you are doing, I don't understand why you would be unsetting the value at index, leaving a gap in your array's numerical index. Why not use array_filter() or array_splice() for this operation?

---

$str = '';
if(is_null($length) || !is_int($length) || (is_int($length) && $length < 0)) {
    $str .= 'random_str: Length is invalid. Length must be a positive integer. Value Provided: ' .
        var_export($length) . PHP_EOL;
}
if(strlen($keyspace) == 0) {
    $str .= 'random_str: Keyspace cannot be of length 0. Length must be a positive integer.' . PHP_EOL;
}
if(!empty($str)) {
    throw new InvalidArgumentException($str);
}

This seems an odd way to construct your exception messaging.

Why not simply:

if (empty($length) || !is_int($length) || $length < 0) {
    throw new InvalidArgumentException(...);
}
if (empty($keyspace) || !is_string($keyspace)) {
    throw new InvalidArgumentException(...);
}

Note that I have cleaned up some of the conditionals to be more appropriate as well. I don't think that you need to worry about messaging about exception for both parameters at once. This would be very unusual in practice, and not that meaningful anyway in debugging your application, as at the end of the day you still have an exception being thrown. If you solve the first error, and the second error is still not solved, it will immediately reveal itself.

It seems odd that in both of these pieces of code you have reviewed that this is the only place you are throwing an exception. I think if you commit to using exceptions (something I would strongly encourage), you need to be committed to using them throughout your codebase.

You are not consistent with multibyte string function usage in this method.