Merged
Conversation
|
Code Coverage SummaryDiff against GaelO2Results for commit: df7548a Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces server-side 2FA support (via Laravel Fortify primitives + custom API endpoints) and updates the authentication flow/tests to require 2FA for administrators and to support TOTP and recovery codes.
Changes:
- Add 2FA persistence fields to
usersand enable Fortify’s 2FA feature set. - Implement custom API endpoints for 2FA challenge, setup (QR), confirmation, and recovery code generation.
- Extend/adjust feature tests and user payload expectations to reflect added 2FA data.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
GaelO2/app/Http/Controllers/AuthController.php |
Adds custom 2FA login/challenge/setup/recovery endpoints and modifies login response flow. |
GaelO2/app/Providers/FortifyServiceProvider.php |
Adds Fortify integration hooks and runtime config overrides. |
GaelO2/config/fortify.php |
New Fortify configuration enabling 2FA and setting guard/middleware/limiters. |
GaelO2/routes/api.php |
Exposes new 2FA-related routes. |
GaelO2/database/migrations/2025_03_17_200000_add_two_factor_columns_to_users_table.php |
Adds 2FA secret, recovery codes, and confirmation timestamp columns. |
GaelO2/app/Models/User.php |
Enables Fortify’s TwoFactorAuthenticatable on the User model. |
GaelO2/app/GaelO/UseCases/Login/Login.php |
Detects enabled 2FA at login time and returns userId + 2FA requirement info. |
GaelO2/app/GaelO/UseCases/Login/LoginResponse.php |
Adds use2FA and userId to login response DTO. |
GaelO2/app/GaelO/Entities/UserEntity.php |
Adds twoFactorConfirmedAt mapping from DB response. |
GaelO2/tests/Feature/TestUser/LoginTest.php |
Adds/updates tests for admin 2FA requirement, challenge flow, and recovery code usage. |
GaelO2/tests/Feature/TestUser/UserTest.php |
Updates expected user payload field count. |
GaelO2/config/app.php |
Registers the app’s FortifyServiceProvider. |
GaelO2/composer.json / GaelO2/composer.lock |
Adds Fortify and refreshes dependency lockfile. |
.github/workflows/publish.yml |
Updates publishing workflow branch trigger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| //Route 2FA | ||
| Route::post('/two-factor-challenge', [AuthController::class, 'twoFactorChallenge']); |
There was a problem hiding this comment.
/two-factor-challenge is currently defined outside the existing throttle:public-apis group (and without any throttle middleware). This endpoint is security-sensitive and should be rate-limited (e.g. add it to the throttle:public-apis group or apply a dedicated throttle:two-factor middleware) to reduce brute-force risk.
Suggested change
| Route::post('/two-factor-challenge', [AuthController::class, 'twoFactorChallenge']); | |
| Route::post('/two-factor-challenge', [AuthController::class, 'twoFactorChallenge'])->middleware('throttle:public-apis'); |
Comment on lines
+263
to
+272
| public function confirm(Request $request) | ||
| { | ||
| $confirmed = $request->user()->confirmTwoFactorAuth($request->code); | ||
|
|
||
| if (!$confirmed) { | ||
| return response()->json(['errors' => ['code' => ['Invalid Two Factor Authentication code.']]], 422); | ||
| } | ||
|
|
||
| return response()->json(); | ||
| } |
There was a problem hiding this comment.
This confirm() action calls $request->user()->confirmTwoFactorAuth(...), but that method doesn’t exist on the User model (no definition found in the codebase) and there’s no route pointing to this controller method. Please remove this dead code (or implement the method + route if it’s intended) to avoid future runtime fatals.
Suggested change
| public function confirm(Request $request) | |
| { | |
| $confirmed = $request->user()->confirmTwoFactorAuth($request->code); | |
| if (!$confirmed) { | |
| return response()->json(['errors' => ['code' => ['Invalid Two Factor Authentication code.']]], 422); | |
| } | |
| return response()->json(); | |
| } |
Comment on lines
+69
to
+93
| // Login doit retourner twoFA=true et challenge_token | ||
| $response = $this->json('POST', '/api/login', [ | ||
| 'email' => 'administrator@gaelo.fr', | ||
| 'password' => 'administrator' | ||
| ]); | ||
|
|
||
| $content = json_decode($response->content(), true); | ||
|
|
||
| $this->assertTrue($content['twoFA']); | ||
| $this->assertArrayHasKey('challenge_token', $content); | ||
|
|
||
| $challengeToken = $content['challenge_token']; | ||
|
|
||
| // Mocker TwoFactorAuthenticationProvider | ||
| // pour simuler un code TOTP valide sans vrai secret | ||
| $this->mock(TwoFactorAuthenticationProvider::class, function ($mock) { | ||
| $mock->shouldReceive('verify')->once()->andReturn(true); | ||
| }); | ||
|
|
||
| // Challenge 2FA avec le challenge_token récupéré | ||
| $response = $this->json('POST', '/api/two-factor-challenge', [ | ||
| 'challenge_token' => $challengeToken, | ||
| 'code' => '000000' // pas important car le provider est mocké | ||
| ]); | ||
|
|
There was a problem hiding this comment.
These 2FA flow tests don’t assert that the login/challenge endpoints were successful before asserting on the JSON body. Adding ->assertSuccessful() (or explicit expected status codes) will make failures clearer and prevent false positives when an endpoint returns an error JSON shape.
Comment on lines
+18
to
+19
| 'guard' => 'sanctum', | ||
|
|
There was a problem hiding this comment.
fortify.guard is set to sanctum, but this repository’s config/auth.php does not define a sanctum guard (only web and api). Since Fortify expects this to be a valid guard name from auth.guards, either add a sanctum guard definition in auth.php or update this setting to a guard that actually exists to prevent runtime misconfiguration.
| use Illuminate\Auth\Access\AuthorizationException; | ||
| use Illuminate\Routing\UrlGenerator; | ||
| use Laravel\Fortify\Contracts\TwoFactorAuthenticationProvider; | ||
| use Laravel\Fortify\Actions\GenerateNewRecoveryCodes; |
There was a problem hiding this comment.
GenerateNewRecoveryCodes is imported but not used in this controller. Please remove the unused import to avoid dead dependencies/confusion about which Fortify code path is used.
Suggested change
| use Laravel\Fortify\Actions\GenerateNewRecoveryCodes; |
salimkanoun
commented
Mar 23, 2026
Collaborator
Author
salimkanoun
left a comment
salimkanoun
left a comment
There was a problem hiding this comment.
Cf commentaire
|
|
||
| return response()->json([ | ||
| 'onboarded' => $loginResponse->onboarded, | ||
| 'twoFA' => true, |
Collaborator
Author
There was a problem hiding this comment.
mettre needs2FA pour harmoniser avec le cas ou c'est administrator
| $challengeToken = $request->input('challenge_token'); | ||
|
|
||
| if (!$challengeToken) { | ||
| return response()->json(['message' => 'Session expirée.'], 422); |
| $userId = Cache::pull('2fa_challenge_' . $challengeToken); | ||
|
|
||
| if (!$userId) { | ||
| return response()->json(['message' => 'Session expirée ou déjà utilisée.'], 422); |
|
|
||
| if (!$valid) { | ||
| return response()->json([ | ||
| 'errors' => ['code' => ['Code invalide.']] |
| * Must be in the auth:sanctum group but NOT in the onboarded group | ||
| * (admin may not be onboarded yet when setting up 2FA). | ||
| */ | ||
| public function getSetup2FA(Request $request, TwoFactorAuthenticationProvider $provider): JsonResponse |
Collaborator
Author
There was a problem hiding this comment.
Pourquoi get ? setup2FA non ?
Si on crée c'est un POST pas un get
Collaborator
Author
There was a problem hiding this comment.
ou alors getSetup2FAQrCode
Collaborator
Author
There was a problem hiding this comment.
Je mettrai plutot en post pour l'API et le nom de cette fonction setup2FA
|
|
||
| //Route initialization 2FA | ||
| Route::middleware(['auth:sanctum', 'verified', 'activated'])->group(function () { | ||
| Route::get('user/two-factor-setup', [AuthController::class, 'getSetup2FA']); |
| $this->assertEquals('Bearer', $content['token_type']); | ||
| } | ||
|
|
||
| public function testLoginAdministratorWith2FARecoveryCode() |
Collaborator
Author
There was a problem hiding this comment.
et tu devrait ajouter un test qui fail si tu donne pas le bon recovery code, pour check que le code securise bien la validité du recovery code
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 16 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+151
to
+199
| public function setup2FA(Request $request, TwoFactorAuthenticationProvider $provider): JsonResponse | ||
| { | ||
| $user = $request->user(); | ||
|
|
||
| // 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() | ||
| )) | ||
| ])->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}"; | ||
|
|
||
| $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']); | ||
| } |
There was a problem hiding this comment.
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.
Collaborator
Author
There was a problem hiding this comment.
@Xeno-linux il a peut etre raison, on des tests la dessus ?
There was a problem hiding this comment.
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)
Comment on lines
+57
to
+62
| return response()->json([ | ||
| 'id' => $user->id, | ||
| //'onboarded' => $loginResponse->onboarded, | ||
| 'needs2FA' => true | ||
|
|
||
| ]); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Je crois c'est ça qui bloquer mon front (vue que j'avais un 401)
| $this->assertEquals(5, sizeof($responseArray)); | ||
| //Each User has full details | ||
| $this->assertEquals(17, sizeof( array_keys($responseArray[0]) )); | ||
| $this->assertEquals(18, sizeof( array_keys($responseArray[0]) )); |
There was a problem hiding this comment.
This test asserts the number of keys in the user payload (sizeof(array_keys(...))). With the addition of 2FA-related fields, this is likely to keep breaking as the API evolves and doesn’t verify the actual field(s) that matter. Consider asserting the presence/absence of specific expected keys instead (e.g., assert that two_factor_confirmed_at is present for the administrator view).
Comment on lines
+96
to
+101
| // 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); | ||
| } |
There was a problem hiding this comment.
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).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Laravel13
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 32 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $user = $request->user(); | ||
|
|
||
| if($user->id !== $userId){ | ||
| throw new Exception("Can't activate 2FA for a thrid party account"); |
There was a problem hiding this comment.
setup2FA() denies access by throwing a generic Exception when the authenticated user ID doesn’t match the route {id}. Unhandled generic exceptions will surface as 500s; this should return a controlled 403 (e.g., abort(403, ...) or throw AuthorizationException). Also fix the typo in the message (“third”).
Suggested change
| throw new Exception("Can't activate 2FA for a thrid party account"); | |
| abort(403, "Can't activate 2FA for a third party account"); |
Comment on lines
+204
to
+206
| if($user->id !== $userId){ | ||
| throw new Exception("Can't activate 2FA for a thrid party account"); | ||
| } |
There was a problem hiding this comment.
confirmSetup2FA() uses the same generic Exception for authorization failures, which will likely produce a 500 response. Return/throw a proper 403 (e.g., abort(403, ...) or AuthorizationException) so clients get a consistent auth error.
| $user = $request->user(); | ||
|
|
||
| if($user->id !== $userId){ | ||
| throw new Exception("Can't generate recovery codes for a thrid party account"); |
There was a problem hiding this comment.
generateRecoveryCodes2FA() throws a generic Exception when {id} doesn’t match the authenticated user. This should be a 403 response, not a 500; use abort(403, ...) / AuthorizationException like delete2FA() does.
Suggested change
| throw new Exception("Can't generate recovery codes for a thrid party account"); | |
| abort(403, "Can't generate recovery codes for a thrid party account"); |
salimkanoun
commented
Apr 3, 2026
| $userEntity->emailVerifiedAt = $array['email_verified_at']; | ||
| $userEntity->lastConnection = $array['last_connection']; | ||
| $userEntity->onboardingVersion = $array['onboarding_version']; | ||
| $userEntity->twoFactorConfirmedAt = $array['two_factor_confirmed_at'] ?? null; |
Collaborator
Author
There was a problem hiding this comment.
@Xeno-linux Passer en boolean is2FaEnabled
Comment on lines
+39
to
+40
| 'two_factor_confirmed_at' | ||
| ])->toArray(); |
Collaborator
Author
There was a problem hiding this comment.
enelver les hidder pour eviter l'appel a makeVisible
Comment on lines
+148
to
+151
| return $user->makeVisible([ | ||
| 'two_factor_secret', | ||
| 'two_factor_recovery_codes', | ||
| 'two_factor_confirmed_at' |
Collaborator
Author
There was a problem hiding this comment.
enelver les hidder pour eviter l'appel a makeVisible
Comment on lines
+177
to
+179
| 'two_factor_recovery_codes' => encrypt(json_encode( | ||
| collect(range(1, 8))->map(fn() => Str::random(10) . '-' . Str::random(10))->all() | ||
| )) |
Collaborator
Author
There was a problem hiding this comment.
@Xeno-linux généré mais non output, ne pas générer ici les recovery code
Comment on lines
+272
to
+274
| $user->forceFill(['two_factor_secret' => Null])->save(); | ||
| $user->forceFill(['two_factor_recovery_codes' => Null])->save(); | ||
| $user->forceFill(['two_factor_confirmed_at' => Null])->save(); |
Comment on lines
36
to
41
| protected $hidden = [ | ||
| 'remember_token' | ||
| 'remember_token', | ||
| 'two_factor_secret', | ||
| 'two_factor_recovery_codes', | ||
| 'two_factor_confirmed_at', | ||
| ]; |
Collaborator
Author
There was a problem hiding this comment.
enlever le hidden on gere la visibilité vers le front dans notre code
|
|
||
|
|
||
| //Route 2FA | ||
| Route::post('/two-factor-challenge', [AuthController::class, 'twoFactorChallenge'])->middleware('throttle:public-apis'); |
Collaborator
Author
There was a problem hiding this comment.
Séparer en deux routes
/two-factor-challenge/totp
/two-factor-challenge/recovery-code
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 32 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+158
to
+165
| /** | ||
| * 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. | ||
| */ | ||
| public function setup2FA(Request $request, TwoFactorAuthenticationProvider $provider, int $userId): JsonResponse | ||
| { | ||
| $user = $request->user(); |
There was a problem hiding this comment.
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.
Comment on lines
+1
to
+7
| { | ||
| "ubuntu": { | ||
| "300": "ubuntu_300_c45603aff308508f7844bb0eb50312a5", | ||
| "normal": "ubuntu_normal_586fc05279d08db69a17eeb5f9ccbb6d", | ||
| "500": "ubuntu_500_59133bb6d75a9ff112a46f540911904c", | ||
| "bold": "ubuntu_bold_c41075d7a21980b166df8847c9303a6e" | ||
| } |
There was a problem hiding this comment.
This file hard-codes absolute, environment-specific paths like /var/www/storage/fonts/..., which will be incorrect in CI and non-container deployments and makes the repository non-portable. Consider generating this file at runtime, storing relative paths, or omitting it from version control if it is an artifact (dompdf font cache).
Comment on lines
16
to
40
| "directorytree/imapengine-laravel": "^1.1", | ||
| "giggsey/libphonenumber-for-php": "^9.0", | ||
| "guzzlehttp/guzzle": "^7.9", | ||
| "laravel/framework": "^12.0", | ||
| "laravel/fortify": "^1.36", | ||
| "laravel/framework": "^13.0", | ||
| "laravel/sanctum": "^4.0", | ||
| "laravel/tinker": "^2.10", | ||
| "laravel/tinker": "^3.0", | ||
| "league/flysystem-ftp": "^3.29", | ||
| "league/flysystem-sftp-v3": "^3.0", | ||
| "league/mime-type-detection": "^1.16", | ||
| "maennchen/zipstream-php": "^3.1", | ||
| "phpoffice/phpspreadsheet": "^5.0", | ||
| "sentry/sentry-laravel": "^4.13", | ||
| "spatie/db-dumper": "^3.8", | ||
| "spatie/db-dumper": "^4.0", | ||
| "staudenmeir/eloquent-has-many-deep": "^1.21" | ||
| }, | ||
| "require-dev": { | ||
| "fakerphp/faker": "^1.24", | ||
| "laravel/telescope": "^5.5", | ||
| "mockery/mockery": "^1.6", | ||
| "nunomaduro/collision": "^8.6", | ||
| "pestphp/pest": "^3.0", | ||
| "pestphp/pest": "^4.0", | ||
| "phpstan/phpstan": "^2.1", | ||
| "phpunit/phpunit": "^11.0", | ||
| "phpunit/phpunit": "^12.0", | ||
| "spatie/laravel-ignition": "^2.9" |
There was a problem hiding this comment.
composer.json changes dependency constraints (Laravel framework/tinker/pest/phpunit/spatie db-dumper) but this PR doesn’t include an updated composer.lock. For reproducible builds (and to keep CI consistent), regenerate and commit composer.lock along with these version bumps.
Comment on lines
+70
to
+80
| // Mocke TwoFactorAuthenticationProvider | ||
| // to simulate TOTP code validation without secret | ||
| $this->mock(TwoFactorAuthenticationProvider::class, function ($mock) { | ||
| $mock->shouldReceive('verify')->once()->andReturn(true); | ||
| }); | ||
|
|
||
| // Challenge 2FA | ||
| $response = $this->json('POST', '/api/two-factor-challenge', [ | ||
| 'challenge_token' => $challengeToken, | ||
| 'code' => '000000' // not important because the provider is mocke | ||
| ]); |
There was a problem hiding this comment.
Minor typos in comments: “Mocke” / “mocke” should be “Mock” / “mocked” (or similar). Keeping test comments clean helps readability when these tests fail in CI.
| $user = $request->user(); | ||
|
|
||
| if($user->id !== $userId){ | ||
| throw new Exception("Can't activate 2FA for a thrid party account"); |
There was a problem hiding this comment.
Typo in error message: “thrid party account” should be “third party account”. Also consider returning a 403/authorization error rather than throwing a generic exception.
Suggested change
| throw new Exception("Can't activate 2FA for a thrid party account"); | |
| abort(403, "Can't activate 2FA for a third party account"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.