Laravel 5.4: Authentication System

Question

I have recently coded my own authentication system and wondered if there was any way I could make it better? I coded it with Laravel 5.4. getView is called on Route::get and onPost is called on Route::post

use Auth;
use App\User;
use App\Http\Controllers\Controller;
use Validator;
use Redirect;
use Illuminate\Http\Request;
use App\Database\Frontend\User\Player;
use App\Database\Frontend\WebsiteLogin;

class LoginController extends Controller
{
    public function getView()
    {
        return view('frontend.login');
    }

    public function onPost(Request $request) 
    {
        $validator = Validator::make($request->all(), [
            'mail'     => 'required|email|exists:users',
            'password'  => 'required'
        ]);

        if ( $validator->fails()) {
            return Redirect::to('/login')->withErrors($validator->messages());
        }
        else {
            if (!Auth::attempt(['mail' => $request->input('mail'), 'password' => $request->input('password')])) {
                $this->addNewWebsiteLogin($request, Player::where('mail', $request->input('mail'))->pluck('id')->first(), "0");
                return Redirect::to('/login')->withMessage('Email and password do not match')->withColor('danger');
            }
            else {
                $this->addNewWebsiteLogin($request, Auth::user()->id, "1");

                $user = Auth::user();
                $user->last_online = time();
                $user->save();

                if (config('frontend.government_only') && (Auth::Guest() || Auth::user()->roleplay->government_id == 0)) {
                    Auth::logout();
                    return Redirect::to('/login')->withMessage(config('frontend.site_name') . ' is only open to government individuals at this moment, too bad.')->withColor('danger');
                }

                return Redirect::to('/home')->withMessage('Welcome back!');
            }
        }
    }

    private function addNewWebsiteLogin(Request $request, $userId, $status) {
        $websiteLogin                    = new WebsiteLogin;
        $websiteLogin->user_id           = $userId;
        $websiteLogin->request_ip        = $request->ip();
        $websiteLogin->request_system    = 'TODO';
        $websiteLogin->request_browser   = 'TODO';
        $websiteLogin->login_status      = $status;
        $websiteLogin->save();
    }
}

Answer 1

You can use Laravel's form requests to keep rules and validation logic out of controllers.

Answer 2

• You have unnecessary else blocks in your onPost() method that can be removed because you are returning in the if conditions.
• You have some really long lines of code that make it hard to read. you should break your lines when you have > ~80 characters.

This will clean up your code to something like:

public function onPost(Request $request) 
{
    $validator = Validator::make($request->all(), [
        'mail'     => 'required|email|exists:users',
        'password'  => 'required'
    ]);

    if ( $validator->fails()) {
        return Redirect::to('/login')->withErrors($validator->messages());
    }

    if (!Auth::attempt([
        'mail' => $request->input('mail'),
        'password' => $request->input('password')
    ])) {
        $this->addNewWebsiteLogin(
            $request,
            Player::where('mail', $request->input('mail'))
                ->pluck('id')->first(),
            "0"
        );
        return Redirect::to('/login')
            ->withMessage('Email and password do not match')
            ->withColor('danger');
    }

    $this->addNewWebsiteLogin($request, Auth::user()->id, "1");
    $user = Auth::user();
    $user->last_online = time();
    $user->save();

    if ( config('frontend.government_only') &&
        (Auth::Guest() || Auth::user()->roleplay->government_id == 0)
    ) {
        Auth::logout();
        return Redirect::to('/login')
            ->withMessage(
                config('frontend.site_name') . ' is only open to ' .
                'government individuals at this moment, too bad.'
            )
            ->withColor('danger');
    }

    return Redirect::to('/home')->withMessage('Welcome back!');
}

---

Do you really want to use timestamps (via time() function) to store datetime information in the database? I would suggest using datetime or timestamp fields in the database, as you will most likely want to do something querying against these fields in the database for one reason or another (even if just for reporting purposes), and it is much easier to work with these field types in the database rather than UNIX timestamps.

---

Your code id very happy-path oriented. For example, you just assume all your database operations will work as expected as there is no real error handling around them. For example, what if Player::where('mail', $request->input('mail'))->pluck('id')->first() does not return a value? You are already passing this directly into another function, just assuming a user ID is returned.

---

Auth::user()->roleplay->government_id == 0

Here, and really in general, you should consider using exact comparison === if you are really trying to compare that calculated value vs. 0 as an integer. It is probably a good habit to use exact comparisons as the default way you write code, and you will find your code less fragile to unexpected truthy/falsey evaluation behavior. Only use loose comparisons when there is a very specific reason to do so.