diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index 3b82379ac..3fd7a6cfa 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -377,6 +377,46 @@ func syncUserAvatarFromOpenID(s *xorm.Session, u *user.User, pictureURL string) return nil } +// fallbackSearchUsers builds the ordered list of local-user lookups used to link an OIDC +// login to an existing account when the provider has email and/or username fallback enabled. +// GetUserWithEmail ANDs all non-zero fields, so the email (when set) is combined with each +// username candidate. +func fallbackSearchUsers(cl *claims, provider *Provider, idToken *oidc.IDToken) []*user.User { + fallbackEmail := "" + if provider.EmailFallback { + // Used alone, allow for someone to connect from various provider to the same account. + // Discouraged for untrusted providers where someone can set email without verification. + // Note: mapping on email prevents auto-updating the user email. + fallbackEmail = cl.Email + } + + // Try the subject first (keeps working for IdPs where sub == username), then the + // preferred_username. The latter lets providers with an opaque sub (e.g. a random + // UUID, like PocketID) still link to an existing local account. + var searches []*user.User + if provider.UsernameFallback { + // Skip empty username candidates: GetUserWithEmail ANDs only non-zero fields, so a + // {Issuer, Username:"", Email:""} would degenerate to an issuer-only lookup and link + // an arbitrary local user. idToken.Subject is non-empty per OIDC, but guard anyway. + if idToken.Subject != "" { + searches = append(searches, &user.User{Issuer: user.IssuerLocal, Username: idToken.Subject, Email: fallbackEmail}) + } + preferred := strings.ReplaceAll(cl.PreferredUsername, " ", "-") + if preferred != "" && preferred != idToken.Subject { + searches = append(searches, &user.User{Issuer: user.IssuerLocal, Username: preferred, Email: fallbackEmail}) + } + } + // EmailFallback without UsernameFallback: a single email-only lookup (the caller only + // runs this when at least one fallback is enabled, so EmailFallback is guaranteed here). + // Only add it when there is a real email — an empty email would degenerate to an + // issuer-only lookup and link an arbitrary local user. + if len(searches) == 0 && cl.Email != "" { + searches = append(searches, &user.User{Issuer: user.IssuerLocal, Email: cl.Email}) + } + + return searches +} + func getOrCreateUser(s *xorm.Session, cl *claims, provider *Provider, idToken *oidc.IDToken) (u *user.User, err error) { // set defaults @@ -402,33 +442,21 @@ func getOrCreateUser(s *xorm.Session, cl *claims, provider *Provider, idToken *o if !alreadyCreatedFromIssuer && (provider.EmailFallback || provider.UsernameFallback) { - // try finding the user on fallback mappingproperties + // try finding the user on fallback mapping properties + for _, searchUser := range fallbackSearchUsers(cl, provider, idToken) { + u, err = user.GetUserWithEmail(s, searchUser) + if err != nil && !user.IsErrUserDoesNotExist(err) && !user.IsErrUserStatusError(err) { + return nil, err + } + fallbackMatchFound = err == nil || user.IsErrUserStatusError(err) - searchUser := &user.User{ - Issuer: user.IssuerLocal, - } - if provider.UsernameFallback { - // Match oidc subject on username as each is unique identifier in its own referential - // Discouraged if multiple account providers are used. - searchUser.Username = idToken.Subject - } - if provider.EmailFallback { - // Used alone, allow for someone to connect from various provider to the same account - // Discouraged for untrusted provider where someone can set email without verification - // Note : mapping on email prevent from auto-updating user email - searchUser.Email = cl.Email - } - - // Check if the user exists for the given fallback matching options - u, err = user.GetUserWithEmail(s, searchUser) - if err != nil && !user.IsErrUserDoesNotExist(err) && !user.IsErrUserStatusError(err) { - return nil, err - } - fallbackMatchFound = err == nil || user.IsErrUserStatusError(err) - - // Same as above: disabled/locked user found via fallback — return early. - if fallbackMatchFound && user.IsErrUserStatusError(err) { - return u, nil + // Same as above: disabled/locked user found via fallback — return early. + if fallbackMatchFound && user.IsErrUserStatusError(err) { + return u, nil + } + if fallbackMatchFound { + break + } } } diff --git a/pkg/modules/auth/openid/openid_test.go b/pkg/modules/auth/openid/openid_test.go index 05ade6745..35bb27547 100644 --- a/pkg/modules/auth/openid/openid_test.go +++ b/pkg/modules/auth/openid/openid_test.go @@ -254,11 +254,61 @@ func TestGetOrCreateUser(t *testing.T) { assert.Equal(t, user.IssuerLocal, u.Issuer, "User should be a local one") assert.Equal(t, 11, int(u.ID), "user id 11 expected") }) + t.Run("ProviderFallback: Match to existing local user on preferred_username when sub differs", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + cl := &claims{ + PreferredUsername: "user11", + } + provider := &Provider{ + UsernameFallback: true, + } + // PocketID-style: the subject is an opaque UUID that does not match any local username. + idToken := &oidc.IDToken{Issuer: "https://some.issuer", Subject: "c0ffee00-dead-beef-cafe-000000000011"} + + u, err := getOrCreateUser(s, cl, provider, idToken) + require.NoError(t, err) + err = s.Commit() + require.NoError(t, err) + + assert.Equal(t, "user11", u.Username, "should link to the local user matching preferred_username") + assert.Equal(t, user.IssuerLocal, u.Issuer, "User should be a local one") + assert.Equal(t, 11, int(u.ID), "user id 11 expected") + + // No duplicate user must be created for the opaque subject. + db.AssertMissing(t, "users", map[string]interface{}{ + "subject": idToken.Subject, + }) + }) + t.Run("ProviderFallback: Falls back to sub when preferred_username is empty", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + cl := &claims{ + PreferredUsername: "", + } + provider := &Provider{ + UsernameFallback: true, + } + idToken := &oidc.IDToken{Issuer: "https://some.issuer", Subject: "user11"} + + u, err := getOrCreateUser(s, cl, provider, idToken) + require.NoError(t, err) + assert.Equal(t, idToken.Subject, u.Username, "subject should match username") + assert.Equal(t, user.IssuerLocal, u.Issuer, "User should be a local one") + assert.Equal(t, 11, int(u.ID), "user id 11 expected") + }) t.Run("ProviderFallback: Match to existing local user on email", func(t *testing.T) { db.LoadAndAssertFixtures(t) s := db.NewSession() defer s.Close() + usersBefore, err := s.Count(&user.User{}) + require.NoError(t, err) + cl := &claims{ Email: "user11@example.com", } @@ -272,6 +322,42 @@ func TestGetOrCreateUser(t *testing.T) { assert.Equal(t, cl.Email, u.Email, "email should match") assert.Equal(t, user.IssuerLocal, u.Issuer, "User should be a local one") assert.Equal(t, 11, int(u.ID), "user id 11 expected") + + // The email-only fallback must link the existing user, not create a duplicate. + usersAfter, err := s.Count(&user.User{}) + require.NoError(t, err) + assert.Equal(t, usersBefore, usersAfter, "no new user should have been created") + }) + t.Run("ProviderFallback: empty email claim does not link to an arbitrary local user", func(t *testing.T) { + db.LoadAndAssertFixtures(t) + s := db.NewSession() + defer s.Close() + + usersBefore, err := s.Count(&user.User{}) + require.NoError(t, err) + + // EmailFallback on, no username fallback, and the IdP sent no email claim. The + // email-only search must not degenerate to an issuer-only lookup matching an + // arbitrary local user. With no email there is nothing safe to match on, so the + // flow falls through to user creation (which then errors because an email is + // required) rather than silently linking an existing local account. + cl := &claims{ + Email: "", + PreferredUsername: "brandNewOidcUser", + } + provider := &Provider{ + EmailFallback: true, + } + idToken := &oidc.IDToken{Issuer: "https://some.issuer", Subject: "opaque-subject-no-email"} + + u, err := getOrCreateUser(s, cl, provider, idToken) + // Must not have linked an existing local user. + require.Error(t, err, "an empty email must not silently link an existing local user") + assert.Nil(t, u, "no existing local user should be returned for an empty email claim") + + usersAfter, err := s.Count(&user.User{}) + require.NoError(t, err) + assert.Equal(t, usersBefore, usersAfter, "no user should have been linked or created from an empty email claim") }) t.Run("ProviderFallback: Match to existing local user on username and email", func(t *testing.T) {