diff --git a/README.md b/README.md index 7a6c275..f0e9519 100644 --- a/README.md +++ b/README.md @@ -173,12 +173,18 @@ What it will not do: produces a code before it holds a session. Enrolled is enrolled, whichever door you came through. -**Who has to enrol TOTP here.** An account whose only way in is the provider — no -local password — is exempt: the provider is the sole gate, and the second factor -is the provider's to enforce. Enforce MFA there, because this console has stopped -asking for one. An account that keeps a password alongside SSO is *not* exempt and -must still enrol, because that password is a way in the provider never sees. -Enrolment stays available to everyone either way, exempt or not. +**Who has to enrol TOTP here.** Any account linked to the provider is exempt — +including one an administrator created locally and later signed into with SSO. The +second factor is the provider's to enforce for those accounts, so enforce MFA +there: this console has stopped asking. Accounts the provider knows nothing about, +which is every account on an install without SSO, are unchanged. + +The exemption follows the account rather than the session, which has a cost worth +naming: a linked account that keeps a local password can be entered by that +password without the provider seeing it, and so without its MFA. **Switching +password sign-in off is what closes that**, and is the intended configuration for +an install relying on the provider for its second factor. Enrolment stays +available to everyone either way, exempt or not. ## What is new relative to `mwdeploy` diff --git a/app/Http/Middleware/RequireTwoFactor.php b/app/Http/Middleware/RequireTwoFactor.php index 377b64a..64b55e5 100644 --- a/app/Http/Middleware/RequireTwoFactor.php +++ b/app/Http/Middleware/RequireTwoFactor.php @@ -17,11 +17,10 @@ * Read-only accounts are unaffected, and the enrolment screen itself is exempt * so people can actually get enrolled. * - * Accounts that can only be entered through the identity provider are exempt too - * — see User::requiresTwoFactor(). They have no password, so the provider is the - * only gate and the second factor is the provider's to enforce; an account that - * keeps a password alongside is still required to enrol, because that password is - * a way in the provider never sees. + * Accounts linked to the identity provider are exempt too — see + * User::requiresTwoFactor(). The second factor is the provider's to enforce for + * them, which is why an install relying on that should also switch password + * sign-in off: a local password is a way in the provider never sees. */ final class RequireTwoFactor { diff --git a/app/Models/User.php b/app/Models/User.php index af40e55..5da92db 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -122,20 +122,22 @@ public function apps(): array * True when this account can change production and therefore must have TOTP * enrolled *here*. * - * Accounts that can only be entered through the identity provider are exempt: - * they have no password, so the provider is the sole gate, and second-factor - * policy is the provider's to enforce. Demanding a second, console-local - * factor on top would be asking someone to carry two authenticators for one - * sign-in, and section 3.5.1 is about there being a second factor at all — - * not about this application owning it. + * Accounts linked to the identity provider are exempt. Second-factor policy + * is the provider's to enforce for them, and demanding a second, + * console-local factor on top is asking someone to carry two authenticators + * for one sign-in. Section 3.5.1 is about there being a second factor at all + * — not about this application owning it. * - * An account that also has a password is not exempt, and that distinction is - * the whole point: a password is a way in that never touches the provider, so - * whatever MFA the provider enforces is not on that path. + * The exemption follows the account, not the session, and that is a deliberate + * choice with a cost: a linked account that *also* keeps a local password can + * be entered by that password without the provider seeing it, and so without + * whatever MFA the provider enforces. Switching password sign-in off (see + * OidcSettings::passwordLoginAllowed()) is what closes that, and is the + * intended configuration for an install that relies on this. */ public function requiresTwoFactor(): bool { - if ($this->signsInOnlyWithSingleSignOn()) { + if ($this->usesSingleSignOn()) { return false; } @@ -143,11 +145,11 @@ public function requiresTwoFactor(): bool } /** - * Whether the identity provider is this account's only way in. + * Whether this account is linked to the identity provider. */ - public function signsInOnlyWithSingleSignOn(): bool + public function usesSingleSignOn(): bool { - return $this->oidc_subject !== null && ! filled($this->password); + return $this->oidc_subject !== null; } public function hasTwoFactorEnabled(): bool diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index 9bd23bc..14c6ff9 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -339,7 +339,9 @@ Then in the UI, in this order: proven — keep one local account, and know that `CONSOLE_FORCE_PASSWORD_LOGIN=true` in `.env` puts the form back on the day the IdP is what broke. Enforce MFA at the provider: accounts whose only way - in is SSO are not asked to enrol TOTP here. The console also links an existing + in is SSO are not asked to enrol TOTP here — nor is any account linked to it, + so switch the password form off if you are relying on the provider for the + second factor. The console also links an existing account on the email address the provider states, without requiring an `email_verified` claim — which assumes an internal provider that approves access to this application, so keep it one. diff --git a/resources/js/console/pages/AuthenticationSettingsPage.vue b/resources/js/console/pages/AuthenticationSettingsPage.vue index 3000006..0fc2e4e 100644 --- a/resources/js/console/pages/AuthenticationSettingsPage.vue +++ b/resources/js/console/pages/AuthenticationSettingsPage.vue @@ -497,9 +497,10 @@ const save = async () => {

{{ linkedAccounts }} account{{ linkedAccounts === 1 ? '' : 's' }} currently sign in this - way. They have no password, so the provider is their only gate — enforce MFA there, - because this console does not ask those accounts to enrol TOTP. An account that keeps a - password alongside single sign-on still has to. + way. This console does not ask any of them to enrol TOTP — the second factor is the + provider's to enforce, so make sure it does. An account that keeps a password alongside + can still be entered without the provider seeing it, which is what switching password + sign-in off above closes.

diff --git a/tests/Feature/TwoFactorEnforcementTest.php b/tests/Feature/TwoFactorEnforcementTest.php index 6d6111a..0e9bcd6 100644 --- a/tests/Feature/TwoFactorEnforcementTest.php +++ b/tests/Feature/TwoFactorEnforcementTest.php @@ -114,13 +114,13 @@ public function a_guest_hitting_the_api_is_refused(): void } #[Test] - public function an_account_that_can_only_sign_in_through_the_provider_is_not_forced_to_enrol(): void + public function an_account_linked_to_the_provider_is_not_forced_to_enrol(): void { /* - * The provider is that account's only gate — it has no password — so the - * second factor is the provider's to enforce, and asking someone to carry - * a second authenticator for one sign-in buys nothing. What it does buy - * is a console its own SSO accounts cannot reach. + * The second factor is the provider's to enforce for these accounts, and + * asking someone to carry a second authenticator for one sign-in buys + * nothing. What it does buy is a console its own SSO accounts cannot + * reach. */ $user = $this->userWithPermissions([Permissions::DEPLOY_CORE], twoFactor: false); $user->forceFill(['password' => null, 'oidc_subject' => 'sso-1'])->save(); @@ -133,16 +133,32 @@ public function an_account_that_can_only_sign_in_through_the_provider_is_not_for } #[Test] - public function an_sso_account_that_also_has_a_password_must_still_enrol(): void + public function a_linked_account_that_kept_its_password_is_exempt_too(): void { /* - * The distinction that makes the exemption safe: a password is a way in - * the provider never sees, so whatever MFA the provider enforces is not - * on that path. + * The common case, and the one the first pass got wrong: an account an + * administrator created locally and then signed into with the provider. + * The exemption follows the account, not the session. + * + * The cost is real and deliberate — that password is a way in the + * provider never sees, so its MFA is not on that path. Switching password + * sign-in off is what closes it. */ $user = $this->userWithPermissions([Permissions::DEPLOY_CORE], twoFactor: false); $user->forceFill(['oidc_subject' => 'sso-2'])->save(); + $this->assertFalse($user->fresh()->requiresTwoFactor()); + + $this->actingAs($user->fresh())->get('/')->assertOk(); + } + + #[Test] + public function a_local_account_with_no_provider_link_must_still_enrol(): void + { + // The requirement is unchanged for accounts the provider knows nothing + // about, which is every account on an install without single sign-on. + $user = $this->userWithPermissions([Permissions::DEPLOY_CORE], twoFactor: false); + $this->assertTrue($user->fresh()->requiresTwoFactor()); $this->actingAs($user->fresh())->get('/')->assertRedirect(route('two-factor.setup'));