diff --git a/.gitignore b/.gitignore index 2f4820729..71bfc5b01 100644 --- a/.gitignore +++ b/.gitignore @@ -28,3 +28,4 @@ node_modules/* .claude/worktrees docs/superpowers .worktrees/ +local.mk diff --git a/Makefile b/Makefile index 959b939c5..a3214ae3d 100644 --- a/Makefile +++ b/Makefile @@ -41,6 +41,12 @@ trivy-scan: --severity HIGH,CRITICAL --ignore-unfixed --exit-code 1 clean: rm -rf build +# Local-only dev overrides. Create local.mk (gitignored) and set DEV_FLAGS to +# extra `make dev` flags — real OAuth client IDs/secrets, SMTP creds, etc. +# Example local.mk: +# DEV_FLAGS = --github-client-id=xxx --github-client-secret=yyy +-include local.mk + dev: @PRIVATE_KEY=$$(printf '%s\n' \ "-----BEGIN RSA PRIVATE KEY-----" \ @@ -88,7 +94,8 @@ dev: --admin-secret=admin \ --client-id=kbyuFDidLLm280LIwVFiazOqjO3ty8KH \ --client-secret=60Op4HFM0I8ajz0WdiStAbziZ-VFQttXuxixHHs2R7r7-CW8GR79l-mmLqMhc-Sa \ - --allowed-origins=localhost:8080,localhost:8090,localhost:9091,localhost:5173,localhost:5174 + --allowed-origins=localhost:8080,localhost:8090,localhost:9091,localhost:5173,localhost:5174 \ + $(DEV_FLAGS) test: go clean --testcache && TEST_DBS="sqlite" $(GO_TEST_ALL) diff --git a/cmd/root.go b/cmd/root.go index e1b4afd7c..1e571bf48 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -60,11 +60,14 @@ var ( defaultFacebookScopes = []string{"public_profile", "email"} defaultMicrosoftScopes = []string{"openid", "profile", "email"} defaultTwitchScopes = []string{"openid", "user:read:email"} - defaultLinkedinScopes = []string{"r_liteprofile", "r_emailaddress"} - defaultAppleScopes = []string{"email", "name"} - defaultDiscordScopes = []string{"identify", "email"} - defaultTwitterScopes = []string{"tweet.read", "users.read"} - defaultRobloxScopes = []string{"openid", "profile"} + // LinkedIn's current product is "Sign In with LinkedIn using OpenID + // Connect"; the legacy r_liteprofile/r_emailaddress scopes are not + // provisioned for apps onboarded to it. + defaultLinkedinScopes = []string{"openid", "profile", "email"} + defaultAppleScopes = []string{"email", "name"} + defaultDiscordScopes = []string{"identify", "email"} + defaultTwitterScopes = []string{"tweet.read", "users.read"} + defaultRobloxScopes = []string{"openid", "profile"} // Default RPS cap per IP; raised from 10 to reduce false positives on busy UIs. defaultRateLimitRPS = 30 defaultRateLimitBurst = 20 diff --git a/e2e-playground/mocks/mock-oauth/server.ts b/e2e-playground/mocks/mock-oauth/server.ts index 54711cc8b..43b4fe1fa 100644 --- a/e2e-playground/mocks/mock-oauth/server.ts +++ b/e2e-playground/mocks/mock-oauth/server.ts @@ -24,11 +24,15 @@ function defaultProfile(provider: string): Record { const email = `mock-user@${provider}.example.com`; switch (provider) { case 'github': - return { name: 'Mock User', email, avatar_url: 'https://example.com/avatar.png' }; + // Mixed types on purpose: GitHub's real GET /user carries a numeric id + // and boolean flags alongside the strings. + return { id: 583231, login: 'mockuser', name: 'Mock User', email, avatar_url: 'https://example.com/avatar.png', public_repos: 8, site_admin: false, company: null }; case 'facebook': return { first_name: 'Mock', last_name: 'User', email, picture: { data: { url: 'https://example.com/avatar.png' } } }; case 'linkedin': - return { localizedFirstName: 'Mock', localizedLastName: 'User' }; + // OIDC userinfo shape (api.linkedin.com/v2/userinfo), which replaced the + // legacy /v2/me + /v2/emailAddress pair. + return { sub: 'mock-linkedin-sub', name: 'Mock User', given_name: 'Mock', family_name: 'User', picture: 'https://example.com/a.png', email, email_verified: true }; case 'discord': // Flat shape matching Discord's real GET /users/@me response // (processDiscordUserInfo, internal/http_handlers/oauth_callback.go, @@ -153,12 +157,7 @@ app.get(['/:provider/userinfo', '/:provider/user', '/:provider/@me', '/:provider app.get('/:provider/user/emails', (req, res) => { const profile = (profiles[req.params.provider] || defaultProfile(req.params.provider)) as { email?: string }; - res.json([{ email: profile.email || 'mock-user@github.example.com', primary: true }]); -}); - -app.get('/:provider/emailAddress', (req, res) => { - const profile = (profiles[req.params.provider] || defaultProfile(req.params.provider)) as { email?: string }; - res.json({ elements: [{ 'handle~': { emailAddress: profile.email || 'mock-user@linkedin.example.com' } }] }); + res.json([{ email: profile.email || 'mock-user@github.example.com', primary: true, verified: true }]); }); if (require.main === module) { diff --git a/internal/constants/oauth_info_urls.go b/internal/constants/oauth_info_urls.go index 58fadc8d5..9170dab3e 100644 --- a/internal/constants/oauth_info_urls.go +++ b/internal/constants/oauth_info_urls.go @@ -11,9 +11,16 @@ const ( // Get github user emails when user info email is empty Ref: https://stackoverflow.com/a/35387123 GithubUserEmails = "https://api.github.com/user/emails" - // Ref: https://docs.microsoft.com/en-us/linkedin/shared/integrations/people/profile-api - LinkedInUserInfoURL = "https://api.linkedin.com/v2/me?projection=(id,localizedFirstName,localizedLastName,emailAddress,profilePicture(displayImage~:playableStreams))" - LinkedInEmailURL = "https://api.linkedin.com/v2/emailAddress?q=members&projection=(elements*(handle~))" + // LinkedInUserInfoURL is the OpenID Connect userinfo endpoint published in + // LinkedIn's own discovery document + // (https://www.linkedin.com/oauth/.well-known/openid-configuration). It + // returns sub/name/given_name/family_name/picture/locale/email/ + // email_verified in one call, replacing the legacy /v2/me + + // /v2/emailAddress pair, whose r_liteprofile/r_emailaddress scopes are not + // provisioned for apps onboarded via "Sign In with LinkedIn using OpenID + // Connect". + // Ref: https://learn.microsoft.com/en-us/linkedin/consumer/integrations/self-serve/sign-in-with-linkedin-v2 + LinkedInUserInfoURL = "https://api.linkedin.com/v2/userinfo" // TwitterUserInfoURL requests confirmed_email as a sparse-fieldset field // alongside the always-present id/name/profile_image_url/username. Per diff --git a/internal/http_handlers/oauth_callback.go b/internal/http_handlers/oauth_callback.go index 3eb4900c3..3cd17c3f9 100644 --- a/internal/http_handlers/oauth_callback.go +++ b/internal/http_handlers/oauth_callback.go @@ -456,6 +456,55 @@ func (h *httpProvider) OAuthCallbackHandler() gin.HandlerFunc { } } +// 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: +// +// 1. Type safety. IdPs emit claims whose types don't match the storage +// schema - Microsoft Entra's `roles` is an "Array of strings" (ID token +// claims reference) while schemas.User.Roles is a string, and a single +// mismatch fails the whole decode, so every login for a tenant that +// assigns app roles broke with "unable to extract claims". +// 2. No claim can land in a storage-only column (_id, roles, +// signup_methods, is_active, created_at ...) merely by sharing its json +// tag. +type oidcClaims struct { + Email string `json:"email"` + GivenName string `json:"given_name"` + FamilyName string `json:"family_name"` + MiddleName string `json:"middle_name"` + Nickname string `json:"nickname"` + Gender string `json:"gender"` + Birthdate string `json:"birthdate"` + PhoneNumber string `json:"phone_number"` + Picture string `json:"picture"` +} + +// toUser maps the claims onto a user, leaving absent claims nil so they don't +// overwrite stored values with empty strings. +func (c *oidcClaims) toUser() *schemas.User { + user := &schemas.User{} + for _, f := range []struct { + value string + dest **string + }{ + {c.Email, &user.Email}, + {c.GivenName, &user.GivenName}, + {c.FamilyName, &user.FamilyName}, + {c.MiddleName, &user.MiddleName}, + {c.Nickname, &user.Nickname}, + {c.Gender, &user.Gender}, + {c.Birthdate, &user.Birthdate}, + {c.PhoneNumber, &user.PhoneNumber}, + {c.Picture, &user.Picture}, + } { + if f.value != "" { + *f.dest = refs.NewStringRef(f.value) + } + } + return user +} + func (h *httpProvider) processGoogleUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { log := h.Log.With().Str("func", "processGoogleUserInfo").Logger() cfg, err := h.OAuthProvider.GetOAuthConfig(ctx, constants.AuthRecipeMethodGoogle) @@ -491,13 +540,23 @@ func (h *httpProvider) processGoogleUserInfo(ctx *gin.Context, code string) (*sc log.Debug().Err(err).Msg("Failed to verify ID Token") return nil, fmt.Errorf("unable to verify id_token: %s", err.Error()) } - user := &schemas.User{} - if err := idToken.Claims(&user); err != nil { + 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 user, nil + return claims.toUser(), nil +} + +// setGithubHeaders applies the headers GitHub's REST API docs ask every +// request to carry: a Bearer credential plus the explicit media type and API +// version, so a future default-version bump can't silently change the payload +// shape under us. +func setGithubHeaders(req *http.Request, accessToken string) { + req.Header.Set("Authorization", "Bearer "+accessToken) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") } func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*schemas.User, error) { @@ -525,9 +584,7 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc 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()) } - req.Header.Set( - "Authorization", fmt.Sprintf("token %s", oauth2Token.AccessToken), - ) + setGithubHeaders(req, oauth2Token.AccessToken) response, err := client.Do(req) if err != nil { @@ -546,13 +603,21 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc return nil, fmt.Errorf("failed to request github user info: %s", string(body)) } - userRawData := make(map[string]string) + // Only the three fields below are used. A typed struct (rather than a + // map[string]string) is required: GitHub's /user payload also carries + // numbers (id, public_repos), booleans (site_admin) and nulls, any of + // which fails a whole-map string decode. + var userRawData struct { + Name string `json:"name"` + AvatarURL string `json:"avatar_url"` + Email string `json:"email"` + } 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()) } - name := strings.Split(userRawData["name"], " ") + name := strings.Split(userRawData.Name, " ") firstName := "" lastName := "" if len(name) >= 1 && strings.TrimSpace(name[0]) != "" { @@ -562,13 +627,14 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc lastName = name[1] } - picture := userRawData["avatar_url"] - email := userRawData["email"] + picture := userRawData.AvatarURL + email := userRawData.Email if email == "" { type GithubUserEmails struct { - Email string `json:"email"` - Primary bool `json:"primary"` + Email string `json:"email"` + Primary bool `json:"primary"` + Verified bool `json:"verified"` } // fetch using /users/email endpoint @@ -577,9 +643,7 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc log.Debug().Err(err).Msg("Failed to create github emails request") return nil, fmt.Errorf("error creating github user info request: %s", err.Error()) } - req.Header.Set( - "Authorization", fmt.Sprintf("token %s", oauth2Token.AccessToken), - ) + setGithubHeaders(req, oauth2Token.AccessToken) response, err := client.Do(req) if err != nil { @@ -605,12 +669,25 @@ func (h *httpProvider) processGithubUserInfo(ctx *gin.Context, code string) (*sc return nil, fmt.Errorf("failed to parse github user email: %s", err.Error()) } + // GET /user/emails lists every address on the account, verified or + // not. An unverified address proves nothing about who controls it, and + // the caller looks the user up by email - accepting one would let a + // GitHub account that merely *typed* someone else's address log into + // that person's existing Authorizer account. Only verified addresses + // are eligible; the primary one wins. for _, userEmail := range emailData { + if !userEmail.Verified { + continue + } email = userEmail.Email if userEmail.Primary { break } } + if email == "" { + log.Debug().Msg("No verified email on github account") + return nil, fmt.Errorf("failed to get a verified email address from github") + } } user := &schemas.User{ @@ -662,22 +739,34 @@ func (h *httpProvider) processFacebookUserInfo(ctx *gin.Context, code string) (* 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)) } - userRawData := make(map[string]interface{}) + // Typed decode, not fmt.Sprintf over a map: Graph API omits `email` + // entirely when "no valid email address is available" (user/reference/user), + // and formatting a missing key stored the literal string "" as the + // user's email. Same for first_name/last_name. + var userRawData struct { + Email string `json:"email"` + FirstName string `json:"first_name"` + LastName string `json:"last_name"` + Picture struct { + Data struct { + URL string `json:"url"` + } `json:"data"` + } `json:"picture"` + } 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()) } - email := fmt.Sprintf("%v", userRawData["email"]) - - picture := "" - if picObj, ok := userRawData["picture"].(map[string]interface{}); ok { - if picData, ok := picObj["data"].(map[string]interface{}); ok { - picture = fmt.Sprintf("%v", picData["url"]) - } + 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") } - firstName := fmt.Sprintf("%v", userRawData["first_name"]) - lastName := fmt.Sprintf("%v", userRawData["last_name"]) + + picture := userRawData.Picture.Data.URL + firstName := userRawData.FirstName + lastName := userRawData.LastName user := &schemas.User{ GivenName: &firstName, @@ -704,10 +793,8 @@ func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (* } userInfoURL := constants.LinkedInUserInfoURL - emailURL := constants.LinkedInEmailURL if mockBase := h.TestOAuthBaseURL(constants.AuthRecipeMethodLinkedIn); mockBase != "" { userInfoURL = mockBase + "/userinfo" - emailURL = mockBase + "/emailAddress" } client := http.Client{} req, err := http.NewRequest("GET", userInfoURL, nil) @@ -737,80 +824,33 @@ func (h *httpProvider) processLinkedInUserInfo(ctx *gin.Context, code string) (* return nil, fmt.Errorf("failed to request linkedin user info: %s", string(body)) } - userRawData := make(map[string]interface{}) + // 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"` + } 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()) } - req, err = http.NewRequest("GET", emailURL, nil) - if err != nil { - log.Debug().Err(err).Msg("Failed to create linkedin email info request") - return nil, fmt.Errorf("error creating linkedin user info request: %s", err.Error()) - } - req.Header = http.Header{ - "Authorization": []string{fmt.Sprintf("Bearer %s", oauth2Token.AccessToken)}, - } - - response, err = client.Do(req) - if err != nil { - log.Debug().Err(err).Msg("Failed to request linkedin email info") - return nil, err - } - - defer func() { _ = response.Body.Close() }() - body, err = io.ReadAll(response.Body) - if err != nil { - log.Debug().Err(err).Msg("Failed to read linkedin email info response body") - return nil, fmt.Errorf("failed to read linkedin email 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)) - } - emailRawData := make(map[string]interface{}) - if err := json.Unmarshal(body, &emailRawData); err != nil { - log.Debug().Err(err).Msg("Failed to unmarshal linkedin email info") - return nil, fmt.Errorf("failed to parse linkedin email info: %s", err.Error()) - } - - firstName, _ := userRawData["localizedFirstName"].(string) - lastName, _ := userRawData["localizedLastName"].(string) - - // Safely extract profile picture from nested LinkedIn structure - profilePicture := "" - if pp, ok := userRawData["profilePicture"].(map[string]interface{}); ok { - if di, ok := pp["displayImage~"].(map[string]interface{}); ok { - if elems, ok := di["elements"].([]interface{}); ok && len(elems) > 0 { - if elem, ok := elems[0].(map[string]interface{}); ok { - if ids, ok := elem["identifiers"].([]interface{}); ok && len(ids) > 0 { - if id, ok := ids[0].(map[string]interface{}); ok { - profilePicture, _ = id["identifier"].(string) - } - } - } - } - } - } - - // Safely extract email from nested LinkedIn structure - emailAddress := "" - if elems, ok := emailRawData["elements"].([]interface{}); ok && len(elems) > 0 { - if elem, ok := elems[0].(map[string]interface{}); ok { - if handle, ok := elem["handle~"].(map[string]interface{}); ok { - emailAddress, _ = handle["emailAddress"].(string) - } - } - } - if emailAddress == "" { + // `email` is documented as optional - it is only present when the member + // granted the `email` scope. Without it there is no identity key at all + // (LinkedIn's `sub` is pairwise per-app), so this is a hard error rather + // 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") } user := &schemas.User{ - GivenName: &firstName, - FamilyName: &lastName, - Picture: &profilePicture, - Email: &emailAddress, + GivenName: &userRawData.GivenName, + FamilyName: &userRawData.FamilyName, + Picture: &userRawData.Picture, + Email: &userRawData.Email, } return user, nil @@ -954,8 +994,13 @@ func (h *httpProvider) processDiscordUserInfo(ctx *gin.Context, code string) (*s log.Debug().Msg("Discord user info missing id") return nil, fmt.Errorf("discord response missing id field") } - avatar, _ := userRawData["avatar"].(string) - profilePicture := fmt.Sprintf("https://cdn.discordapp.com/avatars/%s/%s.png", discordID, avatar) + // `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 + // dead ".../avatars//.png" link, so leave the picture unset instead. + profilePicture := "" + if avatar, ok := userRawData["avatar"].(string); ok && avatar != "" { + profilePicture = fmt.Sprintf("https://cdn.discordapp.com/avatars/%s/%s.png", discordID, avatar) + } email := resolveDiscordEmail(discordID, userRawData) @@ -1157,13 +1202,13 @@ func (h *httpProvider) processMicrosoftUserInfo(ctx *gin.Context, code string) ( log.Debug().Err(err).Msg("Failed to verify ID Token") return nil, fmt.Errorf("unable to verify id_token: %s", err.Error()) } - user := &schemas.User{} - if err := idToken.Claims(&user); err != nil { + 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 user, nil + return claims.toUser(), nil } // process twitch user information @@ -1208,13 +1253,13 @@ func (h *httpProvider) processTwitchUserInfo(ctx *gin.Context, code string) (*sc return nil, fmt.Errorf("unable to verify id_token: %s", err.Error()) } - user := &schemas.User{} - if err := idToken.Claims(&user); err != nil { + 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 user, nil + return claims.toUser(), nil } // process roblox user information diff --git a/internal/http_handlers/oauth_github_test.go b/internal/http_handlers/oauth_github_test.go new file mode 100644 index 000000000..c850e1003 --- /dev/null +++ b/internal/http_handlers/oauth_github_test.go @@ -0,0 +1,124 @@ +package http_handlers + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "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/oauth" +) + +// newGithubTestHTTPProvider mirrors newRobloxTestHTTPProvider +// (oauth_roblox_test.go) for GitHub. +func newGithubTestHTTPProvider(t *testing.T, mockBase string) *httpProvider { + t.Helper() + config.TestOAuthMockBaseOverride = mockBase + t.Cleanup(func() { config.TestOAuthMockBaseOverride = "" }) + logger := zerolog.Nop() + cfg := &config.Config{ + Env: constants.E2EEnv, + GithubClientID: "test-client", + GithubClientSecret: "test-secret", + } + oauthProvider, err := oauth.New(cfg, &oauth.Dependencies{Log: &logger}) + require.NoError(t, err) + return &httpProvider{ + Config: cfg, + Dependencies: Dependencies{ + Log: &logger, + OAuthProvider: oauthProvider, + }, + } +} + +// newGithubTestServer mocks the two GitHub endpoints the handler calls: +// /userinfo (GET https://api.github.com/user) and /user/emails. +func newGithubTestServer(t *testing.T, userinfoBody map[string]interface{}, emails []map[string]interface{}) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "mock-access-token", + "token_type": "bearer", + }) + }) + mux.HandleFunc("/userinfo", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(userinfoBody) + }) + mux.HandleFunc("/user/emails", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(emails) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +// githubProfile is GitHub's real GET /user shape, trimmed but keeping the +// mixed value types the live API always returns: numeric id/counts, boolean +// site_admin, and null for unset optional fields. A response of only string +// values (which an over-simplified mock would produce) never occurs in +// production. +func githubProfile(email interface{}) map[string]interface{} { + return map[string]interface{}{ + "login": "ada", + "id": 583231, + "node_id": "MDQ6VXNlcjU4MzIzMQ==", + "avatar_url": "https://avatars.githubusercontent.com/u/583231?v=4", + "name": "Ada Lovelace", + "company": nil, + "email": email, + "hireable": nil, + "public_repos": 8, + "followers": 210, + "site_admin": false, + } +} + +// TestProcessGithubUserInfo_MixedTypePayload is the regression guard: the +// handler used to decode GitHub's /user response into a map[string]string, +// which fails outright with "json: cannot unmarshal number into Go value of +// type string" on the numeric id every real response carries - every GitHub +// login broke at the callback. +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") + require.NoError(t, err) + + require.NotNil(t, user.Email) + assert.Equal(t, "ada@example.com", *user.Email) + require.NotNil(t, user.GivenName) + assert.Equal(t, "Ada", *user.GivenName) + require.NotNil(t, user.FamilyName) + assert.Equal(t, "Lovelace", *user.FamilyName) + require.NotNil(t, user.Picture) + assert.Equal(t, "https://avatars.githubusercontent.com/u/583231?v=4", *user.Picture) +} + +// TestProcessGithubUserInfo_NullEmailFallsBackToEmailsEndpoint covers the +// common case: users who keep their email private get `"email": null` on +// /user, so the handler falls back to /user/emails and picks the primary. +func TestProcessGithubUserInfo_NullEmailFallsBackToEmailsEndpoint(t *testing.T) { + server := newGithubTestServer(t, githubProfile(nil), []map[string]interface{}{ + {"email": "secondary@example.com", "primary": false, "verified": true}, + {"email": "primary@example.com", "primary": true, "verified": true}, + }) + h := newGithubTestHTTPProvider(t, server.URL) + + user, err := h.processGithubUserInfo(testGinContext(), "code") + require.NoError(t, err) + + require.NotNil(t, user.Email) + assert.Equal(t, "primary@example.com", *user.Email) +} diff --git a/internal/http_handlers/oauth_providers_docs_test.go b/internal/http_handlers/oauth_providers_docs_test.go new file mode 100644 index 000000000..ffeef9a8b --- /dev/null +++ b/internal/http_handlers/oauth_providers_docs_test.go @@ -0,0 +1,304 @@ +package http_handlers + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "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/oauth" + "github.com/authorizerdev/authorizer/internal/refs" +) + +// This file pins each provider handler to the payload its provider's own +// documentation says it returns - specifically the parts the handlers used to +// get wrong: mixed-type JSON, optional/nullable fields, and array-typed +// claims. + +// newOAuthTestServer mocks a provider's /token plus one userinfo-style route. +func newOAuthTestServer(t *testing.T, routes map[string]interface{}) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "access_token": "mock-access-token", + "token_type": "bearer", + }) + }) + for path, body := range routes { + body := body + mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(body) + }) + } + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +func newOAuthTestProvider(t *testing.T, mockBase string, apply func(*config.Config)) *httpProvider { + t.Helper() + config.TestOAuthMockBaseOverride = mockBase + t.Cleanup(func() { config.TestOAuthMockBaseOverride = "" }) + logger := zerolog.Nop() + cfg := &config.Config{Env: constants.E2EEnv} + apply(cfg) + oauthProvider, err := oauth.New(cfg, &oauth.Dependencies{Log: &logger}) + require.NoError(t, err) + return &httpProvider{ + Config: cfg, + Dependencies: Dependencies{ + Log: &logger, + OAuthProvider: oauthProvider, + }, + } +} + +// --- Microsoft / Google / Twitch: OIDC ID token claims ----------------------- + +// TestOIDCClaims_ArrayValuedClaimsDoNotBreakDecode is the regression guard for +// decoding ID tokens straight into schemas.User: Microsoft Entra emits `roles` +// as an "Array of strings" (ID token claims reference) while +// schemas.User.Roles is a string, so every login for a tenant that assigns app +// roles failed with "unable to extract claims". +func TestOIDCClaims_ArrayValuedClaimsDoNotBreakDecode(t *testing.T) { + // A realistic Entra v2.0 ID token payload. + payload := []byte(`{ + "aud": "6731de76-14a6-49ae-97bc-6eba6914391e", + "iss": "https://login.microsoftonline.com/tenant/v2.0", + "iat": 1735689600, + "name": "Ada Lovelace", + "given_name": "Ada", + "family_name": "Lovelace", + "email": "ada@contoso.com", + "preferred_username": "ada@contoso.com", + "roles": ["Admin", "Reader"], + "groups": ["7d0f7fbd-1cbe-4f2b-9d5c-4a0f1b1a3f9c"], + "wids": ["62e90394-69f5-4237-9190-012177145e10"], + "hasgroups": true, + "ver": "2.0" + }`) + + claims := &oidcClaims{} + require.NoError(t, json.Unmarshal(payload, claims), "array-valued claims must not fail the decode") + + user := claims.toUser() + assert.Equal(t, "ada@contoso.com", refs.StringValue(user.Email)) + assert.Equal(t, "Ada", refs.StringValue(user.GivenName)) + assert.Equal(t, "Lovelace", refs.StringValue(user.FamilyName)) + // Storage-only columns are never fed from claims, whatever the IdP sends. + assert.Empty(t, user.Roles) + assert.Empty(t, user.ID) + assert.Empty(t, user.SignupMethods) +} + +// TestOIDCClaims_AbsentClaimsStayNil keeps absent claims from overwriting +// stored values with empty strings. +func TestOIDCClaims_AbsentClaimsStayNil(t *testing.T) { + claims := &oidcClaims{} + require.NoError(t, json.Unmarshal([]byte(`{"email":"a@b.com"}`), claims)) + + user := claims.toUser() + require.NotNil(t, user.Email) + assert.Nil(t, user.GivenName) + assert.Nil(t, user.Picture) + assert.Nil(t, user.PhoneNumber) +} + +// --- Facebook --------------------------------------------------------------- + +func facebookProfile(withEmail bool) map[string]interface{} { + profile := map[string]interface{}{ + "id": "10224444555566666", + "first_name": "Ada", + "last_name": "Lovelace", + "name": "Ada Lovelace", + "picture": map[string]interface{}{ + "data": map[string]interface{}{ + "height": 50, + "is_silhouette": false, + "url": "https://scontent.xx.fbcdn.net/v/ada.jpg", + "width": 50, + }, + }, + } + if withEmail { + profile["email"] = "ada@example.com" + } + return profile +} + +func TestProcessFacebookUserInfo_RealProfile(t *testing.T) { + server := newOAuthTestServer(t, map[string]interface{}{"/userinfo": facebookProfile(true)}) + h := newOAuthTestProvider(t, server.URL, func(c *config.Config) { + c.FacebookClientID = "test-client" + c.FacebookClientSecret = "test-secret" + }) + + user, err := h.processFacebookUserInfo(testGinContext(), "code") + require.NoError(t, err) + + assert.Equal(t, "ada@example.com", refs.StringValue(user.Email)) + assert.Equal(t, "Ada", refs.StringValue(user.GivenName)) + assert.Equal(t, "Lovelace", refs.StringValue(user.FamilyName)) + assert.Equal(t, "https://scontent.xx.fbcdn.net/v/ada.jpg", refs.StringValue(user.Picture)) +} + +// TestProcessFacebookUserInfo_MissingEmailIsAnError is the regression guard: +// Graph API omits `email` when "no valid email address is available", and the +// handler used to fmt.Sprintf the missing key into the literal string "" +// and store that as the user's email address. +func TestProcessFacebookUserInfo_MissingEmailIsAnError(t *testing.T) { + server := newOAuthTestServer(t, map[string]interface{}{"/userinfo": facebookProfile(false)}) + h := newOAuthTestProvider(t, server.URL, func(c *config.Config) { + c.FacebookClientID = "test-client" + c.FacebookClientSecret = "test-secret" + }) + + user, err := h.processFacebookUserInfo(testGinContext(), "code") + require.Error(t, err) + assert.Nil(t, user) + assert.NotContains(t, err.Error(), "") +} + +// --- LinkedIn --------------------------------------------------------------- + +// TestProcessLinkedInUserInfo_OIDCUserinfo covers the migration off the legacy +// /v2/me + /v2/emailAddress pair (r_liteprofile/r_emailaddress, not +// provisioned for apps onboarded to "Sign In with LinkedIn using OpenID +// Connect") onto the discovery-published /v2/userinfo endpoint. Payload is +// LinkedIn's own documented sample response. +func TestProcessLinkedInUserInfo_OIDCUserinfo(t *testing.T) { + server := newOAuthTestServer(t, map[string]interface{}{ + "/userinfo": map[string]interface{}{ + "sub": "782bbtaQ", + "name": "John Doe", + "given_name": "John", + "family_name": "Doe", + "picture": "https://media.licdn-ei.com/dms/image/ada", + "locale": "en-US", + "email": "doe@email.com", + "email_verified": true, + }, + }) + h := newOAuthTestProvider(t, server.URL, func(c *config.Config) { + c.LinkedinClientID = "test-client" + c.LinkedinClientSecret = "test-secret" + }) + + user, err := h.processLinkedInUserInfo(testGinContext(), "code") + require.NoError(t, err) + + assert.Equal(t, "doe@email.com", refs.StringValue(user.Email)) + assert.Equal(t, "John", refs.StringValue(user.GivenName)) + assert.Equal(t, "Doe", refs.StringValue(user.FamilyName)) + assert.Equal(t, "https://media.licdn-ei.com/dms/image/ada", refs.StringValue(user.Picture)) +} + +// TestProcessLinkedInUserInfo_OptionalEmailAbsent: LinkedIn documents `email` +// as optional, and `sub` is pairwise per-app so it is no usable identity key. +func TestProcessLinkedInUserInfo_OptionalEmailAbsent(t *testing.T) { + server := newOAuthTestServer(t, map[string]interface{}{ + "/userinfo": map[string]interface{}{ + "sub": "782bbtaQ", + "given_name": "John", + "family_name": "Doe", + }, + }) + h := newOAuthTestProvider(t, server.URL, func(c *config.Config) { + c.LinkedinClientID = "test-client" + c.LinkedinClientSecret = "test-secret" + }) + + user, err := h.processLinkedInUserInfo(testGinContext(), "code") + require.Error(t, err) + assert.Nil(t, user) +} + +// --- GitHub: unverified email addresses -------------------------------------- + +// TestProcessGithubUserInfo_UnverifiedEmailsRejected guards the account-linking +// hole: GET /user/emails lists unverified addresses too, and the caller looks +// an existing account up by email - so accepting an unverified address would +// let a GitHub account that merely typed someone else's address sign in as +// them. +func TestProcessGithubUserInfo_UnverifiedEmailsRejected(t *testing.T) { + server := newGithubTestServer(t, githubProfile(nil), []map[string]interface{}{ + {"email": "victim@example.com", "primary": true, "verified": false}, + }) + h := newGithubTestHTTPProvider(t, server.URL) + + user, err := h.processGithubUserInfo(testGinContext(), "code") + require.Error(t, err) + assert.Nil(t, user) +} + +// TestProcessGithubUserInfo_PrefersVerifiedPrimary: an unverified address must +// not win over the verified primary one. +func TestProcessGithubUserInfo_PrefersVerifiedPrimary(t *testing.T) { + server := newGithubTestServer(t, githubProfile(nil), []map[string]interface{}{ + {"email": "unverified@example.com", "primary": false, "verified": false}, + {"email": "primary@example.com", "primary": true, "verified": true}, + }) + h := newGithubTestHTTPProvider(t, server.URL) + + user, err := h.processGithubUserInfo(testGinContext(), "code") + require.NoError(t, err) + assert.Equal(t, "primary@example.com", refs.StringValue(user.Email)) +} + +// --- Discord: nullable avatar ------------------------------------------------ + +// TestProcessDiscordUserInfo_NullAvatar: `avatar` is ?string in Discord's user +// object; building a CDN URL from an empty hash produced a dead link. +func TestProcessDiscordUserInfo_NullAvatar(t *testing.T) { + server := newOAuthTestServer(t, map[string]interface{}{ + "/userinfo": map[string]interface{}{ + "id": "80351110224678912", + "username": "ada", + "global_name": nil, + "avatar": nil, + "email": "ada@example.com", + "verified": true, + }, + }) + h := newOAuthTestProvider(t, server.URL, func(c *config.Config) { + c.DiscordClientID = "test-client" + c.DiscordClientSecret = "test-secret" + }) + + user, err := h.processDiscordUserInfo(testGinContext(), "code") + require.NoError(t, err) + + assert.Equal(t, "ada@example.com", refs.StringValue(user.Email)) + assert.Empty(t, refs.StringValue(user.Picture), "no avatar hash must not yield a dead CDN URL") +} + +func TestProcessDiscordUserInfo_WithAvatar(t *testing.T) { + server := newOAuthTestServer(t, map[string]interface{}{ + "/userinfo": map[string]interface{}{ + "id": "80351110224678912", + "username": "ada", + "avatar": "8342729096ea3675442027381ff50dfe", + "email": "ada@example.com", + }, + }) + h := newOAuthTestProvider(t, server.URL, func(c *config.Config) { + c.DiscordClientID = "test-client" + c.DiscordClientSecret = "test-secret" + }) + + user, err := h.processDiscordUserInfo(testGinContext(), "code") + require.NoError(t, err) + assert.Equal(t, + "https://cdn.discordapp.com/avatars/80351110224678912/8342729096ea3675442027381ff50dfe.png", + refs.StringValue(user.Picture)) +} diff --git a/web/app/package-lock.json b/web/app/package-lock.json index 83ad6295e..931cac253 100644 --- a/web/app/package-lock.json +++ b/web/app/package-lock.json @@ -9,7 +9,7 @@ "version": "1.0.0", "license": "Apache-2.0", "dependencies": { - "@authorizerdev/authorizer-react": "2.2.0-rc.5", + "@authorizerdev/authorizer-react": "2.2.0-rc.6", "react": "^18.3.1", "react-dom": "^18.3.1", "react-is": "^18.3.1", @@ -28,9 +28,9 @@ } }, "node_modules/@authorizerdev/authorizer-js": { - "version": "3.3.0-rc.4", - "resolved": "https://registry.npmjs.org/@authorizerdev/authorizer-js/-/authorizer-js-3.3.0-rc.4.tgz", - "integrity": "sha512-OtI9bZOn5bLFPfMwXj0Eh9VOO04VEmoK3pr8vTsmb5ieejITRvFjMOm/tmCLTzIEE0uk0tToMAD3tkUSqGGBwQ==", + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/@authorizerdev/authorizer-js/-/authorizer-js-3.3.0.tgz", + "integrity": "sha512-sxVroZPffB9IvmCSEE9ZFXplO6AyCfKsUjazUL/G7Cq4WN1diV4K376hY6UbFzSHcZ1K4UTvD1uzqZI2Pa1bwA==", "license": "MIT", "dependencies": { "cross-fetch": "^4.1.0" @@ -43,12 +43,12 @@ } }, "node_modules/@authorizerdev/authorizer-react": { - "version": "2.2.0-rc.5", - "resolved": "https://registry.npmjs.org/@authorizerdev/authorizer-react/-/authorizer-react-2.2.0-rc.5.tgz", - "integrity": "sha512-xcGEMJHqFzZMIKvVGl/BLj1ObiSraUlIb2gN+5C/te3EkUQ0ZOlbqxsb+lk2BSI/FJC5mPoqLgOA9JiiY3I7CQ==", + "version": "2.2.0-rc.6", + "resolved": "https://registry.npmjs.org/@authorizerdev/authorizer-react/-/authorizer-react-2.2.0-rc.6.tgz", + "integrity": "sha512-gQU92EnsH2L6olR6Vu2ryj24MGxINmROoWtOVLGzHkdj5fAhkGQGJPV7BNdpMnTJdnNeo7oc7K2QNjqDW4zbyg==", "license": "MIT", "dependencies": { - "@authorizerdev/authorizer-js": "^3.3.0-rc.4", + "@authorizerdev/authorizer-js": "^3.3.0", "@storybook/preset-scss": "^1.0.3", "validator": "^13.11.0" }, diff --git a/web/app/package.json b/web/app/package.json index 109a2fe60..a131ac7d9 100644 --- a/web/app/package.json +++ b/web/app/package.json @@ -13,7 +13,7 @@ "author": "Lakhan Samani", "license": "Apache-2.0", "dependencies": { - "@authorizerdev/authorizer-react": "2.2.0-rc.5", + "@authorizerdev/authorizer-react": "2.2.0-rc.6", "react": "^18.3.1", "react-dom": "^18.3.1", "react-is": "^18.3.1", diff --git a/web/app/src/pages/settings.tsx b/web/app/src/pages/settings.tsx index 6e1c2dbe8..0045df5c9 100644 --- a/web/app/src/pages/settings.tsx +++ b/web/app/src/pages/settings.tsx @@ -1,4 +1,3 @@ -import { useEffect, useState } from 'react'; import { AuthorizerMFASetup, useAuthorizer, @@ -6,19 +5,7 @@ import { import { Link } from 'react-router-dom'; export default function Settings() { - const { user, config, authorizerRef } = useAuthorizer(); - // AuthorizerMFASetup only knows what the caller tells it - there's no - // per-user enrolment signal for TOTP/email-OTP/SMS-OTP, but passkeys can - // be checked directly so the Passkey row can highlight as already set up. - const [passkeyRegistered, setPasskeyRegistered] = useState(false); - - useEffect(() => { - authorizerRef.webauthnCredentials().then(({ data, errors }) => { - if (!errors?.length && data) { - setPasskeyRegistered(data.length > 0); - } - }); - }, [authorizerRef]); + const { user, config } = useAuthorizer(); return (
@@ -35,8 +22,11 @@ export default function Settings() { emailOtp: config.is_email_otp_mfa_enabled, smsOtp: config.is_sms_otp_mfa_enabled, }} + // What this user already enrolled (server-side truth, part of the + // user fragment) - those tiles render as "Enabled"/"Manage" instead + // of offering a fresh setup. + enrolledMethods={user?.enrolled_mfa_methods} heading="Add a second step to sign in" - passkeyRegistered={passkeyRegistered} />