diff --git a/models/user/external_login_user.go b/models/user/external_login_user.go index b91c385938..1069b0f283 100644 --- a/models/user/external_login_user.go +++ b/models/user/external_login_user.go @@ -80,8 +80,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 @@ -118,6 +121,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 671a245508..7bed3523ed 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 cbf5f9670a..888ca55e50 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 index dd018af6bb..bcd4981149 100644 --- a/tests/integration/auth_oauth2_test.go +++ b/tests/integration/auth_oauth2_test.go @@ -130,8 +130,73 @@ func TestMigrateAzureADV2ToOIDC(t *testing.T) { }) } +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) + }) +} + // newFakeOIDCServer starts an httptest.Server that implements the minimum OIDC endpoints needed to complete a sign-in flow: func newFakeOIDCServer(t *testing.T, sub, oid string) *httptest.Server { + return newFakeOIDCServerWithProfile(t, sub, oid, sub+"@example.com", "OIDC Test User") +} + +func newFakeOIDCServerWithProfile(t *testing.T, sub, oid, email, name string) *httptest.Server { t.Helper() var srv *httptest.Server @@ -169,8 +234,8 @@ func newFakeOIDCServer(t *testing.T, sub, oid string) *httptest.Server { // sub MUST match the id_token sub; goth rejects mismatches. _ = json.NewEncoder(w).Encode(map[string]any{ "sub": sub, - "email": sub + "@example.com", - "name": "OIDC Test User", + "email": email, + "name": name, }) default: http.NotFound(w, r)