Login using two-factor authentication

Question

I want to make the code below prettier but I don't know what to change and how to change. I know that the controller should be easy, delegating actions related to models or to services or something else.

The task the code below solves is to authenticate a client with 2 factory authentication. If a client has 2fa enabled, then additional field 2faCode is required, otherwise only email and password are required. I really don't like so many ifs for such a simple task. The code should be easy and readable.

/**
* Get a JWT via given credentials.
*
* @param LoginUserRequest $request
* @return JsonResponse
*/
public function login(LoginUserRequest $request): JsonResponse
{
   $data = $request->validated();
   if (!$token = auth()->attempt(['email' => $data['email'], 'password' => $data['password']])) {
       return response()->json(['error' => 'Unauthorized. Credentials are wrong'], 401);
   }
   $user = auth()->user();
   if (!$user->has2fa) {
       return $this->respondWithToken($token);
   }
   if (!isset($data['2faCode'])) {
       return response()->json(['error' => 'Unauthorized. 2faCode is not specified but it is required'], 401);
   }
   $checkResult = $this->googleAuthenticator->verifyCode(env('GOOGLE_AUTHENTICATOR_SECRET'), $data['2faCode'], 10);
   if ($checkResult) {
      return $this->respondWithToken($token);
   } else {
      return response()->json(['error' => 'Unauthorized. Wrong 2faCode.'], 401);
   }}

Answer 1

I actually think you are doing it ok. You made the choice to test a number of conditions in a logical order and return early in case of failure. Thus each test takes only a few lines of code and is clearly delimited. The control flow is easy to grasp.

You could have done it differently using nested ifs and testing for positive conditions like this:

{
   $data = $request->validated();
   if ($token = auth()->attempt(['email' => $data['email'], 'password' => $data['password']])) {
       $user = auth()->user();
       if (!$user->has2fa) {
           return $this->respondWithToken($token);
       }
       if (isset($data['2faCode'])) {
           $checkResult = $this->googleAuthenticator->verifyCode(env('GOOGLE_AUTHENTICATOR_SECRET'), $data['2faCode'], 10);
           if ($checkResult) {
              return $this->respondWithToken($token);
           } else {
              return response()->json(['error' => 'Unauthorized. Wrong 2faCode.'], 401);
           }
       } else {
           return response()->json(['error' => 'Unauthorized. 2faCode is not specified but it is required'], 401);
       }
   } else {
       return response()->json(['error' => 'Unauthorized. Credentials are wrong'], 401);
   }

}

Pardon the poor transposition. You get the point: this approach is much less comprehensible because the whole context has to be analyzed.

Now:

if (!$user->has2fa) {
   return $this->respondWithToken($token);
}

It's possible I misunderstood but did you mean:

if ($user->has2fa) {
   return $this->respondWithToken($token);
}

If that is the case, and barring any possible misunderstanding on my end then it may be justified to combine the two ifs in a nested block. Possibly like this:

if (!$user->has2fa) {
    if (!isset($data['2faCode'])) {
       return response()->json(['error' => 'Unauthorized. 2faCode is not specified but it is required'], 401);
    }
    return $this->respondWithToken($token);
}

---

One simple thing you could also have done is simply add line spacing between your blocks.

There is lots of ifs you say, in fact there are just four and if you are going to test disparate conditions there are not many better ways. A switch/case block is a possibility with some adjustments but I don't see the added value here. You would end up with a bigger control block like the counter-example above.

To sum up I think your code is ok as-is, it's about 15 lines of effective code and not really screaming for a massive refactor.