diff --git a/README.md b/README.md index 2114ad1..7a6c275 100644 --- a/README.md +++ b/README.md @@ -121,17 +121,20 @@ What it does with what the provider says: subjects do not. An account already linked to one subject is never relinked to another on the strength of an email address — that is an administrator's call, made by clearing the old subject. -* **An existing account is only claimed on a verified email.** Otherwise an IdP - that lets someone type an arbitrary address into a profile page would be a way to - walk into an account that already exists. Linking keeps that account's roles, - its TOTP enrolment and its deployment history. - - `email_verified` is optional in the spec, and several providers — Authentik among - them — never send it. **Trust email addresses the provider does not mark as - verified** covers that case, and is on by default: without it, an account that - already exists here could never be linked. A provider that explicitly answers - `email_verified: false` is refused either way — that is it telling you it does - not vouch for the address. +* **An existing account is claimed on its email address**, and `email_verified` is + not consulted. Linking keeps that account's roles, its TOTP enrolment and its + deployment history. + + This assumes what is true of the provider it is built for: an internal IdP that + gates access to this application behind manual approval, so the address it states + is exactly as trustworthy as the identity it states. Point the console at a + provider that hands out accounts to strangers and that assumption is gone — + someone could then set their profile email to a colleague's and inherit their + roles. The claim is still read, and still decides whether the local address + counts as verified; it just no longer decides who may sign in. + + An account already linked to a *different* subject is never re-claimed on an + email address, whatever the provider says. * **Groups map to roles, never to permissions.** A group lands someone in the same bucket an administrator would have put them in by hand, so `/access` keeps telling the whole truth about what an account can do. diff --git a/app/Http/Controllers/Api/OidcSettingsController.php b/app/Http/Controllers/Api/OidcSettingsController.php index cbbcf6c..ef86587 100644 --- a/app/Http/Controllers/Api/OidcSettingsController.php +++ b/app/Http/Controllers/Api/OidcSettingsController.php @@ -90,7 +90,6 @@ public function update(Request $request): JsonResponse 'scopes' => ['required', 'string', 'max:255'], 'groups_claim' => ['required', 'string', 'max:120'], - 'trust_provider_email' => ['required', 'boolean'], 'create_users' => ['required', 'boolean'], 'sync_roles' => ['required', 'boolean'], @@ -121,7 +120,6 @@ public function update(Request $request): JsonResponse 'end_session_endpoint' => $validated['end_session_endpoint'] ?? null, 'scopes' => $validated['scopes'], 'groups_claim' => $validated['groups_claim'], - 'trust_provider_email' => $validated['trust_provider_email'], 'create_users' => $validated['create_users'], 'sync_roles' => $validated['sync_roles'], 'allowed_groups' => array_values(array_filter( @@ -279,7 +277,6 @@ private function payload(): array 'end_session_endpoint' => $settings->end_session_endpoint, 'scopes' => (string) $settings->scopes, 'groups_claim' => (string) $settings->groups_claim, - 'trust_provider_email' => (bool) $settings->trust_provider_email, 'create_users' => (bool) $settings->create_users, 'sync_roles' => (bool) $settings->sync_roles, 'allowed_groups' => $settings->allowedGroupList(), diff --git a/app/Models/OidcSettings.php b/app/Models/OidcSettings.php index ff3405d..8184e63 100644 --- a/app/Models/OidcSettings.php +++ b/app/Models/OidcSettings.php @@ -23,7 +23,7 @@ 'enabled', 'label', 'password_login_enabled', 'discovery_url', 'issuer', 'client_id', 'client_secret', 'authorization_endpoint', 'token_endpoint', 'userinfo_endpoint', 'jwks_uri', 'end_session_endpoint', 'scopes', - 'groups_claim', 'trust_provider_email', 'create_users', 'sync_roles', + 'groups_claim', 'create_users', 'sync_roles', 'allowed_groups', 'discovered_at', ])] #[Hidden(['client_secret'])] @@ -44,7 +44,6 @@ final class OidcSettings extends Model 'password_login_enabled' => true, 'scopes' => 'openid profile email groups', 'groups_claim' => 'groups', - 'trust_provider_email' => true, 'create_users' => true, 'sync_roles' => true, ]; @@ -57,7 +56,6 @@ protected function casts(): array return [ 'enabled' => 'boolean', 'password_login_enabled' => 'boolean', - 'trust_provider_email' => 'boolean', 'create_users' => 'boolean', 'sync_roles' => 'boolean', 'allowed_groups' => 'array', @@ -148,19 +146,6 @@ public function passwordLoginAllowed(): bool return (bool) $this->password_login_enabled; } - /** - * Whether an address the provider stated without an `email_verified` claim - * may be treated as verified. - * - * Only ever consulted for a *missing* claim. An explicit - * `email_verified: false` is the provider saying it does not vouch for the - * address, and that is refused regardless — see OidcUserProvisioner. - */ - public function trustsProviderEmail(): bool - { - return (bool) $this->trust_provider_email; - } - /** * @return list */ diff --git a/app/Services/Oidc/OidcIdentity.php b/app/Services/Oidc/OidcIdentity.php index e2c3e1e..f274c74 100644 --- a/app/Services/Oidc/OidcIdentity.php +++ b/app/Services/Oidc/OidcIdentity.php @@ -24,18 +24,14 @@ private function __construct( public readonly string $subject, public readonly string $email, /** - * Whether the address may be treated as verified — which is not the same - * question as what the provider said. See `$emailVerifiedStated`. + * Whether the provider vouched for the address. + * + * Nothing is gated on this — it decides whether the local + * `email_verified_at` is set, and that is all. The claim is optional in + * the spec and plenty of providers, Authentik among them, never send it, + * so a false here means "did not say" as often as it means "said no". */ public readonly bool $emailVerified, - /** - * What the provider actually said, in three states: true, false, or null - * for a provider that sent no `email_verified` claim at all. The claim is - * optional in the spec and plenty of providers omit it, so the difference - * between "said no" and "said nothing" decides both what is allowed and - * what the person signing in is told. - */ - public readonly ?bool $emailVerifiedStated, public readonly string $name, public readonly array $groups, ) {} @@ -46,48 +42,18 @@ private function __construct( public static function fromClaims(array $claims, OidcSettings $settings): self { $email = (string) (Arr::get($claims, 'email') ?? ''); - $stated = self::readEmailVerified($claims); return new self( subject: (string) Arr::get($claims, 'sub'), email: mb_strtolower(trim($email)), - /* - * A provider that says nothing is governed by this install's setting; - * one that says `false` is refused whatever the setting is, because - * that is the provider telling us it does not vouch for the address. - */ - emailVerified: $stated ?? $settings->trustsProviderEmail(), - emailVerifiedStated: $stated, + // Providers are loose with the type: a JSON boolean, the strings + // "true" and "false", and 1/0 are all seen in the wild. + emailVerified: filter_var(Arr::get($claims, 'email_verified'), FILTER_VALIDATE_BOOLEAN), name: self::readName($claims, $email), groups: self::readGroups($claims, (string) $settings->groups_claim), ); } - /** - * The `email_verified` claim, as the three states it really has. - * - * Providers are loose with the type: a JSON boolean, the strings "true" and - * "false", and 1/0 are all seen in the wild. Absent is the state that matters - * most, and it must not be confused with false — which is what - * `filter_var(..., FILTER_VALIDATE_BOOLEAN)` on a missing key does. - * - * @param array $claims - */ - private static function readEmailVerified(array $claims): ?bool - { - if (! Arr::has($claims, 'email_verified')) { - return null; - } - - $value = Arr::get($claims, 'email_verified'); - - if ($value === null || $value === '') { - return null; - } - - return filter_var($value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE) ?? false; - } - /** * Group membership, from whichever claim this install's IdP puts it in. * diff --git a/app/Services/Oidc/OidcUserProvisioner.php b/app/Services/Oidc/OidcUserProvisioner.php index b2f9fd6..26ff7a7 100644 --- a/app/Services/Oidc/OidcUserProvisioner.php +++ b/app/Services/Oidc/OidcUserProvisioner.php @@ -18,9 +18,12 @@ * * * **Who this is.** The subject claim, and only the subject claim, once an * account has one. Emails get reassigned; `sub` does not. - * * **Whether an existing account may be claimed.** Only on a *verified* email. - * An IdP that lets someone set an unverified address would otherwise be a way - * to walk into an existing account by typing its email into a profile page. + * * **Whether an existing account may be claimed.** On the email address, and + * the `email_verified` claim is not consulted. That check assumed a provider + * which hands out accounts to strangers; this one is internal and gates access + * to the application behind manual approval, so the address it states is + * exactly as trustworthy as the identity it states. An account already linked + * to a *different* subject is still never re-claimed — see below. * * **What they may do.** Whatever the group mapping says, and nothing else. * Permissions are never read from the IdP directly — groups map to roles, and * roles are what the access screen already shows. @@ -93,24 +96,6 @@ private function findOrCreate(OidcIdentity $identity, OidcSettings $settings): U : User::query()->whereRaw('LOWER(email) = ?', [$identity->email])->first(); if ($byEmail !== null) { - if (! $identity->emailVerified) { - /* - * Two different failures, and telling them apart is the - * difference between an administrator changing one setting and an - * administrator hunting through logs. `false` is the provider - * declining to vouch for the address; null is a provider that - * sends no `email_verified` claim at all, on an install that has - * not said to trust it. - */ - throw OidcException::because( - $identity->emailVerifiedStated === false - ? 'Your identity provider says your email address is not verified, so it cannot be linked to the existing account here. Verify it with the provider, or ask an administrator to link the account.' - : 'Your identity provider did not say whether your email address is verified, and this console is set not to assume it. An administrator can allow it on the sign-in settings screen, or link the account by hand.', - 'refused to claim account '.$byEmail->getKey().': email_verified was ' - .($identity->emailVerifiedStated === false ? 'false' : 'absent'), - ); - } - /* * An account already claimed by a different subject is not * re-claimed on the strength of an email address. Reaching here @@ -163,6 +148,8 @@ private function findOrCreate(OidcIdentity $identity, OidcSettings $settings): U // callback refuses password sign-in for these accounts outright. $user->password = null; $user->oidc_subject = $identity->subject; + // Recorded, not required: the claim decides whether the local address + // counts as verified, and no longer who may sign in. $user->email_verified_at = $identity->emailVerified ? now() : null; $user->oidc_synced_at = now(); $user->save(); @@ -192,7 +179,7 @@ private function refresh(User $user, OidcIdentity $identity): User ->whereKeyNot($user->getKey()) ->exists(); - if ($emailIsFree && $identity->emailVerified) { + if ($emailIsFree) { $user->email = $identity->email; } diff --git a/database/migrations/2026_07_31_090000_drop_trust_provider_email_from_oidc_settings.php b/database/migrations/2026_07_31_090000_drop_trust_provider_email_from_oidc_settings.php new file mode 100644 index 0000000..9d7cfcd --- /dev/null +++ b/database/migrations/2026_07_31_090000_drop_trust_provider_email_from_oidc_settings.php @@ -0,0 +1,39 @@ +dropColumn('trust_provider_email'); + }); + } + + public function down(): void + { + Schema::table('oidc_settings', function (Blueprint $table) { + $table->boolean('trust_provider_email')->default(true)->after('groups_claim'); + }); + } +}; diff --git a/docs/OPERATIONS.md b/docs/OPERATIONS.md index dba1dba..9bd23bc 100644 --- a/docs/OPERATIONS.md +++ b/docs/OPERATIONS.md @@ -339,7 +339,10 @@ 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. + in is SSO are not asked to enrol TOTP here. 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. 8. A **staging-only** deployment of one small extension, as a smoke test, before anything targets production. diff --git a/resources/js/console/pages/AuthenticationSettingsPage.vue b/resources/js/console/pages/AuthenticationSettingsPage.vue index f7165f3..3000006 100644 --- a/resources/js/console/pages/AuthenticationSettingsPage.vue +++ b/resources/js/console/pages/AuthenticationSettingsPage.vue @@ -483,26 +483,6 @@ const save = async () => { - - idp->configure(['trust_provider_email' => true]); - - $existing = $this->userWithPermissions([Permissions::DEPLOY_CORE], twoFactor: false); + $this->idp->configure(); - // No email_verified key at all, as against one set to false. - $this->signIn(['sub' => 'authentik-1', 'email' => $existing->email]); + foreach ([[], ['email_verified' => true], ['email_verified' => false]] as $index => $claim) { + $existing = User::factory()->create(['password' => null]); - $this->assertAuthenticatedAs($existing->fresh()); - $this->assertSame('authentik-1', $existing->fresh()->oidc_subject); - } + $this->signIn(['sub' => 'authentik-'.$index, 'email' => $existing->email, ...$claim]); - #[Test] - public function a_missing_email_verified_claim_is_refused_when_the_provider_is_not_trusted(): void - { - $this->idp->configure(['trust_provider_email' => false]); + $this->assertAuthenticatedAs($existing->fresh()); + $this->assertSame('authentik-'.$index, $existing->fresh()->oidc_subject); - $existing = $this->userWithPermissions([Permissions::DEPLOY_CORE], twoFactor: false); - - $this->signIn(['sub' => 'authentik-2', 'email' => $existing->email]) - ->assertRedirect(route('login')) - ->assertSessionHasErrors('oidc'); - - $this->assertGuest(); - $this->assertNull($existing->fresh()->oidc_subject); + $this->post(route('logout')); + } } #[Test] - public function an_explicit_email_verified_false_is_refused_even_when_the_provider_is_trusted(): void + public function the_local_verified_timestamp_still_follows_the_claim(): void { - // Trusting a provider that says nothing is a decision about a gap. A - // provider that says "no" has answered, and the answer stands. - $this->idp->configure(['trust_provider_email' => true]); + // Read, recorded, and not gated on: the claim decides whether the local + // address counts as verified, and nothing else. + $this->idp->configure(); - $existing = $this->userWithPermissions([Permissions::DEPLOY_CORE], twoFactor: false); + $this->signIn(['sub' => 'stated-true', 'email' => 'stated@wikioasis.org', 'email_verified' => true]); + $this->assertNotNull(User::query()->where('oidc_subject', 'stated-true')->sole()->email_verified_at); - $this->signIn(['sub' => 'attacker', 'email' => $existing->email, 'email_verified' => false]) - ->assertRedirect(route('login')) - ->assertSessionHasErrors('oidc'); + $this->post(route('logout')); - $this->assertGuest(); - $this->assertNull($existing->fresh()->oidc_subject); + $this->signIn(['sub' => 'said-nothing', 'email' => 'silent@wikioasis.org']); + $this->assertNull(User::query()->where('oidc_subject', 'said-nothing')->sole()->email_verified_at); } #[Test] @@ -458,21 +450,6 @@ public function claims_the_token_leaves_out_are_read_from_userinfo(): void $this->assertSame(['ops'], $existing->fresh(['roles'])->roles->pluck('name')->all()); } - #[Test] - public function an_unverified_email_cannot_claim_an_existing_account(): void - { - $this->idp->configure(); - - $existing = $this->userWithPermissions([Permissions::DEPLOY_CORE]); - - $this->signIn(['sub' => 'attacker', 'email' => $existing->email, 'email_verified' => false]) - ->assertRedirect(route('login')) - ->assertSessionHasErrors('oidc'); - - $this->assertGuest(); - $this->assertNull($existing->fresh()->oidc_subject); - } - #[Test] public function account_creation_can_be_switched_off(): void { diff --git a/tests/Feature/OidcSettingsTest.php b/tests/Feature/OidcSettingsTest.php index 8324f4b..560a2f6 100644 --- a/tests/Feature/OidcSettingsTest.php +++ b/tests/Feature/OidcSettingsTest.php @@ -49,7 +49,6 @@ private function payload(array $overrides = []): array 'jwks_uri' => 'https://sso.example.test/application/o/console/jwks/', 'scopes' => 'openid profile email groups', 'groups_claim' => 'groups', - 'trust_provider_email' => true, 'password_login_enabled' => true, 'create_users' => true, 'sync_roles' => true, @@ -170,7 +169,6 @@ public function a_configuration_can_be_saved_while_switched_off(): void 'password_login_enabled' => true, 'scopes' => 'openid profile email groups', 'groups_claim' => 'groups', - 'trust_provider_email' => true, 'create_users' => true, 'sync_roles' => true, ])