From 6f5def0f372e345fa8d729f8dea3e7c46acb522e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 30 May 2026 13:37:09 -0700 Subject: [PATCH] fix(auth): ignore stale OIDC external login links to organizations (#37875) This fixes an OIDC sign-in edge case where a stale `external_login_user` record can still point to an organization or a deleted user. In that situation, Gitea may keep resolving the external login to the wrong account during sign-in. For affected instances, this matches the behavior reported in #36439 and #37812, where a user signing in with OIDC/Entra ID could appear as an organization, or hit a 404 after that organization was removed. - validate the user resolved from `external_login_user` during OAuth2/OIDC login - ignore stale links when the linked user no longer exists - ignore stale links when the linked user is not an individual user - remove the stale external login row so the sign-in flow can relink the external account to the correct user - Fixes #37812 - Related to #36439 --------- Co-authored-by: silverwind Co-authored-by: Claude (Opus 4.8) --- models/user/external_login_user.go | 13 +- routers/web/auth/oauth.go | 30 +++- .../auth/source/oauth2/source_sync_test.go | 14 +- tests/integration/auth_oauth2_test.go | 156 ++++++++++++++++++ 4 files changed, 192 insertions(+), 21 deletions(-) create mode 100644 tests/integration/auth_oauth2_test.go diff --git a/models/user/external_login_user.go b/models/user/external_login_user.go index 0e764efb9f..2de656961d 100644 --- a/models/user/external_login_user.go +++ b/models/user/external_login_user.go @@ -92,8 +92,11 @@ func init() { } // GetExternalLogin checks if a externalID in loginSourceID scope already exists -func GetExternalLogin(ctx context.Context, externalLoginUser *ExternalLoginUser) (bool, error) { - return db.GetEngine(ctx).Get(externalLoginUser) +func GetExternalLogin(ctx context.Context, loginSourceID int64, externalID string) (*ExternalLoginUser, bool, error) { + return db.Get[ExternalLoginUser](ctx, builder.Eq{ + "external_id": externalID, + "login_source_id": loginSourceID, + }) } // LinkExternalToUser link the external user to the user @@ -130,6 +133,12 @@ func RemoveAllAccountLinks(ctx context.Context, user *User) error { return err } +// RemoveExternalLoginByExternalID removes a specific external login link by its provider-side identifier. +func RemoveExternalLoginByExternalID(ctx context.Context, loginSourceID int64, externalID string) error { + _, err := db.GetEngine(ctx).Where("external_id=? AND login_source_id=?", externalID, loginSourceID).Delete(new(ExternalLoginUser)) + return err +} + // GetUserIDByExternalUserID get user id according to provider and userID func GetUserIDByExternalUserID(ctx context.Context, provider, userID string) (int64, error) { var id int64 diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 824de60313..b8d1d6ac94 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -514,17 +514,33 @@ func oAuth2UserLoginCallback(ctx *context.Context, authSource *auth.Source, requ } // search in external linked users - externalLoginUser := &user_model.ExternalLoginUser{ - ExternalID: gothUser.UserID, - LoginSourceID: authSource.ID, - } - hasUser, err = user_model.GetExternalLogin(request.Context(), externalLoginUser) + externalLoginUser, hasUser, err := user_model.GetExternalLogin(ctx, authSource.ID, gothUser.UserID) if err != nil { return nil, goth.User{}, err } if hasUser { - user, err = user_model.GetUserByID(request.Context(), externalLoginUser.UserID) - return user, gothUser, err + user, err = user_model.GetUserByID(ctx, externalLoginUser.UserID) + if err != nil && !user_model.IsErrUserNotExist(err) { + return nil, goth.User{}, err + } + if err == nil && user.IsIndividual() { + return user, gothUser, nil + } + + // The external login record is stale: the linked user no longer exists, or it exists but is + // not an individual user (only individual users can sign in, so a link pointing at an + // organization, bot or remote user can never resolve). Remove it so the next sign-in can + // relink the external account to the correct user. Nothing is lost, because the link is + // recreated automatically on the next sign-in. + reason := "linked user does not exist" + if err == nil { + reason = fmt.Sprintf("linked user type is %d", user.Type) + } + log.Warn("Ignoring stale external login link [external-id=%s login-source-id=%d user-id=%d]: %s", externalLoginUser.ExternalID, externalLoginUser.LoginSourceID, externalLoginUser.UserID, reason) + + if err := user_model.RemoveExternalLoginByExternalID(ctx, externalLoginUser.LoginSourceID, externalLoginUser.ExternalID); err != nil { + return nil, goth.User{}, err + } } // no user found to login diff --git a/services/auth/source/oauth2/source_sync_test.go b/services/auth/source/oauth2/source_sync_test.go index 2927f3634b..a4808fb22b 100644 --- a/services/auth/source/oauth2/source_sync_test.go +++ b/services/auth/source/oauth2/source_sync_test.go @@ -57,12 +57,7 @@ func TestSource(t *testing.T) { err := source.refresh(t.Context(), provider, e) assert.NoError(t, err) - e := &user_model.ExternalLoginUser{ - ExternalID: e.ExternalID, - LoginSourceID: e.LoginSourceID, - } - - ok, err := user_model.GetExternalLogin(t.Context(), e) + e, ok, err := user_model.GetExternalLogin(t.Context(), e.LoginSourceID, e.ExternalID) assert.NoError(t, err) assert.True(t, ok) assert.Equal(t, "refresh", e.RefreshToken) @@ -82,12 +77,7 @@ func TestSource(t *testing.T) { }) assert.NoError(t, err) - e := &user_model.ExternalLoginUser{ - ExternalID: e.ExternalID, - LoginSourceID: e.LoginSourceID, - } - - ok, err := user_model.GetExternalLogin(t.Context(), e) + e, ok, err := user_model.GetExternalLogin(t.Context(), e.LoginSourceID, e.ExternalID) assert.NoError(t, err) assert.True(t, ok) assert.Empty(t, e.RefreshToken) diff --git a/tests/integration/auth_oauth2_test.go b/tests/integration/auth_oauth2_test.go new file mode 100644 index 0000000000..3294e6d103 --- /dev/null +++ b/tests/integration/auth_oauth2_test.go @@ -0,0 +1,156 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package integration + +import ( + "encoding/base64" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + auth_model "code.gitea.io/gitea/models/auth" + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/json" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/services/auth/source/oauth2" + "code.gitea.io/gitea/tests" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestOIDCIgnoresStaleExternalLoginLinks(t *testing.T) { + defer tests.PrepareTestEnv(t)() + defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)() + defer test.MockVariableValue(&setting.OAuth2Client.AccountLinking, setting.OAuth2AccountLinkingAuto)() + defer test.MockVariableValue(&setting.OAuth2Client.Username, setting.OAuth2UsernameEmail)() + + setup := func(t *testing.T, sourceName, sub, userName, email string) (*auth_model.Source, *user_model.User) { + t.Helper() + srv := newFakeOIDCServerWithProfile(t, sub, sub+"-oid", email, "OIDC Test User") + addOAuth2Source(t, sourceName, oauth2.Source{ + Provider: "openidConnect", + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + OpenIDConnectAutoDiscoveryURL: srv.URL + "/.well-known/openid-configuration", + }) + authSource, err := auth_model.GetActiveOAuth2SourceByAuthName(t.Context(), sourceName) + require.NoError(t, err) + correctUser := &user_model.User{Name: userName, Email: email} + require.NoError(t, user_model.CreateUser(t.Context(), correctUser, &user_model.Meta{})) + return authSource, correctUser + } + + // assertRelinked signs in via OIDC and asserts the stale link now points at the correct individual user. + assertRelinked := func(t *testing.T, authSource *auth_model.Source, sub string, correctUser *user_model.User) { + t.Helper() + doOIDCSignIn(t, authSource.Name) + // external_login_user has no "id" column, so order by the primary key instead + externalLink := unittest.AssertExistsAndLoadBean(t, &user_model.ExternalLoginUser{ExternalID: sub, LoginSourceID: authSource.ID}, unittest.OrderBy("external_id ASC")) + assert.Equal(t, correctUser.ID, externalLink.UserID) + assert.Equal(t, correctUser.Email, externalLink.Email) + assert.Equal(t, "OIDC Test User", externalLink.Name) + } + + t.Run("organization", func(t *testing.T) { + const sub, userName, email = "oidc-stale-org-link-sub", "guizar_m", "guizar_m@example.com" + authSource, correctUser := setup(t, "test-oidc-stale-org-link", sub, userName, email) + org := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 3, Type: user_model.UserTypeOrganization}) + require.NoError(t, user_model.LinkExternalToUser(t.Context(), org, &user_model.ExternalLoginUser{ + ExternalID: sub, + UserID: org.ID, + LoginSourceID: authSource.ID, + Provider: authSource.Name, + })) + assertRelinked(t, authSource, sub, correctUser) + }) + + t.Run("deleted user", func(t *testing.T) { + const sub, userName, email = "oidc-stale-deleted-link-sub", "guizar_d", "guizar_d@example.com" + const deletedUserID = 999999 + authSource, correctUser := setup(t, "test-oidc-stale-deleted", sub, userName, email) + // link the external account to a user id that does not exist, simulating a deleted user + require.NoError(t, user_model.LinkExternalToUser(t.Context(), &user_model.User{ID: deletedUserID}, &user_model.ExternalLoginUser{ + ExternalID: sub, + UserID: deletedUserID, + LoginSourceID: authSource.ID, + Provider: authSource.Name, + })) + assertRelinked(t, authSource, sub, correctUser) + }) +} + +func newFakeOIDCServerWithProfile(t *testing.T, sub, oid, email, name string) *httptest.Server { + t.Helper() + + var srv *httptest.Server + srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/.well-known/openid-configuration": // discovery document + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": srv.URL, + "authorization_endpoint": srv.URL + "/authorize", + "token_endpoint": srv.URL + "/token", + "userinfo_endpoint": srv.URL + "/userinfo", + }) + case "/token": // returns an ID token with both "sub" and "oid" claims so tests can verify which one ends up as ExternalID + claims := map[string]any{ + "iss": srv.URL, + "aud": "test-client-id", + "exp": time.Now().Add(time.Hour).Unix(), + "sub": sub, + "oid": oid, + } + payload, _ := json.Marshal(claims) + header := base64.RawURLEncoding.EncodeToString([]byte(`{"alg":"none"}`)) + + // build a JWT-shaped string whose payload encodes claims. + // goth's decodeJWT only base64-decodes the payload without verifying the signature, so no real signing infrastructure is needed. + idToken := header + "." + base64.RawURLEncoding.EncodeToString(payload) + ".fakesig" + + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "fake-access-token", + "token_type": "Bearer", + "id_token": idToken, + }) + case "/userinfo": + // sub MUST match the id_token sub; goth rejects mismatches. + _ = json.NewEncoder(w).Encode(map[string]any{ + "sub": sub, + "email": email, + "name": name, + }) + default: + http.NotFound(w, r) + } + })) + t.Cleanup(srv.Close) + return srv +} + +// doOIDCSignIn runs a mock OIDC sign-in flow for the given auth source. +func doOIDCSignIn(t *testing.T, sourceName string) { + t.Helper() + session := emptyTestSession(t) + + // Step 1: initiate login + resp := session.MakeRequest(t, NewRequest(t, "GET", "/user/oauth2/"+sourceName), http.StatusTemporaryRedirect) + + // Step 2: extract the UUID state that Gitea embedded in the redirect URL. + location := resp.Header().Get("Location") + u, err := url.Parse(location) + require.NoError(t, err) + state := u.Query().Get("state") + require.NotEmpty(t, state, "redirect to OIDC provider must include state") + + // Step 3: simulate the provider redirecting back. + callbackURL := fmt.Sprintf("/user/oauth2/%s/callback?code=test-code&state=%s", sourceName, url.QueryEscape(state)) + session.MakeRequest(t, NewRequest(t, "GET", callbackURL), http.StatusSeeOther) +}