3
\$\begingroup\$

I have a LoginController in Laravel, but I think it could use some improving, can anyone tell me where I am going wrong? It just seems overall a bit messy, but I can't think of a logical way to improve it.

<?php
namespace App\Http\Controllers\Ec9\Frontend\Guest;

use App\Http\Controllers\Controller;
use App\Database\Ec9\Website\LoginRequest;
use Illuminate\Http\Request;
use Validator;
use Redirect;
use Auth;
use App\Database\Ec9\Other\PlatformSetting;
use App\Database\Ec9\Other\User;
use Jenssegers\Agent\Agent;
use Session;

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

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

        Session::put('last_message_for', 'login');

        if ( $validator->fails()) {
            return Redirect::back()->withErrors($validator->messages());
        }
        else {
            if (!Auth::attempt(['mail' => $request->input('login_email'), 'password' => $request->input('login_password')])) {
                $this->addNewWebsiteLogin($request, User::where('mail', $request->input('login_email'))->pluck('id')->first(), "0");
                return Redirect::back()->withMessage('Opps, you entered an incorrect login.')->withColor('warning');
            }
            else {
                $user = Auth::user();
                $rpInfo = $user->roleplayInformation;

                $this->addNewWebsiteLogin($request, $user->id, "1");

                $country = json_decode(file_get_contents("http://freegeoip.net/json/" . $request->ip()))->country_name;

                if ($user->ip_last != $request->ip() && strlen(Auth::user()->ip_last) > 0) {
                    if ($rpInfo->lock_account_on_different_ip == '1') {
                        $user->lockAccount("Detected a login from a different IP address.");
                    }
                    else if ($rpInfo->lock_account_on_different_country == '1' && strlen(Auth::user()->last_country) > 0 && Auth::user()->last_country != $country) {
                        $user->lockAccount("Detected a login from a different country.");
                    }

                    if ($user->is_locked == '0') {
                        if ($user->website_pin_selection == 'different_ip') {
                            $user->pin_lock = '1';
                        }
                        else if ($user->website_pin_selection == 'different_country' && strlen(Auth::user()->last_country) > 0 && Auth::user()->last_country != $country) {
                            $user->pin_lock = '1';
                        }
                    }
                }

                if ($user->is_locked == '0' && $user->pin_lock == '0' && $user->website_pin_selection == 'every_login') {
                    $user->pin_lock = '1';
                }

                $user->ip_last = $request->ip();
                $user->last_country = $country;
                $user->save();

                return redirect()->route('frontend.user.home');
            }
        }
    }

        private function addNewWebsiteLogin(Request $request, $userId, $status) {
            $agent = new Agent();
            $loginRequest = new LoginRequest;
            $loginRequest->user_id = $userId;
            $loginRequest->password_tried = ($status == '0' ? $request->input('login_password') : '');
            $loginRequest->request_ip = $request->ip();
            $loginRequest->request_device = $agent->isDesktop() ? 'Desktop' : ($agent->isMobile() ? 'Mobile' : 'Tablet');
            $loginRequest->request_system = $agent->platform() . ' ' . $agent->version($agent->platform());
            $loginRequest->request_browser = $agent->browser();
            $loginRequest->request_successful = $status;
            $loginRequest->save();
        }
}
\$\endgroup\$
  • \$\begingroup\$ "can anyone tell me where I am going wrong?" does that imply there are errors/bugs, or is it just an opinionated wrong? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 28 '17 at 23:48
1
\$\begingroup\$

Just refactored the one function. It is hard to tell what your intentions are in some places.

Comments are inline, with reasons as to why I changed things

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

        Session::put('last_message_for', 'login');

        if ( $validator->fails()) {
            return Redirect::back()->withErrors($validator->messages());
        }

        // the prior statement returns, no need for an else here
        // else {
        if (!Auth::attempt(['mail' => $request->input('login_email'), 'password' => $request->input('login_password')])) {
            $this->addNewWebsiteLogin($request, User::where('mail', $request->input('login_email'))->pluck('id')->first(), "0");
            return Redirect::back()->withMessage('Opps, you entered an incorrect login.')->withColor('warning');
        }

        // the prior statement returns, no need for an else here
        // else {

        // we assign user to a variable, but still use Auth::user() thru the following code?
        $user = Auth::user();
        $rpInfo = $user->roleplayInformation;

        $this->addNewWebsiteLogin($request, $user->id, "1");

        // what no error checking, what if this request fails?
        // maybe this should be abstracted in a separate function, for example, $this->getCountryNameByIp($request->ip());
        $country = json_decode(file_get_contents("http://freegeoip.net/json/" . $request->ip()))->country_name;

        // this is not essential, i just find it more readable this way
        // if ($user->ip_last != $request->ip() && strlen(Auth::user()->ip_last) > 0) {
        $is_different_ip = (!empty(Auth::user()->ip_last) && $user->ip_last != $request->ip());

        // not sure why you are checking for string versions of a what is essentially a boolean
        // if ($rpInfo->lock_account_on_different_ip == '1') {
        if ($rpInfo->lock_account_on_different_ip && $is_different_ip) {
            $user->lockAccount("Detected a login from a different IP address.");
        }

        // i have split these conditions up, just for the sake of readability
        // if ($rpInfo->lock_account_on_different_country == '1' && strlen(Auth::user()->last_country) > 0 && Auth::user()->last_country != $country) {
        $is_different_country = (!empty(Auth::user()->last_country) && Auth::user()->last_country != $country);
        if ($rpInfo->lock_account_on_different_country && $is_different_country) {
            $user->lockAccount("Detected a login from a different country.");
        }

        // personal preference again, if you are using booleans, leave the == bit out
        // if ($user->is_locked == '0') {
        if (!$user->is_locked) {

            // to me it seems like you don't need to test for a different country again, but hard to tell without seeing the user class
    //      if ($user->website_pin_selection == 'different_ip') {
    //          $user->pin_lock = '1';
    //      }
    //      else if ($user->website_pin_selection == 'different_country' && strlen(Auth::user()->last_country) > 0 && Auth::user()->last_country != $country) {
    //          $user->pin_lock = '1';
    //      }

            if ($user->website_pin_selection == 'different_ip' || $user->website_pin_selection == 'different_country') {
                $user->pin_lock = '1';
            }
        }

    //  }

        // once again, i prefer boolean tests without the == for readability
        // if ($user->is_locked == '0' && $user->pin_lock == '0' && $user->website_pin_selection == 'every_login') {
        if (!$user->is_locked && !$user->pin_lock && $user->website_pin_selection == 'every_login') {
            $user->pin_lock = '1';
        }

        // not 100% sure on your logic here
        // why are we updating these fields if we detect a different ip or country, shouldn't we keep the original country and ip
        $user->ip_last = $request->ip();
        $user->last_country = $country;
        $user->save();

        return redirect()->route('frontend.user.home');
    //      }
    //  }
    }
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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