Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 72 additions & 17 deletions api/oidc.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"fmt"
"io"
"net/http"
"net/url"
"strings"
"time"

"github.com/gin-gonic/gin"
Expand Down Expand Up @@ -56,6 +58,7 @@ func NewOIDC(conf *config.Configuration, db *database.GormDatabase, userChangeNo
PasswordStrength: conf.PassStrength,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no way for an admin to create user for a specific OIDC identity without turning on link by username in general?

I think an alternative implementation that covers more use cases is always make a user for an OIDC identity but make it locked by default - then give admin users a toggle to approve and unlock these users. This is basically what Gitea/Forgejo does with public idP logins.

Is your usecase for that the admin doesn't want to enable autoregister, and manually whitelists users by either creating them manually or via unlocking?

For me this seems more of an addition than an alternative. The linking is the guard that an oidc user can take over the local account, so this has to be kept regardless, as if you have an local admin user, you probably don't want that a oidc user can take over that account.

I'd see this as extra toggle for autoregister. eg with values false, true, locked. This could be supported also for the normal registration.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this seems more of an addition than an alternative. The linking is the guard that an oidc user can take over the local account, so this has to be kept regardless,

That's correct, when I said "alternative" I didn't mean alternative for the whole PR I meant alternative for the quite complicated (and IMO rigid) user linking logic.

I was trying to say while this is a feature not really a bug, this seems like a must have feature for this to work in a real world capacity. In the previous implementation onboarding can simply be done by creating a user with the same username, but now there is no way to actually onboard an OIDC user without turning on auto register in general.

So in practice I am proposing instead of failing outright when autoregister is not opened, register it anyways but lock it by default and say need admin approval.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, you mean the general logic of this, but it's not specific to the change in this PR (as this PR doesn't really removes functionality, it just changes to a safer default).

To summarize, you're use-case is using a public IdP like GitHub where you don't want to enable registrations, and you don't want to link by username.

Can we track this as a separate issue for "User Locking for OIDC/local registration". Are you having this use-case? otherwise I'd probably wait for user feedback on this. I'd say it's very likely when you are self-hosting gotify, that you're also selfhosting the IdP.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I don't have use case for this personally but I know this is a standard feature on Gitea/Forgejo and a lot of systems in mixed organizations (like universities) are set up this way to whitelist only authorized personnel to certain applications.

SecureCookie: conf.Server.SecureCookie,
AutoRegister: conf.OIDC.AutoRegister,
LinkByUsername: conf.OIDC.LinkByUsername,
pendingSessions: decaymap.NewDecayMap[string, *pendingOIDCSession](time.Now(), pendingSessionMaxAge),
}
}
Expand Down Expand Up @@ -83,6 +86,7 @@ type OIDCAPI struct {
PasswordStrength int
SecureCookie bool
AutoRegister bool
LinkByUsername bool
pendingSessions *decaymap.DecayMap[string, *pendingOIDCSession]
}

Expand Down Expand Up @@ -196,7 +200,7 @@ func (a *OIDCAPI) ElevateHandler(ctx *gin.Context) {
// $ref: "#/definitions/Error"
func (a *OIDCAPI) CallbackHandler() gin.HandlerFunc {
callback := func(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, provider rp.RelyingParty, info *oidc.UserInfo) {
user, status, err := a.resolveUser(info)
user, status, err := a.resolveUser(tokens.IDTokenClaims.GetIssuer(), info)
if err != nil {
http.Error(w, err.Error(), status)
return
Expand Down Expand Up @@ -362,7 +366,7 @@ func (a *OIDCAPI) ExternalTokenHandler(ctx *gin.Context) {
ctx.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to get user info: %w", err))
return
}
user, status, resolveErr := a.resolveUser(info)
user, status, resolveErr := a.resolveUser(tokens.IDTokenClaims.GetIssuer(), info)
if resolveErr != nil {
ctx.AbortWithError(status, resolveErr)
return
Expand All @@ -386,32 +390,83 @@ func (a *OIDCAPI) generateState() (string, error) {
return hex.EncodeToString(nonce), nil
}

// resolveUser looks up or creates a user from OIDC userinfo claims.
func (a *OIDCAPI) resolveUser(info *oidc.UserInfo) (*model.User, int, error) {
// resolveUser looks up, links, or creates the user bound to an OIDC identity.
//
// 1. Look up the user by OIDC id (<iss>#<sub>). If found, use it.
// 2. Otherwise look up a user by the username claim. If one exists, link it to
// this OIDC identity, which requires GOTIFY_OIDC_LINK_BY_USERNAME and
// that the user is not already bound to a different identity.
// 3. Otherwise auto-register a new user, which requires GOTIFY_OIDC_AUTOREGISTER.
func (a *OIDCAPI) resolveUser(issuer string, info *oidc.UserInfo) (*model.User, int, error) {
if issuer == "" {
return nil, http.StatusInternalServerError, errors.New("issuer claim was empty")
}
if _, err := url.Parse(issuer); err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("issuer url %q is not a valid url: %w", issuer, err)
}
if strings.Contains(issuer, "#") {
return nil, http.StatusInternalServerError, fmt.Errorf("issuer url %q may not contain a fragment", issuer)
}
subject := info.GetSubject()
if subject == "" {
return nil, http.StatusInternalServerError, errors.New("subject claim was empty")
}
oidcID := issuer + "#" + subject

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be safe on a compliant IdP but it's nice to check issuer is actually a valid URL without a fragment.


user, err := a.DB.GetUserByOIDC(oidcID)
if err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("database error: %w", err)
}
if user != nil {
return user, 0, nil
}

usernameRaw, ok := info.Claims[a.UsernameClaim]
if !ok {
return nil, http.StatusInternalServerError, fmt.Errorf("username claim %q is missing", a.UsernameClaim)
}
username := fmt.Sprint(usernameRaw)
if username == "" || usernameRaw == nil {
return nil, http.StatusInternalServerError, fmt.Errorf("username claim was empty")
return nil, http.StatusInternalServerError, errors.New("username claim was empty")
}

user, err := a.DB.GetUserByName(username)
byUsername, err := a.DB.GetUserByName(username)
if err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("database error: %w", err)
}
if user == nil {
if !a.AutoRegister {
return nil, http.StatusForbidden, fmt.Errorf("user does not exist and auto-registration is disabled")
}
user = &model.User{Name: username, Admin: false, Pass: nil}
if err := a.DB.CreateUser(user); err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("failed to create user: %w", err)
}
if err := a.UserChangeNotifier.fireUserAdded(user.ID); err != nil {
log.Error().Err(err).Uint("user_id", user.ID).Msg("Could not notify user change")
}
if byUsername != nil {
return a.linkExistingUser(byUsername, oidcID)
}
return a.registerUser(username, oidcID)
}

func (a *OIDCAPI) linkExistingUser(user *model.User, oidcID string) (*model.User, int, error) {
if !a.LinkByUsername {
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)
return nil, http.StatusForbidden, fmt.Errorf("a local user with the username %s already exists and linking by username is disabled", user.Name)
}
if user.OIDCID != nil {
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")
return nil, http.StatusForbidden, fmt.Errorf("the user %s is already bound to a different OIDC identity", user.Name)
}
user.OIDCID = &oidcID
if err := a.DB.UpdateUser(user); err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("failed to bind user to OIDC identity: %w", err)
}
return user, 0, nil
}

func (a *OIDCAPI) registerUser(username, oidcID string) (*model.User, int, error) {
if !a.AutoRegister {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make GOTIFY_REGISTRATION imply this? I can't think of a reason why you want to disable auto register on OIDC but allow general registration.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A usecase could be if you have linking by username enabled, and don't want oidc users to take over an account, but still allow registrations with local accounts.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can combine this in one setting GOTIFY_REGISTRATION=false|oidc|local|true?

@eternal-flame-AD eternal-flame-AD Jun 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A usecase could be if you have linking by username enabled, and don't want oidc users to take over an account, but still allow registrations with local accounts.

Isn't this problem already solved by this PR? you can't take over accounts by crossing over registration methods.

maybe we can combine this in one setting GOTIFY_REGISTRATION=false|oidc|local|true

Yeah I would say merge it into one, or call it GOTIFY_REGISTRATION_METHOD=list. The current system is not bad I just prefer config that don't relate to each other despite being in completely different places.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this problem already solved by this PR? you can't take over accounts by crossing over registration methods.

Nevermind, misunderstood.


The list is a good idea so GOTIFY_REGISTRATION=oidc,local.

Can we make GOTIFY_REGISTRATION imply this? I can't think of a reason why you want to disable auto register on OIDC but allow general registration.

But we want it the other way around, disable general registration, and allow OIDC registration. I think it's valid to allow to configure them separately.

With the current approach, I like that we have a namespace for all oidc related settings. I'd say "showing a registration form" and "automatically creating a user on oidc login" is somewhat different

Now the oidc setting is called autoregister which makes it IMO a little more expressive. E.g.

GOTIFY_OIDC_ENABLED=true
GOTIFY_OIDC_ISSUER=https://auth.example.org
GOTIFY_OIDC_REDIRECTURL=https://gotify.example.org/auth/oidc/callback

GOTIFY_OIDC_AUTOREGISTER=true
# vs
GOTIFY_REGISTRATION=oidc

Maybe we could rename GOTIFY_OIDC_AUTOREGISTER to GOTIFY_OIDC_CREATE_USER to be more explicit.

return nil, http.StatusForbidden, errors.New("user does not exist and auto-registration is disabled")
}
user := &model.User{Name: username, Admin: false, Pass: nil, OIDCID: &oidcID}
if err := a.DB.CreateUser(user); err != nil {
return nil, http.StatusInternalServerError, fmt.Errorf("failed to create user: %w", err)
}
log.Info().Str("oidc_id", oidcID).Str("username", user.Name).Msg("OIDC auto registration")
if err := a.UserChangeNotifier.fireUserAdded(user.ID); err != nil {
log.Error().Err(err).Uint("user_id", user.ID).Msg("Could not notify user change")
}
return user, 0, nil
}
Expand Down
145 changes: 120 additions & 25 deletions api/oidc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/gotify/server/v2/auth"
"github.com/gotify/server/v2/decaymap"
"github.com/gotify/server/v2/mode"
"github.com/gotify/server/v2/model"
"github.com/gotify/server/v2/test"
"github.com/gotify/server/v2/test/testdb"
"github.com/stretchr/testify/assert"
Expand All @@ -19,6 +20,8 @@ import (

var origGenClientToken = generateClientToken

const testIssuer = "https://idp.example.com"

func TestOIDCSuite(t *testing.T) {
suite.Run(t, new(OIDCSuite))
}
Expand Down Expand Up @@ -62,83 +65,172 @@ func (s *OIDCSuite) Test_GenerateState_Unique() {
assert.NotEqual(s.T(), s1, s2)
}

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

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

assert.NoError(s.T(), err)
assert.Equal(s.T(), 0, status)
assert.Equal(s.T(), uint(1), user.ID)
assert.Equal(s.T(), "alice", user.Name)
assert.False(s.T(), s.notified)
}

func (s *OIDCSuite) Test_ResolveUser_LinkByUsername_BindsExistingUser() {
s.a.LinkByUsername = true
s.db.NewUserWithName(1, "alice")

info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
user, _, err := s.a.resolveUser(testIssuer, info)

assert.NoError(s.T(), err)
assert.Equal(s.T(), uint(1), user.ID)
assert.NotNil(s.T(), user.OIDCID)
assert.Equal(s.T(), testIssuer+"#sub-1", *user.OIDCID)
// Binding an existing user is not a registration, so no notification.
assert.False(s.T(), s.notified)

bound, err := s.db.GetUserByOIDC(testIssuer + "#sub-1")
assert.NoError(s.T(), err)
assert.NotNil(s.T(), bound)
assert.Equal(s.T(), uint(1), bound.ID)
}

func (s *OIDCSuite) Test_ResolveUser_InvalidIssuer() {
s.db.NewUserWithName(1, "alice")

info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
_, status, err := s.a.resolveUser("://example.org", info)

assert.EqualError(s.T(), err, `issuer url "://example.org" is not a valid url: parse "://example.org": missing protocol scheme`)
assert.Equal(s.T(), 500, status)
}

func (s *OIDCSuite) Test_ResolveUser_InvalidIssuer_containsFragment() {
s.db.NewUserWithName(1, "alice")

info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
_, status, err := s.a.resolveUser(testIssuer+"#", info)

assert.EqualError(s.T(), err, `issuer url "https://idp.example.com#" may not contain a fragment`)
assert.Equal(s.T(), 500, status)
}

func (s *OIDCSuite) Test_ResolveUser_LinkDisabled_RejectsExistingUsername() {
s.db.NewUserWithName(1, "alice")

info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
_, status, err := s.a.resolveUser(testIssuer, info)

assert.EqualError(s.T(), err, "a local user with the username alice already exists and linking by username is disabled")
assert.Equal(s.T(), 403, status)

// The existing user must not have been bound.
user, _ := s.db.GetUserByName("alice")
assert.Nil(s.T(), user.OIDCID)
}

func (s *OIDCSuite) Test_ResolveUser_LinkByUsername_RejectsDifferentIdentity() {
s.a.LinkByUsername = true
otherID := testIssuer + "#other-sub"
s.db.CreateUser(&model.User{ID: 1, Name: "alice", OIDCID: &otherID})

info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "alice"}}
_, status, err := s.a.resolveUser(testIssuer, info)

assert.EqualError(s.T(), err, "the user alice is already bound to a different OIDC identity")
assert.Equal(s.T(), 403, status)
}

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

assert.NoError(s.T(), err)
assert.Equal(s.T(), 0, status)
assert.Equal(s.T(), "newuser", user.Name)
assert.False(s.T(), user.Admin)
assert.NotNil(s.T(), user.OIDCID)
assert.Equal(s.T(), testIssuer+"#sub-1", *user.OIDCID)
assert.True(s.T(), s.notified)

// verify persisted
dbUser, err := s.db.GetUserByName("newuser")
// Verify persisted and bound.
dbUser, err := s.db.GetUserByOIDC(testIssuer + "#sub-1")
assert.NoError(s.T(), err)
assert.NotNil(s.T(), dbUser)
assert.Equal(s.T(), "newuser", dbUser.Name)
}

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

_, status, err := s.a.resolveUser(info)
_, status, err := s.a.resolveUser(testIssuer, info)

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

func (s *OIDCSuite) Test_ResolveUser_MissingIssuer() {
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{"preferred_username": "newuser"}}

_, status, err := s.a.resolveUser("", info)

assert.EqualError(s.T(), err, "issuer claim was empty")
assert.Equal(s.T(), 500, status)
}

func (s *OIDCSuite) Test_ResolveUser_MissingSubject() {
info := &oidc.UserInfo{Claims: map[string]any{"preferred_username": "newuser"}}

_, status, err := s.a.resolveUser(testIssuer, info)

assert.EqualError(s.T(), err, "subject claim was empty")
assert.Equal(s.T(), 500, status)
}

func (s *OIDCSuite) Test_ResolveUser_MissingClaim() {
info := &oidc.UserInfo{Claims: map[string]any{}}
info := &oidc.UserInfo{Subject: "sub-1", Claims: map[string]any{}}

_, status, err := s.a.resolveUser(info)
_, status, err := s.a.resolveUser(testIssuer, info)

assert.Error(s.T(), err)
assert.EqualError(s.T(), err, `username claim "preferred_username" is missing`)
assert.Equal(s.T(), 500, status)
assert.Contains(s.T(), err.Error(), "preferred_username")
}

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

_, status, err := s.a.resolveUser(info)
_, status, err := s.a.resolveUser(testIssuer, info)

assert.Error(s.T(), err)
assert.EqualError(s.T(), err, "username claim was empty")
assert.Equal(s.T(), 500, status)
}

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

_, status, err := s.a.resolveUser(info)
_, status, err := s.a.resolveUser(testIssuer, info)

assert.Error(s.T(), err)
assert.EqualError(s.T(), err, "username claim was empty")
assert.Equal(s.T(), 500, status)
}

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

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

assert.NoError(s.T(), err)
assert.Equal(s.T(), "alice@example.com", user.Name)
assert.Equal(s.T(), 0, status)
assert.Equal(s.T(), "new@example.com", user.Name)
assert.NotNil(s.T(), user.OIDCID)
}

// --- createClient ---
Expand Down Expand Up @@ -170,6 +262,7 @@ func (s *OIDCSuite) Test_ExternalAuthorizeHandler_MissingFields() {
s.a.ExternalAuthorizeHandler(s.ctx)

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

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

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

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

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