refactor(auth): make OIDC email-only fallback explicit instead of an empty-username sentinel

The fallback block only runs when EmailFallback || UsernameFallback, so when
UsernameFallback is off the else branch (previously an empty-username candidate)
is the email-only lookup path: it must still run one GetUserWithEmail to link an
existing local user by email. Removing it entirely would skip that lookup and
create a duplicate user for the emailfallback-only config.

Replace the empty-string sentinel with an explicit []*user.User candidate list
built in a small helper, keeping the same behavior, and assert no duplicate is
created in the email-only fallback test.
This commit is contained in:
kolaente 2026-06-19 19:38:10 +02:00
parent 0a407e5656
commit 783c0dd52a
2 changed files with 42 additions and 32 deletions

View File

@ -367,6 +367,39 @@ 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 {
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).
if len(searches) == 0 {
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
@ -393,38 +426,7 @@ func getOrCreateUser(s *xorm.Session, cl *claims, provider *Provider, idToken *o
if !alreadyCreatedFromIssuer && (provider.EmailFallback || provider.UsernameFallback) {
// try finding the user on fallback mapping properties
// 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
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 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, "")
}
for _, username := range usernameCandidates {
searchUser := &user.User{
Issuer: user.IssuerLocal,
Username: username,
Email: fallbackEmail,
}
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

View File

@ -306,6 +306,9 @@ func TestGetOrCreateUser(t *testing.T) {
s := db.NewSession()
defer s.Close()
usersBefore, err := s.Count(&user.User{})
require.NoError(t, err)
cl := &claims{
Email: "user11@example.com",
}
@ -319,6 +322,11 @@ 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: Match to existing local user on username and email", func(t *testing.T) {