Merge 3dca98db92f649aa4e61af8ddf93afe4bb136f31 into ce089f498bce32305b2d9e8c6adfd8cb7c82f88f

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

@@ -5,10 +5,9 @@ package oauth2
 
 import (
 	"context"
+	"errors"
 	"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 +48,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" {
+		var retrieveErr *oauth2.RetrieveError
-			// this signals that the token is not valid and the user should be disabled
+		if !errors.As(err, &retrieveErr) || retrieveErr.ErrorCode != "invalid_grant" {
-			shouldDisable = true
-		} else {
 			return err
 		}
-	}
+		log.Info("SyncExternalUsers[%s] dropping invalid refresh token for user %d", source.AuthSource.Name, u.UserID)
 
-	user := &user_model.User{
+		// Refresh tokens can expire or be revoked independently from the
-		LoginName:   u.ExternalID,
+		// upstream account state. Keep the local user active and only clear
-		LoginType:   auth.OAuth2,
+		// the cached tokens until the next successful OAuth sign-in updates them.
-		LoginSource: u.LoginSourceID,
+		u.AccessToken = ""
-	}
+		u.RefreshToken = ""
+		u.ExpiresAt = time.Time{}
 
-	hasUser, err := user_model.GetIndividualUser(ctx, user)
+		return user_model.UpdateExternalUserByExternalID(ctx, u)
-	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.
-			u.AccessToken = ""
-			u.RefreshToken = ""
-			u.ExpiresAt = time.Time{}
-
-			return user_model.UpdateExternalUserByExternalID(ctx, u)
-		})
 	}
 
 	// 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)
 		})
 	})
 }