Laravel LoginController

Question

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();
 }
}

• Sᴀᴍ Onᴇᴌᴀ: "can anyone tell me where I am going wrong?" does that imply there are errors/bugs, or is it just an opinionated wrong?

Answer 1

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');
 // }
 // }
 }