Login Controller follow-up

Question

Can this class can be refactored to be more efficient? I've taken tips from other people who have already helped me refactor it a fair amount, but I was wondering if there was any last minute touch-ups that I could apply.

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());
 }
 
 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');
 }
 
 $user = Auth::user();
 $permissions = PlatformSetting::findSetting('website.login.required_permissions');
 
 if (!$user->roleplayExists()) {
 Auth::logout();
 return Redirect::back()->withMessage('Opps, we suggest you contact support.')->withColor('warning');
 }
 if (strlen($permissions) > 0 && !$user->hasAnyPermissions($permissions)) {
 Auth::logout();
 return Redirect::back()->withMessage('Opps, you don\'t have permission to login.')->withColor('warning');
 }
 
 $this->addNewWebsiteLogin($request, $user->id, "1");
 $country = $user->getCountry();
 $is_different_ip = (!empty(Auth::user()->ip_last) && $user->ip_last != $request->ip());
 if ($rpInfo->lock_account_on_different_ip && $is_different_ip) {
 $user->lockAccount("Detected a login from a different IP address.");
 }
 $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.");
 }
 if (!$user->is_locked) {
 if (($user->website_pin_selection == 'different_ip' && $is_different_ip) || ($user->website_pin_selection == 'different_country' && $is_different_country)) {
 $user->pin_lock = '1';
 }
 else if ($user->website_pin_selection == 'every_login') {
 $user->pin_lock = '1';
 }
 }
 // We do this because otherwhys, when the legit owner logs in he'll have trouble.
 // He'll probably get his account locked, because its set to the attackers country/ip.
 // We'll set the ip_last and last_country on every pin successful enter, to be safe.
 if (!$user->is_locked && !$user->pin_lock) {
 $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();
 }
}

I'm also a bit worried about this function, which is called in LoginController as it could be time consuming or it could be down:

public function getCountry() {
 return json_decode(file_get_contents("http://freegeoip.net/json/" . request()->ip()))->country_name;
 }

I'm not really sure what's best, return an empty country or have a backup API link or what. Can anyone suggest anything?

Answer 1

Your questions

Can this class can be refactored to be more efficient?

As I explained in this review one could create a Form request class and move much of the validation logic there. That would allow the length of the onLoginPost() method to be decreased, and provide more adherence to the separation of concerns principle.

I'm not really sure what's best, return an empty country or have a backup API link or what. Can anyone suggest anything?

One could use a laravel extension - for example: ip2location-laravel. Then if that fails then one could fall back to calling another API.

Other review point

Indentation is inconsistent

Bearing in mind that it may have been due to a pasting issue, the indentation of the addNewWebsiteLogin() method does not match the indentation of the other methods. While it does not make it much more difficult to read, it is slightly distracting.