kodev-org
/
vikunja
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(auth): link OIDC username fallback on preferred_username, not just sub (#2945)

This commit is contained in:
Tink 2026-06-19 20:47:05 +02:00 committed by GitHub
parent b6af132845
commit 81791fd346
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 140 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

62
pkg/modules/auth/openid/openid.go
View File

@ -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
@ -403,23 +443,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
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
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
@ -430,6 +454,10 @@ func getOrCreateUser(s *xorm.Session, cl *claims, provider *Provider, idToken *o
if fallbackMatchFound && user.IsErrUserStatusError(err) {
return u, nil
}
if fallbackMatchFound {
break
}
}
}
if !alreadyCreatedFromIssuer && !fallbackMatchFound {

86
pkg/modules/auth/openid/openid_test.go
View File

@ -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) {