diff --git a/Makefile b/Makefile index 7476ff34a..ae3e5b578 100644 --- a/Makefile +++ b/Makefile @@ -323,7 +323,7 @@ e2e-playground: ## Run the live-playground e2e suite (OIDC/SAML/SCIM/SSO/OAuth/M docker compose -f e2e-playground/docker-compose.yml build; \ status=$$?; \ if [ $$status -eq 0 ]; then \ - docker compose -f e2e-playground/docker-compose.yml up -d --wait authorizer authorizer-sso mock-oauth mock-saml-idp mailpit sms-sink; \ + docker compose -f e2e-playground/docker-compose.yml up -d --wait authorizer authorizer-sso authorizer-email-verify mock-oauth mock-saml-idp mailpit sms-sink; \ status=$$?; \ fi; \ if [ $$status -eq 0 ]; then \ diff --git a/cmd/root.go b/cmd/root.go index c9a92105c..de870ae8a 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -248,6 +248,8 @@ func init() { f.StringVar(&rootArgs.config.MicrosoftClientSecret, "microsoft-client-secret", "", "Client secret for Microsoft") f.StringVar(&rootArgs.config.MicrosoftTenantID, "microsoft-tenant-id", defaultMicrosoftTenantID, "Tenant ID for Microsoft") f.StringSliceVar(&rootArgs.config.MicrosoftScopes, "microsoft-scopes", defaultMicrosoftScopes, "Scopes for Microsoft") + f.StringSliceVar(&rootArgs.config.MicrosoftAllowedTenants, "microsoft-allowed-tenants", nil, "Entra tenant IDs allowed to sign in when --microsoft-tenant-id is a multi-tenant alias (common/organizations/consumers). Empty allows any tenant, but an untrusted tenant's email will not link to an existing account") + f.BoolVar(&rootArgs.config.OAuthAllowUnverifiedProviderEmail, "oauth-allow-unverified-provider-email", false, "Compatibility escape hatch: let a social login whose provider did not attest the email address sign up or return to an account that same provider already owns. It still cannot cross into an account another credential owns. Prefer pinning --microsoft-tenant-id or enabling the xms_edov claim; see docs/email-verification-contract.md") f.StringVar(&rootArgs.config.TwitchClientID, "twitch-client-id", "", "Client ID for Twitch") f.StringVar(&rootArgs.config.TwitchClientSecret, "twitch-client-secret", "", "Client secret for Twitch") f.StringSliceVar(&rootArgs.config.TwitchScopes, "twitch-scopes", defaultTwitchScopes, "Scopes for Twitch") @@ -455,6 +457,26 @@ func runRoot(c *cobra.Command, args []string) { log.Warn().Msg("--encryption-key is not set and has fallen back to --jwt-secret. Secrets at rest (TOTP seeds, OTP digests) are keyed by the same value that signs tokens, so rotating --jwt-secret will lock out every enrolled TOTP user — there is no re-encryption path. Set a distinct --encryption-key now; doing it after users enrol requires them to re-enrol.") } + // Email verification with no way to send email is an unrecoverable trap, not + // a degraded mode: signup creates the account unverified, the verification + // mail never leaves, and every self-service route out of that state (the + // signup link, resend-verification, the login email-OTP fallback) is the + // same mailbox. The user is stranded permanently, and an unverified account + // also blocks a federated login for the same address. Fail at boot, where + // the operator can see it, rather than silently per-user. + if rootArgs.config.EnableEmailVerification && !rootArgs.config.IsEmailServiceEnabled { + log.Fatal().Msg("--enable-email-verification=true requires a working email service, but SMTP is not configured. Users would be created unverified with no way to ever verify. Set --smtp-host, --smtp-port and --smtp-sender-email, or disable email verification.") + } + + // The compatibility escape hatch for unattested federated emails. It is + // narrowed (an unattested address still cannot cross into an account another + // credential owns), but it leaves same-provider collisions open — two Entra + // tenants asserting one address. Warn every boot so it does not quietly + // become permanent. + if rootArgs.config.OAuthAllowUnverifiedProviderEmail { + log.Warn().Msg("--oauth-allow-unverified-provider-email is set: a social login whose provider does not attest the email address may still sign up or return to an account that same provider owns. Cross-credential takeover is still blocked, but two principals of the SAME provider (e.g. two Entra tenants) can collide on one address. Pin --microsoft-tenant-id, set --microsoft-allowed-tenants, or enable the xms_edov optional claim, then remove this flag. See docs/email-verification-contract.md.") + } + // Initialize prometheus metrics metrics.Init() diff --git a/docs/email-verification-contract.md b/docs/email-verification-contract.md new file mode 100644 index 000000000..dda99b6ad --- /dev/null +++ b/docs/email-verification-contract.md @@ -0,0 +1,327 @@ +# Email verification contract + +Authorizer resolves a local account from an email address in several flows — +password signup, magic link, and every social/enterprise federated login. This +document states when an address counts as **verified**, why federated logins are +held to that bar, and what operators need to configure. + +> **Behaviour change in 2.4.0.** A social login whose provider does not attest +> the email address is now refused. See [Upgrading](#upgrading) for what to +> configure if this affects your deployment. + +## Why an address must be attested + +OAuth 2.0 carries no identity claims at all. OpenID Connect added `email` — and +alongside it a **separate `email_verified` boolean**, precisely because `email` +on its own proves nothing about who controls the mailbox. + +That distinction matters here because the email address is what selects the +local account. `GetUserByEmail` decides signup-vs-login, and the login branch +merges the incoming federated identity into whatever account already holds the +address. If an attacker can make a provider assert an address they do not own, +they land in that account's session. + +This is the **nOAuth** attack class. Microsoft Entra is the sharp case: + +- Entra **v2 ID tokens carry no `email_verified` claim at all**. +- Entra's `email` is a *mutable, unverified* directory attribute — any tenant + admin can set it to any string, including someone else's address. +- The multi-tenant endpoints (`common`, `organizations`, `consumers`) sign with + Microsoft's **global** keys, so a token minted in a free attacker-owned tenant + has a valid signature and a valid `aud`. Only the tenant distinguishes it. + +So: register a free Entra tenant, set a user's `email` to `victim@example.com`, +click "Login with Microsoft", and without this contract you are in the victim's +account. + +## The three connection classes + +Authorizer treats connections the same way Auth0 does: + +| Connection class | Is the address attested? | Behaviour | +|---|---|---| +| **Database** (password signup) | No — nobody has vouched for it | `email_verified` is `false` until the user clicks the verification link Authorizer mails them (requires `--enable-email-verification=true`; with verification disabled the address is marked verified at signup) | +| **Social** (Google, Apple, GitHub, …) | Usually yes — the provider vouches | The provider's own signal is imported directly; no separate verification round-trip | +| **Enterprise / Azure AD / OIDC** | **Not guaranteed** — enterprise directories do not promise it | Requires an explicit trust decision from the operator (see below) | + +## Per-provider signal + +Each provider is read from the signal that provider actually emits. There is no +single claim that works everywhere, and treating one provider's silence as +another's "true" is exactly what created the vulnerability. + +| Provider | Signal | Notes | +|---|---|---| +| Google | `email_verified` (ID token) | Standard OIDC | +| Apple | `email_verified` (ID token) | Documented as "a string or Boolean value" — both forms accepted | +| Twitch | `email_verified` (ID token) | Standard OIDC | +| LinkedIn | `email_verified` (userinfo) | Both bool and quoted-string forms accepted | +| Microsoft | `xms_edov`, **or** a trusted tenant | No `email_verified` exists on Entra v2 — see below | +| GitHub | the address is verified by construction | The public `/user` email and the `/user/emails` fallback are both filtered to verified addresses | +| Discord | `verified` (`/users/@me`) | Synthetic fallback address is trusted by construction | +| Facebook | trusted | Graph API only returns a `email` for an account with a confirmed primary address | +| Twitter/X | trusted | `confirmed_email` is confirmed by definition; synthetic fallback is trusted by construction | +| Roblox | `email_verified` (userinfo) | Synthetic fallback is trusted by construction | + +**"Trusted by construction"** means the synthetic fallback addresses +(`discord-@discord.oauth.internal`, and the Twitter/Roblox equivalents) live +on reserved, non-routable domains keyed by the provider's permanent user id. No +real mailbox can occupy them, so they can never collide with an address someone +could otherwise prove they own. + +## Microsoft / Entra specifics + +Two independent things make a Microsoft address trustworthy. Either is enough: + +1. **`xms_edov`** — "email domain owner verified", Microsoft's own attestation + that the token's tenant owns the address's domain. It is an *optional claim*; + enable it in the app registration's token configuration. +2. **A tenant the operator trusts** — the address can then only have come from a + directory you already control or vouch for. + +A tenant is trusted when either: + +- `--microsoft-tenant-id` is a **specific** tenant (a GUID or verified domain + name) rather than one of the multi-tenant aliases `common`, `organizations`, + `consumers`; or +- `--microsoft-allowed-tenants` is set and the token's `tid` is in it. + +Independently of the trust decision, every Microsoft ID token must satisfy: + +- `tid` is present; +- `iss` equals `https://login.microsoftonline.com//v2.0` — a token may not + claim one tenant in `iss` and another in `tid`; +- if a specific tenant is pinned, `tid` matches it; +- if an allowlist is configured, `tid` is in it. + +`--microsoft-tenant-id` defaults to `common`. A deployment left on that default +with no allowlist and no `xms_edov` will now **refuse** Microsoft logins rather +than accept an unattested address. That is deliberate: it is exactly the +exploitable configuration. + +## Configuration + +``` +--microsoft-tenant-id= + Default: common. A specific value pins the tenant and makes its addresses + trustworthy. + +--microsoft-allowed-tenants=, + Entra tenant IDs permitted to sign in when --microsoft-tenant-id is a + multi-tenant alias. Empty allows any tenant, but an untrusted tenant's + email will not link to an existing account. + +--enable-email-verification=true + Database connections only: require the user to click a mailed link before + the address counts as verified. + +--oauth-allow-unverified-provider-email=false + Compatibility escape hatch for 2.3.x upgrades. See below. +``` + +## Compatibility mode + +`--oauth-allow-unverified-provider-email` exists so a deployment upgrading from +2.3.x is not locked out the moment it restarts. It is deliberately **not** a +plain "turn the check off" switch — that would restore the vulnerability +verbatim. + +With the flag set, an unattested address may: + +- create a **brand-new** account — it selects nobody, so it harms nobody; or +- return to an account **this same provider already owns** — a returning user. + +It may **never** merge into an account another credential owns. That one +restriction removes the entire cross-credential takeover: an Entra tenant cannot +reach a password account, a Google account, or any other provider's account, +which is every practical form of the attack. + +**Residual risk it does not cover**, and the reason this mode is temporary: two +principals of the *same* unattested provider — two Entra tenants both asserting +one address — can still collide. Pinning `--microsoft-tenant-id` or setting +`--microsoft-allowed-tenants` closes that, and is the actual fix. The server logs +a warning on every boot while the flag is set. + +### Knock-on effects of compatibility mode + +Two behaviours change to keep compatibility mode honest: + +- **`email_verified` in our own database reflects reality.** A social signup used + to write `email_verified=true` unconditionally. It now does so only when the + provider attested the address, because downstream consumers trust that column + (SAML IdP issuance, for one, refuses to assert an unverified email as the + Subject NameID). An account created in compatibility mode from an unattested + address is stored as unverified — which is the truth. +- **The pre-hijack guard is scoped to other credentials.** That guard deletes an + *unverified* pre-existing account rather than linking to it. It now fires only + when the account was created by a *different* method. An unverified account + this same provider already owns is not a squatter, it is the same principal's + own account — deleting it would recreate the account on every login, silently + dropping its id, roles and org memberships each time. The squatter case + (attacker pre-registers `victim@example.com` by password, victim later signs in + via Google) is unaffected. + +## The pre-hijack delete is now bounded + +Independent of the verification contract, that guard's deletion is now limited +to accounts that hold no state. + +The replacement account is created with a fresh id, so deleting a real account +still **destroys** everything that account owned — its FGA grants, org +memberships, enrolled authenticators, passkeys and federated identities. The +rows are no longer *orphaned* (see below), but they are gone, and the new +account inherits none of them. + +A squatter's account is empty by definition — created to intercept an address and +never used. So before deleting, the callback checks for org memberships, +passkeys, enrolled authenticators and FGA grants. If it finds any, it refuses the +login instead: + +```json +{ + "error": "email_already_registered", + "error_description": "An unverified account already exists for this email address. Verify it or sign in with the method that created it." +} +``` + +Refusing is recoverable; deleting is not. A lookup fault counts as "has state" +for the same reason. The delete, when it does happen, now emits an +`oauth.unverified_account_replaced` audit event carrying the destroyed user id. + +> **Resolved ([#747](https://github.com/authorizerdev/authorizer/issues/747)):** +> `StorageProvider.DeleteUser` used to cascade to sessions and nothing else, so +> every hard delete — including the admin `_delete_user` path — left orphaned +> rows pointing at a dead user id. The worst of them, an orphaned +> `authorizer_federated_identities` row, was a permanent SSO lockout: +> `jitProvisionFederatedUser` resolves a returning principal through it, fails +> closed when the user is gone, and the unique `(org_id, issuer, subject)` triple +> blocks re-provisioning. +> +> The cascade now covers every collection in `schemas.UserOwnedCollections` +> (sessions, federated identities, org memberships, authenticators, passkeys, +> session tokens, MFA sessions) on all six backends, and the admin delete +> additionally purges the user's FGA tuples. Soft deletes (`deactivate_account`, +> revoke access) do **not** cascade — the account is meant to come back. +> +> The `accountHasState` bound above is kept regardless: refusing is still +> recoverable and deleting still is not. + +## How a user verifies their address + +**Signup already mails a verification link.** With `--enable-email-verification`, +`signup` creates the verification request and sends the mail before returning +"Verification email has been sent. Please check your inbox". Clicking that link +is the normal path and needs nothing else. + +The link is valid for 30 minutes. If it expires or never arrives: + +| Route | Who drives it | Notes | +|---|---|---| +| **`resend_verify_email`** | the user | The primary recovery. Mints a fresh link for the same address. | +| **Password login** | the user | An unverified account's password login emails an OTP instead; verifying that OTP marks the address verified (`verify_otp.go`). | +| **`_update_user { email_verified: true }`** | an admin | The operator escape hatch when the user genuinely cannot receive mail. | +| Forgot password | the user | Completing a token reset also verifies the address — a side effect of proving mailbox control, not the route to reach for. | + +Two fixes make the table above actually hold: + +- **`resend_verify_email` no longer dead-ends.** It required a *pending* + verification request. Expired rows are still returned by + `GetVerificationRequestByEmail` (there is no expiry filter), so the usual + expired-link case already worked — but a password-login attempt **purges** the + expired row (`login.go`), and after that the endpoint silently did nothing. + It now mints a fresh request when none exists, gated on the address actually + being unverified so it cannot be used as an open mailer. +- **Admin force-verify works standalone.** `_update_user`'s "at least one param" + gate omitted `email_verified` and `phone_number_verified` even though both are + applied further down, so a call setting only `email_verified` was rejected + unless padded with an unrelated field. + +### Hard requirement: email verification needs a working email service + +`--enable-email-verification=true` with no SMTP configured is now a **fatal +startup error**. Every route in the table above terminates at the same mailbox, +so without a mail path a user is created unverified and can never recover — and +an unverified account also blocks a federated login for that address. Configure +`--smtp-host`, `--smtp-port` and `--smtp-sender-email`, or turn email +verification off. + +### This is not the same as Auth0's post-login email check + +A post-login Action like: + +```js +exports.onExecutePostLogin = async (event, api) => { + if (!event.user.email_verified) { + api.access.deny('Please verify your email address before logging in.'); + } +}; +``` + +is a **login policy**: "should this user, whoever they are, be let in before +confirming their own address?" It is reasonably opt-in, and Authorizer's +equivalent is `--enable-email-verification`. + +What this document describes is **identity resolution**: "which local account +does this federated assertion refer to?" Getting that wrong does not inconvenience +the legitimate user — it hands their account to somebody else. Which is why the +default is secure and the escape hatch is narrowed rather than total. + +## What a refusal looks like + +The callback returns `400` before any local account lookup, so no account is +created and no existing account is touched: + +```json +{ + "error": "email_not_verified", + "error_description": "The identity provider did not confirm that you own this email address." +} +``` + +A `oauth_email_unverified` security metric and an audit event are recorded. + +## Upgrading + +Most deployments need no change — Google, Apple, GitHub, Discord, Facebook, +LinkedIn, Twitch, Twitter and Roblox all supply a signal already. + +**If you use Microsoft login**, pick one: + +- pin `--microsoft-tenant-id` to your tenant (single-tenant deployments — the + common case, and the best option); +- set `--microsoft-allowed-tenants` to the tenants you serve (multi-tenant SaaS); +- enable the `xms_edov` optional claim in your Entra app registration. + +If you need more time, set `--oauth-allow-unverified-provider-email=true` as a +stopgap — existing users keep working and cross-credential takeover stays +blocked. It is not a substitute for one of the three fixes above; see +[Compatibility mode](#compatibility-mode). + +## Tests + +| What | Where | +|---|---| +| Claim decoding, tenant validation, `xms_edov` | `internal/http_handlers/oauth_noauth_test.go` | +| Social vouches / enterprise refused / nOAuth takeover attempt | `e2e-playground/tests/social/email-verification-contract.spec.ts` | +| Database connection stays unverified until the link is clicked | `e2e-playground/tests/email-verification-database.spec.ts` | +| `resend_verify_email` recovery, and that it is not an open mailer | `e2e-playground/tests/email-verification-database.spec.ts` | +| The rendered web/app journey: signup form → "check your inbox" → click link | `e2e-playground/tests/email-verification-ui.spec.ts` | +| Purpose binding, resend, and reset-verifies-email | `internal/integration_tests/verification_token_purpose_test.go` | + +## Operator actions in the dashboard + +The Users table (`web/dashboard`) exposes both operator routes per user, shown +only when the relevant identifier is actually unverified: + +- **Mark Email Verified** — asserts the address is good without mailing anything. + Sends only `email_verified`; deliberately not `email`, since that param drives + the change-address flow in `_update_user` (which clears verification and mails + a new link). +- **Resend Verification Email** — mails a fresh link so the user proves it + themselves. Preferred when the admin has no independent reason to trust the + address. +- **Mark Phone Verified** — the phone equivalent. + +These were previously a single "Verify User" item that appeared only when *both* +the email and the phone were unverified, so a user with a verified phone and an +unverified email had no path at all. diff --git a/e2e-playground/docker-compose.yml b/e2e-playground/docker-compose.yml index 6b6cad8a7..3d61a244e 100644 --- a/e2e-playground/docker-compose.yml +++ b/e2e-playground/docker-compose.yml @@ -254,6 +254,54 @@ services: timeout: 2s retries: 30 + # authorizer-email-verify is the only instance combining basic-auth signup with + # --enable-email-verification=true. It exists because those two cannot coexist + # anywhere else: the shared `authorizer` service has verification OFF (dozens of + # specs rely on signup returning a session immediately), and both magic-link + # instances have it ON but also --enable-magic-link-login, which makes + # AuthorizerSignup hide the password form entirely (web/app renders no signup + # inputs at all). Without this service the rendered "signup -> check your inbox + # -> click link" journey — the user-facing half of the email-verification + # contract, see docs/email-verification-contract.md — is untestable through the UI. + authorizer-email-verify: + build: + context: .. + dockerfile: Dockerfile + ports: + - "8086:8080" + command: + - "--http-port=8080" + - "--url=http://authorizer-email-verify:8080" + - "--database-type=sqlite" + - "--database-url=/authorizer/e2e-test-email-verify.db" + - "--admin-secret=e2e-admin-secret" + - "--jwt-type=HS256" + - "--jwt-secret=e2e-jwt-secret-do-not-use-in-prod" + - "--client-id=e2e-client-id" + - "--client-secret=e2e-client-secret" + - "--enable-signup=true" + - "--enable-email-verification=true" + # Keeps the rendered signup form on the password path (magic link would + # replace it) and keeps a fresh signup off the MFA-offer screen, so the + # "check your inbox" assertion is not racing an interstitial. + - "--disable-mfa=true" + - "--app-cookie-secure=false" + - "--admin-cookie-secure=false" + - "--app-cookie-same-site=lax" + - "--allowed-origins=http://localhost:8086,http://authorizer-email-verify:8080" + - "--smtp-host=mailpit" + - "--smtp-port=1025" + - "--smtp-sender-email=e2e@authorizer.test" + - "--rate-limit-rps=1000" + - "--rate-limit-burst=1000" + depends_on: + mailpit: { condition: service_started } + healthcheck: + test: ["CMD", "wget", "-qO-", "http://localhost:8080/healthz"] + interval: 2s + timeout: 2s + retries: 30 + # authorizer-mfa-enforced is a fifth instance solely for # tests/mfa-routing-matrix.spec.ts (the `mfa-on` project). EnforceMFA # turned out NOT to be a runtime-toggleable admin setting - Task 28 @@ -530,6 +578,7 @@ services: AUTHORIZER_SSO_BASE_URL: http://authorizer-sso:8080 AUTHORIZER_WEBAUTHN_BASE_URL: http://webauthn.e2e-playground.test:8080 AUTHORIZER_MAGIC_LINK_BASE_URL: http://authorizer-magic-link:8080 + AUTHORIZER_EMAIL_VERIFY_BASE_URL: http://authorizer-email-verify:8080 AUTHORIZER_MFA_ENFORCED_BASE_URL: http://authorizer-mfa-enforced:8080 AUTHORIZER_MFA_MAGIC_LINK_BASE_URL: http://authorizer-mfa-magic-link:8080 # Two replicas of ONE deployment over a shared Postgres — see the @@ -553,6 +602,7 @@ services: authorizer-sso: { condition: service_healthy } authorizer-webauthn: { condition: service_healthy } authorizer-magic-link: { condition: service_healthy } + authorizer-email-verify: { condition: service_healthy } authorizer-mfa-enforced: { condition: service_healthy } authorizer-mfa-magic-link: { condition: service_healthy } authorizer-replica-a: { condition: service_healthy } diff --git a/e2e-playground/fixtures/adminClient.ts b/e2e-playground/fixtures/adminClient.ts index c170331e1..316936932 100644 --- a/e2e-playground/fixtures/adminClient.ts +++ b/e2e-playground/fixtures/adminClient.ts @@ -153,15 +153,15 @@ export async function getUserIdByEmail(email: string): Promise { // just that a session was established. export async function getUserByEmail( email: string -): Promise<{ id: string; email: string | null; given_name: string | null; family_name: string | null; signup_methods: string }> { +): Promise<{ id: string; email: string | null; given_name: string | null; family_name: string | null; signup_methods: string; email_verified: boolean }> { const query = gql` query ($params: ListUsersRequest) { - _users(params: $params) { users { id email given_name family_name signup_methods } } + _users(params: $params) { users { id email given_name family_name signup_methods email_verified } } } `; const res = await client.request<{ _users: { - users: { id: string; email: string | null; given_name: string | null; family_name: string | null; signup_methods: string }[]; + users: { id: string; email: string | null; given_name: string | null; family_name: string | null; signup_methods: string; email_verified: boolean }[]; }; }>(query, { params: { query: email } }); const user = res._users.users.find((u) => u.email === email); diff --git a/e2e-playground/mocks/mock-oauth/server.ts b/e2e-playground/mocks/mock-oauth/server.ts index 43b4fe1fa..8b58a4b38 100644 --- a/e2e-playground/mocks/mock-oauth/server.ts +++ b/e2e-playground/mocks/mock-oauth/server.ts @@ -38,13 +38,17 @@ function defaultProfile(provider: string): Record { // (processDiscordUserInfo, internal/http_handlers/oauth_callback.go, // reads id/username/avatar/email directly - no "user" wrapper, that // was /oauth2/@me's shape, which never includes email). - return { id: '123', username: 'mockuser', avatar: 'abc', email }; + // `verified` is Discord's email-confirmation flag; Authorizer refuses to + // resolve a local account from an address the provider hasn't attested. + return { id: '123', username: 'mockuser', avatar: 'abc', email, verified: true }; case 'twitter': return { data: { id: '123', name: 'Mock User', username: 'mockuser', profile_image_url: 'https://example.com/a.png' } }; case 'roblox': - return { name: 'Mock User', nickname: 'mockuser', picture: 'https://example.com/a.png', email }; + return { name: 'Mock User', nickname: 'mockuser', picture: 'https://example.com/a.png', email, email_verified: true }; default: - return { sub: `mock-${provider}-sub`, email, given_name: 'Mock', family_name: 'User' }; + // OIDC `email_verified` (Core §5.1). Google/Apple/Twitch/Microsoft all + // route through here, and the callback rejects an unattested address. + return { sub: `mock-${provider}-sub`, email, email_verified: true, given_name: 'Mock', family_name: 'User' }; } } diff --git a/e2e-playground/playwright.config.ts b/e2e-playground/playwright.config.ts index 7ae9c8797..92954982b 100644 --- a/e2e-playground/playwright.config.ts +++ b/e2e-playground/playwright.config.ts @@ -21,6 +21,8 @@ export default defineConfig({ /sso-discovery\.spec\.ts/, /webauthn\.spec\.ts/, /magic-link\.spec\.ts/, + /email-verification-database\.spec\.ts/, + /email-verification-ui\.spec\.ts/, // Drives authorizer-replica-a/-b directly by absolute URL rather than // this project's baseURL; see the `replica` project below. /replica-shared-state\.spec\.ts/, @@ -79,12 +81,27 @@ export default defineConfig({ // docker-compose.yml for why those can't live on the shared // `authorizer` service. name: 'magic-link', - testMatch: /magic-link\.spec\.ts/, + // email-verification-database.spec.ts rides along here because this is + // the only instance with --enable-email-verification=true, which is what + // makes the pre-click "email_verified: false" state observable at all. + testMatch: [/magic-link\.spec\.ts/, /email-verification-database\.spec\.ts/], use: { ...devices['Desktop Chrome'], baseURL: process.env.AUTHORIZER_MAGIC_LINK_BASE_URL || 'http://localhost:8083', }, }, + { + // Runs against authorizer-email-verify (docker-compose.yml) — the only + // instance combining basic-auth signup with --enable-email-verification, + // which is what makes the rendered "signup -> check your inbox -> click + // link" journey reachable at all. See that service's comment. + name: 'email-verify', + testMatch: /email-verification-ui\.spec\.ts/, + use: { + ...devices['Desktop Chrome'], + baseURL: process.env.AUTHORIZER_EMAIL_VERIFY_BASE_URL || 'http://localhost:8086', + }, + }, { // Runs against authorizer-replica-a AND authorizer-replica-b — two // replicas of ONE deployment sharing a Postgres (docker-compose.yml). diff --git a/e2e-playground/sdk-tests/python/tests/test_social_oauth.py b/e2e-playground/sdk-tests/python/tests/test_social_oauth.py index 97e55e8fc..fe2ce6b2e 100644 --- a/e2e-playground/sdk-tests/python/tests/test_social_oauth.py +++ b/e2e-playground/sdk-tests/python/tests/test_social_oauth.py @@ -65,7 +65,7 @@ def _case(provider: str) -> Case: email = f"{provider}-{uid}@example.com" p: dict[str, Any] if provider == "google": - p = {"sub": f"google-{uid}", "email": email, "given_name": "Ada", "family_name": "Lovelace"} + p = {"sub": f"google-{uid}", "email": email, "email_verified": True, "given_name": "Ada", "family_name": "Lovelace"} return Case(provider, p, "email", email, "Ada", "Lovelace", email) if provider == "github": p = {"name": "Grace Hopper", "email": email, "avatar_url": "https://example.com/a.png"} @@ -79,24 +79,25 @@ def _case(provider: str) -> Case: } return Case(provider, p, "email", email, "Katherine", "Johnson", email) if provider == "linkedin": - p = {"localizedFirstName": "Margaret", "localizedLastName": "Hamilton", "email": email} + p = {"localizedFirstName": "Margaret", "localizedLastName": "Hamilton", "email": email, "email_verified": True} return Case(provider, p, "email", email, "Margaret", "Hamilton", email) if provider == "apple": - p = {"sub": f"apple-{uid}", "email": email, "given_name": "Alan", "family_name": "Turing"} + p = {"sub": f"apple-{uid}", "email": email, "email_verified": True, "given_name": "Alan", "family_name": "Turing"} return Case(provider, p, "email", email, "Alan", "Turing", email) if provider == "discord": - p = {"id": f"discord-{uid}", "username": "gracehopper", "avatar": "abc", "email": email} + p = {"id": f"discord-{uid}", "username": "gracehopper", "avatar": "abc", "email": email, "verified": True} return Case(provider, p, "email", email, "gracehopper", None, email) if provider == "microsoft": p = { "sub": f"microsoft-{uid}", "email": email, + "email_verified": True, "given_name": "Katherine", "family_name": "Johnson", } return Case(provider, p, "email", email, "Katherine", "Johnson", email) if provider == "twitch": - p = {"sub": f"twitch-{uid}", "email": email, "given_name": "Sally", "family_name": "Ride"} + p = {"sub": f"twitch-{uid}", "email": email, "email_verified": True, "given_name": "Sally", "family_name": "Ride"} return Case(provider, p, "email", email, "Sally", "Ride", email) if provider == "roblox": p = { @@ -104,6 +105,7 @@ def _case(provider: str) -> Case: "nickname": "ada", "picture": "https://example.com/a.png", "email": email, + "email_verified": True, } return Case(provider, p, "email", email, "Ada ", "Lovelace", email) if provider == "twitter": diff --git a/e2e-playground/tests/oidc-sso-rp.spec.ts b/e2e-playground/tests/oidc-sso-rp.spec.ts index abdd8385e..1ea0d16de 100644 --- a/e2e-playground/tests/oidc-sso-rp.spec.ts +++ b/e2e-playground/tests/oidc-sso-rp.spec.ts @@ -57,7 +57,7 @@ test.describe('OIDC — SSO relying party (home-realm discovery)', () => { const employeeEmail = `employee@${domain}`; await request.post(`${MOCK_OAUTH_BASE}/${realm}/__configure`, { - data: { profile: { sub: 'employee-1', email: employeeEmail, given_name: 'Ada', family_name: 'Lovelace' } }, + data: { profile: { sub: 'employee-1', email: employeeEmail, email_verified: true, given_name: 'Ada', family_name: 'Lovelace' } }, }); await page.goto('/app'); diff --git a/e2e-playground/tests/social/apple.spec.ts b/e2e-playground/tests/social/apple.spec.ts index 8401c3863..7ad4b8461 100644 --- a/e2e-playground/tests/social/apple.spec.ts +++ b/e2e-playground/tests/social/apple.spec.ts @@ -27,7 +27,7 @@ test.describe('Social login — Apple', () => { // Authorizer's ctx.Request.FormValue("user") resolves identically to a // POST body. Email still comes off the id_token, matching Apple's real // private relay pattern. - profile: { sub: `apple-${crypto.randomUUID()}`, email, given_name: 'Alan', family_name: 'Turing' }, + profile: { sub: `apple-${crypto.randomUUID()}`, email, email_verified: true, given_name: 'Alan', family_name: 'Turing' }, expectedEmail: email, }); @@ -66,7 +66,7 @@ test.describe('Social login — Apple', () => { await runSocialLoginHappyPath(page, request, { provider: 'apple', buttonName: /apple/i, - profile: { sub: `apple-${crypto.randomUUID()}`, email, given_name: 'Grace', family_name: 'Hopper' }, + profile: { sub: `apple-${crypto.randomUUID()}`, email, email_verified: true, given_name: 'Grace', family_name: 'Hopper' }, expectedEmail: email, }); diff --git a/e2e-playground/tests/social/discord.spec.ts b/e2e-playground/tests/social/discord.spec.ts index 0d6866a64..4426fb28b 100644 --- a/e2e-playground/tests/social/discord.spec.ts +++ b/e2e-playground/tests/social/discord.spec.ts @@ -17,7 +17,7 @@ test.describe('Social login — Discord', () => { await runSocialLoginHappyPath(page, request, { provider: 'discord', buttonName: /discord/i, - profile: { id: discordId, username: 'gracehopper', avatar: 'abc123', email }, + profile: { id: discordId, username: 'gracehopper', avatar: 'abc123', email, verified: true }, expectedEmail: email, }); diff --git a/e2e-playground/tests/social/google.spec.ts b/e2e-playground/tests/social/google.spec.ts index 0e6f87aa9..30e17c24d 100644 --- a/e2e-playground/tests/social/google.spec.ts +++ b/e2e-playground/tests/social/google.spec.ts @@ -14,7 +14,7 @@ test.describe('Social login — Google', () => { // profile is signed into a real id_token (server.ts), and // processGoogleUserInfo (internal/http_handlers/oauth_callback.go) // reads given_name/family_name/email/sub straight off its claims. - profile: { sub: `google-${crypto.randomUUID()}`, email, given_name: 'Ada', family_name: 'Lovelace' }, + profile: { sub: `google-${crypto.randomUUID()}`, email, email_verified: true, given_name: 'Ada', family_name: 'Lovelace' }, expectedEmail: email, }); diff --git a/e2e-playground/tests/social/helpers.ts b/e2e-playground/tests/social/helpers.ts index f2ff6a3d1..cbe851795 100644 --- a/e2e-playground/tests/social/helpers.ts +++ b/e2e-playground/tests/social/helpers.ts @@ -146,3 +146,51 @@ export async function runConsentDeniedNegativePath( const replayBody = await replayRes.json(); expect(replayBody.error).toBe('invalid oauth state'); } + +// runSocialLoginExpectingRejection drives the same real redirect chain as +// runSocialLoginHappyPath, but over the API request context so the callback's +// status code and error body are observable (a browser only ever sees the +// rendered result). Returns the parsed callback response. +// +// Used by the email-verification-contract spec: when an IdP hands back an +// address it has not attested, the callback must refuse BEFORE any local +// account is looked up or created, so there is no session and no account +// mutation to assert away afterwards. +export async function runSocialLoginExpectingRejection( + request: APIRequestContext, + baseURL: string, + opts: { provider: string; profile: Record } +): Promise<{ status: number; body: { error?: string; error_description?: string } }> { + await configureProviderProfile(request, opts.provider, opts.profile); + + // 1. Real login initiation — the same route the rendered social button hits. + const redirectUri = `${baseURL}/app`; + const loginRes = await request.get( + `/oauth_login/${opts.provider}?redirect_uri=${encodeURIComponent(redirectUri)}`, + { maxRedirects: 0 } + ); + expect(loginRes.status()).toBe(307); + const authorizeLocation = loginRes.headers()['location']; + expect(authorizeLocation).toBeTruthy(); + + // 2. Mock provider's /authorize issues a real code and bounces to our callback. + const authorizeRes = await request.get(authorizeLocation!, { maxRedirects: 0 }); + expect(authorizeRes.status()).toBe(302); + const callbackLocation = authorizeRes.headers()['location']; + expect(callbackLocation).toBeTruthy(); + + // 3. The callback: full token exchange + id_token verification happens here, + // then the email-attestation gate. + const callbackURL = new URL(callbackLocation!); + const callbackRes = await request.get(`${callbackURL.pathname}${callbackURL.search}`, { + maxRedirects: 0, + }); + let body: { error?: string; error_description?: string } = {}; + try { + body = await callbackRes.json(); + } catch { + // Non-JSON (a redirect body) means the login was NOT rejected; the caller's + // assertion on status/error will report that. + } + return { status: callbackRes.status(), body }; +} diff --git a/e2e-playground/tests/social/microsoft.spec.ts b/e2e-playground/tests/social/microsoft.spec.ts index 99cc982b7..69f796f88 100644 --- a/e2e-playground/tests/social/microsoft.spec.ts +++ b/e2e-playground/tests/social/microsoft.spec.ts @@ -14,7 +14,7 @@ test.describe('Social login — Microsoft', () => { // profile is signed into a real id_token (server.ts), and // processMicrosoftUserInfo (internal/http_handlers/oauth_callback.go) // reads given_name/family_name/email/sub straight off its claims. - profile: { sub: `microsoft-${crypto.randomUUID()}`, email, given_name: 'Katherine', family_name: 'Johnson' }, + profile: { sub: `microsoft-${crypto.randomUUID()}`, email, email_verified: true, given_name: 'Katherine', family_name: 'Johnson' }, expectedEmail: email, }); diff --git a/e2e-playground/tests/social/roblox.spec.ts b/e2e-playground/tests/social/roblox.spec.ts index 4dc8647e2..2ff13deb4 100644 --- a/e2e-playground/tests/social/roblox.spec.ts +++ b/e2e-playground/tests/social/roblox.spec.ts @@ -14,7 +14,7 @@ test.describe('Social login — Roblox', () => { await runSocialLoginHappyPath(page, request, { provider: 'roblox', buttonName: /roblox/i, - profile: { name: 'Ada Lovelace', nickname: 'ada', picture: 'https://example.com/a.png', email }, + profile: { name: 'Ada Lovelace', nickname: 'ada', picture: 'https://example.com/a.png', email, email_verified: true }, expectedEmail: email, }); diff --git a/e2e-playground/tests/social/twitch.spec.ts b/e2e-playground/tests/social/twitch.spec.ts index 42e9d1b28..d10241990 100644 --- a/e2e-playground/tests/social/twitch.spec.ts +++ b/e2e-playground/tests/social/twitch.spec.ts @@ -16,7 +16,7 @@ test.describe('Social login — Twitch', () => { // idToken.Claims(&user) directly off it - identical mechanism to // processMicrosoftUserInfo, so given_name/family_name map through the // same way despite real Twitch's OIDC token not normally carrying them. - profile: { sub: `twitch-${crypto.randomUUID()}`, email, given_name: 'Sally', family_name: 'Ride' }, + profile: { sub: `twitch-${crypto.randomUUID()}`, email, email_verified: true, given_name: 'Sally', family_name: 'Ride' }, expectedEmail: email, }); diff --git a/internal/authenticators/totp/totp.go b/internal/authenticators/totp/totp.go index 41fe176b0..d04ed5ad1 100644 --- a/internal/authenticators/totp/totp.go +++ b/internal/authenticators/totp/totp.go @@ -3,6 +3,8 @@ package totp import ( "bytes" "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "errors" "image/png" @@ -28,6 +30,13 @@ const ( // totpPendingSecretTTLSeconds bounds how long a generated-but-unconfirmed // secret lingers before the user must restart the re-setup. totpPendingSecretTTLSeconds = 10 * 60 + // totpUsedPasscodePrefix namespaces the single-use claim on an already + // redeemed TOTP passcode. See reserveTOTPPasscode. + totpUsedPasscodePrefix = "totp_used:" + // totpPasscodeReuseWindowSeconds must cover every time-step totp.Validate + // will accept — Period 30 with Skew 1 spans three steps — so a redeemed + // code stays claimed for as long as it would otherwise still validate. + totpPasscodeReuseWindowSeconds = 90 ) // pendingTOTPSecret is the memory-store payload for a re-enrollment awaiting @@ -43,6 +52,33 @@ func totpPendingSecretKey(userID string) string { return totpPendingSecretPrefix + userID } +// reserveTOTPPasscode atomically claims a (user, passcode) pair for one use and +// reports whether this caller won the claim. A second redemption of the same +// code inside its acceptance window loses and must be rejected. +// +// Keyed on the passcode rather than the matched time-step because +// totp.Validate does not report which step matched, and a step derived from +// the wall clock at redemption time would let the same code be replayed under a +// different step number a few seconds later — the exact replay this closes. The +// key is a digest, not the code itself, so a store dump yields nothing +// redeemable. TTL covers the full window Validate will accept (Period 30, +// Skew 1 → three steps). +// +// Fails open on a store fault or an unconfigured store: an outage must not +// lock every enrolled user out of their account. +func (p *provider) reserveTOTPPasscode(userID, passcode string) bool { + if p.deps.MemoryStoreProvider == nil { + return true + } + sum := sha256.Sum256([]byte(userID + ":" + passcode)) + key := totpUsedPasscodePrefix + hex.EncodeToString(sum[:]) + claimed, err := p.deps.MemoryStoreProvider.SetCacheNX(key, "1", totpPasscodeReuseWindowSeconds) + if err != nil { + return true + } + return claimed +} + // promotePendingSecret checks whether a pending (unconfirmed) re-enrollment // secret is staged for userID and whether passcode validates against it. When // it does, this call IS the user confirming their re-setup: the pending secret @@ -279,6 +315,17 @@ func (p *provider) Validate(ctx context.Context, passcode string, userID string) return false, nil } + // RFC 6238 §5.2: "the verifier MUST NOT accept the second attempt of the + // same OTP". pquerna/otp is stateless — totp.Validate runs ValidateCustom + // with Period 30 / Skew 1, matching against three time-steps and keeping no + // record of what it accepted — so a code captured by a phishing proxy or a + // single intercepted submission stays replayable for the whole ~90s window. + // Email/SMS OTP is already single-use; TOTP was the outlier. + if !p.reserveTOTPPasscode(userID, passcode) { + log.Debug().Msg("totp passcode replayed within its validity window") + return false, nil + } + // Two reasons we may need to write the row back after a successful // validation: // 1. First-time-ever validation → record VerifiedAt diff --git a/internal/config/config.go b/internal/config/config.go index 763b4c340..5cf6297c1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -338,6 +338,20 @@ type Config struct { MicrosoftClientSecret string // MicrosoftTenantID is the tenant ID for Microsoft OAuth MicrosoftTenantID string + // OAuthAllowUnverifiedProviderEmail is a temporary compatibility escape + // hatch for deployments upgrading from 2.3.x whose social provider does not + // attest email addresses (in practice: Microsoft Entra on a multi-tenant + // alias without xms_edov). It does NOT disable the check — an unattested + // address still may not cross into an account owned by another credential. + // See allowUnverifiedProviderEmail and docs/email-verification-contract.md. + OAuthAllowUnverifiedProviderEmail bool + + // MicrosoftAllowedTenants restricts which Entra tenants may sign in when + // MicrosoftTenantID is a multi-tenant alias ("common", "organizations", + // "consumers"). Empty means no restriction — in that mode the tenant is + // untrusted and the email it asserts will not link to an existing account. + // See validateMicrosoftTenant. + MicrosoftAllowedTenants []string // MicrosoftScopes is the list of scopes for Microsoft OAuth MicrosoftScopes []string diff --git a/internal/constants/audit_event.go b/internal/constants/audit_event.go index b49683ba1..42f29e008 100644 --- a/internal/constants/audit_event.go +++ b/internal/constants/audit_event.go @@ -109,6 +109,12 @@ const ( AuditOTPResentEvent = "user.otp_resent" // AuditVerifyEmailResentEvent is logged when a verification email is resent. AuditVerifyEmailResentEvent = "user.verify_email_resent" + // AuditOAuthUnverifiedAccountReplacedEvent is logged when a social login + // deletes an unverified pre-existing account holding the same email + // (account pre-hijacking defense) and provisions a fresh one for the + // principal who actually controls the address. The account is verified to + // be stateless first, but a row is still destroyed — record it. + AuditOAuthUnverifiedAccountReplacedEvent = "oauth.unverified_account_replaced" // AuditAdminLoginSuccessEvent is logged when an admin successfully authenticates. AuditAdminLoginSuccessEvent = "admin.login_success" diff --git a/internal/crypto/random.go b/internal/crypto/random.go new file mode 100644 index 000000000..047e92c8b --- /dev/null +++ b/internal/crypto/random.go @@ -0,0 +1,20 @@ +package crypto + +import ( + "crypto/rand" + "encoding/base64" +) + +// NewRandomString returns a URL-safe, unpadded base64 encoding of numBytes of +// crypto/rand entropy — suitable for opaque handles (admin session ids, state +// nonces) that must be unguessable but carry no structure. +// +// numBytes is the ENTROPY, not the output length: base64 expands by ~4/3, so 32 +// bytes yields a 43-character string. +func NewRandomString(numBytes int) (string, error) { + buf := make([]byte, numBytes) + if _, err := rand.Read(buf); err != nil { + return "", err + } + return base64.RawURLEncoding.EncodeToString(buf), nil +} diff --git a/internal/grpcsrv/transport/grpc_metadata.go b/internal/grpcsrv/transport/grpc_metadata.go index 81f8a8886..b99ca390f 100644 --- a/internal/grpcsrv/transport/grpc_metadata.go +++ b/internal/grpcsrv/transport/grpc_metadata.go @@ -16,6 +16,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/metadata" + "google.golang.org/grpc/peer" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/service" @@ -50,6 +51,19 @@ func MetaFromGRPC(ctx context.Context) service.RequestMetadata { meta.HostURL = "http://" + authority } } + // Same story for the client address: a pure-gRPC caller sends no forwarded + // headers, which used to leave IPAddress empty. That is not merely a blank + // audit-log field — anything keyed on the client address (the admin-secret + // lockout in token/admin_lockout.go) collapses to ONE bucket shared by every + // gRPC caller, so a handful of wrong guesses locks out every admin client at + // once. peer.Addr is the connection's real remote address, which is both + // always present and, unlike the forwarded headers above, not something the + // caller can set. + if meta.IPAddress == "" { + if pr, ok := peer.FromContext(ctx); ok && pr.Addr != nil { + meta.IPAddress = pr.Addr.String() + } + } // Synthesize an *http.Request mirroring the extracted metadata. Several // migrated service methods (Profile, Permissions, Logout, Session, diff --git a/internal/http_handlers/oauth_account_state.go b/internal/http_handlers/oauth_account_state.go new file mode 100644 index 000000000..cbec05980 --- /dev/null +++ b/internal/http_handlers/oauth_account_state.go @@ -0,0 +1,100 @@ +package http_handlers + +import ( + "context" + + "github.com/authorizerdev/authorizer/internal/authorization/engine" + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/storage" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// accountHasState reports whether a user account holds state that would be +// silently destroyed by deleting it, and names the first thing found. +// +// This exists to bound the pre-hijack guard in OAuthCallbackHandler. That guard +// deletes an *unverified* pre-existing account rather than linking a federated +// identity to it, which is correct for its intended target — an attacker who +// signed up with someone else's address and never verified it, squatting to +// intercept their later social login. A squatter's account is empty by +// definition: created seconds ago, never used. +// +// A real account is not. StorageProvider.DeleteUser cascades to every +// user-keyed table (schemas.UserOwnedCollections, #749), so nothing is left +// dangling — but "cleanly destroyed" is not "safe to destroy". The replacement +// account gets a fresh id, so deleting a real account silently drops its org +// memberships, its enrolled authenticators and passkeys, and its federated +// identities, and the user gets none of it back. Two things are worse than +// merely lost: +// +// - FGA grants are NOT covered by that cascade. The tuple store lives outside +// StorageProvider, and the purge that admin _delete_user runs +// (service.purgeFgaTuplesForUser) is in the service layer, which this +// handler's direct StorageProvider.DeleteUser call does not go through. So +// `user:` grants persist forever while the new account inherits +// none of them. +// - the whole thing is triggered by an UNAUTHENTICATED OAuth callback. Nobody +// proved they own the account being destroyed; they merely presented a +// provider assertion for the same address. +// +// So an account carrying any of this is not a squatter, and must never be +// deleted to resolve an email collision. The caller refuses the login instead, +// which is recoverable; deletion is not. +// +// Fail-closed by design: a storage or FGA fault reports "has state" (the second +// return names it), because the safe answer when we cannot tell is to refuse +// rather than destroy. +func (h *httpProvider) accountHasState(ctx context.Context, user *schemas.User) (bool, string) { + if user == nil || user.ID == "" { + return false, "" + } + + if memberships, _, err := h.StorageProvider.ListOrgMembershipsByUser(ctx, user.ID, nil); err != nil { + return true, "org membership lookup failed" + } else if len(memberships) > 0 { + return true, "org membership" + } + + if creds, err := h.StorageProvider.ListWebauthnCredentialsByUserID(ctx, user.ID); err != nil { + return true, "passkey lookup failed" + } else if len(creds) > 0 { + return true, "passkey" + } + + for _, authenticatorType := range []string{ + constants.EnvKeyTOTPAuthenticator, + constants.EnvKeyEmailOTPAuthenticator, + constants.EnvKeySMSOTPAuthenticator, + } { + // "Not enrolled" is the normal case and every backend reports it as an + // error (the not-found contract), so it must be told apart from "the + // lookup failed" — swallowing both would make a transient storage fault + // look like an empty account and delete somebody's MFA enrollment. + a, err := h.StorageProvider.GetAuthenticatorDetailsByUserId(ctx, user.ID, authenticatorType) + switch { + case storage.IsNotFound(err): + continue + case err != nil: + return true, "authenticator lookup failed" + case a != nil: + return true, "enrolled authenticator" + } + } + + // FGA grants are the ones the user notices losing and the ones no cascade + // would ever clean up. Only ask the engine when one is configured. + if h.AuthzEngine != nil { + res, err := h.AuthzEngine.ReadTuples(ctx, engine.ReadTuplesFilter{ + User: "user:" + user.ID, + PageSize: 1, + }) + if err != nil { + return true, "fga tuple lookup failed" + } + if res != nil && len(res.Tuples) > 0 { + return true, "fga grant" + } + } + + return false, "" +} diff --git a/internal/http_handlers/oauth_account_state_test.go b/internal/http_handlers/oauth_account_state_test.go new file mode 100644 index 000000000..31ad35ec6 --- /dev/null +++ b/internal/http_handlers/oauth_account_state_test.go @@ -0,0 +1,167 @@ +package http_handlers + +import ( + "context" + "errors" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "gorm.io/gorm" + + "github.com/authorizerdev/authorizer/internal/authorization/engine" + "github.com/authorizerdev/authorizer/internal/config" + "github.com/authorizerdev/authorizer/internal/graph/model" + "github.com/authorizerdev/authorizer/internal/storage" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// accountStateStore is a storage.Provider stub covering only the lookups +// accountHasState performs. +type accountStateStore struct { + storage.Provider + memberships []*schemas.OrgMembership + credentials []*schemas.WebauthnCredential + authenticator *schemas.Authenticator + membershipErr error + credentialErr error + authenticatorErr error +} + +func (s *accountStateStore) ListOrgMembershipsByUser(_ context.Context, _ string, _ *model.Pagination) ([]*schemas.OrgMembership, *model.Pagination, error) { + return s.memberships, nil, s.membershipErr +} + +func (s *accountStateStore) ListWebauthnCredentialsByUserID(_ context.Context, _ string) ([]*schemas.WebauthnCredential, error) { + return s.credentials, s.credentialErr +} + +func (s *accountStateStore) GetAuthenticatorDetailsByUserId(_ context.Context, _ string, _ string) (*schemas.Authenticator, error) { + if s.authenticatorErr != nil { + return nil, s.authenticatorErr + } + if s.authenticator == nil { + // gorm's own sentinel, not errors.New("not found"): accountHasState has + // to tell "not enrolled" (the normal case, per the not-found contract) + // apart from "the lookup failed", and a bare error string is + // indistinguishable from a database fault. + return nil, gorm.ErrRecordNotFound + } + return s.authenticator, nil +} + +// stubAuthzEngine reports a fixed set of tuples for any ReadTuples call. +type stubAuthzEngine struct { + engine.AuthorizationEngine + tuples []engine.TupleKey + err error +} + +func (e *stubAuthzEngine) ReadTuples(_ context.Context, _ engine.ReadTuplesFilter) (*engine.ReadTuplesResult, error) { + if e.err != nil { + return nil, e.err + } + return &engine.ReadTuplesResult{Tuples: e.tuples}, nil +} + +// TestAccountHasState guards the bound on the pre-hijack delete. +// +// That guard deletes an unverified pre-existing account rather than linking a +// federated identity to it. StorageProvider.DeleteUser cascades to sessions and +// nothing else, and the replacement account gets a fresh id — so deleting an +// account that holds anything means its FGA grants, org memberships and +// enrolled authenticators are silently lost, and any federated-identity row is +// left pointing at a dead user id, hard-locking that principal out of SSO. +// +// Only a genuinely empty account (a real squatter) may be deleted. +func TestAccountHasState(t *testing.T) { + t.Parallel() + + newProvider := func(store storage.Provider, authz engine.AuthorizationEngine) *httpProvider { + logger := zerolog.Nop() + return &httpProvider{ + Config: &config.Config{}, + Dependencies: Dependencies{ + Log: &logger, + StorageProvider: store, + AuthzEngine: authz, + }, + } + } + user := &schemas.User{ID: "user-1"} + + t.Run("an empty account is deletable", func(t *testing.T) { + t.Parallel() + h := newProvider(&accountStateStore{}, nil) + hasState, what := h.accountHasState(context.Background(), user) + assert.False(t, hasState, "a never-used squatter account holds nothing") + assert.Empty(t, what) + }) + + t.Run("org membership blocks deletion", func(t *testing.T) { + t.Parallel() + h := newProvider(&accountStateStore{ + memberships: []*schemas.OrgMembership{{OrgID: "org-1", UserID: "user-1"}}, + }, nil) + hasState, what := h.accountHasState(context.Background(), user) + assert.True(t, hasState) + assert.Equal(t, "org membership", what) + }) + + t.Run("a passkey blocks deletion", func(t *testing.T) { + t.Parallel() + h := newProvider(&accountStateStore{ + credentials: []*schemas.WebauthnCredential{{UserID: "user-1"}}, + }, nil) + hasState, what := h.accountHasState(context.Background(), user) + assert.True(t, hasState) + assert.Equal(t, "passkey", what) + }) + + t.Run("an enrolled authenticator blocks deletion", func(t *testing.T) { + t.Parallel() + h := newProvider(&accountStateStore{ + authenticator: &schemas.Authenticator{UserID: "user-1"}, + }, nil) + hasState, what := h.accountHasState(context.Background(), user) + assert.True(t, hasState) + assert.Equal(t, "enrolled authenticator", what) + }) + + t.Run("an FGA grant blocks deletion", func(t *testing.T) { + t.Parallel() + // The permissions nothing would ever clean up, and the ones the user + // most visibly loses. + h := newProvider(&accountStateStore{}, &stubAuthzEngine{ + tuples: []engine.TupleKey{{User: "user:user-1", Relation: "viewer", Object: "document:1"}}, + }) + hasState, what := h.accountHasState(context.Background(), user) + assert.True(t, hasState) + assert.Equal(t, "fga grant", what) + }) + + t.Run("a lookup fault reports state rather than allowing deletion", func(t *testing.T) { + t.Parallel() + // When we cannot tell, refusing the login is recoverable and deleting + // an account is not. + for _, tc := range []struct { + name string + store *accountStateStore + authz engine.AuthorizationEngine + want string + }{ + {"membership fault", &accountStateStore{membershipErr: errors.New("db down")}, nil, "org membership lookup failed"}, + {"passkey fault", &accountStateStore{credentialErr: errors.New("db down")}, nil, "passkey lookup failed"}, + // Not the same as "not enrolled": a fault here used to be swallowed + // (`a, _ :=`), so a database blip looked like an empty account and + // deleted somebody's MFA enrollment. + {"authenticator fault", &accountStateStore{authenticatorErr: errors.New("db down")}, nil, "authenticator lookup failed"}, + {"fga fault", &accountStateStore{}, &stubAuthzEngine{err: errors.New("fga down")}, "fga tuple lookup failed"}, + } { + h := newProvider(tc.store, tc.authz) + hasState, what := h.accountHasState(context.Background(), user) + assert.True(t, hasState, tc.name) + assert.Equal(t, tc.want, what, tc.name) + } + }) +} diff --git a/internal/http_handlers/oauth_callback.go b/internal/http_handlers/oauth_callback.go index 3cd17c3f9..01c72ceef 100644 --- a/internal/http_handlers/oauth_callback.go +++ b/internal/http_handlers/oauth_callback.go @@ -103,6 +103,11 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { scopeString := sessionSplit[3] scopes := parseScopes(scopeString) var user *schemas.User + // providerEmailVerified is the provider's own assertion that the + // principal controls the email it returned. It gates every path that + // could attach this login to a pre-existing local account — see the + // linking branch below. + var providerEmailVerified bool oauthCode := ctx.Request.FormValue("code") if oauthCode == "" { log.Debug().Err(err).Msg("Invalid oauth code") @@ -111,13 +116,13 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { } switch provider { case constants.AuthRecipeMethodGoogle: - user, err = h.processGoogleUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processGoogleUserInfo(ctx, oauthCode) case constants.AuthRecipeMethodGithub: - user, err = h.processGithubUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processGithubUserInfo(ctx, oauthCode) case constants.AuthRecipeMethodFacebook: - user, err = h.processFacebookUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processFacebookUserInfo(ctx, oauthCode) case constants.AuthRecipeMethodLinkedIn: - user, err = h.processLinkedInUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processLinkedInUserInfo(ctx, oauthCode) case constants.AuthRecipeMethodApple: var appleUser *AppleUserInfo appleUser, err = parseAppleUserField(ctx.Request.FormValue("user")) @@ -126,9 +131,9 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { ctx.JSON(400, gin.H{"error": "invalid apple user info"}) return } - user, err = h.processAppleUserInfo(ctx, oauthCode, appleUser) + user, providerEmailVerified, err = h.processAppleUserInfo(ctx, oauthCode, appleUser) case constants.AuthRecipeMethodDiscord: - user, err = h.processDiscordUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processDiscordUserInfo(ctx, oauthCode) case constants.AuthRecipeMethodTwitter: // Twitter/X uses PKCE: retrieve the verifier stored at login keyed by state. verifier, verr := h.MemoryStoreProvider.GetAndRemoveState(pkceVerifierKeyPrefix + state) @@ -137,13 +142,13 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { ctx.JSON(400, gin.H{"error": "invalid oauth state"}) return } - user, err = h.processTwitterUserInfo(ctx, oauthCode, verifier) + user, providerEmailVerified, err = h.processTwitterUserInfo(ctx, oauthCode, verifier) case constants.AuthRecipeMethodMicrosoft: - user, err = h.processMicrosoftUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processMicrosoftUserInfo(ctx, oauthCode) case constants.AuthRecipeMethodTwitch: - user, err = h.processTwitchUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processTwitchUserInfo(ctx, oauthCode) case constants.AuthRecipeMethodRoblox: - user, err = h.processRobloxUserInfo(ctx, oauthCode) + user, providerEmailVerified, err = h.processRobloxUserInfo(ctx, oauthCode) default: log.Debug().Err(err).Msg("Invalid oauth provider") err = fmt.Errorf(`invalid oauth provider`) @@ -179,6 +184,38 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { log := log.With().Str("email", refs.StringValue(user.Email)).Logger() isSignUp := false + // An email the identity provider has not attested is attacker-controlled + // input, and every branch below keys the local account off it — the + // lookup above decides signup vs. login, and the login branch merges + // this federated identity into whatever account already holds the + // address. That is the nOAuth account-takeover class: register a free + // Entra tenant, set a user's mutable `email` attribute to the victim's + // address, sign in, and land in the victim's session. The pre-hijack + // guard further down does not help — it only removes *unverified* local + // accounts, and verified accounts are exactly what gets stolen. + // + // OAuth itself proves nothing about email; that is precisely why OIDC + // carries a separate `email_verified` claim (Core §5.1), and why Auth0 + // documents checking it before linking accounts. + if !providerEmailVerified && !h.allowUnverifiedProviderEmail(provider, existingUser, err == nil) { + log.Debug().Str("provider", provider).Msg("Provider did not attest the email address; refusing to resolve a local account") + metrics.RecordAuthEvent(metrics.EventOAuthCallback, metrics.StatusFailure) + metrics.RecordSecurityEvent("oauth_email_unverified", provider) + h.AuditProvider.LogEvent(audit.Event{ + Action: constants.AuditOAuthCallbackFailedEvent, + ActorType: constants.AuditActorTypeUser, + ResourceType: constants.AuditResourceTypeSession, + Metadata: provider, + IPAddress: utils.GetIP(ctx.Request), + UserAgent: utils.GetUserAgent(ctx.Request), + }) + ctx.JSON(400, gin.H{ + "error": "email_not_verified", + "error_description": "The identity provider did not confirm that you own this email address.", + }) + return + } + if err != nil { isSignupEnabled := h.Config.EnableSignup if !isSignupEnabled { @@ -204,8 +241,16 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { } user.Roles = strings.Join(inputRoles, ",") - now := time.Now().Unix() - user.EmailVerifiedAt = &now + // Only record the address as verified when the provider actually + // attested it. This used to be unconditional, which meant a + // compatibility-mode signup from an unattested address wrote + // email_verified=true into our own database — a claim we cannot + // back, and one that downstream consumers trust (SAML IdP issuance + // refuses to assert an unverified email as the Subject NameID). + if providerEmailVerified { + now := time.Now().Unix() + user.EmailVerifiedAt = &now + } user, err = h.StorageProvider.AddUser(ctx, user) if err != nil { log.Debug().Err(err).Msg("Failed to add user") @@ -236,8 +281,61 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { // was never verified, do not link the OAuth identity to it. // Instead, delete the unverified account and treat as a new signup // for the OAuth user who actually controls the email address. - if existingUser.EmailVerifiedAt == nil { - log.Info().Msg("Removing unverified pre-existing account before OAuth signup") + // + // Scoped to accounts some OTHER credential created. An unverified + // account this same provider already owns is not a squatter — it is + // this same principal's own account, created on a previous pass + // through the signup branch above (which, correctly, no longer marks + // an unattested address verified). Deleting it would recreate the + // account on every single login, silently dropping its id, roles and + // org memberships each time. + if existingUser.EmailVerifiedAt == nil && !signupMethodsContain(existingUser.SignupMethods, provider) { + // Deleting is only safe for an account that is actually a + // squatter — created to intercept this address and never used. + // The cascade is clean (#749) but total: an account carrying + // real state would lose its org memberships, enrolled + // authenticators and federated identities outright, and its FGA + // grants would be orphaned, since the tuple purge lives in the + // service layer and this is a direct StorageProvider call. All + // of that on the say-so of an unauthenticated callback. + // Refusing is recoverable; deleting is not. + if hasState, what := h.accountHasState(ctx, existingUser); hasState { + log.Warn(). + Str("reason", what). + Str("existing_user_id", existingUser.ID). + Msg("Refusing OAuth login: an unverified account with this email holds state and must not be replaced") + metrics.RecordAuthEvent(metrics.EventOAuthCallback, metrics.StatusFailure) + metrics.RecordSecurityEvent("oauth_email_collision_stateful_account", provider) + h.AuditProvider.LogEvent(audit.Event{ + Action: constants.AuditOAuthCallbackFailedEvent, + ActorID: existingUser.ID, + ActorType: constants.AuditActorTypeUser, + ActorEmail: refs.StringValue(existingUser.Email), + ResourceType: constants.AuditResourceTypeSession, + Metadata: provider, + IPAddress: utils.GetIP(ctx.Request), + UserAgent: utils.GetUserAgent(ctx.Request), + }) + ctx.JSON(400, gin.H{ + "error": "email_already_registered", + "error_description": "An unverified account already exists for this email address. Verify it first — request a new verification email for this address, or sign in with the method that created the account.", + }) + return + } + log.Info().Str("existing_user_id", existingUser.ID).Msg("Removing unverified pre-existing account before OAuth signup") + // Audited: this destroys an account row, which is + // security-material even when the account was empty. + h.AuditProvider.LogEvent(audit.Event{ + Action: constants.AuditOAuthUnverifiedAccountReplacedEvent, + ActorID: existingUser.ID, + ActorType: constants.AuditActorTypeUser, + ActorEmail: refs.StringValue(existingUser.Email), + ResourceType: constants.AuditResourceTypeUser, + ResourceID: existingUser.ID, + Metadata: provider, + IPAddress: utils.GetIP(ctx.Request), + UserAgent: utils.GetUserAgent(ctx.Request), + }) if err := h.StorageProvider.DeleteUser(ctx, existingUser); err != nil { log.Debug().Err(err).Msg("Failed to delete unverified user") ctx.JSON(500, gin.H{"error": "failed to process OAuth login"}) @@ -456,6 +554,92 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { } } +// allowUnverifiedProviderEmail decides whether a federated login whose provider +// did NOT attest the email address may still resolve a local account. +// +// Default (--oauth-allow-unverified-provider-email=false): never. The address is +// attacker-controlled and it is what selects the account. +// +// Compatibility mode (=true) exists so a deployment upgrading from 2.3.x is not +// locked out the moment it restarts, but it is deliberately NOT a plain "turn +// the check off" switch — that would restore the CVE verbatim. Even in this +// mode, an unattested address may only: +// +// - create a brand-new account (it selects nobody, so it harms nobody), or +// - return to an account THIS SAME PROVIDER already owns — a returning user. +// +// It may never merge into an account some other credential owns. That single +// restriction removes the entire cross-credential takeover: an Entra tenant +// cannot reach a password account, a Google account, or any other provider's +// account, which is every practical form of the attack. +// +// The residual risk it does not cover, and the reason this mode is documented +// as temporary: two principals of the SAME unattested provider (two Entra +// tenants both asserting one address) can still collide. Pinning +// --microsoft-tenant-id or setting --microsoft-allowed-tenants closes that, and +// is the actual fix. +func (h *httpProvider) allowUnverifiedProviderEmail(provider string, existingUser *schemas.User, found bool) bool { + if !h.Config.OAuthAllowUnverifiedProviderEmail { + return false + } + if !found { + // First-time signup: no account is being selected away from anyone. + return true + } + if existingUser == nil { + // "Found" with no row is an inconsistent storage result. Fail closed + // rather than guess which case it was. + return false + } + // Returning user of this same provider, or an attempt to cross into an + // account another credential owns. + return signupMethodsContain(existingUser.SignupMethods, provider) +} + +// signupMethodsContain reports whether a stored comma-separated signup-methods +// list contains an exact method. Deliberately not strings.Contains: the +// provider names include the near-miss pair twitch/twitter, and a substring +// match on a security decision would let one provider inherit the other's +// accounts. +func signupMethodsContain(signupMethods, provider string) bool { + for _, m := range strings.Split(signupMethods, ",") { + if strings.TrimSpace(m) == provider { + return true + } + } + return false +} + +// flexBool decodes a JSON boolean that some IdPs send quoted. Apple documents +// `email_verified` as "a string or Boolean value", and LinkedIn's userinfo has +// shipped both shapes; decoding either into a plain bool fails the whole claim +// set, which would silently turn a verified email into an unverified one. +type flexBool bool + +// UnmarshalJSON accepts true/false, "true"/"false", or anything else (which +// decodes to false — unrecognised is never "verified"). +func (b *flexBool) UnmarshalJSON(data []byte) error { + var raw any + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + *b = flexBool(claimTruthy(raw)) + return nil +} + +// claimTruthy reads a boolean claim out of an untyped JSON value, tolerating +// the quoted-string form some IdPs emit. Used by the providers whose payloads +// are decoded into a map rather than oidcClaims (Apple, Discord, Roblox). +func claimTruthy(v any) bool { + switch t := v.(type) { + case bool: + return t + case string: + return strings.EqualFold(t, "true") + } + return false +} + // oidcClaims is the allow-list of OpenID Connect standard claims Authorizer // maps onto a user. ID tokens are decoded into this and never straight into // schemas.User, for two reasons: @@ -469,6 +653,25 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { // signup_methods, is_active, created_at ...) merely by sharing its json // tag. type oidcClaims struct { + // Subject is the provider-asserted stable identifier for the principal. + // Unlike `email` it is not user-mutable, so it is the only claim safe to + // treat as an identity key. + Subject string `json:"sub"` + // Issuer and TenantID back the Microsoft tenant checks; see + // processMicrosoftUserInfo. Ignored for every other provider. + Issuer string `json:"iss"` + TenantID string `json:"tid"` + // EmailVerified is the provider's assertion that the principal actually + // controls `Email`. Absent decodes to false — an IdP that does not say + // "verified" has not verified anything, and this claim is what stops a + // federated login from linking to somebody else's account. + EmailVerified flexBool `json:"email_verified"` + // XmsEdov ("email domain owner verified") is Microsoft Entra's equivalent. + // Entra v2 ID tokens carry no `email_verified` claim at all, and their + // `email` is a mutable, unverified profile attribute — the distinction + // that makes the nOAuth attack work. + XmsEdov flexBool `json:"xms_edov"` + Email string `json:"email"` GivenName string `json:"given_name"` FamilyName string `json:"family_name"` @@ -505,17 +708,17 @@ func (c *oidcClaims) toUser() *schemas.User { return user } -func (h *httpProvider) processGoogleUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processGoogleUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processGoogleUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodGoogle) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid google exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid google exchange code: %s", err.Error()) } issuer := "https://accounts.google.com" @@ -524,29 +727,30 @@ func (h *httpProvider) processGoogleUserInfo(ctx *gin.Context, code string) (*sc } oidcProvider, err := getOIDCProvider(ctx, issuer) if err != nil { - return nil, fmt.Errorf("failed to create oidc provider: %s", err.Error()) + return nil, false, fmt.Errorf("failed to create oidc provider: %s", err.Error()) } verifier := oidcProvider.Verifier(&oidc.Config{ClientID: h.GoogleClientID}) // Extract the ID Token from OAuth2 token. rawIDToken, ok := oauth2Token.Extra("id_token").(string) if !ok { log.Debug().Err(err).Msg("Failed to extract ID Token from OAuth2 token") - return nil, fmt.Errorf("unable to extract id_token") + return nil, false, fmt.Errorf("unable to extract id_token") } // Parse and verify ID Token payload. idToken, err := verifier.Verify(ctx, rawIDToken) if err != nil { log.Debug().Err(err).Msg("Failed to verify ID Token") - return nil, fmt.Errorf("unable to verify id_token: %s", err.Error()) + return nil, false, fmt.Errorf("unable to verify id_token: %s", err.Error()) } claims := &oidcClaims{} if err := idToken.Claims(claims); err != nil { log.Debug().Err(err).Msg("Failed to parse ID Token claims") - return nil, fmt.Errorf("unable to extract claims") + return nil, false, fmt.Errorf("unable to extract claims") } - return claims.toUser(), nil + // Google asserts control of the address via `email_verified`. + return claims.toUser(), bool(claims.EmailVerified), nil } // setGithubHeaders applies the headers GitHub's REST API docs ask every @@ -559,18 +763,18 @@ func setGithubHeaders(req *http.Request, accessToken string) { req.Header.Set("X-GitHub-Api-Version", "2022-11-28") } -func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processGithubUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodGithub) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid github exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid github exchange code: %s", err.Error()) } userInfoURL := constants.GithubUserInfoURL emailsURL := constants.GithubUserEmails @@ -582,25 +786,25 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc req, err := http.NewRequest("GET", userInfoURL, nil) if err != nil { log.Debug().Err(err).Msg("Failed to create github user info request") - return nil, fmt.Errorf("error creating github user info request: %s", err.Error()) + return nil, false, fmt.Errorf("error creating github user info request: %s", err.Error()) } setGithubHeaders(req, oauth2Token.AccessToken) response, err := client.Do(req) if err != nil { log.Debug().Err(err).Msg("Failed to request github user info") - return nil, err + return nil, false, err } defer func() { _ = response.Body.Close() }() body, err := io.ReadAll(response.Body) if err != nil { log.Debug().Err(err).Msg("Failed to read github user info response body") - return nil, fmt.Errorf("failed to read github response body: %s", err.Error()) + return nil, false, fmt.Errorf("failed to read github response body: %s", err.Error()) } if response.StatusCode >= 400 { log.Debug().Err(err).Str("body", string(body)).Msg("Failed to request github user info") - return nil, fmt.Errorf("failed to request github user info: %s", string(body)) + return nil, false, fmt.Errorf("failed to request github user info: %s", string(body)) } // Only the three fields below are used. A typed struct (rather than a @@ -614,7 +818,7 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc } if err := json.Unmarshal(body, &userRawData); err != nil { log.Debug().Err(err).Msg("Failed to unmarshal github user info") - return nil, fmt.Errorf("failed to parse github user info: %s", err.Error()) + return nil, false, fmt.Errorf("failed to parse github user info: %s", err.Error()) } name := strings.Split(userRawData.Name, " ") @@ -641,32 +845,32 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc req, err := http.NewRequest(http.MethodGet, emailsURL, nil) if err != nil { log.Debug().Err(err).Msg("Failed to create github emails request") - return nil, fmt.Errorf("error creating github user info request: %s", err.Error()) + return nil, false, fmt.Errorf("error creating github user info request: %s", err.Error()) } setGithubHeaders(req, oauth2Token.AccessToken) response, err := client.Do(req) if err != nil { log.Debug().Err(err).Msg("Failed to request github user email") - return nil, err + return nil, false, err } defer func() { _ = response.Body.Close() }() body, err := io.ReadAll(response.Body) if err != nil { log.Debug().Err(err).Msg("Failed to read github user email response body") - return nil, fmt.Errorf("failed to read github response body: %s", err.Error()) + return nil, false, fmt.Errorf("failed to read github response body: %s", err.Error()) } if response.StatusCode >= 400 { log.Debug().Err(err).Str("body", string(body)).Msg("Failed to request github user email") - return nil, fmt.Errorf("failed to request github user info: %s", string(body)) + return nil, false, fmt.Errorf("failed to request github user info: %s", string(body)) } emailData := []GithubUserEmails{} err = json.Unmarshal(body, &emailData) if err != nil { log.Debug().Err(err).Msg("Failed to parse github user email") - return nil, fmt.Errorf("failed to parse github user email: %s", err.Error()) + return nil, false, fmt.Errorf("failed to parse github user email: %s", err.Error()) } // GET /user/emails lists every address on the account, verified or @@ -686,7 +890,7 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc } if email == "" { log.Debug().Msg("No verified email on github account") - return nil, fmt.Errorf("failed to get a verified email address from github") + return nil, false, fmt.Errorf("failed to get a verified email address from github") } } @@ -697,20 +901,20 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc Email: &email, } - return user, nil + return user, true, nil } -func (h *httpProvider) processFacebookUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processFacebookUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processFacebookUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodFacebook) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Invalid facebook exchange code") - return nil, fmt.Errorf("invalid facebook exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid facebook exchange code: %s", err.Error()) } userInfoURL := constants.FacebookUserInfoURL if mockBase := h.TestOAuthBaseURL(constants.AuthRecipeMethodFacebook); mockBase != "" { @@ -720,24 +924,24 @@ func (h *httpProvider) processFacebookUserInfo(ctx *gin.Context, code string) (* req, err := http.NewRequest("GET", userInfoURL+oauth2Token.AccessToken, nil) if err != nil { log.Debug().Err(err).Msg("Error creating facebook user info request") - return nil, fmt.Errorf("error creating facebook user info request: %s", err.Error()) + return nil, false, fmt.Errorf("error creating facebook user info request: %s", err.Error()) } response, err := client.Do(req) if err != nil { log.Debug().Err(err).Msg("Failed to process facebook user") - return nil, err + return nil, false, err } defer func() { _ = response.Body.Close() }() body, err := io.ReadAll(response.Body) if err != nil { log.Debug().Err(err).Msg("Failed to read facebook response") - return nil, fmt.Errorf("failed to read facebook response body: %s", err.Error()) + return nil, false, fmt.Errorf("failed to read facebook response body: %s", err.Error()) } if response.StatusCode >= 400 { log.Debug().Err(err).Str("body", string(body)).Msg("Failed to request facebook user info") - return nil, fmt.Errorf("failed to request facebook user info: %s", string(body)) + return nil, false, fmt.Errorf("failed to request facebook user info: %s", string(body)) } // Typed decode, not fmt.Sprintf over a map: Graph API omits `email` // entirely when "no valid email address is available" (user/reference/user), @@ -755,13 +959,13 @@ func (h *httpProvider) processFacebookUserInfo(ctx *gin.Context, code string) (* } if err := json.Unmarshal(body, &userRawData); err != nil { log.Debug().Err(err).Msg("Failed to unmarshal facebook user info") - return nil, fmt.Errorf("failed to parse facebook user info: %s", err.Error()) + return nil, false, fmt.Errorf("failed to parse facebook user info: %s", err.Error()) } email := userRawData.Email if email == "" { log.Debug().Msg("Facebook user info has no email") - return nil, fmt.Errorf("failed to get email from facebook user info: the account has no available email address") + return nil, false, fmt.Errorf("failed to get email from facebook user info: the account has no available email address") } picture := userRawData.Picture.Data.URL @@ -775,21 +979,21 @@ func (h *httpProvider) processFacebookUserInfo(ctx *gin.Context, code string) (* Email: &email, } - return user, nil + return user, true, nil } -func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processLinkedInUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodLinkedIn) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid linkedin exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid linkedin exchange code: %s", err.Error()) } userInfoURL := constants.LinkedInUserInfoURL @@ -800,7 +1004,7 @@ func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (* req, err := http.NewRequest("GET", userInfoURL, nil) if err != nil { log.Debug().Err(err).Msg("Failed to create linkedin user info request") - return nil, fmt.Errorf("error creating linkedin user info request: %s", err.Error()) + return nil, false, fmt.Errorf("error creating linkedin user info request: %s", err.Error()) } req.Header = http.Header{ "Authorization": []string{fmt.Sprintf("Bearer %s", oauth2Token.AccessToken)}, @@ -809,32 +1013,33 @@ func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (* response, err := client.Do(req) if err != nil { log.Debug().Err(err).Msg("Failed to request linkedin user info") - return nil, err + return nil, false, err } defer func() { _ = response.Body.Close() }() body, err := io.ReadAll(response.Body) if err != nil { log.Debug().Err(err).Msg("Failed to read linkedin user info response body") - return nil, fmt.Errorf("failed to read linkedin response body: %s", err.Error()) + return nil, false, fmt.Errorf("failed to read linkedin response body: %s", err.Error()) } if response.StatusCode >= 400 { log.Debug().Err(err).Str("body", string(body)).Msg("Failed to request linkedin user info") - return nil, fmt.Errorf("failed to request linkedin user info: %s", string(body)) + return nil, false, fmt.Errorf("failed to request linkedin user info: %s", string(body)) } // OIDC userinfo shape (sub/name/given_name/family_name/picture/locale/ // email/email_verified) - one call, no separate /v2/emailAddress hop. var userRawData struct { - GivenName string `json:"given_name"` - FamilyName string `json:"family_name"` - Picture string `json:"picture"` - Email string `json:"email"` + GivenName string `json:"given_name"` + FamilyName string `json:"family_name"` + Picture string `json:"picture"` + Email string `json:"email"` + EmailVerified flexBool `json:"email_verified"` } if err := json.Unmarshal(body, &userRawData); err != nil { log.Debug().Err(err).Msg("Failed to unmarshal linkedin user info") - return nil, fmt.Errorf("failed to parse linkedin user info: %s", err.Error()) + return nil, false, fmt.Errorf("failed to parse linkedin user info: %s", err.Error()) } // `email` is documented as optional - it is only present when the member @@ -843,7 +1048,7 @@ func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (* // than a synthetic-email fallback. if userRawData.Email == "" { log.Debug().Msg("LinkedIn user info has no email") - return nil, fmt.Errorf("failed to extract email from linkedin response") + return nil, false, fmt.Errorf("failed to extract email from linkedin response") } user := &schemas.User{ @@ -853,29 +1058,29 @@ func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (* Email: &userRawData.Email, } - return user, nil + return user, bool(userRawData.EmailVerified), nil } -func (h *httpProvider) processAppleUserInfo(ctx *gin.Context, code string, appleUser *AppleUserInfo) (*schemas.User, error) { +func (h *httpProvider) processAppleUserInfo(ctx *gin.Context, code string, appleUser *AppleUserInfo) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processAppleUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodApple) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } var user = &schemas.User{} oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return user, fmt.Errorf("invalid apple exchange code: %s", err.Error()) + return user, false, fmt.Errorf("invalid apple exchange code: %s", err.Error()) } // Extract the ID Token from OAuth2 token. rawIDToken, ok := oauth2Token.Extra("id_token").(string) if !ok { log.Debug().Err(err).Msg("Failed to extract ID Token from OAuth2 token") - return user, fmt.Errorf("unable to extract id_token") + return user, false, fmt.Errorf("unable to extract id_token") } // Verify the Apple ID token signature, issuer, and audience using OIDC discovery @@ -886,33 +1091,38 @@ func (h *httpProvider) processAppleUserInfo(ctx *gin.Context, code string, apple oidcProvider, err := getOIDCProvider(ctx, issuer) if err != nil { log.Debug().Err(err).Msg("Failed to create Apple OIDC provider") - return user, fmt.Errorf("failed to create oidc provider: %s", err.Error()) + return user, false, fmt.Errorf("failed to create oidc provider: %s", err.Error()) } verifier := oidcProvider.Verifier(&oidc.Config{ClientID: h.AppleClientID}) idToken, err := verifier.Verify(ctx, rawIDToken) if err != nil { log.Debug().Err(err).Msg("Failed to verify Apple ID Token") - return user, fmt.Errorf("unable to verify id_token: %s", err.Error()) + return user, false, fmt.Errorf("unable to verify id_token: %s", err.Error()) } claims := make(map[string]interface{}) if err := idToken.Claims(&claims); err != nil { log.Debug().Err(err).Msg("Failed to parse Apple ID Token claims") - return user, fmt.Errorf("failed to parse claims: %s", err.Error()) + return user, false, fmt.Errorf("failed to parse claims: %s", err.Error()) } if val, ok := claims["email"]; !ok || val == nil { log.Debug().Msg("Failed to extract email from claims.") - return user, fmt.Errorf("unable to extract email, please check the scopes enabled for your app. It needs `email`, `name` scopes") + return user, false, fmt.Errorf("unable to extract email, please check the scopes enabled for your app. It needs `email`, `name` scopes") } else { email, _ := val.(string) user.Email = &email } + // Apple documents `email_verified` as "a string or Boolean value", so it + // arrives as either true or "true" — claimTruthy accepts both. Absent means + // unverified. + emailVerified := claimTruthy(claims["email_verified"]) + user.GivenName = &appleUser.Name.FirstName user.FamilyName = &appleUser.Name.LastName - return user, nil + return user, emailVerified, nil } // processDiscordUserInfo exchanges the Discord OAuth code for the user's @@ -929,17 +1139,17 @@ func (h *httpProvider) processAppleUserInfo(ctx *gin.Context, code string, apple // creating a duplicate account - the same fallback discipline // processTwitterUserInfo uses above for X, which never returns a real email // at all. -func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processDiscordUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodDiscord) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid discord exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid discord exchange code: %s", err.Error()) } userInfoURL := constants.DiscordUserInfoURL @@ -950,7 +1160,7 @@ func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*s req, err := http.NewRequest("GET", userInfoURL, nil) if err != nil { log.Debug().Err(err).Msg("Failed to create Discord user info request") - return nil, fmt.Errorf("error creating Discord user info request: %s", err.Error()) + return nil, false, fmt.Errorf("error creating Discord user info request: %s", err.Error()) } req.Header = http.Header{ "Authorization": []string{fmt.Sprintf("Bearer %s", oauth2Token.AccessToken)}, @@ -959,19 +1169,19 @@ func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*s response, err := client.Do(req) if err != nil { log.Debug().Err(err).Msg("Failed to request Discord user info") - return nil, err + return nil, false, err } defer func() { _ = response.Body.Close() }() body, err := io.ReadAll(response.Body) if err != nil { log.Debug().Err(err).Msg("Failed to read Discord user info response body") - return nil, fmt.Errorf("failed to read Discord response body: %s", err.Error()) + return nil, false, fmt.Errorf("failed to read Discord response body: %s", err.Error()) } if response.StatusCode >= 400 { log.Debug().Err(err).Msg("Failed to request Discord user info") - return nil, fmt.Errorf("failed to request Discord user info: %s", string(body)) + return nil, false, fmt.Errorf("failed to request Discord user info: %s", string(body)) } // Unmarshal the response body into a map. GET /users/@me returns a flat @@ -980,19 +1190,19 @@ func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*s userRawData := make(map[string]interface{}) if err := json.Unmarshal(body, &userRawData); err != nil { log.Debug().Err(err).Msg("Failed to unmarshal Discord response") - return nil, fmt.Errorf("failed to unmarshal Discord response: %s", err.Error()) + return nil, false, fmt.Errorf("failed to unmarshal Discord response: %s", err.Error()) } // Extract the username firstName, ok := userRawData["username"].(string) if !ok { log.Debug().Err(err).Msg("Username is not in expected format or missing in user data") - return nil, fmt.Errorf("username is not in expected format or missing in user data") + return nil, false, fmt.Errorf("username is not in expected format or missing in user data") } discordID, ok := userRawData["id"].(string) if !ok || discordID == "" { log.Debug().Msg("Discord user info missing id") - return nil, fmt.Errorf("discord response missing id field") + return nil, false, fmt.Errorf("discord response missing id field") } // `avatar` is nullable (?string in Discord's user object) for accounts on // the default avatar - building the CDN URL from an empty hash yields a @@ -1003,6 +1213,14 @@ func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*s } email := resolveDiscordEmail(discordID, userRawData) + // GET /users/@me carries a `verified` flag for the account's email. The + // synthetic fallback is trusted by construction: it lives on a reserved + // non-routable domain keyed by Discord's permanent id, so it can never + // collide with an address a real person could prove they own. + emailVerified := claimTruthy(userRawData["verified"]) + if email == discordSyntheticEmail(discordID) { + emailVerified = true + } user := &schemas.User{ GivenName: &firstName, @@ -1010,7 +1228,7 @@ func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*s Email: &email, } - return user, nil + return user, emailVerified, nil } // resolveDiscordEmail prefers the real email Discord returns; falls back to @@ -1040,18 +1258,18 @@ func discordSyntheticEmail(discordID string) string { // returning Twitter user instead of creating a duplicate account on every // login. Operators who opt into X's `users.email` scope + app permission get // a real confirmed_email instead (see TwitterUserInfoURL's doc comment). -func (h *httpProvider) processTwitterUserInfo(ctx *gin.Context, code, verifier string) (*schemas.User, error) { +func (h *httpProvider) processTwitterUserInfo(ctx *gin.Context, code, verifier string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processTwitterUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodTwitter) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code, oauth2.VerifierOption(verifier)) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid twitter exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid twitter exchange code: %s", err.Error()) } userInfoURL := constants.TwitterUserInfoURL @@ -1062,7 +1280,7 @@ func (h *httpProvider) processTwitterUserInfo(ctx *gin.Context, code, verifier s req, err := http.NewRequest("GET", userInfoURL, nil) if err != nil { log.Debug().Err(err).Msg("Failed to create Twitter user info request") - return nil, fmt.Errorf("error creating Twitter user info request: %s", err.Error()) + return nil, false, fmt.Errorf("error creating Twitter user info request: %s", err.Error()) } req.Header = http.Header{ "Authorization": []string{fmt.Sprintf("Bearer %s", oauth2Token.AccessToken)}, @@ -1071,30 +1289,30 @@ func (h *httpProvider) processTwitterUserInfo(ctx *gin.Context, code, verifier s response, err := client.Do(req) if err != nil { log.Debug().Err(err).Msg("Failed to request Twitter user info") - return nil, err + return nil, false, err } defer func() { _ = response.Body.Close() }() body, err := io.ReadAll(response.Body) if err != nil { log.Debug().Err(err).Msg("Failed to read Twitter user info response body") - return nil, fmt.Errorf("failed to read Twitter response body: %s", err.Error()) + return nil, false, fmt.Errorf("failed to read Twitter response body: %s", err.Error()) } if response.StatusCode >= 400 { log.Debug().Err(err).Str("body", string(body)).Msg("Failed to request Twitter user info") - return nil, fmt.Errorf("failed to request Twitter user info: %s", string(body)) + return nil, false, fmt.Errorf("failed to request Twitter user info: %s", string(body)) } responseRawData := make(map[string]interface{}) if err := json.Unmarshal(body, &responseRawData); err != nil { log.Debug().Err(err).Msg("Failed to unmarshal twitter user info") - return nil, fmt.Errorf("failed to parse twitter user info: %s", err.Error()) + return nil, false, fmt.Errorf("failed to parse twitter user info: %s", err.Error()) } userRawData, ok := responseRawData["data"].(map[string]interface{}) if !ok { - return nil, fmt.Errorf("twitter response missing data field") + return nil, false, fmt.Errorf("twitter response missing data field") } // Twitter API does not return E-Mail adresses by default. For that case special privileges have @@ -1113,7 +1331,7 @@ func (h *httpProvider) processTwitterUserInfo(ctx *gin.Context, code, verifier s twitterID, ok := userRawData["id"].(string) if !ok || twitterID == "" { log.Debug().Msg("Twitter user info missing id") - return nil, fmt.Errorf("twitter response missing id field") + return nil, false, fmt.Errorf("twitter response missing id field") } // Currently Twitter API only provides the full name of a user. To fill givenName and familyName @@ -1131,6 +1349,12 @@ func (h *httpProvider) processTwitterUserInfo(ctx *gin.Context, code, verifier s profilePicture, _ := userRawData["profile_image_url"].(string) email := resolveTwitterEmail(twitterID, userRawData) + // X only returns `confirmed_email` to apps granted the `users.email` scope + // plus the app-dashboard permission, and the name says it: X has confirmed + // it. The synthetic fallback is trusted by construction — a reserved + // non-routable domain keyed by X's permanent numeric id, which no real + // mailbox can occupy. + emailVerified := true user := &schemas.User{ Email: &email, @@ -1140,7 +1364,7 @@ func (h *httpProvider) processTwitterUserInfo(ctx *gin.Context, code, verifier s Nickname: &nickname, } - return user, nil + return user, emailVerified, nil } // twitterSyntheticEmail derives a stable, non-routable synthetic email from @@ -1165,17 +1389,17 @@ func resolveTwitterEmail(twitterID string, userRawData map[string]interface{}) s } // process microsoft user information -func (h *httpProvider) processMicrosoftUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processMicrosoftUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processMicrosoftUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodMicrosoft) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid microsoft exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid microsoft exchange code: %s", err.Error()) } issuer := fmt.Sprintf("https://login.microsoftonline.com/%s/v2.0", h.MicrosoftTenantID) if mockBase := h.TestOAuthBaseURL(constants.AuthRecipeMethodMicrosoft); mockBase != "" { @@ -1183,9 +1407,14 @@ func (h *httpProvider) processMicrosoftUserInfo(ctx *gin.Context, code string) ( } oidcProvider, err := getOIDCProvider(ctx, issuer) if err != nil { - return nil, fmt.Errorf("failed to create oidc provider: %s", err.Error()) - } - // we need to skip issuer check because for common tenant it will return internal issuer which does not match + return nil, false, fmt.Errorf("failed to create oidc provider: %s", err.Error()) + } + // The multi-tenant discovery documents ("common"/"organizations"/ + // "consumers") advertise a templated issuer containing {tenantid}, which + // never literally equals the `iss` of a real token, so go-oidc's built-in + // comparison cannot be used. Skipping it is not the same as not checking: + // validateMicrosoftTenant below reconstructs the expected issuer from the + // token's own `tid` and enforces it, plus the operator's tenant policy. verifier := oidcProvider.Verifier(&oidc.Config{ ClientID: h.MicrosoftClientID, SkipIssuerCheck: true, @@ -1194,43 +1423,123 @@ func (h *httpProvider) processMicrosoftUserInfo(ctx *gin.Context, code string) ( rawIDToken, ok := oauth2Token.Extra("id_token").(string) if !ok { log.Debug().Err(err).Msg("Failed to extract ID Token from OAuth2 token") - return nil, fmt.Errorf("unable to extract id_token") + return nil, false, fmt.Errorf("unable to extract id_token") } // Parse and verify ID Token payload. idToken, err := verifier.Verify(ctx, rawIDToken) if err != nil { log.Debug().Err(err).Msg("Failed to verify ID Token") - return nil, fmt.Errorf("unable to verify id_token: %s", err.Error()) + return nil, false, fmt.Errorf("unable to verify id_token: %s", err.Error()) } claims := &oidcClaims{} if err := idToken.Claims(claims); err != nil { log.Debug().Err(err).Msg("Failed to parse ID Token claims") - return nil, fmt.Errorf("unable to extract claims") + return nil, false, fmt.Errorf("unable to extract claims") + } + + // The test double issues tokens from a stand-in issuer with no tenant + // model at all; tenant policy is meaningless there. + if mockBase := h.TestOAuthBaseURL(constants.AuthRecipeMethodMicrosoft); mockBase != "" { + return claims.toUser(), bool(claims.EmailVerified), nil } - return claims.toUser(), nil + tenantPinned, err := validateMicrosoftTenant(claims, h.MicrosoftTenantID, h.Config.MicrosoftAllowedTenants) + if err != nil { + log.Debug().Err(err).Str("tid", claims.TenantID).Msg("Microsoft tenant validation failed") + return nil, false, err + } + + // Entra v2 ID tokens have no `email_verified` claim, and `email` is a + // mutable, unverified profile attribute any tenant admin can set to any + // string — including somebody else's address. Two signals make it + // trustworthy, and nothing else does: + // + // - xms_edov ("email domain owner verified"), Microsoft's own attestation + // that the token's tenant owns the email's domain. It is an optional + // claim; operators enable it in the app registration. + // - the tenant being pinned or allowlisted, which means the address can + // only have come from a directory the operator already trusts. + // + // Without either, this is the nOAuth setup: an attacker registers a free + // Entra tenant, sets a user's `email` to the victim's address, and signs in. + return claims.toUser(), tenantPinned || bool(claims.XmsEdov), nil +} + +// microsoftMultiTenantAliases are the Entra endpoint aliases that accept tokens +// from tenants the operator has never heard of. Any other configured value is a +// specific tenant (a GUID or a verified domain name) and is therefore pinned. +var microsoftMultiTenantAliases = map[string]bool{ + "common": true, + "organizations": true, + "consumers": true, +} + +// validateMicrosoftTenant enforces the operator's tenant policy on a verified +// Entra ID token and reports whether the originating tenant is one the operator +// explicitly trusts. +// +// go-oidc has already checked the signature and that `aud` equals our client id +// — neither of which constrains WHICH tenant minted the token, because the +// multi-tenant endpoints sign with Microsoft's global keys. The tenant is the +// only thing that does, so it is checked here: +// +// - `iss` must be the issuer the token's own `tid` implies, so a token cannot +// claim one tenant in `iss` and another in `tid`; +// - a pinned `--microsoft-tenant-id` must match `tid` exactly; +// - a non-empty `--microsoft-allowed-tenants` must contain `tid`. +// +// Returns true when the tenant was pinned or allowlisted. +func validateMicrosoftTenant(claims *oidcClaims, configuredTenant string, allowedTenants []string) (bool, error) { + tid := strings.TrimSpace(claims.TenantID) + if tid == "" { + return false, fmt.Errorf("microsoft id_token is missing the tid claim") + } + if expected := fmt.Sprintf("https://login.microsoftonline.com/%s/v2.0", tid); claims.Issuer != expected { + return false, fmt.Errorf("microsoft id_token issuer does not match its tenant") + } + + if len(allowedTenants) > 0 { + if !utils.StringSliceContains(allowedTenants, tid) { + return false, fmt.Errorf("microsoft tenant is not allowed") + } + return true, nil + } + + configuredTenant = strings.TrimSpace(configuredTenant) + if !microsoftMultiTenantAliases[strings.ToLower(configuredTenant)] { + // A specific tenant was configured: only that directory may sign in. + if !strings.EqualFold(configuredTenant, tid) { + return false, fmt.Errorf("microsoft id_token was issued by an unexpected tenant") + } + return true, nil + } + + // Multi-tenant with no allowlist. The login is still permitted — this is a + // documented deployment mode — but the tenant is not trusted, so the caller + // must not treat the email as proof of anything. + return false, nil } // process twitch user information -func (h *httpProvider) processTwitchUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processTwitchUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processTwitchUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodTwitch) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid twitch exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid twitch exchange code: %s", err.Error()) } // Extract the ID Token from OAuth2 token. rawIDToken, ok := oauth2Token.Extra("id_token").(string) if !ok { log.Debug().Err(err).Msg("Failed to extract ID Token from OAuth2 token") - return nil, fmt.Errorf("unable to extract id_token") + return nil, false, fmt.Errorf("unable to extract id_token") } issuer := "https://id.twitch.tv/oauth2" if mockBase := h.TestOAuthBaseURL(constants.AuthRecipeMethodTwitch); mockBase != "" { @@ -1239,7 +1548,7 @@ func (h *httpProvider) processTwitchUserInfo(ctx *gin.Context, code string) (*sc oidcProvider, err := getOIDCProvider(ctx, issuer) if err != nil { log.Debug().Err(err).Msg("Failed to create OIDC provider") - return nil, fmt.Errorf("failed to create oidc provider: %s", err.Error()) + return nil, false, fmt.Errorf("failed to create oidc provider: %s", err.Error()) } verifier := oidcProvider.Verifier(&oidc.Config{ ClientID: h.TwitchClientID, @@ -1250,32 +1559,35 @@ func (h *httpProvider) processTwitchUserInfo(ctx *gin.Context, code string) (*sc idToken, err := verifier.Verify(ctx, rawIDToken) if err != nil { log.Debug().Err(err).Msg("Failed to verify ID Token") - return nil, fmt.Errorf("unable to verify id_token: %s", err.Error()) + return nil, false, fmt.Errorf("unable to verify id_token: %s", err.Error()) } claims := &oidcClaims{} if err := idToken.Claims(claims); err != nil { log.Debug().Err(err).Msg("Failed to parse ID Token claims") - return nil, fmt.Errorf("unable to extract claims") + return nil, false, fmt.Errorf("unable to extract claims") } - return claims.toUser(), nil + // Twitch is single-issuer (SkipIssuerCheck above is harmless — signature + // and `aud` already pin the token to Twitch), and its ID token carries the + // standard `email_verified`. + return claims.toUser(), bool(claims.EmailVerified), nil } // process roblox user information -func (h *httpProvider) processRobloxUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { +func (h *httpProvider) processRobloxUserInfo(ctx *gin.Context, code string) (*schemas.User, bool, error) { log := h.Log.With().Str("func", "processRobloxUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodRoblox) if err != nil { log.Debug().Err(err).Msg("Error getting oauth config") - return nil, fmt.Errorf("error getting oauth config: %s", err.Error()) + return nil, false, fmt.Errorf("error getting oauth config: %s", err.Error()) } // Roblox is a confidential client (client_secret set); PKCE is optional and // no code_challenge is sent at login, so no code_verifier is replayed here. oauth2Token, err := cfg.Exchange(ctx, code) if err != nil { log.Debug().Err(err).Msg("Failed to exchange code for token") - return nil, fmt.Errorf("invalid roblox exchange code: %s", err.Error()) + return nil, false, fmt.Errorf("invalid roblox exchange code: %s", err.Error()) } userInfoURL := constants.RobloxUserInfoURL @@ -1286,7 +1598,7 @@ func (h *httpProvider) processRobloxUserInfo(ctx *gin.Context, code string) (*sc req, err := http.NewRequest("GET", userInfoURL, nil) if err != nil { log.Debug().Err(err).Msg("Failed to create roblox user info request") - return nil, fmt.Errorf("error creating roblox user info request: %s", err.Error()) + return nil, false, fmt.Errorf("error creating roblox user info request: %s", err.Error()) } req.Header = http.Header{ "Authorization": []string{fmt.Sprintf("Bearer %s", oauth2Token.AccessToken)}, @@ -1295,25 +1607,25 @@ func (h *httpProvider) processRobloxUserInfo(ctx *gin.Context, code string) (*sc response, err := client.Do(req) if err != nil { log.Debug().Err(err).Msg("Failed to request roblox user info") - return nil, err + return nil, false, err } defer func() { _ = response.Body.Close() }() body, err := io.ReadAll(response.Body) if err != nil { log.Debug().Err(err).Msg("Failed to read roblox user info response body") - return nil, fmt.Errorf("failed to read roblox response body: %s", err.Error()) + return nil, false, fmt.Errorf("failed to read roblox response body: %s", err.Error()) } if response.StatusCode >= 400 { log.Debug().Err(err).Str("body", string(body)).Msg("Failed to request roblox user info") - return nil, fmt.Errorf("failed to request roblox user info: %s", string(body)) + return nil, false, fmt.Errorf("failed to request roblox user info: %s", string(body)) } userRawData := make(map[string]interface{}) if err := json.Unmarshal(body, &userRawData); err != nil { log.Debug().Err(err).Msg("Failed to unmarshal roblox user info") - return nil, fmt.Errorf("failed to parse roblox user info: %s", err.Error()) + return nil, false, fmt.Errorf("failed to parse roblox user info: %s", err.Error()) } firstName := "" @@ -1329,6 +1641,14 @@ func (h *httpProvider) processRobloxUserInfo(ctx *gin.Context, code string) (*sc profilePicture, _ := userRawData["picture"].(string) sub, _ := userRawData["sub"].(string) email := resolveRobloxEmail(sub, userRawData) + // Roblox's userinfo is OIDC-standard, so a real address comes with + // `email_verified`. The synthetic fallback (the default config, which does + // not request the `email` scope) is trusted by construction: reserved + // non-routable domain keyed by the permanent `sub`. + emailVerified := claimTruthy(userRawData["email_verified"]) + if sub != "" && email == robloxSyntheticEmail(sub) { + emailVerified = true + } user := &schemas.User{ GivenName: &firstName, FamilyName: &lastName, @@ -1337,7 +1657,7 @@ func (h *httpProvider) processRobloxUserInfo(ctx *gin.Context, code string) (*sc Email: &email, } - return user, nil + return user, emailVerified, nil } // resolveRobloxEmail prefers the real email Roblox returns; falls back to a diff --git a/internal/http_handlers/oauth_callback_test.go b/internal/http_handlers/oauth_callback_test.go index ee5638b6c..e54fb32a0 100644 --- a/internal/http_handlers/oauth_callback_test.go +++ b/internal/http_handlers/oauth_callback_test.go @@ -221,7 +221,7 @@ func TestProcessAppleUserInfo_ReturningUserNoUserField_Succeeds(t *testing.T) { userField, perr := parseAppleUserField("") require.NoError(t, perr) - user, err := h.processAppleUserInfo(c, "fake-oauth-code", userField) + user, _, err := h.processAppleUserInfo(c, "fake-oauth-code", userField) require.NoError(t, err, "a returning Apple user (no `user` field) must not be rejected") require.NotNil(t, user) require.NotNil(t, user.Email) @@ -252,7 +252,7 @@ func TestProcessAppleUserInfo_FirstTimeSignupWithUserField_Succeeds(t *testing.T userField, perr := parseAppleUserField(`{"email":"first-time@example.com","name":{"firstName":"Ada","lastName":"Lovelace"}}`) require.NoError(t, perr) - user, err := h.processAppleUserInfo(c, "fake-oauth-code", userField) + user, _, err := h.processAppleUserInfo(c, "fake-oauth-code", userField) require.NoError(t, err) require.NotNil(t, user) require.NotNil(t, user.Email) diff --git a/internal/http_handlers/oauth_discord_test.go b/internal/http_handlers/oauth_discord_test.go index f95f1b01f..75a0f316c 100644 --- a/internal/http_handlers/oauth_discord_test.go +++ b/internal/http_handlers/oauth_discord_test.go @@ -84,7 +84,7 @@ func TestProcessDiscordUserInfo_RealEmailUsed(t *testing.T) { server := newDiscordTestServer(t, discordProfile("42", "gracehopper", "abc123", "grace@example.com")) h := newDiscordTestHTTPProvider(t, server.URL) - user, err := h.processDiscordUserInfo(testGinContext(), "code") + user, _, err := h.processDiscordUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) @@ -104,7 +104,7 @@ func TestProcessDiscordUserInfo_AbsentEmailFallsBackToSynthetic(t *testing.T) { server := newDiscordTestServer(t, discordProfile("42", "gracehopper", "abc123", "")) h := newDiscordTestHTTPProvider(t, server.URL) - user, err := h.processDiscordUserInfo(testGinContext(), "code") + user, _, err := h.processDiscordUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) @@ -121,9 +121,9 @@ func TestProcessDiscordUserInfo_SameIDYieldsSameEmailAcrossLogins(t *testing.T) server := newDiscordTestServer(t, profile) h := newDiscordTestHTTPProvider(t, server.URL) - user1, err := h.processDiscordUserInfo(testGinContext(), "code-1") + user1, _, err := h.processDiscordUserInfo(testGinContext(), "code-1") require.NoError(t, err) - user2, err := h.processDiscordUserInfo(testGinContext(), "code-2") + user2, _, err := h.processDiscordUserInfo(testGinContext(), "code-2") require.NoError(t, err) require.NotNil(t, user1.Email) @@ -145,7 +145,7 @@ func TestProcessDiscordUserInfo_MissingID_ReturnsError(t *testing.T) { server := newDiscordTestServer(t, profile) h := newDiscordTestHTTPProvider(t, server.URL) - user, err := h.processDiscordUserInfo(testGinContext(), "code") + user, _, err := h.processDiscordUserInfo(testGinContext(), "code") assert.Error(t, err) assert.Nil(t, user) } @@ -157,7 +157,7 @@ func TestProcessDiscordUserInfo_GivenNameAndPictureMapping(t *testing.T) { server := newDiscordTestServer(t, discordProfile("99", "gracehopper", "xyz789", "grace@example.com")) h := newDiscordTestHTTPProvider(t, server.URL) - user, err := h.processDiscordUserInfo(testGinContext(), "code") + user, _, err := h.processDiscordUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.GivenName) diff --git a/internal/http_handlers/oauth_github_test.go b/internal/http_handlers/oauth_github_test.go index c850e1003..6cfa2af83 100644 --- a/internal/http_handlers/oauth_github_test.go +++ b/internal/http_handlers/oauth_github_test.go @@ -93,7 +93,7 @@ func TestProcessGithubUserInfo_MixedTypePayload(t *testing.T) { server := newGithubTestServer(t, githubProfile("ada@example.com"), nil) h := newGithubTestHTTPProvider(t, server.URL) - user, err := h.processGithubUserInfo(testGinContext(), "code") + user, _, err := h.processGithubUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) @@ -116,7 +116,7 @@ func TestProcessGithubUserInfo_NullEmailFallsBackToEmailsEndpoint(t *testing.T) }) h := newGithubTestHTTPProvider(t, server.URL) - user, err := h.processGithubUserInfo(testGinContext(), "code") + user, _, err := h.processGithubUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) diff --git a/internal/http_handlers/oauth_noauth_test.go b/internal/http_handlers/oauth_noauth_test.go new file mode 100644 index 000000000..00c554faa --- /dev/null +++ b/internal/http_handlers/oauth_noauth_test.go @@ -0,0 +1,254 @@ +package http_handlers + +import ( + "encoding/json" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/authorizerdev/authorizer/internal/config" + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/storage/schemas" +) + +// Regression tests for the nOAuth account-takeover class (audit findings #1 +// and #2). +// +// The attack: a federated login resolves a local account by email, and the +// email an IdP hands back is not necessarily one the principal controls. +// Microsoft Entra is the sharp case — its v2 ID tokens carry NO +// `email_verified` claim at all and `email` is a mutable profile attribute, so +// anyone with a free Entra tenant can set a user's email to the victim's +// address and sign in as them. Three guards close it, and each is pinned here: +// +// 1. claims decode `email_verified` at all (it used to be absent from +// oidcClaims entirely, so it was never consulted); +// 2. the quoted-string form some IdPs emit still reads as verified; +// 3. Microsoft tokens are constrained to a tenant the operator trusts, and an +// untrusted tenant's email is never treated as attested. + +func TestOIDCClaims_EmailVerifiedIsDecoded(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + payload string + want bool + }{ + {"boolean true", `{"email":"a@b.com","email_verified":true}`, true}, + {"boolean false", `{"email":"a@b.com","email_verified":false}`, false}, + // Apple documents email_verified as "a string or Boolean value"; + // LinkedIn has shipped both. Decoding either into a plain bool would + // fail the whole claim set and silently downgrade a verified email. + {"apple string true", `{"email":"a@b.com","email_verified":"true"}`, true}, + {"apple string false", `{"email":"a@b.com","email_verified":"false"}`, false}, + // Absent is the Entra case, and the one that made nOAuth work. + {"absent", `{"email":"a@b.com"}`, false}, + {"unexpected type", `{"email":"a@b.com","email_verified":1}`, false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + claims := &oidcClaims{} + require.NoError(t, json.Unmarshal([]byte(tc.payload), claims)) + assert.Equal(t, tc.want, bool(claims.EmailVerified)) + }) + } +} + +// TestValidateMicrosoftTenant_RejectsForeignTenant is the direct nOAuth +// reproduction. An attacker's own tenant mints a structurally valid token — +// real Microsoft signature, `aud` equal to the target's client id — because the +// multi-tenant endpoints sign with Microsoft's global keys. Only the tenant +// distinguishes it from a legitimate login. +func TestValidateMicrosoftTenant_RejectsForeignTenant(t *testing.T) { + t.Parallel() + const victimTenant = "11111111-1111-1111-1111-111111111111" + const attackerTenant = "99999999-9999-9999-9999-999999999999" + + attacker := &oidcClaims{ + Email: "victim@example.com", + TenantID: attackerTenant, + Issuer: "https://login.microsoftonline.com/" + attackerTenant + "/v2.0", + } + + // Deployment pinned to one tenant: a foreign tenant is refused outright. + trusted, err := validateMicrosoftTenant(attacker, victimTenant, nil) + require.Error(t, err, "a token from an unexpected tenant must not authenticate") + assert.False(t, trusted) + + // Deployment with an allowlist: same outcome. + trusted, err = validateMicrosoftTenant(attacker, "common", []string{victimTenant}) + require.Error(t, err, "a tenant outside the allowlist must not authenticate") + assert.False(t, trusted) + + // Multi-tenant with no allowlist: the login proceeds (a documented + // deployment mode) but the tenant is NOT trusted, so the caller must not + // treat the email as proof of anything. This false is what stops the + // takeover — the callback refuses to resolve a local account from it. + trusted, err = validateMicrosoftTenant(attacker, "common", nil) + require.NoError(t, err) + assert.False(t, trusted, "an arbitrary tenant's email must never be treated as attested") +} + +func TestValidateMicrosoftTenant_AcceptsTrustedTenant(t *testing.T) { + t.Parallel() + const tenant = "11111111-1111-1111-1111-111111111111" + claims := &oidcClaims{ + Email: "ada@contoso.com", + TenantID: tenant, + Issuer: "https://login.microsoftonline.com/" + tenant + "/v2.0", + } + + for _, tc := range []struct { + name string + configured string + allowed []string + }{ + {"pinned tenant", tenant, nil}, + {"pinned tenant, case-insensitive", "11111111-1111-1111-1111-111111111111", nil}, + {"allowlisted under common", "common", []string{"other-tenant", tenant}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + trusted, err := validateMicrosoftTenant(claims, tc.configured, tc.allowed) + require.NoError(t, err) + assert.True(t, trusted, "a tenant the operator trusts may assert an email") + }) + } +} + +// TestValidateMicrosoftTenant_RejectsIssuerTenantMismatch pins the +// defence-in-depth check: a token may not claim one tenant in `iss` and another +// in `tid`. Without it, `tid` could be forged to match the allowlist while the +// token actually came from elsewhere. +func TestValidateMicrosoftTenant_RejectsIssuerTenantMismatch(t *testing.T) { + t.Parallel() + const tenant = "11111111-1111-1111-1111-111111111111" + + _, err := validateMicrosoftTenant(&oidcClaims{ + TenantID: tenant, + Issuer: "https://login.microsoftonline.com/99999999-9999-9999-9999-999999999999/v2.0", + }, tenant, nil) + assert.Error(t, err, "iss and tid must agree") + + _, err = validateMicrosoftTenant(&oidcClaims{ + TenantID: tenant, + Issuer: "https://evil.example.com/" + tenant + "/v2.0", + }, tenant, nil) + assert.Error(t, err, "iss must be a Microsoft issuer") + + _, err = validateMicrosoftTenant(&oidcClaims{ + Issuer: "https://login.microsoftonline.com/" + tenant + "/v2.0", + }, tenant, nil) + assert.Error(t, err, "a token with no tid cannot be placed in any tenant") +} + +// TestAllowUnverifiedProviderEmail pins the compatibility escape hatch. The +// point of these cases is that the flag is NOT "turn the check off" — if it +// were, setting it would restore the CVE verbatim. An unattested address may +// sign up fresh or return to an account its own provider already owns, and +// nothing else. +func TestAllowUnverifiedProviderEmail(t *testing.T) { + t.Parallel() + + newProvider := func(allow bool) *httpProvider { + logger := zerolog.Nop() + return &httpProvider{ + Config: &config.Config{OAuthAllowUnverifiedProviderEmail: allow}, + Dependencies: Dependencies{Log: &logger}, + } + } + + passwordAccount := &schemas.User{SignupMethods: constants.AuthRecipeMethodBasicAuth} + googleAccount := &schemas.User{SignupMethods: constants.AuthRecipeMethodGoogle} + microsoftAccount := &schemas.User{SignupMethods: constants.AuthRecipeMethodMicrosoft} + mixedAccount := &schemas.User{ + SignupMethods: constants.AuthRecipeMethodBasicAuth + "," + constants.AuthRecipeMethodMicrosoft, + } + + t.Run("default refuses everything unattested", func(t *testing.T) { + t.Parallel() + h := newProvider(false) + assert.False(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, nil, false), + "no signup from an unattested address by default") + assert.False(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, microsoftAccount, true), + "not even a returning user of the same provider by default") + }) + + t.Run("compatibility mode still blocks cross-credential takeover", func(t *testing.T) { + t.Parallel() + h := newProvider(true) + + // This IS nOAuth: an unattested Entra address reaching for an account a + // password (or another provider) owns. Must stay refused with the flag on. + assert.False(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, passwordAccount, true), + "an unattested email must never reach a password account") + assert.False(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, googleAccount, true), + "an unattested email must never reach another provider's account") + }) + + t.Run("compatibility mode keeps existing deployments working", func(t *testing.T) { + t.Parallel() + h := newProvider(true) + + assert.True(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, nil, false), + "a brand-new account selects nobody, so it harms nobody") + assert.True(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, microsoftAccount, true), + "a returning user of this same provider must keep working") + assert.True(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, mixedAccount, true), + "an account that already linked this provider is already this provider's") + }) + + t.Run("an inconsistent storage result fails closed", func(t *testing.T) { + t.Parallel() + h := newProvider(true) + // found==true with no row is a storage inconsistency, not a signup. + // Guessing "signup" here would hand an unattested address a free pass. + assert.False(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, nil, true)) + }) + + t.Run("signup-method matching is exact, not substring", func(t *testing.T) { + t.Parallel() + h := newProvider(true) + // twitch/twitter are the near-miss pair; a substring check would let one + // provider inherit the other's accounts. + twitchAccount := &schemas.User{SignupMethods: constants.AuthRecipeMethodTwitch} + assert.False(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodTwitter, twitchAccount, true)) + assert.True(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodTwitch, twitchAccount, true)) + + // Whitespace in a stored list must not defeat the match either way. + spaced := &schemas.User{SignupMethods: "basic_auth, microsoft"} + assert.True(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodMicrosoft, spaced, true)) + assert.False(t, h.allowUnverifiedProviderEmail(constants.AuthRecipeMethodGoogle, spaced, true)) + }) +} + +// TestValidateMicrosoftTenant_XmsEdovIsTheEmailSignal documents that on the +// multi-tenant endpoints the ONLY per-token attestation Microsoft offers is +// xms_edov ("email domain owner verified") — there is no `email_verified` to +// fall back on, which is exactly why the original code trusted an unattested +// address. +func TestValidateMicrosoftTenant_XmsEdovIsTheEmailSignal(t *testing.T) { + t.Parallel() + const tenant = "99999999-9999-9999-9999-999999999999" + payload := `{ + "iss": "https://login.microsoftonline.com/` + tenant + `/v2.0", + "tid": "` + tenant + `", + "email": "victim@example.com", + "xms_edov": true + }` + claims := &oidcClaims{} + require.NoError(t, json.Unmarshal([]byte(payload), claims)) + + trusted, err := validateMicrosoftTenant(claims, "common", nil) + require.NoError(t, err) + assert.False(t, trusted, "an unknown tenant is still not trusted...") + assert.True(t, bool(claims.XmsEdov), "...but xms_edov attests the address independently") + + // And a v2 token without it — the nOAuth payload — attests nothing. + claims = &oidcClaims{} + require.NoError(t, json.Unmarshal([]byte(`{"email":"victim@example.com"}`), claims)) + assert.False(t, bool(claims.XmsEdov)) + assert.False(t, bool(claims.EmailVerified)) +} diff --git a/internal/http_handlers/oauth_providers_docs_test.go b/internal/http_handlers/oauth_providers_docs_test.go index ffeef9a8b..dcda73849 100644 --- a/internal/http_handlers/oauth_providers_docs_test.go +++ b/internal/http_handlers/oauth_providers_docs_test.go @@ -143,7 +143,7 @@ func TestProcessFacebookUserInfo_RealProfile(t *testing.T) { c.FacebookClientSecret = "test-secret" }) - user, err := h.processFacebookUserInfo(testGinContext(), "code") + user, _, err := h.processFacebookUserInfo(testGinContext(), "code") require.NoError(t, err) assert.Equal(t, "ada@example.com", refs.StringValue(user.Email)) @@ -163,7 +163,7 @@ func TestProcessFacebookUserInfo_MissingEmailIsAnError(t *testing.T) { c.FacebookClientSecret = "test-secret" }) - user, err := h.processFacebookUserInfo(testGinContext(), "code") + user, _, err := h.processFacebookUserInfo(testGinContext(), "code") require.Error(t, err) assert.Nil(t, user) assert.NotContains(t, err.Error(), "") @@ -194,7 +194,7 @@ func TestProcessLinkedInUserInfo_OIDCUserinfo(t *testing.T) { c.LinkedinClientSecret = "test-secret" }) - user, err := h.processLinkedInUserInfo(testGinContext(), "code") + user, _, err := h.processLinkedInUserInfo(testGinContext(), "code") require.NoError(t, err) assert.Equal(t, "doe@email.com", refs.StringValue(user.Email)) @@ -218,7 +218,7 @@ func TestProcessLinkedInUserInfo_OptionalEmailAbsent(t *testing.T) { c.LinkedinClientSecret = "test-secret" }) - user, err := h.processLinkedInUserInfo(testGinContext(), "code") + user, _, err := h.processLinkedInUserInfo(testGinContext(), "code") require.Error(t, err) assert.Nil(t, user) } @@ -236,7 +236,7 @@ func TestProcessGithubUserInfo_UnverifiedEmailsRejected(t *testing.T) { }) h := newGithubTestHTTPProvider(t, server.URL) - user, err := h.processGithubUserInfo(testGinContext(), "code") + user, _, err := h.processGithubUserInfo(testGinContext(), "code") require.Error(t, err) assert.Nil(t, user) } @@ -250,7 +250,7 @@ func TestProcessGithubUserInfo_PrefersVerifiedPrimary(t *testing.T) { }) h := newGithubTestHTTPProvider(t, server.URL) - user, err := h.processGithubUserInfo(testGinContext(), "code") + user, _, err := h.processGithubUserInfo(testGinContext(), "code") require.NoError(t, err) assert.Equal(t, "primary@example.com", refs.StringValue(user.Email)) } @@ -275,7 +275,7 @@ func TestProcessDiscordUserInfo_NullAvatar(t *testing.T) { c.DiscordClientSecret = "test-secret" }) - user, err := h.processDiscordUserInfo(testGinContext(), "code") + user, _, err := h.processDiscordUserInfo(testGinContext(), "code") require.NoError(t, err) assert.Equal(t, "ada@example.com", refs.StringValue(user.Email)) @@ -296,7 +296,7 @@ func TestProcessDiscordUserInfo_WithAvatar(t *testing.T) { c.DiscordClientSecret = "test-secret" }) - user, err := h.processDiscordUserInfo(testGinContext(), "code") + user, _, err := h.processDiscordUserInfo(testGinContext(), "code") require.NoError(t, err) assert.Equal(t, "https://cdn.discordapp.com/avatars/80351110224678912/8342729096ea3675442027381ff50dfe.png", diff --git a/internal/http_handlers/oauth_roblox_test.go b/internal/http_handlers/oauth_roblox_test.go index 1693aa83c..1df00431b 100644 --- a/internal/http_handlers/oauth_roblox_test.go +++ b/internal/http_handlers/oauth_roblox_test.go @@ -85,7 +85,7 @@ func TestProcessRobloxUserInfo_RealEmailUsed(t *testing.T) { server := newRobloxTestServer(t, robloxProfile("42", "Ada Lovelace", "ada", "ada@example.com")) h := newRobloxTestHTTPProvider(t, server.URL) - user, err := h.processRobloxUserInfo(testGinContext(), "code") + user, _, err := h.processRobloxUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) @@ -104,7 +104,7 @@ func TestProcessRobloxUserInfo_AbsentEmailFallsBackToSynthetic(t *testing.T) { server := newRobloxTestServer(t, robloxProfile("123456789", "Ada Lovelace", "ada", "")) h := newRobloxTestHTTPProvider(t, server.URL) - user, err := h.processRobloxUserInfo(testGinContext(), "code") + user, _, err := h.processRobloxUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) @@ -122,9 +122,9 @@ func TestProcessRobloxUserInfo_SameIDYieldsSameEmailAcrossLogins(t *testing.T) { server := newRobloxTestServer(t, profile) h := newRobloxTestHTTPProvider(t, server.URL) - user1, err := h.processRobloxUserInfo(testGinContext(), "code-1") + user1, _, err := h.processRobloxUserInfo(testGinContext(), "code-1") require.NoError(t, err) - user2, err := h.processRobloxUserInfo(testGinContext(), "code-2") + user2, _, err := h.processRobloxUserInfo(testGinContext(), "code-2") require.NoError(t, err) require.NotNil(t, user1.Email) @@ -148,7 +148,7 @@ func TestProcessRobloxUserInfo_MissingSubAndEmail_LeavesEmailEmpty(t *testing.T) server := newRobloxTestServer(t, profile) h := newRobloxTestHTTPProvider(t, server.URL) - user, err := h.processRobloxUserInfo(testGinContext(), "code") + user, _, err := h.processRobloxUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) @@ -169,7 +169,7 @@ func TestProcessRobloxUserInfo_EmptySubAndAbsentEmail_LeavesEmailEmpty(t *testin server := newRobloxTestServer(t, profile) h := newRobloxTestHTTPProvider(t, server.URL) - user, err := h.processRobloxUserInfo(testGinContext(), "code") + user, _, err := h.processRobloxUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.Email) @@ -187,7 +187,7 @@ func TestProcessRobloxUserInfo_NameGivenFamilyNicknameMapping(t *testing.T) { server := newRobloxTestServer(t, robloxProfile("99", "Ada Lovelace", "ada99", "ada@example.com")) h := newRobloxTestHTTPProvider(t, server.URL) - user, err := h.processRobloxUserInfo(testGinContext(), "code") + user, _, err := h.processRobloxUserInfo(testGinContext(), "code") require.NoError(t, err) require.NotNil(t, user.GivenName) diff --git a/internal/http_handlers/oauth_twitter_test.go b/internal/http_handlers/oauth_twitter_test.go index 522ff8f8a..96fa40bed 100644 --- a/internal/http_handlers/oauth_twitter_test.go +++ b/internal/http_handlers/oauth_twitter_test.go @@ -98,9 +98,9 @@ func TestProcessTwitterUserInfo_SameIDYieldsSameSyntheticEmail(t *testing.T) { server := newTwitterTestServer(t, twitterProfile("42", "Ada Lovelace", "ada")) h := newTwitterTestHTTPProvider(t, server.URL) - user1, err := h.processTwitterUserInfo(testGinContext(), "code-1", "verifier-1") + user1, _, err := h.processTwitterUserInfo(testGinContext(), "code-1", "verifier-1") require.NoError(t, err) - user2, err := h.processTwitterUserInfo(testGinContext(), "code-2", "verifier-2") + user2, _, err := h.processTwitterUserInfo(testGinContext(), "code-2", "verifier-2") require.NoError(t, err) require.NotNil(t, user1.Email) @@ -117,9 +117,9 @@ func TestProcessTwitterUserInfo_DifferentIDsYieldDifferentEmails(t *testing.T) { serverA := newTwitterTestServer(t, twitterProfile("1", "Alice", "alice")) serverB := newTwitterTestServer(t, twitterProfile("2", "Bob", "bob")) - userA, err := newTwitterTestHTTPProvider(t, serverA.URL).processTwitterUserInfo(testGinContext(), "code-a", "verifier-a") + userA, _, err := newTwitterTestHTTPProvider(t, serverA.URL).processTwitterUserInfo(testGinContext(), "code-a", "verifier-a") require.NoError(t, err) - userB, err := newTwitterTestHTTPProvider(t, serverB.URL).processTwitterUserInfo(testGinContext(), "code-b", "verifier-b") + userB, _, err := newTwitterTestHTTPProvider(t, serverB.URL).processTwitterUserInfo(testGinContext(), "code-b", "verifier-b") require.NoError(t, err) require.NotNil(t, userA.Email) @@ -142,7 +142,7 @@ func TestProcessTwitterUserInfo_MissingID_ReturnsError(t *testing.T) { server := newTwitterTestServer(t, profile) h := newTwitterTestHTTPProvider(t, server.URL) - user, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") + user, _, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") assert.Error(t, err) assert.Nil(t, user) } @@ -156,7 +156,7 @@ func TestProcessTwitterUserInfo_NameGivenFamilyNicknameMapping(t *testing.T) { server := newTwitterTestServer(t, twitterProfile("99", "Ada Lovelace", "ada99")) h := newTwitterTestHTTPProvider(t, server.URL) - user, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") + user, _, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") require.NoError(t, err) require.NotNil(t, user.GivenName) @@ -186,7 +186,7 @@ func TestProcessTwitterUserInfo_ConfirmedEmailPreferredOverSynthetic(t *testing. server := newTwitterTestServer(t, profile) h := newTwitterTestHTTPProvider(t, server.URL) - user, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") + user, _, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") require.NoError(t, err) require.NotNil(t, user.Email) @@ -203,7 +203,7 @@ func TestProcessTwitterUserInfo_AbsentConfirmedEmailFallsBackToSynthetic(t *test server := newTwitterTestServer(t, twitterProfile("42", "Ada Lovelace", "ada")) h := newTwitterTestHTTPProvider(t, server.URL) - user, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") + user, _, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") require.NoError(t, err) require.NotNil(t, user.Email) @@ -219,7 +219,7 @@ func TestProcessTwitterUserInfo_EmptyConfirmedEmailFallsBackToSynthetic(t *testi server := newTwitterTestServer(t, profile) h := newTwitterTestHTTPProvider(t, server.URL) - user, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") + user, _, err := h.processTwitterUserInfo(testGinContext(), "code", "verifier") require.NoError(t, err) require.NotNil(t, user.Email) diff --git a/internal/http_handlers/verify_email.go b/internal/http_handlers/verify_email.go index ba4ca565c..5b76156b8 100644 --- a/internal/http_handlers/verify_email.go +++ b/internal/http_handlers/verify_email.go @@ -76,6 +76,21 @@ func (h *httpProvider) VerifyEmailHandler() gin.HandlerFunc { return } + // Purpose binding: only the email-verification family may complete here. + // Every flow's token lives in one `verification_requests` table keyed by + // the token string alone, so without this a forgot-password token — the + // one credential this endpoint was never meant to see — redeems for a + // full session AND marks the address verified. Same reason the GraphQL + // mutation gates it (service.VerifyEmail); this handler is a separate + // implementation of the same flow and needs the same gate. Generic error + // so it is not an oracle for which flow a leaked token belongs to. + if !service.IsVerifyEmailPurpose(verificationRequest, claim) { + log.Debug().Str("identifier", verificationRequest.Identifier).Msg("Verification token used for the wrong purpose") + errorRes["error"] = "invalid verification token" + utils.HandleRedirectORJsonResponse(c, http.StatusBadRequest, errorRes, generateRedirectURL(redirectURL, errorRes)) + return + } + user, err := h.StorageProvider.GetUserByEmail(c, verificationRequest.Email) if err != nil { log.Debug().Err(err).Msg("Error getting user by email") diff --git a/internal/integration_tests/add_email_template_test.go b/internal/integration_tests/add_email_template_test.go index 76798cb85..b3f4a03d2 100644 --- a/internal/integration_tests/add_email_template_test.go +++ b/internal/integration_tests/add_email_template_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -49,7 +48,7 @@ func TestAddEmailTemplate(t *testing.T) { }) // Add admin cookie for the rest of the tests - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/add_webhook_test.go b/internal/integration_tests/add_webhook_test.go index f2b927a96..894afa025 100644 --- a/internal/integration_tests/add_webhook_test.go +++ b/internal/integration_tests/add_webhook_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -48,7 +47,7 @@ func TestAddWebhookTest(t *testing.T) { }) t.Run("should fail with blank event name", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -66,7 +65,7 @@ func TestAddWebhookTest(t *testing.T) { }) t.Run("should fail with blank endpoint", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -84,7 +83,7 @@ func TestAddWebhookTest(t *testing.T) { }) t.Run("should add webhook", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) // Use UserDeactivatedWebhookEvent to avoid data leakage from other tests diff --git a/internal/integration_tests/admin_logout_test.go b/internal/integration_tests/admin_logout_test.go index 5321f86f6..e4a717830 100644 --- a/internal/integration_tests/admin_logout_test.go +++ b/internal/integration_tests/admin_logout_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" ) // TestAdminLogout tests the logout functionality of the Authorizer application admin. @@ -22,7 +21,7 @@ func TestAdminLogout(t *testing.T) { _, err := ts.GraphQLProvider.AdminLogout(ctx) require.NotNil(t, err) - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/admin_meta_test.go b/internal/integration_tests/admin_meta_test.go index 3eb87edd6..165826714 100644 --- a/internal/integration_tests/admin_meta_test.go +++ b/internal/integration_tests/admin_meta_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -31,7 +30,7 @@ func TestAdminMeta(t *testing.T) { }) require.NoError(t, err) - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/admin_reset_mfa_test.go b/internal/integration_tests/admin_reset_mfa_test.go index 5ae42d95e..c9a33fe8c 100644 --- a/internal/integration_tests/admin_reset_mfa_test.go +++ b/internal/integration_tests/admin_reset_mfa_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/authorizerdev/authorizer/internal/storage/schemas" @@ -78,7 +77,7 @@ func TestAdminResetMFA(t *testing.T) { require.NoError(t, err) require.Len(t, creds, 1) - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/admin_session_test.go b/internal/integration_tests/admin_session_test.go index 10de1e0e0..829876359 100644 --- a/internal/integration_tests/admin_session_test.go +++ b/internal/integration_tests/admin_session_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -31,7 +30,7 @@ func TestAdminSession(t *testing.T) { _, err := ts.GraphQLProvider.AdminLogin(ctx, adminLoginReq) require.NoError(t, err) - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/admin_update_user_enforce_mfa_test.go b/internal/integration_tests/admin_update_user_enforce_mfa_test.go index 16621b8a1..7616555c7 100644 --- a/internal/integration_tests/admin_update_user_enforce_mfa_test.go +++ b/internal/integration_tests/admin_update_user_enforce_mfa_test.go @@ -10,7 +10,6 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/authorizerdev/authorizer/internal/storage/schemas" @@ -37,7 +36,7 @@ func TestAdminUpdateUserEnforceMFA(t *testing.T) { }) require.NoError(t, err) - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -77,7 +76,7 @@ func TestAdminUpdateUserMFAFlagNilToFalse(t *testing.T) { require.NoError(t, err) require.Nil(t, user.IsMultiFactorAuthEnabled, "fixture must start unset to exercise the nil->false transition") - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/audit_logs_test.go b/internal/integration_tests/audit_logs_test.go index 519121295..2315c9e91 100644 --- a/internal/integration_tests/audit_logs_test.go +++ b/internal/integration_tests/audit_logs_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" ) @@ -35,7 +34,7 @@ func TestAdminAuditLogs(t *testing.T) { }) require.NoError(t, err) - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/delete_email_template_test.go b/internal/integration_tests/delete_email_template_test.go index 70120cbd9..63eca9c0d 100644 --- a/internal/integration_tests/delete_email_template_test.go +++ b/internal/integration_tests/delete_email_template_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -45,7 +44,7 @@ func TestDeleteEmailTemplate(t *testing.T) { }) // Add admin cookie for the rest of the tests - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/delete_user_test.go b/internal/integration_tests/delete_user_test.go index 32a61e79d..a7b5c0270 100644 --- a/internal/integration_tests/delete_user_test.go +++ b/internal/integration_tests/delete_user_test.go @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" ) @@ -40,7 +39,7 @@ func TestDeleteUser(t *testing.T) { }) t.Run("should delete user", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/delete_webhook_test.go b/internal/integration_tests/delete_webhook_test.go index ec31f9a51..175cab4ff 100644 --- a/internal/integration_tests/delete_webhook_test.go +++ b/internal/integration_tests/delete_webhook_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -34,7 +33,7 @@ func TestDeleteWebhookTest(t *testing.T) { require.NotNil(t, signupRes.User) // First add a webhook to delete - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/email_templates_test.go b/internal/integration_tests/email_templates_test.go index 78f442913..5cdd13cfe 100644 --- a/internal/integration_tests/email_templates_test.go +++ b/internal/integration_tests/email_templates_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -25,7 +24,7 @@ func TestEmailTemplates(t *testing.T) { }) t.Run("should list email templates with admin auth", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -36,7 +35,7 @@ func TestEmailTemplates(t *testing.T) { }) t.Run("should return no nil entries in email templates list", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/enable_access_test.go b/internal/integration_tests/enable_access_test.go index f4ba6d09b..dd475b346 100644 --- a/internal/integration_tests/enable_access_test.go +++ b/internal/integration_tests/enable_access_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -41,7 +40,7 @@ func TestEnableAccessUser(t *testing.T) { }) t.Run("should fail with blank userid", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -52,7 +51,7 @@ func TestEnableAccessUser(t *testing.T) { }) t.Run("should fail with unknown userid", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -63,7 +62,7 @@ func TestEnableAccessUser(t *testing.T) { }) t.Run("should enable access user", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/fga_test.go b/internal/integration_tests/fga_test.go index 3aacf7235..881d93f5c 100644 --- a/internal/integration_tests/fga_test.go +++ b/internal/integration_tests/fga_test.go @@ -20,7 +20,6 @@ import ( fgaengine "github.com/authorizerdev/authorizer/internal/authorization/engine/openfga" "github.com/authorizerdev/authorizer/internal/config" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/email" "github.com/authorizerdev/authorizer/internal/events" "github.com/authorizerdev/authorizer/internal/graph/model" @@ -168,7 +167,7 @@ func initFGATestSetup(t *testing.T, cfg *config.Config) (*testSetup, engine.Auth // setAdminCookie authenticates the current gin request as super admin. func setAdminCookie(t *testing.T, ts *testSetup) { - h, err := crypto.EncryptPassword(ts.Config.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) ts.GinContext.Request.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) } diff --git a/internal/integration_tests/invite_members_test.go b/internal/integration_tests/invite_members_test.go index b4fa983b5..d56c89b24 100644 --- a/internal/integration_tests/invite_members_test.go +++ b/internal/integration_tests/invite_members_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -44,7 +43,7 @@ func TestInviteMembersUser(t *testing.T) { }) t.Run("should fail to invite user as email sending is disabled", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -59,7 +58,7 @@ func TestInviteMembersUser(t *testing.T) { cfg.IsEmailServiceEnabled = true cfg.EnableBasicAuthentication = true cfg.EnableMagicLinkLogin = true - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -74,7 +73,7 @@ func TestInviteMembersUser(t *testing.T) { cfg.IsEmailServiceEnabled = true cfg.EnableBasicAuthentication = true cfg.EnableMagicLinkLogin = true - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/revoke_access_test.go b/internal/integration_tests/revoke_access_test.go index be7d06534..fe2a29613 100644 --- a/internal/integration_tests/revoke_access_test.go +++ b/internal/integration_tests/revoke_access_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -41,7 +40,7 @@ func TestRevokeAccessUser(t *testing.T) { }) t.Run("should fail with blank userid", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -52,7 +51,7 @@ func TestRevokeAccessUser(t *testing.T) { }) t.Run("should fail with unknown userid", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -63,7 +62,7 @@ func TestRevokeAccessUser(t *testing.T) { }) t.Run("should revoke access", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/signup_events_test.go b/internal/integration_tests/signup_events_test.go index ccc53104a..2fb05d9a3 100644 --- a/internal/integration_tests/signup_events_test.go +++ b/internal/integration_tests/signup_events_test.go @@ -16,7 +16,6 @@ import ( "github.com/stretchr/testify/require" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" ) @@ -88,7 +87,7 @@ func TestSignupEmitsEventsWhenTheAccountIsCreated(t *testing.T) { // first (same pattern as add_email_template_test.go). registerHooks := func(t *testing.T, ts *testSetup, ctx context.Context, endpoint string) { t.Helper() - h, err := crypto.EncryptPassword(ts.Config.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) ts.GinContext.Request.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) for _, ev := range []string{ diff --git a/internal/integration_tests/test_endpoint_test.go b/internal/integration_tests/test_endpoint_test.go index ec42ed962..6aeecd7eb 100644 --- a/internal/integration_tests/test_endpoint_test.go +++ b/internal/integration_tests/test_endpoint_test.go @@ -9,7 +9,6 @@ import ( "time" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -100,7 +99,7 @@ func TestEndpointTest(t *testing.T) { }) // Add admin cookie for the rest of the tests - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/test_helper.go b/internal/integration_tests/test_helper.go index 4619e929a..55eb1e6e5 100644 --- a/internal/integration_tests/test_helper.go +++ b/internal/integration_tests/test_helper.go @@ -455,3 +455,19 @@ func latestMfaSessionCookie(s *testSetup) string { } return latest } + +// newAdminSessionToken mints a real server-side admin session and returns the +// opaque handle to put in the admin cookie. +// +// Tests used to write newAdminSessionToken(ts) into the cookie +// directly, because that IS what the cookie carried: a bcrypt of the admin +// secret, re-derivable by anyone holding the secret and validated by comparing +// against it. The admin cookie is now an opaque handle backed by a memory-store +// entry, so that it can expire and be revoked — a captured cookie previously +// worked forever and logout could not invalidate it. +// +// Signature deliberately mirrors the old crypto.EncryptPassword call so every +// call site keeps its `h, err := ...` shape. +func newAdminSessionToken(ts *testSetup) (string, error) { + return ts.TokenProvider.NewAdminSession() +} diff --git a/internal/integration_tests/totp_at_rest_test.go b/internal/integration_tests/totp_at_rest_test.go index c3e9ce627..faf1776b6 100644 --- a/internal/integration_tests/totp_at_rest_test.go +++ b/internal/integration_tests/totp_at_rest_test.go @@ -174,14 +174,25 @@ func TestTOTPAtRest(t *testing.T) { user := mkUser(t) authConfig, err := ts.AuthenticatorProvider.Generate(ctx, user.ID) require.NoError(t, err) + // Two validations from DIFFERENT time-steps: passcodes are single-use + // now (RFC 6238 §5.2), so replaying one is refused by design. Skew 1 + // accepts t and t+1, so both of these are valid codes. code, err := totp.GenerateCode(authConfig.Secret, time.Now()) require.NoError(t, err) ok1, err := ts.AuthenticatorProvider.Validate(ctx, code, user.ID) require.NoError(t, err) require.True(t, ok1) - ok2, err := ts.AuthenticatorProvider.Validate(ctx, code, user.ID) + nextCode, err := totp.GenerateCode(authConfig.Secret, time.Now().Add(30*time.Second)) + require.NoError(t, err) + ok2, err := ts.AuthenticatorProvider.Validate(ctx, nextCode, user.ID) require.NoError(t, err) require.True(t, ok2) + + // And the replay itself is refused — the property the single-use + // reservation exists for. + replayed, err := ts.AuthenticatorProvider.Validate(ctx, code, user.ID) + require.NoError(t, err) + require.False(t, replayed, "a TOTP passcode must not validate twice") }) t.Run("Validate is idempotent on already-encrypted rows", func(t *testing.T) { diff --git a/internal/integration_tests/totp_resetup_safety_test.go b/internal/integration_tests/totp_resetup_safety_test.go index 75f2f86c8..d073151f4 100644 --- a/internal/integration_tests/totp_resetup_safety_test.go +++ b/internal/integration_tests/totp_resetup_safety_test.go @@ -46,7 +46,11 @@ func TestTOTPResetupDoesNotDesyncUntilConfirmed(t *testing.T) { // Initial enrollment + confirmation → row is verified with secret #1. enroll1, err := ts.AuthenticatorProvider.Generate(ctx, user.ID) require.NoError(t, err) - code1, err := totp.GenerateCode(enroll1.Secret, time.Now()) + // Each passcode is single-use now (RFC 6238 §5.2), so every Validate below + // draws from a DIFFERENT time-step. totp.Validate accepts t-1, t and t+1 + // (Period 30, Skew 1), which is exactly three distinct codes per secret. + stepBack, stepNow, stepFwd := time.Now().Add(-30*time.Second), time.Now(), time.Now().Add(30*time.Second) + code1, err := totp.GenerateCode(enroll1.Secret, stepBack) require.NoError(t, err) ok, err := ts.AuthenticatorProvider.Validate(ctx, code1, user.ID) require.NoError(t, err) @@ -62,7 +66,7 @@ func TestTOTPResetupDoesNotDesyncUntilConfirmed(t *testing.T) { assert.Equal(t, liveSecret, getRow().Secret, "re-setup must NOT overwrite the live secret before confirmation") // The previously-working authenticator must keep validating. - oldCode, err := totp.GenerateCode(enroll1.Secret, time.Now()) + oldCode, err := totp.GenerateCode(enroll1.Secret, stepNow) require.NoError(t, err) ok, err = ts.AuthenticatorProvider.Validate(ctx, oldCode, user.ID) require.NoError(t, err) @@ -70,7 +74,7 @@ func TestTOTPResetupDoesNotDesyncUntilConfirmed(t *testing.T) { assert.Equal(t, liveSecret, getRow().Secret, "validating the old code must not promote the pending secret") // Confirm the new code → the pending secret is promoted to the live row. - newCode, err := totp.GenerateCode(enroll2.Secret, time.Now()) + newCode, err := totp.GenerateCode(enroll2.Secret, stepNow) require.NoError(t, err) ok, err = ts.AuthenticatorProvider.Validate(ctx, newCode, user.ID) require.NoError(t, err) @@ -78,12 +82,12 @@ func TestTOTPResetupDoesNotDesyncUntilConfirmed(t *testing.T) { assert.NotEqual(t, liveSecret, getRow().Secret, "after confirmation the new secret must be live") // The old secret must no longer validate; the new one must. - oldCodeAfter, err := totp.GenerateCode(enroll1.Secret, time.Now()) + oldCodeAfter, err := totp.GenerateCode(enroll1.Secret, stepFwd) require.NoError(t, err) ok, _ = ts.AuthenticatorProvider.Validate(ctx, oldCodeAfter, user.ID) assert.False(t, ok, "after promotion the old secret must no longer validate") - newCodeAfter, err := totp.GenerateCode(enroll2.Secret, time.Now()) + newCodeAfter, err := totp.GenerateCode(enroll2.Secret, stepFwd) require.NoError(t, err) ok, err = ts.AuthenticatorProvider.Validate(ctx, newCodeAfter, user.ID) require.NoError(t, err) diff --git a/internal/integration_tests/update_email_template_test.go b/internal/integration_tests/update_email_template_test.go index 4047cb25a..1163a5fdd 100644 --- a/internal/integration_tests/update_email_template_test.go +++ b/internal/integration_tests/update_email_template_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -48,7 +47,7 @@ func TestUpdateEmailTemplate(t *testing.T) { }) // Add admin cookie for the rest of the tests - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/update_user_test.go b/internal/integration_tests/update_user_test.go index f59c19171..35ec0360e 100644 --- a/internal/integration_tests/update_user_test.go +++ b/internal/integration_tests/update_user_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -45,7 +44,7 @@ func TestUpdateUser(t *testing.T) { }) t.Run("should update user", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -57,7 +56,7 @@ func TestUpdateUser(t *testing.T) { }) t.Run("should reject duplicate phone number", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/update_webhook_test.go b/internal/integration_tests/update_webhook_test.go index b5cf63c88..aed5fef08 100644 --- a/internal/integration_tests/update_webhook_test.go +++ b/internal/integration_tests/update_webhook_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -35,7 +34,7 @@ func TestUpdateWebhookTest(t *testing.T) { require.NotNil(t, signupRes.User) // First add a webhook to update - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) assert.Nil(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/user_test.go b/internal/integration_tests/user_test.go index a85c53b48..2ea58f9bb 100644 --- a/internal/integration_tests/user_test.go +++ b/internal/integration_tests/user_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -42,7 +41,7 @@ func TestUser(t *testing.T) { }) t.Run("should get user by ID", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -56,7 +55,7 @@ func TestUser(t *testing.T) { }) t.Run("should get user by email", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -69,7 +68,7 @@ func TestUser(t *testing.T) { }) t.Run("should fail for non-existent user", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/users_test.go b/internal/integration_tests/users_test.go index a1ad68403..76456d0e3 100644 --- a/internal/integration_tests/users_test.go +++ b/internal/integration_tests/users_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -40,7 +39,7 @@ func TestUsers(t *testing.T) { }) t.Run("should list users with admin auth", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -52,7 +51,7 @@ func TestUsers(t *testing.T) { }) t.Run("should support pagination", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -70,7 +69,7 @@ func TestUsers(t *testing.T) { }) t.Run("should filter users by case-insensitive search query", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/verification_requests_test.go b/internal/integration_tests/verification_requests_test.go index 1ced4c106..4910dfd1a 100644 --- a/internal/integration_tests/verification_requests_test.go +++ b/internal/integration_tests/verification_requests_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -33,7 +32,7 @@ func TestVerificationRequests(t *testing.T) { }) t.Run("should list verification requests with admin auth", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/verification_token_purpose_test.go b/internal/integration_tests/verification_token_purpose_test.go new file mode 100644 index 000000000..d8d813725 --- /dev/null +++ b/internal/integration_tests/verification_token_purpose_test.go @@ -0,0 +1,384 @@ +package integration_tests + +import ( + "net/http" + "net/url" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/graph/model" + "github.com/authorizerdev/authorizer/internal/refs" +) + +// Audit finding #3: verification-token purpose confusion. +// +// Magic-link, signup-verification, invite and forgot-password tokens all live +// in one `verification_requests` table keyed by the token string, and +// GetVerificationRequestByToken matches on the token alone. Neither consumer +// used to check what the token was actually minted for, so any leaked +// verification link (referer leakage, proxy/access logs, a shared URL) could be +// POSTed to the wrong endpoint: +// +// - a magic-link token redeemed at ResetPassword sets an attacker-chosen +// password AND appends basic_auth to the account's signup methods, +// escalating a one-shot passwordless capability into durable account +// takeover; +// - a forgot-password token redeemed at VerifyEmail hands out a full session. +// +// Both directions are pinned below, along with the happy paths, so the guard +// cannot be tightened into breaking legitimate flows. + +func TestVerificationTokenPurposeBinding(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + cfg.EnableMagicLinkLogin = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + t.Run("a magic-link token cannot be redeemed at ResetPassword", func(t *testing.T) { + email := "purpose_magic_" + uuid.NewString() + "@authorizer.dev" + + // Mint a real magic-link token through the real flow. + _, err := ts.GraphQLProvider.MagicLinkLogin(ctx, &model.MagicLinkLoginRequest{Email: email}) + require.NoError(t, err) + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeMagicLinkLogin) + require.NoError(t, err) + require.NotEmpty(t, vr.Token) + + res, err := ts.GraphQLProvider.ResetPassword(ctx, &model.ResetPasswordRequest{ + Token: refs.NewStringRef(vr.Token), + Password: "AttackerChosen@123", + ConfirmPassword: "AttackerChosen@123", + }) + require.Error(t, err, "a magic-link token must not be redeemable for a password change") + assert.Nil(t, res) + assert.Contains(t, err.Error(), "invalid token") + + // The account must be untouched: no password set, no basic_auth added + // to signup methods (that append is what makes this an ATO rather than + // a nuisance). + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + if user.Password != nil { + assert.NotEqual(t, nil, bcrypt.CompareHashAndPassword([]byte(*user.Password), []byte("AttackerChosen@123")), + "the attacker's password must not have been set") + } + assert.NotContains(t, user.SignupMethods, constants.AuthRecipeMethodBasicAuth, + "a refused reset must not add basic_auth to the account") + + // The token itself must still be intact for its real purpose. + still, err := ts.StorageProvider.GetVerificationRequestByToken(ctx, vr.Token) + require.NoError(t, err) + assert.Equal(t, constants.VerificationTypeMagicLinkLogin, still.Identifier) + }) + + t.Run("a signup-verification token cannot be redeemed at ResetPassword", func(t *testing.T) { + email := "purpose_signup_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + res, err := ts.GraphQLProvider.ResetPassword(ctx, &model.ResetPasswordRequest{ + Token: refs.NewStringRef(vr.Token), + Password: "AttackerChosen@123", + ConfirmPassword: "AttackerChosen@123", + }) + require.Error(t, err, "a signup token must not be redeemable for a password change") + assert.Nil(t, res) + + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + require.NotNil(t, user.Password) + assert.Error(t, bcrypt.CompareHashAndPassword([]byte(*user.Password), []byte("AttackerChosen@123")), + "the attacker's password must not have been set") + }) + + t.Run("a forgot-password token cannot be redeemed at VerifyEmail", func(t *testing.T) { + email := "purpose_forgot_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + // Consume the signup verification so the account is live. + signupVR, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: signupVR.Token}) + require.NoError(t, err) + + _, err = ts.GraphQLProvider.ForgotPassword(ctx, &model.ForgotPasswordRequest{Email: &email}) + require.NoError(t, err) + forgotVR, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeForgotPassword) + require.NoError(t, err) + + res, err := ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: forgotVR.Token}) + require.Error(t, err, "a forgot-password token must not be redeemable for a session") + assert.Nil(t, res) + assert.Contains(t, err.Error(), "invalid verification token") + }) + + t.Run("each token still works for the purpose it was minted for", func(t *testing.T) { + // Signup token -> VerifyEmail. + email := "purpose_happy_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + signupVR, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + verified, err := ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: signupVR.Token}) + require.NoError(t, err, "the signup token must still complete signup") + require.NotNil(t, verified) + + // Forgot-password token -> ResetPassword. + _, err = ts.GraphQLProvider.ForgotPassword(ctx, &model.ForgotPasswordRequest{Email: &email}) + require.NoError(t, err) + forgotVR, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeForgotPassword) + require.NoError(t, err) + reset, err := ts.GraphQLProvider.ResetPassword(ctx, &model.ResetPasswordRequest{ + Token: refs.NewStringRef(forgotVR.Token), + Password: "NewPassword@123", + ConfirmPassword: "NewPassword@123", + }) + require.NoError(t, err, "the forgot-password token must still reset the password") + require.NotNil(t, reset) + + // Magic-link token -> VerifyEmail (the magic-link family IS served here). + magicEmail := "purpose_happy_magic_" + uuid.NewString() + "@authorizer.dev" + _, err = ts.GraphQLProvider.MagicLinkLogin(ctx, &model.MagicLinkLoginRequest{Email: magicEmail}) + require.NoError(t, err) + magicVR, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, magicEmail, constants.VerificationTypeMagicLinkLogin) + require.NoError(t, err) + magicRes, err := ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: magicVR.Token}) + require.NoError(t, err, "a magic-link token must still complete a magic-link login") + require.NotNil(t, magicRes) + }) +} + +// TestVerificationTokenPurposeBindingREST pins the same guard on GET +// /verify_email. +// +// That route is a SEPARATE implementation from the GraphQL mutation — it does +// its own GetVerificationRequestByToken / ParseJWTToken / ValidateJWTClaims — +// and it is the URL every verification and magic-link mail actually points at +// (utils.GetEmailVerificationURL). Gating only the mutation left the route that +// receives real traffic wide open: a forgot-password token redeemed here issued +// a full session AND marked the address verified. The same split already caused +// the MFA gate to be missed on this handler once (see +// TestVerifyEmailRESTEndpointMFAGate), so both directions are pinned here. +func TestVerificationTokenPurposeBindingREST(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + httpClient := &http.Client{ + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + } + hitVerifyEmail := func(t *testing.T, token string) *http.Response { + t.Helper() + resp, err := httpClient.Get(ts.HttpServer.URL + "/verify_email?token=" + url.QueryEscape(token) + + "&redirect_uri=" + url.QueryEscape("http://localhost:3000/callback")) + require.NoError(t, err) + t.Cleanup(func() { _ = resp.Body.Close() }) + return resp + } + + email := "purpose_rest_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + signupVR, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + + // The handler reports both success and failure as a 307 to redirect_uri; + // what separates them is whether the Location carries tokens or an error. + t.Run("a signup token still completes verification here", func(t *testing.T) { + resp := hitVerifyEmail(t, signupVR.Token) + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + assert.Contains(t, resp.Header.Get("Location"), "access_token=", + "the purpose this route serves must keep working") + + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + assert.NotNil(t, user.EmailVerifiedAt) + }) + + t.Run("a forgot-password token is refused", func(t *testing.T) { + _, err := ts.GraphQLProvider.ForgotPassword(ctx, &model.ForgotPasswordRequest{Email: &email}) + require.NoError(t, err) + forgotVR, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeForgotPassword) + require.NoError(t, err) + + resp := hitVerifyEmail(t, forgotVR.Token) + require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + location := resp.Header.Get("Location") + assert.NotContains(t, location, "access_token=", + "a password-reset token must not be redeemable for a session") + assert.Contains(t, location, "error=") + + // The token must survive: refusing it here cannot consume the reset the + // rightful owner is still holding. + still, err := ts.StorageProvider.GetVerificationRequestByToken(ctx, forgotVR.Token) + require.NoError(t, err, "the forgot-password request must not have been deleted") + assert.Equal(t, constants.VerificationTypeForgotPassword, still.Identifier) + }) +} + +// TestResetPasswordVerifiesEmail pins the self-service recovery path. +// +// A forgot-password token is emailed to the address, is single-use and +// nonce-bound, so completing the reset proves control of that mailbox and must +// mark the address verified. This used to only happen for accounts that did not +// already have basic_auth among their signup methods — so an unverified +// password account could complete a reset and stay unverified forever, with no +// self-service way out. +// +// That matters because an unverified account now blocks a federated login for +// the same address (account pre-hijacking defense): forgot-password is the one +// recovery the rightful mailbox owner can drive without an admin. +func TestResetPasswordVerifiesEmail(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "reset_verifies_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + + // The account exists but nobody has proven control of the address yet — + // exactly the state that blocks a social login for the same email. + before, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + require.Nil(t, before.EmailVerifiedAt, "signup with verification enabled leaves the address unverified") + require.Contains(t, before.SignupMethods, constants.AuthRecipeMethodBasicAuth) + + _, err = ts.GraphQLProvider.ForgotPassword(ctx, &model.ForgotPasswordRequest{Email: &email}) + require.NoError(t, err) + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeForgotPassword) + require.NoError(t, err) + + _, err = ts.GraphQLProvider.ResetPassword(ctx, &model.ResetPasswordRequest{ + Token: refs.NewStringRef(vr.Token), + Password: "NewPassword@123", + ConfirmPassword: "NewPassword@123", + }) + require.NoError(t, err) + + after, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + assert.NotNil(t, after.EmailVerifiedAt, + "receiving and redeeming the emailed token proves mailbox control, so the address is now verified") + assert.Equal(t, before.ID, after.ID, "recovery must not replace the account") +} + +// TestResendVerifyEmailMintsFreshRequest pins the primary self-service recovery. +// +// Signup already mails a verification link, and an expired verification row is +// still returned by GetVerificationRequestByEmail (no expiry filter), so the +// ordinary expired-link case always worked. The gap was narrower: a password +// login attempt PURGES the expired row (login.go), and after that +// resend_verify_email found nothing and silently did nothing — leaving the user +// with no way to ask for a new link, and therefore no way to verify at all. +func TestResendVerifyEmailMintsFreshRequest(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "resend_fresh_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + + // Signup mails a link and records the request. + original, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err, "signup must create a verification request") + + // Simulate the purge a post-expiry password login performs. + require.NoError(t, ts.StorageProvider.DeleteVerificationRequest(ctx, original)) + _, err = ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.Error(t, err, "no pending verification request remains") + + // The user asks for a new link. This used to silently do nothing. + _, err = ts.GraphQLProvider.ResendVerifyEmail(ctx, &model.ResendVerifyEmailRequest{ + Email: email, + Identifier: constants.VerificationTypeBasicAuthSignup, + }) + require.NoError(t, err) + + fresh, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err, "a fresh verification request must be minted when none is pending") + assert.NotEqual(t, original.Token, fresh.Token, "the new link must be a new token") + + // And it actually verifies the address end to end. + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: fresh.Token}) + require.NoError(t, err, "the resent link must complete verification") + + user, err := ts.StorageProvider.GetUserByEmail(ctx, email) + require.NoError(t, err) + assert.NotNil(t, user.EmailVerifiedAt) +} + +// TestResendVerifyEmailIsNotAnOpenMailer guards the other side of that change: +// minting on demand must not let anyone who knows a registered address use this +// endpoint to send them mail. +func TestResendVerifyEmailIsNotAnOpenMailer(t *testing.T) { + cfg := getTestConfig() + cfg.IsEmailServiceEnabled = true + cfg.EnableEmailVerification = true + ts := initTestSetup(t, cfg) + _, ctx := createContext(ts) + + email := "resend_mailer_" + uuid.NewString() + "@authorizer.dev" + _, err := ts.GraphQLProvider.SignUp(ctx, &model.SignUpRequest{ + Email: &email, + Password: "Password@123", + ConfirmPassword: "Password@123", + }) + require.NoError(t, err) + vr, err := ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + require.NoError(t, err) + _, err = ts.GraphQLProvider.VerifyEmail(ctx, &model.VerifyEmailRequest{Token: vr.Token}) + require.NoError(t, err) + + // Address is now verified — there is nothing left to verify. + _, err = ts.GraphQLProvider.ResendVerifyEmail(ctx, &model.ResendVerifyEmailRequest{ + Email: email, + Identifier: constants.VerificationTypeBasicAuthSignup, + }) + require.NoError(t, err, "the response stays generic so it is not an existence oracle") + + _, err = ts.StorageProvider.GetVerificationRequestByEmail(ctx, email, constants.VerificationTypeBasicAuthSignup) + assert.Error(t, err, "no verification request may be minted for an already-verified address") +} diff --git a/internal/integration_tests/webhook_logs_test.go b/internal/integration_tests/webhook_logs_test.go index 11dc479fc..4d3812c2a 100644 --- a/internal/integration_tests/webhook_logs_test.go +++ b/internal/integration_tests/webhook_logs_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -25,7 +24,7 @@ func TestWebhookLogs(t *testing.T) { }) t.Run("should list webhook logs with admin auth", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/webhook_regression_test.go b/internal/integration_tests/webhook_regression_test.go index af81f8358..27c5d5354 100644 --- a/internal/integration_tests/webhook_regression_test.go +++ b/internal/integration_tests/webhook_regression_test.go @@ -7,7 +7,6 @@ import ( "github.com/authorizerdev/authorizer/internal/asyncutil" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -26,7 +25,7 @@ func TestSignupStillFiresWebhookEndToEnd(t *testing.T) { ts := initTestSetup(t, cfg) req, ctx := createContext(ts) - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/integration_tests/webhooks_test.go b/internal/integration_tests/webhooks_test.go index 2120a4f44..3d056994c 100644 --- a/internal/integration_tests/webhooks_test.go +++ b/internal/integration_tests/webhooks_test.go @@ -5,7 +5,6 @@ import ( "testing" "github.com/authorizerdev/authorizer/internal/constants" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/refs" "github.com/google/uuid" @@ -27,7 +26,7 @@ func TestWebhooks(t *testing.T) { }) t.Run("should list webhooks with admin auth", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -38,7 +37,7 @@ func TestWebhooks(t *testing.T) { }) t.Run("should add and get single webhook", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) @@ -69,7 +68,7 @@ func TestWebhooks(t *testing.T) { }) t.Run("should fail get webhook with invalid ID", func(t *testing.T) { - h, err := crypto.EncryptPassword(cfg.AdminSecret) + h, err := newAdminSessionToken(ts) require.NoError(t, err) req.Header.Set("Cookie", fmt.Sprintf("%s=%s", constants.AdminCookieName, h)) diff --git a/internal/service/admin_auth.go b/internal/service/admin_auth.go index d7ce3d12d..a751928ed 100644 --- a/internal/service/admin_auth.go +++ b/internal/service/admin_auth.go @@ -2,14 +2,14 @@ package service import ( "context" - "crypto/subtle" + "fmt" "github.com/authorizerdev/authorizer/internal/audit" "github.com/authorizerdev/authorizer/internal/constants" "github.com/authorizerdev/authorizer/internal/cookie" - "github.com/authorizerdev/authorizer/internal/crypto" "github.com/authorizerdev/authorizer/internal/graph/model" "github.com/authorizerdev/authorizer/internal/metrics" + "github.com/gin-gonic/gin" ) // AdminLogin validates the admin secret (constant-time) and, on success, emits @@ -18,7 +18,22 @@ import ( // establishes one. Logic migrated from internal/graphql/admin_login.go. func (p *provider) AdminLogin(ctx context.Context, meta RequestMetadata, params *model.AdminLoginRequest) (*model.Response, *ResponseSideEffects, error) { log := p.Log.With().Str("func", "AdminLogin").Logger() - if subtle.ConstantTimeCompare([]byte(params.AdminSecret), []byte(p.Config.AdminSecret)) != 1 { + // Throttled, constant-time comparison. Both this and the + // x-authorizer-admin-secret header path share one budget so neither can be + // brute-forced while the other is limited. + valid, locked := p.TokenProvider.VerifyAdminSecret(meta.IPAddress, params.AdminSecret) + if locked { + log.Warn().Str("ip", meta.IPAddress).Msg("Admin login locked: too many failed attempts") + p.AuditProvider.LogEvent(audit.Event{ + Action: constants.AuditAdminLoginFailedEvent, + Protocol: meta.Protocol, ActorType: constants.AuditActorTypeAdmin, + ResourceType: constants.AuditResourceTypeAdminSession, + IPAddress: meta.IPAddress, + UserAgent: meta.UserAgent, + }) + return nil, nil, TooManyRequests("too many failed attempts, please try again later") + } + if !valid { log.Debug().Msg("Invalid admin secret") metrics.RecordAuthEvent(metrics.EventAdminLogin, metrics.StatusFailure) metrics.RecordSecurityEvent("invalid_admin_secret", "admin_login") @@ -32,12 +47,17 @@ func (p *provider) AdminLogin(ctx context.Context, meta RequestMetadata, params return nil, nil, Unauthenticated("invalid admin secret") } - hashedKey, err := crypto.EncryptPassword(p.Config.AdminSecret) + // A server-side session handle, not a hash of the secret: the old cookie was + // a stateless bearer credential with no expiry and no revocation path, so a + // captured copy worked until the operator rotated AdminSecret — which killed + // every admin session at once. See token/admin_session.go. + sessionID, err := p.TokenProvider.NewAdminSession() if err != nil { + log.Debug().Err(err).Msg("Failed to create admin session") return nil, nil, err } side := &ResponseSideEffects{} - side.AddCookie(cookie.BuildAdminCookie(meta.HostURL, hashedKey, p.Config.AdminCookieSecure)) + side.AddCookie(cookie.BuildAdminCookie(meta.HostURL, sessionID, p.Config.AdminCookieSecure)) metrics.RecordAuthEvent(metrics.EventAdminLogin, metrics.StatusSuccess) p.AuditProvider.LogEvent(audit.Event{ @@ -56,6 +76,14 @@ func (p *provider) AdminLogout(ctx context.Context, meta RequestMetadata) (*mode if err := p.requireSuperAdmin(ctx, meta); err != nil { return nil, nil, err } + // Actually end the session server-side. Clearing the cookie only asks THIS + // browser to forget its copy; it does nothing about a copy an attacker + // already exfiltrated, which is the case logout exists for. + if sessionID, err := p.adminSessionID(meta); err == nil && sessionID != "" { + if err := p.TokenProvider.RevokeAdminSession(sessionID); err != nil { + p.Log.Debug().Err(err).Msg("Failed to revoke admin session") + } + } side := &ResponseSideEffects{} side.AddCookie(cookie.BuildDeleteAdminCookie(meta.HostURL, p.Config.AdminCookieSecure)) @@ -76,12 +104,26 @@ func (p *provider) AdminSession(ctx context.Context, meta RequestMetadata) (*mod if err := p.requireSuperAdmin(ctx, meta); err != nil { return nil, nil, err } - hashedKey, err := crypto.EncryptPassword(p.Config.AdminSecret) - if err != nil { - return nil, nil, err + // Extend the live session rather than minting a new one: the caller already + // proved they hold a valid handle to get past requireSuperAdmin, and reusing + // it keeps "refresh" from silently multiplying sessions. + // + // A caller with no cookie session still reaches here legitimately — the + // gRPC/REST transports authenticate super-admins with the + // x-authorizer-admin-secret header and carry no admin cookie at all. They + // get a freshly minted session, which is what this endpoint did for every + // caller before sessions became server-side. Refusing them would have been a + // silent break of both transports. + sessionID, err := p.adminSessionID(meta) + if err != nil || sessionID == "" || p.TokenProvider.RefreshAdminSession(sessionID) != nil { + sessionID, err = p.TokenProvider.NewAdminSession() + if err != nil { + p.Log.Debug().Err(err).Msg("Failed to create admin session") + return nil, nil, err + } } side := &ResponseSideEffects{} - side.AddCookie(cookie.BuildAdminCookie(meta.HostURL, hashedKey, p.Config.AdminCookieSecure)) + side.AddCookie(cookie.BuildAdminCookie(meta.HostURL, sessionID, p.Config.AdminCookieSecure)) return &model.Response{Message: "admin session refreshed successfully"}, side, nil } @@ -112,3 +154,13 @@ func (p *provider) AdminMeta(ctx context.Context, meta RequestMetadata) (*model. IsMultiFactorAuthServiceEnabled: p.isMFAServiceAvailable(), }, nil, nil } + +// adminSessionID reads the caller's admin session handle off the request. +// Transport-agnostic: RequestMetadata carries the raw request for both the gin +// and gRPC paths, mirroring how ResetPassword reads the MFA cookie. +func (p *provider) adminSessionID(meta RequestMetadata) (string, error) { + if meta.Request == nil { + return "", fmt.Errorf("no request") + } + return cookie.GetAdminCookie(&gin.Context{Request: meta.Request}) +} diff --git a/internal/service/login.go b/internal/service/login.go index 958fe6e9b..f4ce775cf 100644 --- a/internal/service/login.go +++ b/internal/service/login.go @@ -32,6 +32,35 @@ import ( // ops visibility. const loginGenericErrMsg = "invalid credentials" +const ( + // loginMaxFailedAttempts / loginLockoutWindowSeconds bound how many wrong + // passwords a single account will accept before the password compare stops + // running at all. The global per-IP rate limiter does not help here: an + // attacker guessing one victim's password rotates source IPs (an IPv6 /64, + // a botnet, open proxies) and faces no per-account cap at all. Values match + // the TOTP/OTP lockout (verify_otp.go) so a user sees one consistent policy + // across every credential. + // + // The trade-off this makes, stated plainly rather than wished away: ANY + // per-account lockout is a denial of service against that account, and this + // one is keyed on user id but reachable by anyone who knows the email. Six + // wrong guesses lock the account, and because IncrementCache refreshes the + // TTL on every attempt the window slides — an attacker spending one request + // every few minutes keeps a victim locked out indefinitely. The window is + // kept short so a real user recovers quickly on their own, and it is the + // same policy verify_otp.go applies, so a user sees one consistent rule + // across every credential. Removing the DoS entirely needs something this + // lockout is not (progressive delay, or throttling the source rather than + // the account); until then, blocking the password brute force is judged the + // better of the two exposures. + loginMaxFailedAttempts = 5 + loginLockoutWindowSeconds = 15 * 60 + // loginLockoutCachePrefix namespaces the per-user counter, keyed by user + // id (never by email) so the counter itself cannot be used to probe which + // addresses are registered. + loginLockoutCachePrefix = "login_failed_attempts:" +) + // mfaScopeKey namespaces the requested scope carried across an MFA // interruption, keyed by the MFA session id so it expires with it. func mfaScopeKey(mfaSession string) string { @@ -315,6 +344,25 @@ func (p *provider) Login(ctx context.Context, meta RequestMetadata, params *mode } } } + // Per-account lockout: atomically reserve this attempt's slot BEFORE the + // password compare, then check whether it exceeded the budget. Deliberately + // increment-then-check, exactly as verify_otp.go documents: IncrementCache + // hands out strictly increasing unique counts under concurrency, so at most + // loginMaxFailedAttempts requests can ever reach the compare in a window no + // matter how many arrive at once. Check-then-increment would let arbitrarily + // many parallel requests read the same pre-increment count and all pass, + // parallelizing the brute force this exists to stop. + loginLockKey := loginLockoutCachePrefix + user.ID + if attempts, incErr := p.MemoryStoreProvider.IncrementCache(loginLockKey, loginLockoutWindowSeconds); incErr != nil { + // A memory-store fault must not be counted as a user failure or lock a + // legitimate user out during an outage — same fail-open stance as the + // OTP path. + log.Debug().Err(incErr).Msg("Failed to increment login failed-attempt counter") + } else if attempts > loginMaxFailedAttempts { + metrics.RecordSecurityEvent("login_locked", "login") + log.Warn().Int64("attempts", attempts).Str("ip", meta.IPAddress).Msg("Login locked: too many failed attempts") + return nil, nil, TooManyRequests(`too many failed attempts, please try again later`) + } if user.Password == nil { // A basic_auth user with no stored hash (e.g. a pre-fix Couchbase // record that never persisted one) must fail the same way as a @@ -340,6 +388,12 @@ func (p *provider) Login(ctx context.Context, meta RequestMetadata, params *mode }) return nil, nil, Unauthenticated(loginGenericErrMsg) } + // The password is proved: clear the counter so a legitimate user who + // mistyped a few times starts fresh. Any MFA factor below carries its own + // independent lockout. + if cErr := p.MemoryStoreProvider.DeleteCacheByPrefix(loginLockKey); cErr != nil { + log.Debug().Err(cErr).Msg("Failed to reset login failed-attempt counter") + } roles := p.Config.DefaultRoles currentRoles := strings.Split(user.Roles, ",") if len(params.Roles) > 0 { diff --git a/internal/service/resend_verify_email.go b/internal/service/resend_verify_email.go index a7becb5b7..ecfd8447e 100644 --- a/internal/service/resend_verify_email.go +++ b/internal/service/resend_verify_email.go @@ -52,19 +52,36 @@ func (p *provider) ResendVerifyEmail(ctx context.Context, meta RequestMetadata, return genericResponse, nil, nil } - verificationRequest, err := p.StorageProvider.GetVerificationRequestByEmail(ctx, params.Email, params.Identifier) - if err != nil { - log.Debug().Err(err).Str("reason", "verification_request_not_found").Msg("resend verify email silently dropped") - return genericResponse, nil, nil - } + hostname := meta.HostURL + // The redirect the new link lands on. Reuse the pending request's when + // there is one, so a resend behaves exactly like the original mail. + redirectURI := hostname + "/app" - // delete current verification and create new one - err = p.StorageProvider.DeleteVerificationRequest(ctx, verificationRequest) - if err != nil { - log.Debug().Err(err).Msg("Failed to delete verification request") + verificationRequest, err := p.StorageProvider.GetVerificationRequestByEmail(ctx, params.Email, params.Identifier) + switch { + case err == nil && verificationRequest != nil: + redirectURI = verificationRequest.RedirectURI + // delete current verification and create new one + if delErr := p.StorageProvider.DeleteVerificationRequest(ctx, verificationRequest); delErr != nil { + log.Debug().Err(delErr).Msg("Failed to delete verification request") + } + default: + // No pending request. This used to silently drop, which made the + // endpoint useless in exactly the situation it exists for: verification + // requests expire after 30 minutes, so a user who came back later had no + // way to ask for a new link at all — and no way to verify their address, + // since the link is the only self-service proof of mailbox control. + // + // Mint a fresh one instead, but only when there is genuinely something + // to verify. Resending to an already-verified address would let anyone + // who knows it use this endpoint as a mailer. + if !p.resendNeedsFreshVerification(user, params.Identifier) { + log.Debug().Str("reason", "nothing_to_verify").Msg("resend verify email silently dropped") + return genericResponse, nil, nil + } + log.Debug().Msg("no pending verification request; minting a fresh one") } - hostname := meta.HostURL _, nonceHash, err := utils.GenerateNonce() if err != nil { log.Debug().Msg("Failed to generate nonce") @@ -75,7 +92,7 @@ func (p *provider) ResendVerifyEmail(ctx context.Context, meta RequestMetadata, Nonce: nonceHash, HostName: hostname, LoginMethod: constants.AuthRecipeMethodBasicAuth, - }, verificationRequest.RedirectURI, params.Identifier) + }, redirectURI, params.Identifier) if err != nil { log.Debug().Err(err).Msg("Failed to create verification token") } @@ -85,7 +102,7 @@ func (p *provider) ResendVerifyEmail(ctx context.Context, meta RequestMetadata, ExpiresAt: time.Now().Add(time.Minute * 30).Unix(), Email: params.Email, Nonce: nonceHash, - RedirectURI: verificationRequest.RedirectURI, + RedirectURI: redirectURI, }) if err != nil { log.Debug().Err(err).Msg("Failed to add verification request") @@ -96,7 +113,7 @@ func (p *provider) ResendVerifyEmail(ctx context.Context, meta RequestMetadata, _ = p.EmailProvider.SendEmail([]string{params.Email}, params.Identifier, map[string]any{ "user": user.ToMap(), "organization": utils.GetOrganization(p.Config), - "verification_url": utils.GetEmailVerificationURL(verificationToken, hostname, verificationRequest.RedirectURI), + "verification_url": utils.GetEmailVerificationURL(verificationToken, hostname, redirectURI), }) }) p.AuditProvider.LogEvent(audit.Event{ @@ -112,3 +129,28 @@ func (p *provider) ResendVerifyEmail(ctx context.Context, meta RequestMetadata, return genericResponse, nil, nil } + +// resendNeedsFreshVerification reports whether minting a brand-new verification +// request for this user is warranted when none is pending. +// +// Only the email-verification family qualifies, and only when the address is +// actually still unverified. Without this, the endpoint would happily mail a +// fresh link to any address a caller names — including already-verified ones — +// turning it into an open mailer for anyone who knows a registered address. +// +// Magic-link login is deliberately excluded: it has its own entry point +// (MagicLinkLogin) which applies its own policy, and a "resend" that mints a +// login link on demand is a login endpoint wearing a different name. +func (p *provider) resendNeedsFreshVerification(user *schemas.User, identifier string) bool { + if user == nil { + return false + } + switch identifier { + case constants.VerificationTypeBasicAuthSignup, + constants.VerificationTypeUpdateEmail, + constants.VerificationTypeInviteMember: + return user.EmailVerifiedAt == nil + default: + return false + } +} diff --git a/internal/service/reset_password.go b/internal/service/reset_password.go index 2c81e9b2f..44ed3f0a1 100644 --- a/internal/service/reset_password.go +++ b/internal/service/reset_password.go @@ -81,6 +81,13 @@ func (p *provider) ResetPassword(ctx context.Context, meta RequestMetadata, para log.Debug().Err(err).Msg("Failed to validate jwt claims") return nil, nil, InvalidArgument(`invalid token`) } + // Purpose binding: a magic-link or signup token must not be redeemable + // here. Same generic error as every other failure so this is not an + // oracle for which flow a leaked token belongs to. + if !verificationPurposeAllowed(verificationRequest, claim, verificationPurposesResetPassword) { + log.Debug().Str("identifier", verificationRequest.Identifier).Msg("Verification token used for the wrong purpose") + return nil, nil, InvalidArgument(`invalid token`) + } email = claim["sub"].(string) user, err = p.StorageProvider.GetUserByEmail(ctx, email) if err != nil { @@ -144,11 +151,20 @@ func (p *provider) ResetPassword(ctx context.Context, meta RequestMetadata, para signupMethod := user.SignupMethods if !strings.Contains(signupMethod, constants.AuthRecipeMethodBasicAuth) && isTokenVerification { signupMethod = signupMethod + "," + constants.AuthRecipeMethodBasicAuth - // helpful if user has not signed up with basic auth - if user.EmailVerifiedAt == nil { - now := time.Now().Unix() - user.EmailVerifiedAt = &now - } + } + // Completing a token reset proves control of the mailbox: the token was + // emailed to this address, is single-use, and is nonce-bound. So it verifies + // the address, whatever the account's existing signup methods are. + // + // This used to be nested inside the branch above, so it only ever fired for + // an account that did NOT already have basic_auth — meaning an unverified + // password account could complete a reset and still be left unverified, with + // no self-service way out. That matters now that an unverified account + // blocks a federated login for the same address: forgot-password is the one + // recovery path the rightful mailbox owner can drive alone. + if isTokenVerification && user.EmailVerifiedAt == nil { + now := time.Now().Unix() + user.EmailVerifiedAt = &now } if !strings.Contains(signupMethod, constants.AuthRecipeMethodMobileOTP) && isOtpVerification { signupMethod = signupMethod + "," + constants.AuthRecipeMethodMobileOTP diff --git a/internal/service/verification_purpose.go b/internal/service/verification_purpose.go new file mode 100644 index 000000000..82d30defd --- /dev/null +++ b/internal/service/verification_purpose.go @@ -0,0 +1,71 @@ +package service + +import ( + "github.com/golang-jwt/jwt/v4" + + "github.com/authorizerdev/authorizer/internal/constants" + "github.com/authorizerdev/authorizer/internal/storage/schemas" + "github.com/authorizerdev/authorizer/internal/utils" +) + +var ( + // verificationPurposesResetPassword is the only purpose ResetPassword's + // token leg accepts. ForgotPassword is the sole minter of it. + verificationPurposesResetPassword = []string{ + constants.VerificationTypeForgotPassword, + } + // verificationPurposesVerifyEmail is the email-verification family the + // verify-email endpoints legitimately complete: signup, magic-link login, an + // admin invite, and an email-change confirmation. Notably absent is + // forgot_password — redeeming one here would hand out a full session on a + // token minted for a password reset. + verificationPurposesVerifyEmail = []string{ + constants.VerificationTypeBasicAuthSignup, + constants.VerificationTypeMagicLinkLogin, + constants.VerificationTypeUpdateEmail, + constants.VerificationTypeInviteMember, + } +) + +// verificationPurposeAllowed reports whether a verification request and the +// signed `token_type` claim of the token that produced it were both issued for +// one of the purposes the calling endpoint actually serves. +// +// Tokens for every flow — signup, magic link, invite, email change, forgot +// password — share one `verification_requests` table keyed by the token string +// alone, so GetVerificationRequestByToken will happily hand a magic-link row to +// ResetPassword. Without this gate any leaked one-time login link (referer +// leakage, proxy logs, a shared URL) is redeemable for a password change, which +// also appends basic_auth to the account's signup methods — a scoped, one-shot +// capability escalated into durable account takeover. The reverse (a +// forgot-password token redeemed at VerifyEmail for a full session) works too. +// +// Both the stored Identifier and the signed claim are checked. Neither alone is +// sufficient: the DB row is selected by the attacker-supplied token, and the +// claim alone would reject the invite flow, which deliberately signs +// `invite_member` while storing `magic_link_login` as the identifier when magic +// link login is enabled (admin_access.go). +func verificationPurposeAllowed(req *schemas.VerificationRequest, claim jwt.MapClaims, allowed []string) bool { + if req == nil { + return false + } + if !utils.StringSliceContains(allowed, req.Identifier) { + return false + } + tokenType, _ := claim["token_type"].(string) + return utils.StringSliceContains(allowed, tokenType) +} + +// IsVerifyEmailPurpose is the exported form of the check for the email +// verification family, for callers outside this package. +// +// Exported because there are TWO implementations of email verification over the +// same token table — the GraphQL mutation (VerifyEmail, verify_email.go) and the +// REST handler behind GET /verify_email, which is the URL every verification and +// magic-link mail actually points at (utils.GetEmailVerificationURL). They share +// no code, so a gate applied to only one of them is not a gate: a forgot-password +// token rejected by the mutation stays redeemable at the REST route for a full +// session. Both must call this. +func IsVerifyEmailPurpose(req *schemas.VerificationRequest, claim jwt.MapClaims) bool { + return verificationPurposeAllowed(req, claim, verificationPurposesVerifyEmail) +} diff --git a/internal/service/verify_email.go b/internal/service/verify_email.go index d6cd94361..148beafd8 100644 --- a/internal/service/verify_email.go +++ b/internal/service/verify_email.go @@ -53,6 +53,13 @@ func (p *provider) VerifyEmail(ctx context.Context, meta RequestMetadata, params return nil, nil, InvalidArgument(`invalid verification token`) } + // Purpose binding: only the email-verification family may complete here. A + // forgot-password token must not be redeemable for a session. + if !IsVerifyEmailPurpose(verificationRequest, claim) { + log.Debug().Str("identifier", verificationRequest.Identifier).Msg("Verification token used for the wrong purpose") + return nil, nil, InvalidArgument(`invalid verification token`) + } + email := claim["sub"].(string) log.Debug().Str("email", email).Msg("Email verified successfully") user, err := p.StorageProvider.GetUserByEmail(ctx, email) diff --git a/internal/token/admin_lockout.go b/internal/token/admin_lockout.go new file mode 100644 index 000000000..afe0e47e4 --- /dev/null +++ b/internal/token/admin_lockout.go @@ -0,0 +1,118 @@ +package token + +import ( + "crypto/subtle" + "strconv" + + "github.com/authorizerdev/authorizer/internal/metrics" +) + +const ( + // adminSecretMaxFailedAttempts / adminSecretLockoutWindowSeconds throttle + // online guessing of the admin secret. + // + // The admin secret is the single highest-privilege credential in the system, + // and until now the only thing standing between an attacker and unlimited + // guesses was the shared 30rps request limiter — the same budget ordinary + // traffic gets. The budget here is deliberately much tighter and much + // longer-lived than the login lockout: nobody legitimately mistypes an + // admin secret dozens of times, and unlike a user account there is no + // self-service recovery an attacker could grief by tripping it. + adminSecretMaxFailedAttempts = 10 + adminSecretLockoutWindowSeconds = int64(15 * 60) + // adminSecretLockoutPrefix namespaces the counter. Keyed by client IP: + // there is only one admin secret, so a per-principal key would be a single + // global counter that any attacker could use to lock out every operator. + adminSecretLockoutPrefix = "admin_secret_failed_attempts:" + // adminSecretLockoutUnknownIP is the bucket for callers whose address we + // could not determine. It is deliberately a NAMED bucket rather than the + // empty string: keying on "" silently merges every unidentifiable caller + // into one counter, which is how a pure-gRPC deployment (no forwarded + // headers, no peer address) turns 10 wrong guesses into an outage for every + // admin client at once. Callers should ensure this is never needed — + // transport.MetaFromGRPC falls back to the gRPC peer address for exactly + // that reason — but if it is, the shared bucket must at least be visible in + // the key rather than looking like a real client. + adminSecretLockoutUnknownIP = "unknown" +) + +// VerifyAdminSecret is the single gate for every admin-secret comparison — +// AdminLogin's cookie-establishing check and the x-authorizer-admin-secret +// header path both route through it, so neither can be brute-forced while the +// other is throttled. +// +// FAILED attempts are counted, not all attempts. The counter is read before the +// comparison and incremented only when the comparison fails. The alternative — +// increment-then-check, which the login and OTP lockouts use — is right for +// those because they gate a human typing a password a few times a minute, but +// wrong here: this same function runs on EVERY request that authenticates with +// the x-authorizer-admin-secret header, so counting successes means an +// integration issuing more than adminSecretMaxFailedAttempts concurrent admin +// calls from one address gets 401s while presenting the CORRECT secret. The +// concurrency argument for increment-then-check does not apply either: it exists +// to stop parallel requests reading one stale pre-increment count and all +// passing, which matters when each parallel request is an independent guess at a +// short secret. Here every parallel request carries the same operator-chosen +// secret, and overshooting the budget by the in-flight count costs an attacker +// nothing they did not already have. +// +// Returns (valid, locked). A locked caller never reaches the comparison at all, +// so the lockout cannot itself be used as a timing oracle for the secret. +// +// What this does NOT protect against, stated plainly so it is not mistaken for a +// boundary: +// +// - clientIP comes from utils.GetIP / RequestMetadata, which prefer the +// X-Real-Ip and X-Forwarded-For request headers. On a deployment that is not +// behind a proxy that overwrites them, those are attacker-controlled: a +// guesser rotates the header per request and never fills a bucket. Fixing +// that needs trusted-proxy configuration this server does not yet have. +// - a distributed guesser gets adminSecretMaxFailedAttempts per source +// regardless. +// +// It is defence in depth that makes naive online guessing expensive, not a +// substitute for a high-entropy AdminSecret. The entropy is the control. +func (p *provider) VerifyAdminSecret(clientIP, candidate string) (valid bool, locked bool) { + // An unconfigured secret must never authenticate anything, empty candidate + // included. + if p.config.AdminSecret == "" || candidate == "" { + return false, false + } + + // The throttle is defence in depth around the comparison, never a + // precondition for it. A provider built without a memory store (unit tests, + // and any future wiring that omits it) must still authenticate correctly + // rather than nil-panic in a request path — an unrecovered panic here would + // take down the whole process, turning a missing dependency into an outage. + if p.dependencies == nil || p.dependencies.MemoryStoreProvider == nil { + return subtle.ConstantTimeCompare([]byte(candidate), []byte(p.config.AdminSecret)) == 1, false + } + + if clientIP == "" { + clientIP = adminSecretLockoutUnknownIP + } + lockKey := adminSecretLockoutPrefix + clientIP + // Fail open on a store fault: an outage must not lock every operator out of + // their own admin console. Same stance as the login/OTP paths. + if spent, err := p.dependencies.MemoryStoreProvider.GetCache(lockKey); err != nil { + p.dependencies.Log.Debug().Err(err).Msg("Failed to read admin-secret failed-attempt counter") + } else if attempts, _ := strconv.ParseInt(spent, 10, 64); attempts >= adminSecretMaxFailedAttempts { + metrics.RecordSecurityEvent("admin_secret_locked", "admin_auth") + p.dependencies.Log.Warn().Int64("attempts", attempts).Str("ip", clientIP).Msg("Admin secret verification locked: too many failed attempts") + return false, true + } + + if subtle.ConstantTimeCompare([]byte(candidate), []byte(p.config.AdminSecret)) != 1 { + if _, err := p.dependencies.MemoryStoreProvider.IncrementCache(lockKey, adminSecretLockoutWindowSeconds); err != nil { + p.dependencies.Log.Debug().Err(err).Msg("Failed to increment admin-secret failed-attempt counter") + } + return false, false + } + + // Correct secret: clear the budget so a legitimate operator who fat-fingered + // it a few times starts fresh. + if err := p.dependencies.MemoryStoreProvider.DeleteCacheByPrefix(lockKey); err != nil { + p.dependencies.Log.Debug().Err(err).Msg("Failed to reset admin-secret failed-attempt counter") + } + return true, false +} diff --git a/internal/token/admin_session.go b/internal/token/admin_session.go new file mode 100644 index 000000000..56a3fbbcd --- /dev/null +++ b/internal/token/admin_session.go @@ -0,0 +1,82 @@ +package token + +import ( + "fmt" + + "github.com/authorizerdev/authorizer/internal/crypto" +) + +const ( + // adminSessionCachePrefix namespaces server-side admin sessions in the + // memory store. + adminSessionCachePrefix = "admin_session:" + // AdminSessionTTLSeconds is the absolute lifetime of an admin session. The + // cookie's own Max-Age is a browser-side hint only — a captured cookie + // ignores it entirely — so this is the value that actually bounds exposure. + // Deliberately short: the admin session is the highest-privilege credential + // in the system. + AdminSessionTTLSeconds = int64(8 * 3600) + // adminSessionIDBytes is the entropy of the opaque session handle. 256 bits + // of crypto/rand means the handle needs no constant-time comparison — it is + // not guessable and is never derived from a secret. + adminSessionIDBytes = 32 +) + +// NewAdminSession mints a server-side admin session and returns the opaque +// handle to put in the cookie. +// +// The cookie used to carry bcrypt(AdminSecret), which made it a re-derivable +// stateless bearer credential: no `exp` inside it, no server-side record, and +// therefore no way to expire or revoke one. Any capture — a proxy log, an XSS +// exfil on the dashboard, a shared machine — granted admin access indefinitely +// until the operator rotated AdminSecret, which in turn killed every admin +// session at once. Logout could only ask the browser to drop its copy; it could +// not invalidate a copy someone else already held. +// +// A random handle backed by a store entry fixes all three: it expires (absolute +// TTL), it revokes (delete the entry), and revoking one session leaves the +// others alone. +func (p *provider) NewAdminSession() (string, error) { + sessionID, err := crypto.NewRandomString(adminSessionIDBytes) + if err != nil { + return "", err + } + if err := p.dependencies.MemoryStoreProvider.SetCache(adminSessionCachePrefix+sessionID, "1", AdminSessionTTLSeconds); err != nil { + return "", err + } + return sessionID, nil +} + +// ValidateAdminSession reports whether an opaque admin session handle is still +// live. A handle that was never issued, has expired, or has been revoked by +// logout all fail identically. +func (p *provider) ValidateAdminSession(sessionID string) error { + if sessionID == "" { + return fmt.Errorf("unauthorized") + } + val, err := p.dependencies.MemoryStoreProvider.GetCache(adminSessionCachePrefix + sessionID) + if err != nil || val == "" { + return fmt.Errorf("unauthorized") + } + return nil +} + +// RefreshAdminSession extends a live session's absolute TTL. Used by the +// AdminSession refresh operation, which already required a valid session to +// reach. +func (p *provider) RefreshAdminSession(sessionID string) error { + if err := p.ValidateAdminSession(sessionID); err != nil { + return err + } + return p.dependencies.MemoryStoreProvider.SetCache(adminSessionCachePrefix+sessionID, "1", AdminSessionTTLSeconds) +} + +// RevokeAdminSession deletes the session server-side, so a cookie copy an +// attacker already holds stops working. This is the part a stateless cookie +// could never do. +func (p *provider) RevokeAdminSession(sessionID string) error { + if sessionID == "" { + return nil + } + return p.dependencies.MemoryStoreProvider.DeleteCacheByPrefix(adminSessionCachePrefix + sessionID) +} diff --git a/internal/token/admin_token.go b/internal/token/admin_token.go index fad7616d6..afa9df28e 100644 --- a/internal/token/admin_token.go +++ b/internal/token/admin_token.go @@ -1,12 +1,11 @@ package token import ( - "crypto/subtle" "fmt" "github.com/authorizerdev/authorizer/internal/cookie" + "github.com/authorizerdev/authorizer/internal/utils" "github.com/gin-gonic/gin" - "golang.org/x/crypto/bcrypt" ) // TODO remove if not used @@ -19,18 +18,23 @@ import ( // return crypto.EncryptPassword(adminSecret) // } -// GetAdminAuthToken helps in getting the admin token from the request cookie +// GetAdminAuthToken helps in getting the admin token from the request cookie. +// +// The cookie carries an opaque server-side session handle, validated by store +// lookup. It used to carry bcrypt(AdminSecret) and be validated by comparing +// against the secret — which made it a stateless bearer credential with no +// expiry and no revocation path: a captured cookie stayed valid forever, and +// logout could only ask the browser to forget its own copy. See +// admin_session.go. func (p *provider) GetAdminAuthToken(gc *gin.Context) (string, error) { - token, err := cookie.GetAdminCookie(gc) - if err != nil || token == "" { + sessionID, err := cookie.GetAdminCookie(gc) + if err != nil || sessionID == "" { return "", fmt.Errorf("unauthorized") } - err = bcrypt.CompareHashAndPassword([]byte(token), []byte(p.config.AdminSecret)) - if err != nil { + if err := p.ValidateAdminSession(sessionID); err != nil { return "", fmt.Errorf(`unauthorized`) } - - return token, nil + return sessionID, nil } // IsSuperAdmin checks if user is super admin @@ -50,7 +54,11 @@ func (p *provider) IsSuperAdmin(gc *gin.Context) bool { if secret == "" { return false } - return subtle.ConstantTimeCompare([]byte(secret), []byte(p.config.AdminSecret)) == 1 + // Throttled: this header is an unauthenticated guess at the single + // highest-privilege credential in the system, and the only limiter in + // front of it used to be the shared 30rps budget ordinary traffic gets. + valid, _ := p.VerifyAdminSecret(utils.GetIP(gc.Request), secret) + return valid } return token != "" diff --git a/internal/token/provider.go b/internal/token/provider.go index 51b0738c7..9a558cec9 100644 --- a/internal/token/provider.go +++ b/internal/token/provider.go @@ -51,8 +51,22 @@ type Provider interface { CreateSessionToken(cfg *AuthTokenConfig) (*SessionData, string, int64, error) // CreateVerificationToken creates a verification token CreateVerificationToken(authTokenConfig *AuthTokenConfig, redirectURL string, tokenType string) (string, error) - // GetAd + // GetAdminAuthToken returns the caller's live admin session handle, or an + // error if the cookie is absent, unknown, expired, or revoked. GetAdminAuthToken(gc *gin.Context) (string, error) + // NewAdminSession mints a server-side admin session and returns the opaque + // cookie handle. See admin_session.go for why the cookie is a handle rather + // than a hash of the admin secret. + NewAdminSession() (string, error) + // RefreshAdminSession extends a live admin session's absolute TTL. + RefreshAdminSession(sessionID string) error + // RevokeAdminSession invalidates an admin session server-side, so logout + // actually ends it even for a cookie copy someone else holds. + RevokeAdminSession(sessionID string) error + // VerifyAdminSecret is the single throttled gate for every admin-secret + // comparison. Returns (valid, locked); a locked caller never reaches the + // comparison. + VerifyAdminSecret(clientIP, candidate string) (valid bool, locked bool) // GetAccessToken gets access token from request GetAccessToken(gc *gin.Context) (string, error) // GetIDToken gets id token from request diff --git a/web/dashboard/src/graphql/mutation/index.ts b/web/dashboard/src/graphql/mutation/index.ts index 4e77dae05..7f73c90db 100644 --- a/web/dashboard/src/graphql/mutation/index.ts +++ b/web/dashboard/src/graphql/mutation/index.ts @@ -30,6 +30,14 @@ export const UpdateUser = ` } `; +export const ResendVerifyEmail = ` + mutation resendVerifyEmail($params: ResendVerifyEmailRequest!) { + resend_verify_email(params: $params) { + message + } + } +`; + export const DeleteUser = ` mutation deleteUser($params: DeleteUserRequest!) { _delete_user(params: $params) { diff --git a/web/dashboard/src/pages/Users.tsx b/web/dashboard/src/pages/Users.tsx index d039a7138..db3bf13f0 100644 --- a/web/dashboard/src/pages/Users.tsx +++ b/web/dashboard/src/pages/Users.tsx @@ -13,7 +13,12 @@ import { Search, } from 'lucide-react'; import { UserDetailsQuery, AdminRolesQuery } from '../graphql/queries'; -import { EnableAccess, RevokeAccess, UpdateUser } from '../graphql/mutation'; +import { + EnableAccess, + RevokeAccess, + UpdateUser, + ResendVerifyEmail, +} from '../graphql/mutation'; import { copyTextToClipboard, getGraphQLErrorMessage } from '../utils'; import EditUserModal from '../components/EditUserModal'; import DeleteUserModal from '../components/DeleteUserModal'; @@ -178,16 +183,17 @@ export default function Users() { setPaginationProps({ ...paginationProps, ...value }); }; - const userVerificationHandler = async (user: User) => { - const { id, email, phone_number } = user; - let params: Record = {}; - if (email) { - params = { id, email, email_verified: true }; - } - if (phone_number) { - params = { id, phone_number, phone_number_verified: true }; - } - const res = await client.mutation(UpdateUser, { params }).toPromise(); + // Force-verify without touching the address itself. Deliberately does NOT + // send `email`/`phone_number`: those params drive the change-address flow in + // _update_user (which clears verification and mails a new link), and there is + // nothing to change here. + const markVerifiedHandler = async ( + user: User, + field: 'email_verified' | 'phone_number_verified', + ) => { + const res = await client + .mutation(UpdateUser, { params: { id: user.id, [field]: true } }) + .toPromise(); if (res.error) { toast.error( getGraphQLErrorMessage(res.error, 'User verification failed'), @@ -198,6 +204,27 @@ export default function Users() { updateUserList(); }; + // The other half of the operator toolkit: rather than asserting the address + // is good, mail the user a fresh link so they prove it themselves. Preferred + // when the admin has no independent reason to trust the address. + const resendVerificationHandler = async (user: User) => { + if (!user.email) { + return; + } + const res = await client + .mutation(ResendVerifyEmail, { + params: { email: user.email, identifier: 'basic_auth_signup' }, + }) + .toPromise(); + if (res.error) { + toast.error( + getGraphQLErrorMessage(res.error, 'Failed to send verification email'), + ); + } else { + toast.success('Verification email sent'); + } + }; + const updateAccessHandler = async ( id: string, action: UpdateAccessActions, @@ -415,14 +442,40 @@ export default function Users() { - {!user.email_verified && - !user.phone_number_verified && ( + {/* Split per identifier: the combined item only + appeared when BOTH were unverified, so a user with a + verified phone but an unverified email had no way to + get their email verified at all — and an unverified + email now also blocks a federated login for that + address. */} + {user.email && !user.email_verified && ( + <> userVerificationHandler(user)} + onClick={() => + markVerifiedHandler(user, 'email_verified') + } > - Verify User + Mark Email Verified - )} + resendVerificationHandler(user)} + > + Resend Verification Email + + + )} + {user.phone_number && !user.phone_number_verified && ( + + markVerifiedHandler( + user, + 'phone_number_verified', + ) + } + > + Mark Phone Verified + + )}