-
Notifications
You must be signed in to change notification settings - Fork 4
2 fa #782
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
2 fa #782
Changes from 8 commits
5a81d68
5be2036
f7d8c3d
b282882
497ccf1
5460d88
b1e4d68
0f871e0
a8df842
397fc7c
cc879f5
71ff67d
bf42684
a686fd4
9a01561
eca8a98
c190e81
42085c0
0e0e6cb
24a9993
235b9cc
25ff289
4dbffb4
88faad2
9caf865
83f3e04
dff7bf7
df7548a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ on: | |
| branches: | ||
| - GaelO2 | ||
| - GaelO2-dev | ||
| - imap-lib | ||
| - 2FA | ||
| tags: | ||
| - '*' | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,8 @@ class UserEntity | |
|
|
||
| public ?array $roles; | ||
|
|
||
| public ?string $twoFactorConfirmedAt; | ||
|
|
||
| public static function fillFromDBReponseArray(array $array): UserEntity | ||
| { | ||
| $userEntity = new UserEntity(); | ||
|
|
@@ -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; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Xeno-linux Passer en boolean is2FaEnabled |
||
| return $userEntity; | ||
|
salimkanoun marked this conversation as resolved.
|
||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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(); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. enelver les hidder pour eviter l'appel a makeVisible
salimkanoun marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| public function delete($id): void | ||
|
|
@@ -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' | ||
|
salimkanoun marked this conversation as resolved.
Outdated
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. enelver les hidder pour eviter l'appel a makeVisible |
||
| ])->toArray(); | ||
|
salimkanoun marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| public function isExistingEmail(String $email): bool | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 | ||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||
|
|
@@ -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(); | ||||||||||||||||||||||||||||||||
|
salimkanoun marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||||||
| $isAdmin = $user->administrator; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| if ($isAdmin && !$use2FA) { | ||||||||||||||||||||||||||||||||
| return response()->json([ | ||||||||||||||||||||||||||||||||
| 'id' => $user->id, | ||||||||||||||||||||||||||||||||
| //'onboarded' => $loginResponse->onboarded, | ||||||||||||||||||||||||||||||||
| 'needs2FA' => true | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||
| 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.
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 ?
There was a problem hiding this comment.
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)
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
Copilot
AI
Apr 3, 2026
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Copilot
AI
Mar 24, 2026
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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)
Copilot
AI
Apr 3, 2026
There was a problem hiding this comment.
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.
| // 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]); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
||
|
salimkanoun marked this conversation as resolved.
|
||
| /** | ||
| * The attributes that are mass assignable. | ||
|
|
@@ -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', | ||
| ]; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
|
||
| /** | ||
|
|
@@ -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' | ||
| ]; | ||
| } | ||
|
|
||
|
|
||
| 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']]); | ||
|
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); | ||
| }); | ||
|
salimkanoun marked this conversation as resolved.
|
||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.