Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -163,3 +163,8 @@ L5_SWAGGER_CONST_TOKEN_URL='/oauth2/token'
# L5_FORMAT_TO_USE_FOR_DOCS=yaml
L5_SWAGGER_GENERATE_ALWAYS=true # Dev setting
L5_SWAGGER_OPEN_API_SPEC_VERSION=3.1.2

# 2FA recovery-code enrollment (SDS idp-mfa.md §4.12, §4.14)
TWO_FACTOR_RECOVERY_ENROLLMENT_WINDOW=600
TWO_FACTOR_MAX_ENROLL_REQUESTS=5
TWO_FACTOR_ENROLL_WINDOW_MINUTES=15
2 changes: 2 additions & 0 deletions .github/workflows/pull_request_frontend_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ jobs:
php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-oauth2-consent@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-oauth2-trust@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-enroll@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-profile@test.com '1Qaz2wsx!'
php artisan idp:create-oauth2-test-client
- name: Install Playwright Chromium
run: npx playwright install --with-deps chromium
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/push_frontend_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ jobs:
php artisan idp:create-super-admin mfa-oauth2@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-oauth2-consent@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-oauth2-trust@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-enroll@test.com '1Qaz2wsx!'
php artisan idp:create-super-admin mfa-profile@test.com '1Qaz2wsx!'
php artisan idp:create-oauth2-test-client
- name: Install Playwright Chromium
run: npx playwright install --with-deps chromium
Expand Down
99 changes: 92 additions & 7 deletions app/Http/Controllers/Api/UserApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use App\Http\Controllers\UserValidationRulesFactory;
use App\ModelSerializers\SerializerRegistry;
use App\Services\Auth\IRecoveryCodeService;
use App\Services\Auth\RecoveryCodesBatch;
use Auth\Repositories\IUserRepository;
use Auth\User;
use Exception;
Expand Down Expand Up @@ -282,13 +283,18 @@ public function enableTwoFactor()
$user = Auth::user();
$method = $data['method'];

if ($user->isTwoFactorEnabled()) {
return $this->error412(['method' => ['Two-factor authentication is already enabled. Use the regenerate recovery codes endpoint to rotate your codes.']]);
// shouldRequire2FA() covers both a stored two_factor_enabled flag and
// group enforcement. An enforced admin must not mint a batch here without
// the password (and would leave two_factor_enabled=true behind, keeping
// them enrolled after leaving the group): they get codes through enroll
// (inside the enrollment window) or regenerate (with the password).
if ($user->shouldRequire2FA()) {
return $this->error412(['method' => ['Two-factor authentication is already required for this account. Use the recovery codes section to set up or regenerate your codes.']]);
}

$codes = $this->recovery_code_service->enableTwoFactorAndGenerateCodes($user, $method);
$batch = $this->recovery_code_service->enableTwoFactorAndGenerateCodes($user, $method);

return $this->ok(['recovery_codes' => $codes]);
return $this->ok($this->batchPayload($user, $batch));
});
}

Expand All @@ -305,20 +311,99 @@ public function regenerateRecoveryCodes()

return $this->processRequest(function () {
$data = Request::all();
// current_password is enforced by the service: required for a user
// with a password; a user without one is accepted instead inside the
// enrollment window (SDS idp-mfa.md §4.10.3).
$validator = Validator::make($data, [
'current_password' => 'required|string',
'current_password' => 'sometimes|nullable|string',
]);

if (!$validator->passes()) {
return $this->error412($validator->getMessageBag()->getMessages());
}

$codes = $this->recovery_code_service->regenerateRecoveryCodes(Auth::user(), $data['current_password']);
$user = Auth::user();
$batch = $this->recovery_code_service->regenerateRecoveryCodes($user, $data['current_password'] ?? null);

return $this->ok($this->batchPayload($user, $batch));
});
}

/**
* Generates a recovery-code batch without the password for a user who
* requires enrollment, right after a real 2FA challenge (SDS idp-mfa.md
* §4.10.3). Rate limited by 2fa.rate:enroll, keyed by the authenticated user.
*
* @return \Illuminate\Http\JsonResponse|mixed
*/
public function enrollRecoveryCodes()
{
if (!Auth::check())
return $this->error403();

return $this->ok(['recovery_codes' => $codes]);
return $this->processRequest(function () {
$user = Auth::user();

if (!$this->recovery_code_service->requiresRecoveryCodeEnrollment($user)) {
return $this->error412(['recovery_codes' => ['Recovery codes enrollment is not required for this account.']]);
}

if (!$this->recovery_code_service->isWithinEnrollmentWindow($user)) {
return $this->error412(['recovery_codes' => ['Your last two-factor verification is too old. Sign out and sign in again, then set up your recovery codes.']]);
}

$batch = $this->recovery_code_service->generateRecoveryCodes($user);

return $this->ok($this->batchPayload($user, $batch));
});
}

/**
* Confirms that the user saved the batch they were shown ("I've Saved These
* Codes"). The confirmation is bound to the exact batch.
*
* @return \Illuminate\Http\JsonResponse|mixed
*/
public function acknowledgeRecoveryCodes()
{
if (!Auth::check())
return $this->error403();

return $this->processRequest(function () {
$validator = Validator::make(Request::all(), [
'batch_id' => 'required|integer',
]);

if (!$validator->passes()) {
return $this->error412($validator->getMessageBag()->getMessages());
}

$user = Auth::user();

if (!$this->recovery_code_service->acknowledgeRecoveryCodes($user, (int)Request::input('batch_id'))) {
return $this->error412(['batch_id' => ['This is not your current batch of recovery codes. Generate a new one.']]);
}

return $this->ok($this->recovery_code_service->getStatus($user)->toArray());
});
}

/**
* 200 body of every generation endpoint: the plaintext codes, the batch
* identifier produced by the generating transaction and the recovery-codes
* standing (RecoveryCodesStatus keys), so the UI updates without a reload.
*/
private function batchPayload(User $user, RecoveryCodesBatch $batch): array
{
return array_merge(
[
'recovery_codes' => $batch->getCodes(),
'batch_id' => $batch->getBatchId(),
],
$this->recovery_code_service->getStatus($user)->toArray()
);
}

public function revokeAllMyTokens()
{
if (!Auth::check())
Expand Down
54 changes: 51 additions & 3 deletions app/Http/Controllers/UserController.php
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,7 @@ public function verify2FA()

// Second factor verified: establish the session.
$this->auth_service->loginUser($user, $pending->shouldRemember());
$this->markMFACompleted($user);

if ($trust_device) {
// Best-effort: the OTP is already redeemed and the session
Expand Down Expand Up @@ -806,8 +807,21 @@ public function verify2FA()
// code with no way to recover it client-side. A real top-level navigation to
// this URL lets the browser complete that chain natively instead - CORS never
// applies to page navigations, only to XHR/fetch.
// The recovery-codes standing rides along so the login page knows when
// to show the enrollment modal (recovery_codes_enrollment_required).
$redirect = $this->login_strategy->postLogin();
return $this->ok(['redirect_url' => $redirect->getTargetUrl()]);
return $this->ok(array_merge(
[
'redirect_url' => $redirect->getTargetUrl(),
// Auth::login() regenerates the session id AND its CSRF token
// (SessionGuard::updateSession() -> Store::regenerate()), so the
// token the login page was rendered with is stale from here on.
// The enrollment calls the page makes next (enroll / acknowledge)
// must send this one.
'csrf_token' => csrf_token(),
],
$this->recovery_code_service->getStatus($user)->toArray()
));
} catch (ValidationException $ex) {
Log::warning($ex);
return $this->error412($ex->getMessages());
Expand Down Expand Up @@ -881,6 +895,7 @@ public function verify2FARecovery()
}

$this->auth_service->loginUser($user, $pending->shouldRemember());
$this->markMFACompleted($user);
$strategy->clearPendingState();
$this->clearMFAUISessionState();

Expand All @@ -904,7 +919,15 @@ public function verify2FARecovery()
// when they've just burned into their last few codes (see RecoveryCodesStatus).
$redirect = $this->login_strategy->postLogin();
return $this->ok(array_merge(
['redirect_url' => $redirect->getTargetUrl()],
[
'redirect_url' => $redirect->getTargetUrl(),
// Auth::login() regenerates the session id AND its CSRF token
// (SessionGuard::updateSession() -> Store::regenerate()), so the
// token the login page was rendered with is stale from here on.
// The enrollment calls the page makes next (enroll / acknowledge)
// must send this one.
'csrf_token' => csrf_token(),
],
$this->recovery_code_service->getStatus($user)->toArray()
));
} catch (ValidationException $ex) {
Expand All @@ -916,6 +939,23 @@ public function verify2FARecovery()
}
}

/**
* Records that this session just completed a real 2FA challenge for the user
* (SDS idp-mfa.md §4.10.3). Called only by verify2FA / verify2FARecovery,
* after AuthService::loginUser() succeeded: Auth::login() migrates the
* session id but keeps its data, so the key survives, and writing it before a
* failed login would leave a stale marker. It bounds the window in which the
* enrollment endpoint may generate a batch without the password. Trusted-device
* sign-ins and every other session establishment never write it.
*/
private function markMFACompleted(User $user): void
{
Session::put(MFAConstants::SESSION_KEY_COMPLETED_AT, [
'user_id' => $user->getId(),
'at' => time(),
]);
}

/**
* Re-issues a 2FA challenge for the pending login and returns the challenge payload.
*
Expand Down Expand Up @@ -1163,6 +1203,9 @@ public function getProfile()
$lang2Code[] = $lang;
}

$status = $this->recovery_code_service->getStatus($user);
$window_open = $this->recovery_code_service->isWithinEnrollmentWindow($user);

return View::make("profile", array_merge([
'user' => json_encode(SerializerRegistry::getInstance()->getSerializer(
$user, SerializerRegistry::SerializerType_Private)->serialize()),
Expand All @@ -1172,7 +1215,12 @@ public function getProfile()
'countries' => CountryList::getCountries(),
'languages' => $lang2Code,
'two_factor_enabled' => $user->shouldRequire2FA(),
], $this->recovery_code_service->getStatus($user)->toArray()));
// The session completed a 2FA challenge within the enrollment window:
// a batch can be generated without the password.
'recovery_codes_window_open' => $window_open,
// Enrollment is required AND reachable now: the panel offers "Set up recovery codes".
'recovery_codes_enrollment_available' => $status->isEnrollmentRequired() && $window_open,
], $status->toArray()));
}

public function deleteTrustedSite($id)
Expand Down
9 changes: 9 additions & 0 deletions app/Http/Middleware/Authenticate.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,15 @@ public function handle($request, Closure $next, $guard = null)
$current_user->getGroupsNice()
)
);
// An XHR that expects JSON must reach its controller: the first
// authenticated request after a deep-link sign-in would otherwise be
// answered with a 302 to backurl, and the caller would read an HTML page
// instead of its JSON (e.g. the recovery-code enrollment call made by
// the login page). backurl stays in the session for the real navigation.
if ($request->expectsJson()) {
return $next($request);
}

$redirect = Session::get('backurl');
if (!empty($redirect)) {
Session::forget('backurl');
Expand Down
34 changes: 34 additions & 0 deletions app/Repositories/DoctrineUserRecoveryCodeRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,38 @@ public function deleteAllForUser(User $user): int
->setParameter('user', $user);
return (int) $qb->getQuery()->execute();
}

public function lockUserRow(User $user): void
{
// SELECT ... FOR UPDATE with HINT_REFRESH: re-hydrates the managed User
// from the locked row. EntityManager::lock() alone takes the lock but
// leaves the in-memory state untouched, so a value changed by a
// concurrent transaction (recovery_codes_acknowledged_at) would be stale.
$this->getEntityManager()
->createQueryBuilder()
->select('u')
->from(User::class, 'u')
->where('u.id = :id')
->setParameter('id', $user->getId())
->getQuery()
->setLockMode(\Doctrine\DBAL\LockMode::PESSIMISTIC_WRITE)
->setHint(\Doctrine\ORM\Query::HINT_REFRESH, true)
->getOneOrNullResult();
}

public function getCurrentBatchId(User $user): ?int
{
$em = $this->getEntityManager();
$em->flush();

$id = $em->createQueryBuilder()
->select('MIN(c.id)')
->from(UserRecoveryCode::class, 'c')
->where('c.user = :user')
->setParameter('user', $user)
->getQuery()
->getSingleScalarResult();

return is_null($id) ? null : (int) $id;
}
}
Loading
Loading