Skip to content
Merged

2 fa #782

Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ on:
branches:
- GaelO2
- GaelO2-dev
- imap-lib
- 2FA
Comment thread
salimkanoun marked this conversation as resolved.
tags:
- '*'

Expand Down
3 changes: 3 additions & 0 deletions GaelO2/app/GaelO/Entities/UserEntity.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class UserEntity

public ?array $roles;

public ?string $twoFactorConfirmedAt;

public static function fillFromDBReponseArray(array $array): UserEntity
{
$userEntity = new UserEntity();
Expand All @@ -44,6 +46,7 @@ public static function fillFromDBReponseArray(array $array): UserEntity
$userEntity->emailVerifiedAt = $array['email_verified_at'];
$userEntity->lastConnection = $array['last_connection'];
$userEntity->onboardingVersion = $array['onboarding_version'];
$userEntity->twoFactorConfirmedAt = $array['two_factor_confirmed_at'] ?? null;
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Xeno-linux Passer en boolean is2FaEnabled

return $userEntity;
Comment thread
salimkanoun marked this conversation as resolved.
}

Expand Down
10 changes: 8 additions & 2 deletions GaelO2/app/GaelO/Repositories/UserRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@ public function __construct(User $user, Role $roles, CenterUser $centerUser, Stu

public function find($id): array
{
return $this->userModel->findOrFail($id)->toArray();
return $this->userModel->findOrFail($id)->makeVisible([
'two_factor_confirmed_at'
])->toArray();
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enelver les hidder pour eviter l'appel a makeVisible

Comment thread
salimkanoun marked this conversation as resolved.
Outdated
}

public function delete($id): void
Expand Down Expand Up @@ -143,7 +145,11 @@ public function getUserByEmail(String $email, bool $withTrashed = false): array
$user = $this->userModel->where('email', strtolower($email))->sole();
}

return $user->toArray();
return $user->makeVisible([
'two_factor_secret',
'two_factor_recovery_codes',
'two_factor_confirmed_at'
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enelver les hidder pour eviter l'appel a makeVisible

])->toArray();
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
}

public function isExistingEmail(String $email): bool
Expand Down
8 changes: 7 additions & 1 deletion GaelO2/app/GaelO/UseCases/Login/Login.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,17 @@ public function execute(LoginRequest $loginRequest, LoginResponse $loginResponse

//if everything OK => Login
if ($user['email_verified_at'] !== null && $attempts < 3) {

// Detect if 2FA is enable and is validate for this user
$twoFactorEnabled = !empty($user['two_factor_secret']) && !empty($user['two_factor_confirmed_at']);

$this->updateDbOnSuccess($user, $loginRequest->ip);

$loginResponse->onboarded = !(Util::isVersionHigher(FrameworkAdapter::getConfig('onboarding_version'), $user['onboarding_version']));
$loginResponse->use2FA = $twoFactorEnabled;
$loginResponse->status = 200;
$loginResponse->statusText = "OK";
//should not happen
$loginResponse->userId = $user['id'];
} else {
throw new GaelOUnauthorizedException("Unknown email/password");
}
Expand Down
2 changes: 2 additions & 0 deletions GaelO2/app/GaelO/UseCases/Login/LoginResponse.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ class LoginResponse
public ?bool $onboarded = null;
public int $status;
public string $statusText;
public ?bool $use2FA = null;
public ?int $userId = null;
}
173 changes: 173 additions & 0 deletions GaelO2/app/Http/Controllers/AuthController.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,14 @@
use App\GaelO\UseCases\Login\LoginRequest;
use App\GaelO\UseCases\Login\LoginResponse;
use App\GaelO\Util;
use Illuminate\Http\JsonResponse;
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Cache;
use Illuminate\Support\Str;
use App\Models\User;
use Illuminate\Auth\Access\AuthorizationException;
use Illuminate\Routing\UrlGenerator;
use Laravel\Fortify\Contracts\TwoFactorAuthenticationProvider;

class AuthController extends Controller
{
Expand All @@ -29,16 +33,44 @@ public function login(Request $request, Login $login, LoginRequest $loginRequest
$login->execute($loginRequest, $loginResponse);

if ($loginResponse->status === 200) {
$userId = $loginResponse->userId;
$use2FA = $loginResponse->use2FA;

if ($use2FA) {
// Temporary Opaque Token. available for 5 minutes
$challengeToken = Str::uuid()->toString();
Cache::put('2fa_challenge_' . $challengeToken, $userId, now()->addMinutes(5));

return response()->json([
'onboarded' => $loginResponse->onboarded,
'needs2FA' => true,
'challenge_token' => $challengeToken
], 200);
}


// Regular Login
$user = User::where('email', strtolower($request->email))->sole();
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
$isAdmin = $user->administrator;

if ($isAdmin && !$use2FA) {
return response()->json([
'id' => $user->id,
//'onboarded' => $loginResponse->onboarded,
'needs2FA' => true

]);
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In login(), when the user is an administrator and 2FA is not yet enabled ($isAdmin && !$use2FA), the API returns needs2FA: true but does not return either an access token or a challenge_token. Since the 2FA setup endpoints are behind auth:sanctum, this leaves administrators with no way to initialize 2FA and complete login. Consider either (a) issuing a temporary challenge_token that can be used to call the 2FA setup/confirm endpoints without a Sanctum token, or (b) allowing password-only login until 2FA has been initialized/confirmed, then requiring the 2FA challenge on subsequent logins.

Suggested change
return response()->json([
'id' => $user->id,
//'onboarded' => $loginResponse->onboarded,
'needs2FA' => true
]);
$tokenResult = $user->createToken('GaelO');
return response()->json([
'id' => $user->id,
'onboarded' => $loginResponse->onboarded,
'access_token' => $tokenResult->plainTextToken,
'token_type' => 'Bearer',
'needs2FA' => true,
], 200);

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Xeno-linux il a raison ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je crois c'est ça qui bloquer mon front (vue que j'avais un 401)

}

$tokenResult = $user->createToken('GaelO');

return response()->json([
'id' => $user->id,
'onboarded' => $loginResponse->onboarded,
'access_token' => $tokenResult->plainTextToken,
'token_type' => 'Bearer'
], 200);

} else {
return $this->getJsonResponse($loginResponse->body, $loginResponse->status, $loginResponse->statusText);
}
Expand All @@ -50,6 +82,145 @@ public function logout(Request $request)
return response()->json();
}

public function twoFactorChallenge(
Comment thread
salimkanoun marked this conversation as resolved.
Request $request,
TwoFactorAuthenticationProvider $provider
): JsonResponse {

$challengeToken = $request->input('challenge_token');

if (!$challengeToken) {
return response()->json(['message' => 'Session expired.'], 422);
}

// pull = get + delete in one operaion (single use)
$userId = Cache::pull('2fa_challenge_' . $challengeToken);

if (!$userId) {
return response()->json(['message' => 'Session expired or already use.'], 422);
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

twoFactorChallenge() uses Cache::pull() to fetch the userId, which deletes the challenge token immediately. This means a user only gets one attempt: any invalid code (or even a missing code/recovery_code) consumes the challenge and forces a full re-login to get a new token. Consider using Cache::get() for validation attempts and only deleting the cache entry after a successful 2FA validation (or tracking remaining attempts separately).

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Xeno-linux il a raison


$user = User::findOrFail($userId);

$code = $request->input('code');
$recoveryCode = $request->input('recovery_code');

$valid = false;

if ($recoveryCode) {
$codes = json_decode(decrypt($user->two_factor_recovery_codes), true);
$index = array_search($recoveryCode, $codes);

Comment thread
salimkanoun marked this conversation as resolved.
Outdated
if ($index !== false) {
array_splice($codes, $index, 1);
$user->forceFill([
'two_factor_recovery_codes' => encrypt(json_encode($codes))
])->save();
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
$valid = true;
}

} elseif ($code) {
$valid = $provider->verify(
decrypt($user->two_factor_secret),
$code
);
}

if (!$valid) {
return response()->json([
'errors' => ['code' => ['invalid code']]
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
], 422);
}

$tokenResult = $user->createToken('GaelO');

return response()->json([
'id' => $user->id,
'access_token' => $tokenResult->plainTextToken,
'token_type' => 'Bearer'
], 200);
}

/**
* Generate 2FA secret + QR code SVG for the authenticated user.
* Route protected by auth:sanctum — the Bearer token from login
* is sent explicitly by the front before being stored in Redux.
* Must be in the auth:sanctum group but NOT in the onboarded group
* (admin may not be onboarded yet when setting up 2FA).
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
*/
public function setup2FA(Request $request, TwoFactorAuthenticationProvider $provider): JsonResponse
{
$user = $request->user();
Comment on lines +192 to +197
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docblock for setup2FA() says this route “Must be in the auth:sanctum group but NOT in the onboarded group”, but the route is currently registered inside the ['auth:sanctum', 'verified', 'activated', 'onboarded'] group in routes/api.php. Either update the documentation or move the route so behavior and docs match.

Copilot uses AI. Check for mistakes.

// Generate secret if not already set
if (empty($user->two_factor_secret)) {
$user->forceFill([
'two_factor_secret' => encrypt($provider->generateSecretKey()),
'two_factor_recovery_codes' => encrypt(json_encode(
collect(range(1, 8))->map(fn() => Str::random(10) . '-' . Str::random(10))->all()
))
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Xeno-linux généré mais non output, ne pas générer ici les recovery code

])->save();
}

$appName = urlencode(config('app.name'));
$email = urlencode($user->email);
$secret = decrypt($user->two_factor_secret);
$otpauthUrl = "otpauth://totp/{$appName}:{$email}?secret={$secret}&issuer={$appName}";
Comment thread
salimkanoun marked this conversation as resolved.

$renderer = new \BaconQrCode\Renderer\ImageRenderer(
new \BaconQrCode\Renderer\RendererStyle\RendererStyle(192),
new \BaconQrCode\Renderer\Image\SvgImageBackEnd()
);
$svg = (new \BaconQrCode\Writer($renderer))->writeString($otpauthUrl);

return response()->json(['svg' => $svg]);
}

/**
* Confirm 2FA setup by verifying the TOTP code for the authenticated user.
*/
public function confirmSetup2FA(Request $request, TwoFactorAuthenticationProvider $provider): JsonResponse
{
$user = $request->user();

if (empty($user->two_factor_secret)) {
return response()->json(['message' => '2FA not initialized'], 422);
}

$valid = $provider->verify(decrypt($user->two_factor_secret), $request->input('code'));

if (!$valid) {
return response()->json(['errors' => ['code' => ['invalid code']]], 422);
}

$user->forceFill(['two_factor_confirmed_at' => now()])->save();

return response()->json(['message' => '2FA activated']);
}
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New 2FA setup endpoints (setup2FA / confirmSetup2FA) introduce significant auth behavior but don’t appear to have feature tests yet (current tests cover login + challenge + recovery code generation only). Adding tests for setup (returns SVG / persists secret) and confirm (sets two_factor_confirmed_at, rejects invalid codes) would help prevent regressions.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Xeno-linux il a peut etre raison, on des tests la dessus ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je suis pas sur. il me semble que y'a pas de test actuellement sur le setup et sur la validation des recovery codes (à rajouter du coup même si faudra que je trouve comment décrypter un qrcode svg pour en extraire le code dans les testes)



public function generateRecoveryCodes2FA(Request $request): JsonResponse
{
$user = $request->user();

if (!$user) {
Comment thread
salimkanoun marked this conversation as resolved.
return response()->json(['message' => 'Unauthenticated'], 401);
}

if (empty($user->two_factor_secret) || empty($user->two_factor_confirmed_at)) {
return response()->json(['message' => '2FA Not activated'], 422);
}

// Decrypt to return the code on the front side
$codes = $user->recoveryCodes();
foreach ($codes as $code) {
$user->replaceRecoveryCode($code);
}

$updatedCodes = $user->recoveryCodes();
return response()->json(['recoveryCodes' => $updatedCodes]);
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generateRecoveryCodes2FA() generates and saves new recovery codes, then immediately loops through them calling $user->replaceRecoveryCode($code) before returning. This causes extra writes and can change the codes that are returned vs. originally generated. Prefer returning $user->recoveryCodes() directly after saving (or return the generated $codes variable) without mutating them again.

Suggested change
// Decrypt to return the code on the front side
$codes = $user->recoveryCodes();
foreach ($codes as $code) {
$user->replaceRecoveryCode($code);
}
$updatedCodes = $user->recoveryCodes();
return response()->json(['recoveryCodes' => $updatedCodes]);
return response()->json(['recoveryCodes' => $codes]);

Copilot uses AI. Check for mistakes.
}

public function getMagicLink(Request $request, UrlGenerator $urlGenerator)
{

Expand Down Expand Up @@ -89,4 +260,6 @@ public function getSystem(Request $request, GetSystem $getSystem, GetSystemReque
$getSystem->execute($getSystemRequest, $getSystemResponse);
return $this->getJsonResponse($getSystemResponse->body, $getSystemResponse->status, $getSystemResponse->statusText);
}


}
11 changes: 8 additions & 3 deletions GaelO2/app/Models/User.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@
use Illuminate\Notifications\Notifiable;
use Illuminate\Database\Eloquent\SoftDeletes;
use Laravel\Sanctum\HasApiTokens;
use Laravel\Fortify\TwoFactorAuthenticatable;

class User extends Authenticatable implements CanResetPassword, MustVerifyEmail
{
use Notifiable, SoftDeletes, HasApiTokens, HasFactory, PasswordsCanResetPassword;
use Notifiable, SoftDeletes, HasApiTokens, HasFactory, PasswordsCanResetPassword, TwoFactorAuthenticatable;

Comment thread
salimkanoun marked this conversation as resolved.
/**
* The attributes that are mass assignable.
Expand All @@ -33,7 +34,10 @@ class User extends Authenticatable implements CanResetPassword, MustVerifyEmail
* @var array
*/
protected $hidden = [
'remember_token'
'remember_token',
'two_factor_secret',
'two_factor_recovery_codes',
'two_factor_confirmed_at',
];
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enlever le hidden on gere la visibilité vers le front dans notre code


/**
Expand All @@ -60,7 +64,8 @@ protected function casts(): array
'orthanc_password' => 'string',
'api_token' => 'string',
'email_verified_at' => 'datetime',
'onboarding_version' => 'string'
'onboarding_version' => 'string',
'two_factor_confirmed_at' => 'datetime'
];
}

Expand Down
24 changes: 24 additions & 0 deletions GaelO2/app/Providers/FortifyServiceProvider.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php

namespace App\Providers;

use Illuminate\Support\ServiceProvider;
use Laravel\Fortify\Fortify;

class FortifyServiceProvider extends ServiceProvider
{
public function register(): void
{
// Forcer toutes les routes Fortify sous le middleware API avec auth Sanctum
config(['fortify.middleware' => ['api', 'auth:sanctum']]);
Comment thread
salimkanoun marked this conversation as resolved.
Outdated
}

public function boot(): void
{
Fortify::ignoreRoutes();
// No view for 2FA challenge
Fortify::twoFactorChallengeView(function () {
return response()->json(['message' => 'Unauthorized'], 401);
});
Comment thread
salimkanoun marked this conversation as resolved.
}
}
1 change: 1 addition & 0 deletions GaelO2/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
"directorytree/imapengine-laravel": "^1.1",
"giggsey/libphonenumber-for-php": "^9.0",
"guzzlehttp/guzzle": "^7.9",
"laravel/fortify": "^1.36",
"laravel/framework": "^12.0",
"laravel/sanctum": "^4.0",
"laravel/tinker": "^2.10",
Expand Down
Loading
Loading