From 600ff0cebd10dd2049f898aedafd213b1498a1f1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 7 May 2026 12:45:35 -0700 Subject: [PATCH 1/2] Fix oauth2 sync to inactive user wrongly --- services/auth/source/oauth2/providers_test.go | 3 ++ services/auth/source/oauth2/source_sync.go | 49 +++---------------- .../auth/source/oauth2/source_sync_test.go | 16 +++++- 3 files changed, 26 insertions(+), 42 deletions(-) diff --git a/services/auth/source/oauth2/providers_test.go b/services/auth/source/oauth2/providers_test.go index 08c50b12a9..6b4922a785 100644 --- a/services/auth/source/oauth2/providers_test.go +++ b/services/auth/source/oauth2/providers_test.go @@ -4,6 +4,7 @@ package oauth2 import ( + "errors" "time" "github.com/markbates/goth" @@ -39,6 +40,8 @@ func (p *fakeProvider) RefreshToken(refreshToken string) (*oauth2.Token, error) return nil, &oauth2.RetrieveError{ ErrorCode: "invalid_grant", } + case "error": + return nil, errors.New("refresh failed") default: return &oauth2.Token{ AccessToken: "token", diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go index 445e281a06..6c14f99775 100644 --- a/services/auth/source/oauth2/source_sync.go +++ b/services/auth/source/oauth2/source_sync.go @@ -7,8 +7,6 @@ import ( "context" "time" - "code.gitea.io/gitea/models/auth" - "code.gitea.io/gitea/models/db" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/log" @@ -49,53 +47,22 @@ func (source *Source) Sync(ctx context.Context, updateExisting bool) error { func (source *Source) refresh(ctx context.Context, provider goth.Provider, u *user_model.ExternalLoginUser) error { log.Trace("Syncing login_source_id=%d external_id=%s expiration=%s", u.LoginSourceID, u.ExternalID, u.ExpiresAt) - shouldDisable := false - token, err := provider.RefreshToken(u.RefreshToken) if err != nil { - if err, ok := err.(*oauth2.RetrieveError); ok && err.ErrorCode == "invalid_grant" { - // this signals that the token is not valid and the user should be disabled - shouldDisable = true - } else { - return err - } - } + if retrieveErr, ok := err.(*oauth2.RetrieveError); ok && retrieveErr.ErrorCode == "invalid_grant" { + log.Info("SyncExternalUsers[%s] dropping invalid refresh token for external login %s", source.AuthSource.Name, u.ExternalID) - user := &user_model.User{ - LoginName: u.ExternalID, - LoginType: auth.OAuth2, - LoginSource: u.LoginSourceID, - } - - hasUser, err := user_model.GetIndividualUser(ctx, user) - if err != nil { - return err - } - - // If the grant is no longer valid, disable the user and - // delete local tokens. If the OAuth2 provider still - // recognizes them as a valid user, they will be able to login - // via their provider and reactivate their account. - if shouldDisable { - log.Info("SyncExternalUsers[%s] disabling user %d", source.AuthSource.Name, user.ID) - - return db.WithTx(ctx, func(ctx context.Context) error { - if hasUser { - user.IsActive = false - err := user_model.UpdateUserCols(ctx, user, "is_active") - if err != nil { - return err - } - } - - // Delete stored tokens, since they are invalid. This - // also provents us from checking this in subsequent runs. + // Refresh tokens can expire or be revoked independently from the + // upstream account state. Keep the local user active and only clear + // the cached tokens until the next successful OAuth sign-in updates them. u.AccessToken = "" u.RefreshToken = "" u.ExpiresAt = time.Time{} return user_model.UpdateExternalUserByExternalID(ctx, u) - }) + } else { + return err + } } // Otherwise, update the tokens diff --git a/services/auth/source/oauth2/source_sync_test.go b/services/auth/source/oauth2/source_sync_test.go index 2927f3634b..d1af1091db 100644 --- a/services/auth/source/oauth2/source_sync_test.go +++ b/services/auth/source/oauth2/source_sync_test.go @@ -95,7 +95,21 @@ func TestSource(t *testing.T) { u, err := user_model.GetUserByID(t.Context(), user.ID) assert.NoError(t, err) - assert.False(t, u.IsActive) + assert.True(t, u.IsActive) + }) + + t.Run("unexpected error", func(t *testing.T) { + err := source.refresh(t.Context(), provider, &user_model.ExternalLoginUser{ + ExternalID: "external", + UserID: user.ID, + LoginSourceID: user.LoginSource, + RefreshToken: "error", + }) + assert.ErrorContains(t, err, "refresh failed") + + u, err := user_model.GetUserByID(t.Context(), user.ID) + assert.NoError(t, err) + assert.True(t, u.IsActive) }) }) } From 3dca98db92f649aa4e61af8ddf93afe4bb136f31 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 7 May 2026 22:54:55 +0200 Subject: [PATCH 2/2] refactor(oauth2): apply review feedback to refresh error handling - use errors.As so wrapped *oauth2.RetrieveError still matches - early-return on non-invalid_grant errors instead of if/else - log UserID instead of ExternalID to avoid leaking provider-side identifiers (which can be username/email) into INFO logs Co-Authored-By: Claude (Opus 4.7) --- services/auth/source/oauth2/source_sync.go | 25 +++++++++++----------- 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/services/auth/source/oauth2/source_sync.go b/services/auth/source/oauth2/source_sync.go index 6c14f99775..171d1c9ea7 100644 --- a/services/auth/source/oauth2/source_sync.go +++ b/services/auth/source/oauth2/source_sync.go @@ -5,6 +5,7 @@ package oauth2 import ( "context" + "errors" "time" user_model "code.gitea.io/gitea/models/user" @@ -49,20 +50,20 @@ func (source *Source) refresh(ctx context.Context, provider goth.Provider, u *us token, err := provider.RefreshToken(u.RefreshToken) if err != nil { - if retrieveErr, ok := err.(*oauth2.RetrieveError); ok && retrieveErr.ErrorCode == "invalid_grant" { - log.Info("SyncExternalUsers[%s] dropping invalid refresh token for external login %s", source.AuthSource.Name, u.ExternalID) - - // Refresh tokens can expire or be revoked independently from the - // upstream account state. Keep the local user active and only clear - // the cached tokens until the next successful OAuth sign-in updates them. - u.AccessToken = "" - u.RefreshToken = "" - u.ExpiresAt = time.Time{} - - return user_model.UpdateExternalUserByExternalID(ctx, u) - } else { + var retrieveErr *oauth2.RetrieveError + if !errors.As(err, &retrieveErr) || retrieveErr.ErrorCode != "invalid_grant" { return err } + log.Info("SyncExternalUsers[%s] dropping invalid refresh token for user %d", source.AuthSource.Name, u.UserID) + + // Refresh tokens can expire or be revoked independently from the + // upstream account state. Keep the local user active and only clear + // the cached tokens until the next successful OAuth sign-in updates them. + u.AccessToken = "" + u.RefreshToken = "" + u.ExpiresAt = time.Time{} + + return user_model.UpdateExternalUserByExternalID(ctx, u) } // Otherwise, update the tokens