4
\$\begingroup\$

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 revised AuthController (initial request) 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 still have, though, is there any benefit to having my application almost completely static?

Keep in mind that this application is nearly to testing, however, there are a few pieces that might not be polished.

AuthController

<?php

namespace App\Http\Controllers;

use App\Libraries\Cryptor;
use App\Libraries\ErrorLogging;
use App\Libraries\RandomObjectGeneration;
use App\Models\Sessions;
use App\Models\Two_Factor;
use App\Models\User;
use App\Models\User_Permissions;
use Illuminate\Database\QueryException;
use Illuminate\Http\Request;
use League\Flysystem\Exception;

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) {
    try {
        if($request->input('emailText') != $request->input('confirmEmailText')) {
            return redirect()->route('register');
        }

        $email = $request->input('emailText');
        $username = $request->input('usernameText');
        $password = RandomObjectGeneration::random_str(intval(getenv('DEFAULT_LENGTH_PASSWORDS')),true);

        $user = User::create([
            'username' => $username,
            'email' => $email,
            'first_name' => $request->input('firstNameText'),
            'last_name' => $request->input('lastNameText'),
            'middle_initial' => $request->input('middleInitialText'),
            'password' => password_hash($password,PASSWORD_DEFAULT),
            'two_factor_enabled' => 0,
        ]);

        EmailController::sendNewAccountEmail($user,$password);
        return redirect()->route('users');

    } catch(QueryException $qe) {
        if(strpos($qe->getMessage(),"1062 Duplicate entry 'admin'") !== false) {
            return redirect()->route('register'); //return with username exists error
        }
        return redirect()->route('register'); //return with unknown error

    } catch(Exception $e) {
        ErrorLogging::logError($e);
        return abort('500');
    }
}

/**
 * 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) {
    try {
        $user = User::where('username',$request->input('usernameText'))->first();
        $password = $request->input('passwordText');
        if(empty($user) || !password_verify($password,$user->password)) {
            return redirect()->route('login');
        }

        User::updateUser($user,$user->email,password_hash($password,PASSWORD_DEFAULT),$user->two_factor_enabled);

        $session = Sessions::where('user_id',$user->id)->first();
        if(!empty($session)) {
            $session->delete();
        }

        $ip = $_SERVER['REMOTE_ADDR'];
        $cryptor = new Cryptor();

        if($user->two_factor_enabled === 1) {
            $twoFactor = Two_Factor::where([
                'user_id' => $user->id, 'ip_address' => $ip
            ])->first();
            if(!empty($twoFactor)) {
                $twoFactor->delete();
            }

            $code = RandomObjectGeneration::random_str(6,false,'1234567890');
            $twoFactor = Two_Factor::create([
                'user_id' => $user->id,
                'ip_address' => $_SERVER['REMOTE_ADDR'],
                'code' => password_hash($code,PASSWORD_DEFAULT)
            ]);

            EmailController::sendTwoFactorEmail($user,$code);

            $newSession = Sessions::create([
                'user_id' => $user->id,
                'ip_address' => $ip,
                'two_factor_id' => $twoFactor->id,
                'authenticated' => 0
            ]);

            $encryptedSession = $cryptor->encrypt($newSession->id);
            \Session::put('sessionId',$encryptedSession);

            return redirect()->route('2fa');
        }

        $newSession = Sessions::create([
            'user_id' => $user->id,
            'ip_address' => $ip,
            'authenticated' => 1
        ]);

        $encryptedSession = $cryptor->encrypt($newSession->id);
        \Session::put('sessionId',$encryptedSession);

        $intended = \Session::pull('intended');
        if($intended) {
            return redirect()->to($intended);
        }
        return redirect()->route('authHome');

    } catch(Exception $e) {
        ErrorLogging::logError($e);
        return abort('500');
    }
}

/**
 * generateTwoFactorPage
 * Route for generating the 2FA page.
 *
 * @return \Illuminate\Http\RedirectResponse | \Illuminate\View\View
 */
public static function generateTwoFactorPage() {
    try {
        if(\Session::has('sessionId')) {
            $cryptor = new Cryptor();

            $sessionId = $cryptor->decrypt(\Session::get('sessionId'));
            $session = Sessions::where('id',$sessionId)->first();

            $sessionCheck = self::activeSessionCheck($session);
            if(!is_null($sessionCheck)) {
                return $sessionCheck;
            }

            if(!is_null($session->two_factor_id)) {
                return view('auth.2fa');
            }
        }
        return redirect()->route('login');

    } catch(Exception $e) {
        ErrorLogging::logError($e);
        return abort('500');
    }
}

/**
 * twoFactorVerify
 * Validates the 2FA code to authenticate the user.
 *
 * @param   Request         $request
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function twoFactorVerify(Request $request) {
    try {
        if(!\Session::has('sessionId')) {
            return redirect()->route('login');
        }
        $cryptor = new Cryptor();

        $sessionId = $cryptor->decrypt(\Session::get('sessionId'));
        $session = Sessions::where('id',$sessionId)->first();

        $sessionCheck = self::activeSessionCheck($session);
        if(!is_null($sessionCheck)) {
            return $sessionCheck;
        }

        $twoFactor = Two_Factor::where([
            'user_id' => $session->user_id, 'ip_address' => $_SERVER['REMOTE_ADDR']
        ])->first();

        if(!password_verify($request->input('codeText'),$twoFactor->code)) {
            return redirect()->route('2fa');
        }

        $session->update([
            'two_factor_id' => null,
            'authenticated' => 1
        ]);

        $twoFactor->delete();

        $intended = \Session::pull('intended');
        if($intended) {
            return redirect()->to($intended);
        }
        return redirect()->route('authHome');

    } catch(Exception $e) {
        ErrorLogging::logError($e);
        return abort('500');
    }
}

/**
 * resend2FA
 * Generates and sends a new 2FA code.
 *
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function resend2FA() {
    try {
        if(!\Session::has('sessionId')) {
            return redirect()->route('login');
        }
        $cryptor = new Cryptor();

        $sessionId = $cryptor->decrypt(\Session::get('sessionId'));
        $session = Sessions::where('id',$sessionId)->first();

        $sessionCheck = self::activeSessionCheck($session);
        if(!is_null($sessionCheck)) {
            return $sessionCheck;
        }

        $user = User::where('id',$session->user_id)->first();
        if(empty($user)) {
            return self::logout();
        }

        $twoFactor = Two_Factor::where([
            'user_id' => $session->user_id, 'ip_address' => $_SERVER['REMOTE_ADDR']
        ])->first();
        if(!empty($twoFactor)) {
            $twoFactor->delete();
        }

        $code = RandomObjectGeneration::random_str(6, '1234567890');
        Two_Factor::create([
            'user_id' => $session->user_id,
            'ip_address' => $_SERVER['REMOTE_ADDR'],
            'code' => password_hash($code,PASSWORD_DEFAULT)
        ]);

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

    } catch(Exception $e) {
        ErrorLogging::logError($e);
        return abort('500');
    }
}

/**
 * activeSessionCheck
 * Helper function to check session objects.
 *
 * @param   Sessions    $session            The session to check.
 * @return  \Illuminate\Http\RedirectResponse | null
 */
private static function activeSessionCheck(Sessions $session) {
    if($session->ip_address !== $_SERVER['REMOTE_ADDR']) {
        $session->delete();
        \Session::forget('sessionId');
        return redirect()->route('login');
    }

    if($session->authenticated === 1) {
        return redirect()->route('authHome');
    }
    return null;
}

/**
 * check
 * Validates if the user is authenticated on this IP Address.
 *
 * @return  bool
 */
public static function check() {
    if(!\Session::has('sessionId')) {
        return false;
    }
    $cryptor = new Cryptor();

    $sessionId = $cryptor->decrypt(\Session::get('sessionId'));
    $session = Sessions::where('id', $sessionId)->first();

    if($session->ip_address !== $_SERVER['REMOTE_ADDR']) {
        $session->delete();
        \Session::forget('sessionId');
        return false;
    }
    return true;
}

/**
 * adminCheck
 * Validates if the user is an authenticated admin user.
 *
 * @return bool
 */
public static function adminCheck() {
    $check = self::check();
    if(!$check) {
        return $check;
    }

    $cryptor = new Cryptor();

    $sessionId = $cryptor->decrypt(\Session::get('sessionId'));
    $session = Sessions::where('id', $sessionId)->first();

    $user = User::where('id',$session->user_id)->first();
    if(empty($user)) {
        $session->delete();
        \Session::forget('sessionId');
        return false;
    }

    if($user->user_type !== 1) {
        return false;
    }
    return true;
}

/**
 * logout
 * Removes session variables storing the authenticated account.
 *
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function logout() {
    $cryptor = new Cryptor();

    $sessionId = $cryptor->decrypt(\Session::get('sessionId'));
    Sessions::where('id', $sessionId)->first()->delete();
    \Session::forget('sessionId');

    return redirect()->route('login');
}

/**
 * generateLogin
 * Generates the login page.
 *
 * @return \Illuminate\Http\RedirectResponse | \Illuminate\View\View
 */
public static function generateLogin() {
    if(self::check()) {
        return redirect()->route('authHome');
    }
    return view('auth.login');
}

/**
 * generateRegister
 * Generates the register page if the user is an admin.
 *
 * @return \Illuminate\Http\RedirectResponse | \Illuminate\View\View
 */
public static function generateRegister() {
    if(self::adminCheck()) {
        $permissions = User_Permissions::all();
        $variables = array('permissions'=>$permissions);
        return view('auth.register')->with($variables);
    }
    return abort('401');
}

/**
 * authRequired
 * Adds session variable for return redirect and then redirects to login page.
 *
 * @return  \Illuminate\Http\RedirectResponse
 */
public static function authRequired() {
    \Session::put('intended',$_SERVER['REQUEST_URI']);
    return redirect()->route('login');
}
}

Random Object Generation Library

<?php

namespace App\Libraries;

use Doctrine\Instantiator\Exception\InvalidArgumentException;


class RandomObjectGeneration
{
const KEYSPACE = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ';
const PASSWORD_KEYSPACE = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ!@#$%&';

/**
 * random_str
 * Generates a random string.
 *
 * @param   int                         $length         Length of string to be returned
 * @param   bool                        $passwordFlag   Boolean flag identifying whether string will be a password
 * @param   string                      $keyspace       Allowed characters to be used in string
 * @throws  InvalidArgumentException
 * @return  string
 */
public static function random_str($length, $passwordFlag = false, $keyspace = RandomObjectGeneration::KEYSPACE)
{
    if($passwordFlag) {
        $keyspace = RandomObjectGeneration::PASSWORD_KEYSPACE;
    }
    if(empty($length) || !is_int($length) || $length < 0) {
        $message = 'Random String Generation: Length is Invalid. Length must be a positive integer. Value Provided: ' .
            var_export($length);
        throw new InvalidArgumentException($message);
    }
    if(empty($keyspace) || !is_string($keyspace)) {
        $message = 'Random String Generation: Invalid Keyspace';
        throw new InvalidArgumentException($message);
    }
    $str = '';
    $max = mb_strlen($keyspace) - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}
}

Sessions Model

<?php

namespace App\Models;

use Illuminate\Database\Eloquent\Model;

class Sessions extends Model
{
protected $table = 'sessions';

protected $primaryKey = 'id';

protected $fillable = ['user_id',
    'ip_address',
    'two_factor_id',
    'authenticated'
];
}

ErrorLogging

<?php

namespace App\Libraries;


use Illuminate\Support\Facades\Log;

class ErrorLogging
{
public static function logError(\Exception $e) {
    $message = $e->getCode() . ': ' . $e->getMessage() . PHP_EOL;
    $message .= $e->getTraceAsString() . PHP_EOL;
    $message .= str_repeat('-',100) . PHP_EOL . PHP_EOL;
    Log::error($message);
}
}
\$\endgroup\$
4
  • \$\begingroup\$ why don't you use built in Laravel auth mechanism? laravel.com/docs/5.4/authentication#authentication-quickstart \$\endgroup\$
    – Dan Cantir
    Feb 17, 2017 at 15:35
  • \$\begingroup\$ @DanCantir This is for my undergraduate thesis. Part of the goals are to show functionality and my ability. While the built-in auth is nice, I found that implementing it does benefit me enough for the depth I want to reach. You're more than welcome to propose an answer saying use Laravel auth, but that doesn't answer this question. \$\endgroup\$
    – Trojan404
    Feb 17, 2017 at 21:09
  • \$\begingroup\$ yep, it will not answer the question. I will try to make a review later, assuming it's "forbidden" to use Laravel built in stuffs. \$\endgroup\$
    – Dan Cantir
    Feb 17, 2017 at 21:44
  • \$\begingroup\$ @Dan not entirely forbidden, just advised against. Using built in can be useful when your taking it and then augmenting it. For instance, my email logic: codereview.stackexchange.com/questions/154875/email-controller. But other times, it's better to show your intuitiveness and creativity with custom logic. \$\endgroup\$
    – Trojan404
    Feb 17, 2017 at 22:12

1 Answer 1

1
+50
\$\begingroup\$

Disclaimer:

I strongly recommend to use Laravel built in functionalities for this, but since it's "advised against" using it and this is a university work, below is my review:

Let's start with AuthController.


create method:

This code:

if($request->input('emailText') != $request->input('confirmEmailText')) {
    return redirect()->route('register');
}

I would suggest first making sure that email value is not empty and that is a valid e-mail.

Instead, I would create a custom FormRequest.

<?php

namespace App\Http\Requests;

use Illuminate\Foundation\Http\FormRequest;

class CreateUserRequest extends FormRequest
{
    public function authorize()
    {
        return true; // Change this as per your needs
    }

    public function rules()
    {
        return [
            'emailText' => 'required|email',
            'confirmEmailText' => 'same:emailText',
            'usernameText' => 'required',
            //...
        ];
    }
}

Then, you can just specify it as a type of your $request parameter like this:

public static function create(CreateUserRequest $request) {

Also, make sure you check the currently available validation rules and how to create your own.

For example, this lines:

if(strpos($qe->getMessage(),"1062 Duplicate entry 'admin'") !== false) {
            return redirect()->route('register'); //return with username exists error
        }

can be avoided with the following rule: 'email' => 'unique:users,username'

So, how are the validation rules evaluated? All you need to do is type-hint the request on your controller method. The incoming form request is validated before the controller method is called, meaning you do not need to clutter your controller with any validation logic.

Then, this line:

EmailController::sendNewAccountEmail($user,$password);

I suggesting using Eloquent Events and Observers. If you will read the docs, I am sure you will understand how to use them.


authenticate method:

As i said before, validation should be in a separate FormRequest.

Then, instead of \Session::put('sessionId',$encryptedSession); and \Session::pull('intended')

I would suggest using just session(['sessionId' => '$encryptedSession']) and session('intended'). It looks much more clean.


Will update this answer with the review of other methods from this controller shortly.


General:

Naming conversions: Use singular nouns for class names.

So, instead of SessionsModel, I would use SessionModel or better Session. Instead of ErrorLogging, I would use ErrorLogger or better Logger. The same goes for RandomObjectGeneration: RandomObjectGenerator is better. Nice answer about this here.

I have also noticed all your methods are static, however I am not sure I understand why.

\$\endgroup\$
1
  • \$\begingroup\$ Good points with the custom request. However, I don't think I can implement the observer. The way I've got my system set up right now, I email the new user their password. This has to be a decrypted version of the password but for passwords I hash them. One way not two way. \$\endgroup\$
    – Trojan404
    Feb 18, 2017 at 20:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.