From 9e8f9cbb0749d8a65c5ca2039e6b0f58f153e2d3 Mon Sep 17 00:00:00 2001 From: Sai Sankeerth Date: Mon, 30 Dec 2024 14:53:21 +0530 Subject: [PATCH 1/2] fix: invalid error response handling during oauth refresh flow --- services/oauth/v2/common/constants.go | 9 +++-- services/oauth/v2/oauth.go | 31 +++++++++++----- services/oauth/v2/oauth_test.go | 53 +++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 14 deletions(-) diff --git a/services/oauth/v2/common/constants.go b/services/oauth/v2/common/constants.go index a655a46b44..38c680923f 100644 --- a/services/oauth/v2/common/constants.go +++ b/services/oauth/v2/common/constants.go @@ -5,10 +5,11 @@ const ( // CategoryAuthStatusInactive Identifier to be sent from destination(during transformation/delivery) CategoryAuthStatusInactive = "AUTH_STATUS_INACTIVE" // RefTokenInvalidGrant Identifier for invalid_grant or access_denied errors(during refreshing the token) - RefTokenInvalidGrant = "ref_token_invalid_grant" - TimeOutError = "timeout" - NetworkError = "network_error" - None = "none" + RefTokenInvalidGrant = "ref_token_invalid_grant" + RefTokenInvalidResponse = "INVALID_REFRESH_RESPONSE" + TimeOutError = "timeout" + NetworkError = "network_error" + None = "none" DestKey ContextKey = "destination" SecretKey ContextKey = "secret" diff --git a/services/oauth/v2/oauth.go b/services/oauth/v2/oauth.go index 1be55950a0..d06c42baad 100644 --- a/services/oauth/v2/oauth.go +++ b/services/oauth/v2/oauth.go @@ -330,17 +330,28 @@ func (h *OAuthHandler) GetRefreshTokenErrResp(response string, accountSecret *Ac h.Logger.Debugn("Failed with error response", logger.NewErrorField(err)) message = fmt.Sprintf("Unmarshal of response unsuccessful: %v", response) errorType = "unmarshallableResponse" - } else if gjson.Get(response, "body.code").String() == common.RefTokenInvalidGrant { - // User (or) AccessToken (or) RefreshToken has been revoked - bodyMsg := gjson.Get(response, "body.message").String() - if bodyMsg == "" { - // Default message - h.Logger.Debugn("Unable to get body.message", logger.NewStringField("Response", response)) - message = ErrPermissionOrTokenRevoked.Error() - } else { - message = bodyMsg + } else { + code := gjson.Get(response, "body.code").String() + switch code { + case common.RefTokenInvalidGrant: + // User (or) AccessToken (or) RefreshToken has been revoked + bodyMsg := gjson.Get(response, "body.message").String() + if bodyMsg == "" { + // Default message + h.Logger.Debugn("Unable to get body.message", logger.NewStringField("Response", response)) + message = ErrPermissionOrTokenRevoked.Error() + } else { + message = bodyMsg + } + errorType = common.RefTokenInvalidGrant + case common.RefTokenInvalidResponse: + errorType = code + msg := gjson.Get(response, "body.message").String() + if msg == "" { + msg = "invalid refresh response" + } + message = msg } - errorType = common.RefTokenInvalidGrant } return errorType, message } diff --git a/services/oauth/v2/oauth_test.go b/services/oauth/v2/oauth_test.go index 26182f3133..418ab0cd20 100644 --- a/services/oauth/v2/oauth_test.go +++ b/services/oauth/v2/oauth_test.go @@ -1,7 +1,9 @@ package v2_test import ( + "bytes" "fmt" + "io" "math/rand" "net" "net/http" @@ -576,6 +578,57 @@ var _ = Describe("Oauth", func() { Expect(response).To(Equal(expectedResponse)) Expect(err).To(MatchError(fmt.Errorf("error occurred while fetching/refreshing account info from CP: mock mock 127.0.0.1:1234->127.0.0.1:12340: read: connection timed out"))) }) + + It("refreshToken function call when stored cache is same as provided secret and cpApiCall returns a failed response because of faulty implementation in some downstream service", func() { + refreshTokenParams := &v2.RefreshTokenParams{ + AccountID: "123", + WorkspaceID: "456", + DestDefName: "testDest", + Destination: Destination, + Secret: []byte(`{"access_token":"storedAccessToken","refresh_token":"dummyRefreshToken","developer_token":"dummyDeveloperToken"}`), + } + + ctrl := gomock.NewController(GinkgoT()) + mockHttpClient := mockhttpclient.NewMockHttpClient(ctrl) + cpResponseString := `{ + "status": 500, + "code": "INVALID_REFRESH_RESPONSE", + "body": { + "code": "INVALID_REFRESH_RESPONSE", + "message": "Missing required token fields in refresh response" + }, + "access_token":"storedAccessToken", + "refresh_token":"dummyRefreshToken", + "developer_token":"dummyDeveloperToken" + }` + mockHttpClient.EXPECT().Do(gomock.Any()).Return(&http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString(cpResponseString)), + }, nil) + cpConnector := controlplane.NewConnector( + config.Default, + controlplane.WithClient(mockHttpClient), + controlplane.WithStats(stats.Default), + ) + + mockTokenProvider := mock_oauthV2.NewMockTokenProvider(ctrl) + mockTokenProvider.EXPECT().Identity().Return(&testutils.BasicAuthMock{}) + oauthHandler := v2.NewOAuthHandler(mockTokenProvider, + v2.WithCache(v2.NewCache()), + v2.WithLocker(kitsync.NewPartitionRWLocker()), + v2.WithStats(stats.Default), + v2.WithLogger(logger.NewLogger().Child("MockOAuthHandler")), + v2.WithCpConnector(cpConnector), + ) + statusCode, response, err := oauthHandler.RefreshToken(refreshTokenParams) + Expect(statusCode).To(Equal(http.StatusInternalServerError)) + expectedResponse := &v2.AuthResponse{ + Err: "INVALID_REFRESH_RESPONSE", + ErrorMessage: "Missing required token fields in refresh response", + } + Expect(response).To(Equal(expectedResponse)) + Expect(err).To(MatchError(fmt.Errorf("error occurred while fetching/refreshing account info from CP: Missing required token fields in refresh response"))) + }) }) Describe("Test AuthStatusToggle function", func() { From aaca8c14c23e746ee288cc572fa19b1580cd03ca Mon Sep 17 00:00:00 2001 From: Sai Sankeerth Date: Thu, 2 Jan 2025 12:17:38 +0530 Subject: [PATCH 2/2] chore: adding more tests to increase coverage --- services/oauth/v2/oauth_test.go | 100 ++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/services/oauth/v2/oauth_test.go b/services/oauth/v2/oauth_test.go index 418ab0cd20..604724f9ee 100644 --- a/services/oauth/v2/oauth_test.go +++ b/services/oauth/v2/oauth_test.go @@ -629,6 +629,106 @@ var _ = Describe("Oauth", func() { Expect(response).To(Equal(expectedResponse)) Expect(err).To(MatchError(fmt.Errorf("error occurred while fetching/refreshing account info from CP: Missing required token fields in refresh response"))) }) + + It("refreshToken function call when stored cache is same as provided secret and cpApiCall returns a failed response because of invalid refresh response without message", func() { + refreshTokenParams := &v2.RefreshTokenParams{ + AccountID: "123", + WorkspaceID: "456", + DestDefName: "testDest", + Destination: Destination, + Secret: []byte(`{"access_token":"storedAccessToken","refresh_token":"dummyRefreshToken","developer_token":"dummyDeveloperToken"}`), + } + + ctrl := gomock.NewController(GinkgoT()) + mockHttpClient := mockhttpclient.NewMockHttpClient(ctrl) + cpResponseString := `{ + "status": 500, + "code": "INVALID_REFRESH_RESPONSE", + "body": { + "code": "INVALID_REFRESH_RESPONSE" + }, + "access_token":"storedAccessToken", + "refresh_token":"dummyRefreshToken", + "developer_token":"dummyDeveloperToken" + }` + mockHttpClient.EXPECT().Do(gomock.Any()).Return(&http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString(cpResponseString)), + }, nil) + cpConnector := controlplane.NewConnector( + config.Default, + controlplane.WithClient(mockHttpClient), + controlplane.WithStats(stats.Default), + ) + + mockTokenProvider := mock_oauthV2.NewMockTokenProvider(ctrl) + mockTokenProvider.EXPECT().Identity().Return(&testutils.BasicAuthMock{}) + oauthHandler := v2.NewOAuthHandler(mockTokenProvider, + v2.WithCache(v2.NewCache()), + v2.WithLocker(kitsync.NewPartitionRWLocker()), + v2.WithStats(stats.Default), + v2.WithLogger(logger.NewLogger().Child("MockOAuthHandler")), + v2.WithCpConnector(cpConnector), + ) + statusCode, response, err := oauthHandler.RefreshToken(refreshTokenParams) + Expect(statusCode).To(Equal(http.StatusInternalServerError)) + expectedResponse := &v2.AuthResponse{ + Err: "INVALID_REFRESH_RESPONSE", + ErrorMessage: "invalid refresh response", + } + Expect(response).To(Equal(expectedResponse)) + Expect(err).To(MatchError(fmt.Errorf("error occurred while fetching/refreshing account info from CP: invalid refresh response"))) + }) + + It("refreshToken function call when stored cache is same as provided secret and cpApiCall returns a failed response because of invalid refresh response without message", func() { + refreshTokenParams := &v2.RefreshTokenParams{ + AccountID: "123", + WorkspaceID: "456", + DestDefName: "testDest", + Destination: Destination, + Secret: []byte(`{"access_token":"storedAccessToken","refresh_token":"dummyRefreshToken","developer_token":"dummyDeveloperToken"}`), + } + + ctrl := gomock.NewController(GinkgoT()) + mockHttpClient := mockhttpclient.NewMockHttpClient(ctrl) + cpResponseString := fmt.Sprintf(`{ + "status": 500, + "code": "%[1]s", + "body": { + "code": "%[1]s" + }, + "access_token":"storedAccessToken", + "refresh_token":"dummyRefreshToken", + "developer_token":"dummyDeveloperToken" + }`, common.RefTokenInvalidGrant) + mockHttpClient.EXPECT().Do(gomock.Any()).Return(&http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(bytes.NewBufferString(cpResponseString)), + }, nil) + cpConnector := controlplane.NewConnector( + config.Default, + controlplane.WithClient(mockHttpClient), + controlplane.WithStats(stats.Default), + ) + + mockTokenProvider := mock_oauthV2.NewMockTokenProvider(ctrl) + mockTokenProvider.EXPECT().Identity().Return(&testutils.BasicAuthMock{}) + oauthHandler := v2.NewOAuthHandler(mockTokenProvider, + v2.WithCache(v2.NewCache()), + v2.WithLocker(kitsync.NewPartitionRWLocker()), + v2.WithStats(stats.Default), + v2.WithLogger(logger.NewLogger().Child("MockOAuthHandler")), + v2.WithCpConnector(cpConnector), + ) + statusCode, response, err := oauthHandler.RefreshToken(refreshTokenParams) + Expect(statusCode).To(Equal(http.StatusBadRequest)) + expectedResponse := &v2.AuthResponse{ + Err: common.RefTokenInvalidGrant, + ErrorMessage: v2.ErrPermissionOrTokenRevoked.Error(), + } + Expect(response).To(Equal(expectedResponse)) + Expect(err).To(MatchError(fmt.Errorf("invalid grant"))) + }) }) Describe("Test AuthStatusToggle function", func() {