Skip to content

fix: MFA enrolment state in settings + OAuth provider contract audit - #740

Merged
lakhansamani merged 4 commits into
mainfrom
fix/mfa-enrolled-state
Aug 3, 2026
Merged

fix: MFA enrolment state in settings + OAuth provider contract audit#740
lakhansamani merged 4 commits into
mainfrom
fix/mfa-enrolled-state

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Three related fixes found while manually testing /app.

1. Manage MFA showed "Set up" for methods already enrolled

A user with TOTP verified opened Manage MFA and every tile — authenticator app, email OTP, SMS OTP — read Set up, as if nothing was enrolled. Only the passkey tile reflected reality, because it was the one thing the page fetched separately (webauthnCredentials()).

Nothing was wrong server-side: User.enrolled_mfa_methods already reports exactly what is verified, and authorizer-js already fetches it in the user fragment. The picker had no prop to receive it. Fixed in authorizerdev/authorizer-react#73 (released as 2.2.0-rc.6); this PR passes user.enrolled_mfa_methods through and drops the now-redundant webauthnCredentials() round-trip.

To be explicit about the original question: a user cannot end up with multiple TOTP secrets. Authenticators are one row per (user_id, method), and re-running setup stages a pending secret that only replaces the live one once a code from it is confirmed (internal/integration_tests/totp_resetup_safety_test.go). The bug was purely that the UI misreported the state.

2. Every GitHub login was failing

Failed to unmarshal github user info: json: cannot unmarshal number into Go value of type string

processGithubUserInfo decoded GET /user into a map[string]string, but the payload carries a numeric id, integer counts and booleans — one mismatch fails the whole decode.

3. Audit of all ten provider handlers against the providers' own docs

Provider Defect
GitHub map[string]string decode of a mixed-type payload (above)
GitHub /user/emails lists unverified addresses and the code accepted them. The callback resolves accounts by email, so a GitHub account that merely typed someone else's address could resolve to that person's existing user. Verified addresses only now
Google / Microsoft / Twitch idToken.Claims(&user) decoded ID-token claims straight into schemas.User. Entra emits roles as an array of strings while User.Roles is a string → login broke with "unable to extract claims" for any tenant assigning app roles. Claims could also reach storage-only columns (_id, signup_methods, is_active) by json tag. Now decoded into an explicit oidcClaims allow-list
Facebook Graph omits email when no valid address exists; fmt.Sprintf("%v", missing) stored the literal string "<nil>" as the email
LinkedIn /v2/me + /v2/emailAddress need r_liteprofile/r_emailaddress, never provisioned for apps onboarded to "Sign In with LinkedIn using OpenID Connect" — LinkedIn login was dead for any recent app. Moved to the discovery-published /v2/userinfo
Discord avatar is nullable; an empty hash built the dead URL .../avatars/<id>/.png
GitHub Authorization: tokenBearer, plus Accept and X-GitHub-Api-Version

Verified correct with no change: Apple (user form field is first-authorization-only, already handled), Twitter/X (confirmed_email is a real user.fields value, PKCE correct), Roblox, Google/Microsoft discovery and audience verification.

⚠️ Behavior change

Default LinkedIn scopes are now openid profile email (were r_liteprofile r_emailaddress). Operators pinning --linkedin-scopes to the old values must update them.

4. Dev credentials out of the Makefile

make dev appends $(DEV_FLAGS) from an optional gitignored local.mk, so real OAuth client secrets used for local testing stop living as an uncommitted Makefile edit that's one git add -A from being published.

Tests

internal/http_handlers/oauth_github_test.go and oauth_providers_docs_test.go — 10 tests, each pinned to the payload shape the provider's docs publish: Entra's array-valued roles/groups, LinkedIn's documented sample userinfo response, Discord's null avatar, GitHub's mixed-type /user and unverified-email list. The GitHub test reproduces the exact production error when the fix is reverted. The e2e mock OAuth server now serves the real shapes too.

Verification

go build ./..., go vet ./..., make test, make lint (0 issues) — all pass. web/app type-checks and builds against the published 2.2.0-rc.6.

The MFA settings page showed every method as "Set up", including the
authenticator app a user had already enrolled. Pass the server's
user.enrolled_mfa_methods to AuthorizerMFASetup (2.2.0-rc.6) so those
tiles read "Enabled"/"Manage".

Drops the extra webauthnCredentials() round-trip: the same field
already reports passkey enrolment.
Audited all ten process*UserInfo paths against provider docs. Fixes:

- github: /user has a numeric id, so decoding into map[string]string
  failed every login ("cannot unmarshal number into Go value of type
  string"). Typed struct now.
- github: /user/emails lists unverified addresses; accepting one let a
  GitHub account that merely typed someone else's address resolve to
  that person's existing user. Verified addresses only.
- oidc (google/microsoft/twitch): id_token claims decoded straight into
  schemas.User. Entra emits `roles` as an array while User.Roles is a
  string, breaking login for tenants with app roles; claims could also
  reach storage-only columns by tag name. Decode into an allow-list.
- facebook: `email` is omitted when no valid address exists, and
  fmt.Sprintf over the missing key stored the literal "<nil>".
- linkedin: /v2/me + /v2/emailAddress need r_liteprofile/r_emailaddress,
  which apps onboarded to Sign In with LinkedIn (OIDC) never get. Moved
  to the discovery-published /v2/userinfo; scopes now openid/profile/email.
- discord: `avatar` is nullable; an empty hash built a dead CDN URL.
- github: Bearer + Accept + X-GitHub-Api-Version per current docs.
`make dev` now appends $(DEV_FLAGS) from an optional, gitignored
local.mk, so real OAuth client IDs/secrets stay out of the tree instead
of living as an uncommitted Makefile edit that's one `git add -A` away
from being published.
@lakhansamani
lakhansamani merged commit 47c8c32 into main Aug 3, 2026
6 checks passed
@lakhansamani
lakhansamani deleted the fix/mfa-enrolled-state branch August 3, 2026 22:12
lakhansamani added a commit that referenced this pull request Aug 4, 2026
#740 moved the handler and mock to /v2/userinfo; the spec still
configured localizedFirstName/localizedLastName, so given_name came
back empty.
lakhansamani added a commit that referenced this pull request Aug 6, 2026
The spec still configured the mock with localizedFirstName/localizedLastName
from the legacy /v2/me + /v2/emailAddress pair. #740 migrated the handler
and the mock's default profile to the OIDC userinfo shape, but __configure
REPLACES the default wholesale, so the test was sending a payload
processLinkedInUserInfo cannot read — given_name landed empty and the
failure named no cause.

Its comment described the removed two-call flow as current; corrected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant