diff --git a/pkg/modules/auth/openid/openid.go b/pkg/modules/auth/openid/openid.go index 47ade7dc9..c0ad196ec 100644 --- a/pkg/modules/auth/openid/openid.go +++ b/pkg/modules/auth/openid/openid.go @@ -392,33 +392,52 @@ 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 - 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 - } + // GetUserWithEmail ANDs all non-zero fields, so email is combined with each + // username candidate when EmailFallback is also enabled. + fallbackEmail := "" 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 + fallbackEmail = 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 + // 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 usernameCandidates []string + if provider.UsernameFallback { + usernameCandidates = append(usernameCandidates, idToken.Subject) + preferred := strings.ReplaceAll(cl.PreferredUsername, " ", "-") + if preferred != "" && preferred != idToken.Subject { + usernameCandidates = append(usernameCandidates, preferred) + } + } else { + usernameCandidates = append(usernameCandidates, "") } - 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 + for _, username := range usernameCandidates { + searchUser := &user.User{ + Issuer: user.IssuerLocal, + Username: username, + Email: fallbackEmail, + } + + 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 + } + if fallbackMatchFound { + break + } } } diff --git a/pkg/modules/auth/openid/openid_test.go b/pkg/modules/auth/openid/openid_test.go index 05ade6745..46f284d71 100644 --- a/pkg/modules/auth/openid/openid_test.go +++ b/pkg/modules/auth/openid/openid_test.go @@ -254,6 +254,53 @@ 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()