Skip to content

Commit fe3e366

Browse files
jmattheisjmattheis
authored
Merge pull request #977 from gotify/oidc-link-user
OIDC link user by iss/sub
2 parents 3db7dbc + 6229f36 commit fe3e366

11 files changed

Lines changed: 504 additions & 53 deletions

File tree

‎api/oidc.go‎

Lines changed: 72 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"fmt"
99
"io"
1010
"net/http"
11+
"net/url"
12+
"strings"
1113
"time"
1214

1315
"github.com/gin-gonic/gin"
@@ -56,6 +58,7 @@ func NewOIDC(conf *config.Configuration, db *database.GormDatabase, userChangeNo
5658
PasswordStrength: conf.PassStrength,
5759
SecureCookie: conf.Server.SecureCookie,
5860
AutoRegister: conf.OIDC.AutoRegister,
61+
LinkByUsername: conf.OIDC.LinkByUsername,
5962
pendingSessions: decaymap.NewDecayMap[string, *pendingOIDCSession](time.Now(), pendingSessionMaxAge),
6063
}
6164
}
@@ -83,6 +86,7 @@ type OIDCAPI struct {
8386
PasswordStrength int
8487
SecureCookie bool
8588
AutoRegister bool
89+
LinkByUsername bool
8690
pendingSessions *decaymap.DecayMap[string, *pendingOIDCSession]
8791
}
8892

@@ -196,7 +200,7 @@ func (a *OIDCAPI) ElevateHandler(ctx *gin.Context) {
196200
// $ref: "#/definitions/Error"
197201
func (a *OIDCAPI) CallbackHandler() gin.HandlerFunc {
198202
callback := func(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, provider rp.RelyingParty, info *oidc.UserInfo) {
199-
user, status, err := a.resolveUser(info)
203+
user, status, err := a.resolveUser(tokens.IDTokenClaims.GetIssuer(), info)
200204
if err != nil {
201205
http.Error(w, err.Error(), status)
202206
return
@@ -362,7 +366,7 @@ func (a *OIDCAPI) ExternalTokenHandler(ctx *gin.Context) {
362366
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to get user info: %w", err))
363367
return
364368
}
365-
user, status, resolveErr := a.resolveUser(info)
369+
user, status, resolveErr := a.resolveUser(tokens.IDTokenClaims.GetIssuer(), info)
366370
if resolveErr != nil {
367371
ctx.AbortWithError(status, resolveErr)
368372
return
@@ -386,32 +390,83 @@ func (a *OIDCAPI) generateState() (string, error) {
386390
return hex.EncodeToString(nonce), nil
387391
}
388392

389-
// resolveUser looks up or creates a user from OIDC userinfo claims.
390-
func (a *OIDCAPI) resolveUser(info *oidc.UserInfo) (*model.User, int, error) {
393+
// resolveUser looks up, links, or creates the user bound to an OIDC identity.
394+
//
395+
// 1. Look up the user by OIDC id (<iss>#<sub>). If found, use it.
396+
// 2. Otherwise look up a user by the username claim. If one exists, link it to
397+
// this OIDC identity, which requires GOTIFY_OIDC_LINK_BY_USERNAME and
398+
// that the user is not already bound to a different identity.
399+
// 3. Otherwise auto-register a new user, which requires GOTIFY_OIDC_AUTOREGISTER.
400+
func (a *OIDCAPI) resolveUser(issuer string, info *oidc.UserInfo) (*model.User, int, error) {
401+
if issuer == "" {
402+
return nil, http.StatusInternalServerError, errors.New("issuer claim was empty")
403+
}
404+
if _, err := url.Parse(issuer); err != nil {
405+
return nil, http.StatusInternalServerError, fmt.Errorf("issuer url %q is not a valid url: %w", issuer, err)
406+
}
407+
if strings.Contains(issuer, "#") {
408+
return nil, http.StatusInternalServerError, fmt.Errorf("issuer url %q may not contain a fragment", issuer)
409+
}
410+
subject := info.GetSubject()
411+
if subject == "" {
412+
return nil, http.StatusInternalServerError, errors.New("subject claim was empty")
413+
}
414+
oidcID := issuer + "#" + subject
415+
416+
user, err := a.DB.GetUserByOIDC(oidcID)
417+
if err != nil {
418+
return nil, http.StatusInternalServerError, fmt.Errorf("database error: %w", err)
419+
}
420+
if user != nil {
421+
return user, 0, nil
422+
}
423+
391424
usernameRaw, ok := info.Claims[a.UsernameClaim]
392425
if !ok {
393426
return nil, http.StatusInternalServerError, fmt.Errorf("username claim %q is missing", a.UsernameClaim)
394427
}
395428
username := fmt.Sprint(usernameRaw)
396429
if username == "" || usernameRaw == nil {
397-
return nil, http.StatusInternalServerError, fmt.Errorf("username claim was empty")
430+
return nil, http.StatusInternalServerError, errors.New("username claim was empty")
398431
}
399432

400-
user, err := a.DB.GetUserByName(username)
433+
byUsername, err := a.DB.GetUserByName(username)
401434
if err != nil {
402435
return nil, http.StatusInternalServerError, fmt.Errorf("database error: %w", err)
403436
}
404-
if user == nil {
405-
if !a.AutoRegister {
406-
return nil, http.StatusForbidden, fmt.Errorf("user does not exist and auto-registration is disabled")
407-
}
408-
user = &model.User{Name: username, Admin: false, Pass: nil}
409-
if err := a.DB.CreateUser(user); err != nil {
410-
return nil, http.StatusInternalServerError, fmt.Errorf("failed to create user: %w", err)
411-
}
412-
if err := a.UserChangeNotifier.fireUserAdded(user.ID); err != nil {
413-
log.Error().Err(err).Uint("user_id", user.ID).Msg("Could not notify user change")
414-
}
437+
if byUsername != nil {
438+
return a.linkExistingUser(byUsername, oidcID)
439+
}
440+
return a.registerUser(username, oidcID)
441+
}
442+
443+
func (a *OIDCAPI) linkExistingUser(user *model.User, oidcID string) (*model.User, int, error) {
444+
if !a.LinkByUsername {
445+
log.Warn().Str("oidc_id", oidcID).Str("username", user.Name).Msgf("OIDC login rejected: a local user with the username already exists and %s is disabled", config.EnvOIDCLinkByUsername)
446+
return nil, http.StatusForbidden, fmt.Errorf("a local user with the username %s already exists and linking by username is disabled", user.Name)
447+
}
448+
if user.OIDCID != nil {
449+
log.Warn().Str("oidc_id", oidcID).Str("bound_oidc_id", *user.OIDCID).Str("username", user.Name).Msg("OIDC login rejected: the username is already bound to a different OIDC identity")
450+
return nil, http.StatusForbidden, fmt.Errorf("the user %s is already bound to a different OIDC identity", user.Name)
451+
}
452+
user.OIDCID = &oidcID
453+
if err := a.DB.UpdateUser(user); err != nil {
454+
return nil, http.StatusInternalServerError, fmt.Errorf("failed to bind user to OIDC identity: %w", err)
455+
}
456+
return user, 0, nil
457+
}
458+
459+
func (a *OIDCAPI) registerUser(username, oidcID string) (*model.User, int, error) {
460+
if !a.AutoRegister {
461+
return nil, http.StatusForbidden, errors.New("user does not exist and auto-registration is disabled")
462+
}
463+
user := &model.User{Name: username, Admin: false, Pass: nil, OIDCID: &oidcID}
464+
if err := a.DB.CreateUser(user); err != nil {
465+
return nil, http.StatusInternalServerError, fmt.Errorf("failed to create user: %w", err)
466+
}
467+
log.Info().Str("oidc_id", oidcID).Str("username", user.Name).Msg("OIDC auto registration")
468+
if err := a.UserChangeNotifier.fireUserAdded(user.ID); err != nil {
469+
log.Error().Err(err).Uint("user_id", user.ID).Msg("Could not notify user change")
415470
}
416471
return user, 0, nil
417472
}

‎api/oidc_test.go‎

Lines changed: 120 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/gotify/server/v2/auth"
1111
"github.com/gotify/server/v2/decaymap"
1212
"github.com/gotify/server/v2/mode"
13+
"github.com/gotify/server/v2/model"
1314
"github.com/gotify/server/v2/test"
1415
"github.com/gotify/server/v2/test/testdb"
1516
"github.com/stretchr/testify/assert"
@@ -19,6 +20,8 @@ import (
1920

2021
var origGenClientToken = generateClientToken
2122

23+
const testIssuer = "https://idp.example.com"
24+
2225
func TestOIDCSuite(t *testing.T) {
2326
suite.Run(t, new(OIDCSuite))
2427
}
@@ -62,83 +65,172 @@ func (s *OIDCSuite) Test_GenerateState_Unique() {
6265
assert.NotEqual(s.T(), s1, s2)
6366
}
6467

65-
func (s *OIDCSuite) Test_ResolveUser_ExistingUser() {
66-
s.db.NewUserWithName(1, "alice")
68+
func (s *OIDCSuite) Test_ResolveUser_ReturningUser_MatchedByOIDCID() {
69+
oidcID := testIssuer + "#sub-1"
70+
s.db.CreateUser(&model.User{ID: 1, Name: "alice", OIDCID: &oidcID})
6771

68-
info := &oidc.UserInfo{Claims: map[string]any{"preferred_username": "alice"}}
69-
user, status, err := s.a.resolveUser(info)
72+
// The username claim differs from the stored name; the binding still matches.
73+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "renamed"}}
74+
user, status, err := s.a.resolveUser(testIssuer, info)
7075

7176
assert.NoError(s.T(), err)
7277
assert.Equal(s.T(), 0, status)
78+
assert.Equal(s.T(), uint(1), user.ID)
7379
assert.Equal(s.T(), "alice", user.Name)
80+
assert.False(s.T(), s.notified)
81+
}
82+
83+
func (s *OIDCSuite) Test_ResolveUser_LinkByUsername_BindsExistingUser() {
84+
s.a.LinkByUsername = true
85+
s.db.NewUserWithName(1, "alice")
86+
87+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
88+
user, _, err := s.a.resolveUser(testIssuer, info)
89+
90+
assert.NoError(s.T(), err)
7491
assert.Equal(s.T(), uint(1), user.ID)
92+
assert.NotNil(s.T(), user.OIDCID)
93+
assert.Equal(s.T(), testIssuer+"#sub-1", *user.OIDCID)
94+
// Binding an existing user is not a registration, so no notification.
7595
assert.False(s.T(), s.notified)
96+
97+
bound, err := s.db.GetUserByOIDC(testIssuer + "#sub-1")
98+
assert.NoError(s.T(), err)
99+
assert.NotNil(s.T(), bound)
100+
assert.Equal(s.T(), uint(1), bound.ID)
101+
}
102+
103+
func (s *OIDCSuite) Test_ResolveUser_InvalidIssuer() {
104+
s.db.NewUserWithName(1, "alice")
105+
106+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
107+
_, status, err := s.a.resolveUser("://example.org", info)
108+
109+
assert.EqualError(s.T(), err, `issuer url "://example.org" is not a valid url: parse "://example.org": missing protocol scheme`)
110+
assert.Equal(s.T(), 500, status)
111+
}
112+
113+
func (s *OIDCSuite) Test_ResolveUser_InvalidIssuer_containsFragment() {
114+
s.db.NewUserWithName(1, "alice")
115+
116+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
117+
_, status, err := s.a.resolveUser(testIssuer+"#", info)
118+
119+
assert.EqualError(s.T(), err, `issuer url "https://idp.example.com#" may not contain a fragment`)
120+
assert.Equal(s.T(), 500, status)
121+
}
122+
123+
func (s *OIDCSuite) Test_ResolveUser_LinkDisabled_RejectsExistingUsername() {
124+
s.db.NewUserWithName(1, "alice")
125+
126+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
127+
_, status, err := s.a.resolveUser(testIssuer, info)
128+
129+
assert.EqualError(s.T(), err, "a local user with the username alice already exists and linking by username is disabled")
130+
assert.Equal(s.T(), 403, status)
131+
132+
// The existing user must not have been bound.
133+
user, _ := s.db.GetUserByName("alice")
134+
assert.Nil(s.T(), user.OIDCID)
135+
}
136+
137+
func (s *OIDCSuite) Test_ResolveUser_LinkByUsername_RejectsDifferentIdentity() {
138+
s.a.LinkByUsername = true
139+
otherID := testIssuer + "#other-sub"
140+
s.db.CreateUser(&model.User{ID: 1, Name: "alice", OIDCID: &otherID})
141+
142+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
143+
_, status, err := s.a.resolveUser(testIssuer, info)
144+
145+
assert.EqualError(s.T(), err, "the user alice is already bound to a different OIDC identity")
146+
assert.Equal(s.T(), 403, status)
76147
}
77148

78149
func (s *OIDCSuite) Test_ResolveUser_AutoRegister() {
79-
info := &oidc.UserInfo{Claims: map[string]any{"preferred_username": "newuser"}}
80-
user, status, err := s.a.resolveUser(info)
150+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "newuser"}}
151+
user, status, err := s.a.resolveUser(testIssuer, info)
81152

82153
assert.NoError(s.T(), err)
83154
assert.Equal(s.T(), 0, status)
84155
assert.Equal(s.T(), "newuser", user.Name)
85156
assert.False(s.T(), user.Admin)
157+
assert.NotNil(s.T(), user.OIDCID)
158+
assert.Equal(s.T(), testIssuer+"#sub-1", *user.OIDCID)
86159
assert.True(s.T(), s.notified)
87160

88-
// verify persisted
89-
dbUser, err := s.db.GetUserByName("newuser")
161+
// Verify persisted and bound.
162+
dbUser, err := s.db.GetUserByOIDC(testIssuer + "#sub-1")
90163
assert.NoError(s.T(), err)
91164
assert.NotNil(s.T(), dbUser)
165+
assert.Equal(s.T(), "newuser", dbUser.Name)
92166
}
93167

94168
func (s *OIDCSuite) Test_ResolveUser_AutoRegisterDisabled() {
95169
s.a.AutoRegister = false
96-
info := &oidc.UserInfo{Claims: map[string]any{"preferred_username": "newuser"}}
170+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "newuser"}}
97171

98-
_, status, err := s.a.resolveUser(info)
172+
_, status, err := s.a.resolveUser(testIssuer, info)
99173

100-
assert.Error(s.T(), err)
174+
assert.EqualError(s.T(), err, "user does not exist and auto-registration is disabled")
101175
assert.Equal(s.T(), 403, status)
102176
s.db.AssertUsernameNotExist("newuser")
103177
}
104178

179+
func (s *OIDCSuite) Test_ResolveUser_MissingIssuer() {
180+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "newuser"}}
181+
182+
_, status, err := s.a.resolveUser("", info)
183+
184+
assert.EqualError(s.T(), err, "issuer claim was empty")
185+
assert.Equal(s.T(), 500, status)
186+
}
187+
188+
func (s *OIDCSuite) Test_ResolveUser_MissingSubject() {
189+
info := &oidc.UserInfo{Claims: map[string]any{"preferred_username": "newuser"}}
190+
191+
_, status, err := s.a.resolveUser(testIssuer, info)
192+
193+
assert.EqualError(s.T(), err, "subject claim was empty")
194+
assert.Equal(s.T(), 500, status)
195+
}
196+
105197
func (s *OIDCSuite) Test_ResolveUser_MissingClaim() {
106-
info := &oidc.UserInfo{Claims: map[string]any{}}
198+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{}}
107199

108-
_, status, err := s.a.resolveUser(info)
200+
_, status, err := s.a.resolveUser(testIssuer, info)
109201

110-
assert.Error(s.T(), err)
202+
assert.EqualError(s.T(), err, `username claim "preferred_username" is missing`)
111203
assert.Equal(s.T(), 500, status)
112-
assert.Contains(s.T(), err.Error(), "preferred_username")
113204
}
114205

115206
func (s *OIDCSuite) Test_ResolveUser_EmptyClaim() {
116-
info := &oidc.UserInfo{Claims: map[string]any{"preferred_username": ""}}
207+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": ""}}
117208

118-
_, status, err := s.a.resolveUser(info)
209+
_, status, err := s.a.resolveUser(testIssuer, info)
119210

120-
assert.Error(s.T(), err)
211+
assert.EqualError(s.T(), err, "username claim was empty")
121212
assert.Equal(s.T(), 500, status)
122213
}
123214

124215
func (s *OIDCSuite) Test_ResolveUser_NilClaim() {
125-
info := &oidc.UserInfo{Claims: map[string]any{"preferred_username": nil}}
216+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": nil}}
126217

127-
_, status, err := s.a.resolveUser(info)
218+
_, status, err := s.a.resolveUser(testIssuer, info)
128219

129-
assert.Error(s.T(), err)
220+
assert.EqualError(s.T(), err, "username claim was empty")
130221
assert.Equal(s.T(), 500, status)
131222
}
132223

133224
func (s *OIDCSuite) Test_ResolveUser_CustomClaim() {
134225
s.a.UsernameClaim = "email"
135-
s.db.NewUserWithName(1, "alice@example.com")
136226

137-
info := &oidc.UserInfo{Claims: map[string]any{"email": "alice@example.com"}}
138-
user, _, err := s.a.resolveUser(info)
227+
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"email": "new@example.com"}}
228+
user, status, err := s.a.resolveUser(testIssuer, info)
139229

140230
assert.NoError(s.T(), err)
141-
assert.Equal(s.T(), "alice@example.com", user.Name)
231+
assert.Equal(s.T(), 0, status)
232+
assert.Equal(s.T(), "new@example.com", user.Name)
233+
assert.NotNil(s.T(), user.OIDCID)
142234
}
143235

144236
// --- createClient ---
@@ -170,6 +262,7 @@ func (s *OIDCSuite) Test_ExternalAuthorizeHandler_MissingFields() {
170262
s.a.ExternalAuthorizeHandler(s.ctx)
171263

172264
assert.Equal(s.T(), 400, s.recorder.Code)
265+
assert.Contains(s.T(), s.ctx.Errors.Last().Error(), "'CodeChallenge' failed on the 'required' tag")
173266
}
174267

175268
// --- ExternalTokenHandler ---
@@ -181,6 +274,7 @@ func (s *OIDCSuite) Test_ExternalTokenHandler_InvalidJSON() {
181274
s.a.ExternalTokenHandler(s.ctx)
182275

183276
assert.Equal(s.T(), 400, s.recorder.Code)
277+
assert.Contains(s.T(), s.ctx.Errors.Last().Error(), "invalid character")
184278
}
185279

186280
func (s *OIDCSuite) Test_ExternalTokenHandler_UnknownState() {
@@ -192,4 +286,5 @@ func (s *OIDCSuite) Test_ExternalTokenHandler_UnknownState() {
192286
s.a.ExternalTokenHandler(s.ctx)
193287

194288
assert.Equal(s.T(), 400, s.recorder.Code)
289+
assert.EqualError(s.T(), s.ctx.Errors.Last(), "unknown or expired state")
195290
}

0 commit comments

Comments
 (0)