The UserCheckController only performs one task, which is checking if we can respond to the two-factor authentication if it's enabled. I suggest using an invocable controller, like this:
class UserCheckController extends Controller {
public function __invoke(LoginCheckUserRequest $request) {}
}
I also prefer to extract the business logic to an action (sometimes referred to as a command bus), for example Actions\CustomerPortal\HandleUserLogin. This provides better organization by placing it under the CustomerPortal namespace.
// app/CustomerPortal/Actions/HandleUserLogin.php
namespace App\CustomerPortal\Actions;
class HandleUserLogin
{
public function handle(string $login, $deviceId){}
}
I'm not a fan of building model queries directly within the codebase. I prefer using repositories or scopes. For now, I will suggest scopes. We can move the logic to fetch the user with a phone number under a named scope within the user model, or trait that contains our custom queries like so:
class User extends Authenticatable {
//...
//...
public function scopeByPhoneNumber($query, $phoneNumber)
{
return $query->whereEncrypted('login', $phoneNumber)->has('customerUser');
}
}
Now we can update our action to use that to fetch the related user, like this:
namespace App\CustomerPortal\Actions;
use App\Models\User;
class HandleUserLogin
{
public function handle(string $login){
$user = User::byPhoneNumber($login)->first();
}
}
Let's handle the scenario when the user is not found and determine how to respond accordingly. I suggest throwing an exception in this case.
namespace App\CustomerPortal\Actions;
use App\Models\User;
use App\CustomerPortal\Exceptions\ResourceNotFound;
class HandleUserLogin
{
public function handle(string $login){
$user = User::byPhoneNumber($login)->first();
if(!$user) {
throw new ResourceNotFound("user model is not found for the give phone number: {$login})
}
}
}
Now, by throwing the exception, we can catch it within the controller and react accordingly.
namespace
use App\CustomerPortal\Exceptions\ResourceNotFound;
use App\CustomerPortal\Services\Logs\Logger; // this is going to be an interface
class UserCheckController extends Controller {
public function __invoke(LoginCheckUserRequest $request, HandleUserLogin $handleUserLogin, Logger $logger) {
try {
$handleUserLogin->handle($request->get('login'), $request->header('V2-Device-Unique-ID'));
} catch (ResourceNotFound $e) {
$responseData = [
'two_factor_auth' => [
'request_id' => time(),
'method' => (string) Auth2FAMethod::SMS()
],
];
// TODO: We may also improve this by injecting it into the controller.
$response = JsonApiResponseService::success($responseData);
$logger->log($response);
return $response;
}
}
}
The looger service will be something like this:
namespace App\CustomerPortal\Services\Logs;
use Illuminate\Http\JsonResponse;
interface Logger
{
public function logs(JsonResponse $login);
}
Database logger service
namespace App\CustomerPortal\Services\Logs;
use App\CustomerPortal\Services\Logs\Logger
use Illuminate\Http\JsonResponse;
use App\Models\LogService;
interface DbLogger implements Logger
{
public function logs(JsonResponse $login) {
$dbt = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 3);
LogService::logApiResponse(0, $response, [
'back_trace' => $dbt[0] ?? null
]);
}
}
We just need to register our binding using laravel service provider this way laravel knows which service use when automaticlly resolve it:
$this->app->bind(Logger::class, DbLogger::class)
Now let's get back to our action and tweak the part for checking if two-factor authentication is required for the current user. I think having that logic within the user model is not bad, but grouping it under a trait and including it with the model will clean up the User model a bit and encapsulate the behaviors for the related responsibility in one place.
Im going to call the trait for example TwoFactorAuthenticable:
use App\Models\Traits;
trait TwoFactorAuthenticable {
public function handleTwoFactorRequirement(string $devideId, bool $autoLogin = false):void {
// business login to enable/disable two factor requirements
}
public function isTwoFactorRequired():Boolean {
return $this->requires_2fa === true;
}
}
Let's update our action to reflect the recent changes. I'm going to throw an exception when the 2Fac is not enabled:
namespace App\CustomerPortal\Actions;
use App\Models\User;
use App\CustomerPortal\Exceptions\ResourceNotFound;
use App\CustomerPortal\Exceptions\TwoFactorAuthenticationDiabled;
class HandleUserLogin
{
public function handle(string $login, $deviceId){
$user = User::byPhoneNumber($login)->first();
if(!$user) {
throw new ResourceNotFound("user model is not found for the give phone number: {$login})
}
$user->handleTwoFactorRequirement($deviceId);
if($user->isTwoFactorRequired()) {
throw new TwoFactorAuthenticationDiabled("The current user {$user->id} 2Fac is disabled");
}
}
}
Now we can update our controller to catch that exception and respond to the user:
use App\CustomerPortal\Exceptions\ResourceNotFound;
use App\CustomerPortal\Exceptions\TwoFactorAuthenticationDiabled;
use App\CustomerPortal\Services\Logs\Logger; // this is going to be an interface
class UserCheckController extends Controller {
public function __invoke(LoginCheckUserRequest $request, HandleUserLogin $handleUserLogin, Logger $logger) {
try {
$handleUserLogin->handle($request->get('login'), $request->header('V2-Device-Unique-ID'));
} catch (ResourceNotFound | TwoFactorAuthenticationDiabled $e) {
$responseData = [];
if($e instanceof ResourceNotFound) {
$responseData['two_factor_auth'] = [
'request_id' => time(),
'method' => (string) Auth2FAMethod::SMS()
];
}
if($e instanceof TwoFactorAuthenticationDiabled) {
$responseData['two_factor_auth'] = null;
}
// TODO: We may also improve this by injecting it into the controller.
$response = JsonApiResponseService::success($responseData);
$logger->log($response);
return $response;
}
}
}
Let's tackle the part where the 2FAC is enabled, and we need to proceed with the 2FAC login. For now, that logic is hidden within a trait. I think we better use the service directly. I will first tackle the part for refactoring the TwoFactorAuthService before coming back to our action and using it. I see that to request the two-factor authentication, we have three options: TOTP, ONE TOUCH, and SMS. I think this is a good use case for introducing the Strategy pattern to clean up the code and make it flexible and easy to test (I can't see any tested code, which I think is a must). Let me suggest my solution to improve this service.
(I will tackle this part tomorrow inchallah :) ) Hints:
- Will introduce queued job to handle the 3th party calls.
- Using strategy patterm to request the 2Fac login using the proper option(TOTP | ONE TOUCH | SMS).