From 6cd3a5458e5d276147df6ada79f0f9e0c471b99d Mon Sep 17 00:00:00 2001 From: Misi Date: Mon, 16 Dec 2024 17:03:39 +0100 Subject: [PATCH] Auth: Return error when retries have been exhausted for OAuth token refresh (#98034) Return error when retries for DB lock have been exhausted in oauth_token.go --- .../authn/authnimpl/sync/oauth_token_sync.go | 5 ++ pkg/services/oauthtoken/oauth_token.go | 3 +- pkg/services/oauthtoken/oauth_token_test.go | 55 ++++++++++++++++++- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/pkg/services/authn/authnimpl/sync/oauth_token_sync.go b/pkg/services/authn/authnimpl/sync/oauth_token_sync.go index ec6f8e926b6..28fc69f1159 100644 --- a/pkg/services/authn/authnimpl/sync/oauth_token_sync.go +++ b/pkg/services/authn/authnimpl/sync/oauth_token_sync.go @@ -98,6 +98,11 @@ func (s *OAuthTokenSync) SyncOauthTokenHook(ctx context.Context, id *authn.Ident return nil, nil } + if errors.Is(refreshErr, oauthtoken.ErrRetriesExhausted) { + ctxLogger.Warn("Retries have been exhausted for locking the DB for OAuth token refresh", "id", id.ID, "error", refreshErr) + return nil, refreshErr + } + ctxLogger.Error("Failed to refresh OAuth access token", "id", id.ID, "error", refreshErr) // log the user out diff --git a/pkg/services/oauthtoken/oauth_token.go b/pkg/services/oauthtoken/oauth_token.go index cbb4cf67b97..fc9b64edc6f 100644 --- a/pkg/services/oauthtoken/oauth_token.go +++ b/pkg/services/oauthtoken/oauth_token.go @@ -35,6 +35,7 @@ var ( ErrNoRefreshTokenFound = errors.New("no refresh token found") ErrNotAnOAuthProvider = errors.New("not an oauth provider") ErrCouldntRefreshToken = errors.New("could not refresh token") + ErrRetriesExhausted = errors.New("retries exhausted") ) type Service struct { @@ -270,7 +271,7 @@ func (o *Service) TryTokenRefresh(ctx context.Context, usr identity.Requester, s if attempts < 5 { return nil } - return ErrCouldntRefreshToken + return ErrRetriesExhausted } var newToken *oauth2.Token diff --git a/pkg/services/oauthtoken/oauth_token_test.go b/pkg/services/oauthtoken/oauth_token_test.go index f20128526e2..36b5528a5c7 100644 --- a/pkg/services/oauthtoken/oauth_token_test.go +++ b/pkg/services/oauthtoken/oauth_token_test.go @@ -62,7 +62,11 @@ func (f *FakeAuthInfoStore) DeleteAuthInfo(ctx context.Context, cmd *login.Delet return f.ExpectedError } -func TestService_TryTokenRefresh(t *testing.T) { +func TestIntegration_TryTokenRefresh(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + unexpiredToken := &oauth2.Token{ AccessToken: "testaccess", RefreshToken: "testrefresh", @@ -86,6 +90,7 @@ func TestService_TryTokenRefresh(t *testing.T) { socialConnector *socialtest.MockSocialConnector socialService *socialtest.FakeSocialService + store db.DB service *Service } @@ -251,6 +256,30 @@ func TestService_TryTokenRefresh(t *testing.T) { }, expectedToken: unexpiredTokenWithIDToken, }, + { + desc: "should return ErrRetriesExhausted when lock cannot be acquired", + identity: &authn.Identity{ID: "1234", Type: claims.TypeUser, AuthenticatedBy: login.GenericOAuthModule}, + setup: func(env *environment) { + env.socialService.ExpectedAuthInfoProvider = &social.OAuthInfo{ + UseRefreshToken: true, + } + env.authInfoService.ExpectedUserAuth = &login.UserAuth{ + AuthModule: login.GenericOAuthModule, + AuthId: "subject", + UserId: 1234, + OAuthAccessToken: unexpiredTokenWithIDToken.AccessToken, + OAuthRefreshToken: unexpiredTokenWithIDToken.RefreshToken, + OAuthExpiry: unexpiredTokenWithIDToken.Expiry, + OAuthTokenType: unexpiredTokenWithIDToken.TokenType, + OAuthIdToken: EXPIRED_ID_TOKEN, + } + _ = env.store.WithDbSession(context.Background(), func(sess *db.Session) error { + _, err := sess.Exec(`INSERT INTO server_lock (operation_uid, last_execution, version) VALUES (?, ?, ?)`, "oauth-refresh-token-1234", time.Now().Add(2*time.Second).Unix(), 0) + return err + }) + }, + expectedErr: ErrRetriesExhausted, + }, } for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { @@ -265,6 +294,7 @@ func TestService_TryTokenRefresh(t *testing.T) { socialService: &socialtest.FakeSocialService{ ExpectedConnector: socialConnector, }, + store: store, } if tt.setup != nil { @@ -308,7 +338,11 @@ func TestService_TryTokenRefresh(t *testing.T) { } } -func TestService_TryTokenRefresh_WithExternalSessions(t *testing.T) { +func TestIntegration_TryTokenRefresh_WithExternalSessions(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + unexpiredToken := &oauth2.Token{ AccessToken: "testaccess", RefreshToken: "testrefresh", @@ -338,6 +372,7 @@ func TestService_TryTokenRefresh_WithExternalSessions(t *testing.T) { socialConnector *socialtest.MockSocialConnector socialService *socialtest.FakeSocialService + store db.DB service *Service } @@ -509,6 +544,21 @@ func TestService_TryTokenRefresh_WithExternalSessions(t *testing.T) { }, expectedToken: unexpiredTokenWithIDToken, }, + { + desc: "should return ErrRetriesExhausted when lock cannot be acquired", + identity: &authn.Identity{ID: "1234", Type: claims.TypeUser, AuthenticatedBy: login.GenericOAuthModule}, + setup: func(env *environment) { + env.socialService.ExpectedAuthInfoProvider = &social.OAuthInfo{ + UseRefreshToken: true, + } + + _ = env.store.WithDbSession(context.Background(), func(sess *db.Session) error { + _, err := sess.Exec(`INSERT INTO server_lock (operation_uid, last_execution, version) VALUES (?, ?, ?)`, "oauth-refresh-token-1234-1", time.Now().Add(2*time.Second).Unix(), 0) + return err + }) + }, + expectedErr: ErrRetriesExhausted, + }, } for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { @@ -523,6 +573,7 @@ func TestService_TryTokenRefresh_WithExternalSessions(t *testing.T) { socialService: &socialtest.FakeSocialService{ ExpectedConnector: socialConnector, }, + store: store, } if tt.setup != nil {