Fix oauth2 sync to inactive user wrongly

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",

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

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)
 		})
 	})
 }