fix: handle serialization errors that can be thrown by call to 'Commi…

…t' (#403)
ory · Mar 26, 2020 · 35a1558 · 35a1558
1 parent b1ba04e
commit 35a1558
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 9 deletions.
diff --git a/handler/oauth2/flow_refresh.go b/handler/oauth2/flow_refresh.go
@@ -137,22 +137,22 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con
 
 	ts, err := c.TokenRevocationStorage.GetRefreshTokenSession(ctx, signature, nil)
 	if err != nil {
-		return handleRefreshTokenEndpointResponseStorageError(ctx, c.TokenRevocationStorage, err)
+		return handleRefreshTokenEndpointResponseStorageError(ctx, true, c.TokenRevocationStorage, err)
 	} else if err := c.TokenRevocationStorage.RevokeAccessToken(ctx, ts.GetID()); err != nil {
-		return handleRefreshTokenEndpointResponseStorageError(ctx, c.TokenRevocationStorage, err)
+		return handleRefreshTokenEndpointResponseStorageError(ctx, true, c.TokenRevocationStorage, err)
 	} else if err := c.TokenRevocationStorage.RevokeRefreshToken(ctx, ts.GetID()); err != nil {
-		return handleRefreshTokenEndpointResponseStorageError(ctx, c.TokenRevocationStorage, err)
+		return handleRefreshTokenEndpointResponseStorageError(ctx, true, c.TokenRevocationStorage, err)
 	}
 
 	storeReq := requester.Sanitize([]string{})
 	storeReq.SetID(ts.GetID())
 
 	if err := c.TokenRevocationStorage.CreateAccessTokenSession(ctx, accessSignature, storeReq); err != nil {
-		return handleRefreshTokenEndpointResponseStorageError(ctx, c.TokenRevocationStorage, err)
+		return handleRefreshTokenEndpointResponseStorageError(ctx, true, c.TokenRevocationStorage, err)
 	}
 
 	if err := c.TokenRevocationStorage.CreateRefreshTokenSession(ctx, refreshSignature, storeReq); err != nil {
-		return handleRefreshTokenEndpointResponseStorageError(ctx, c.TokenRevocationStorage, err)
+		return handleRefreshTokenEndpointResponseStorageError(ctx, true, c.TokenRevocationStorage, err)
 	}
 
 	responder.SetAccessToken(accessToken)
@@ -162,16 +162,18 @@ func (c *RefreshTokenGrantHandler) PopulateTokenEndpointResponse(ctx context.Con
 	responder.SetExtra("refresh_token", refreshToken)
 
 	if err := storage.MaybeCommitTx(ctx, c.TokenRevocationStorage); err != nil {
-		return errors.WithStack(fosite.ErrServerError.WithDebug(err.Error()))
+		return handleRefreshTokenEndpointResponseStorageError(ctx, false, c.TokenRevocationStorage, err)
 	}
 
 	return nil
 }
 
-func handleRefreshTokenEndpointResponseStorageError(ctx context.Context, store TokenRevocationStorage, storageErr error) (err error) {
+func handleRefreshTokenEndpointResponseStorageError(ctx context.Context, rollback bool, store TokenRevocationStorage, storageErr error) (err error) {
 	defer func() {
-		if rbErr := storage.MaybeRollbackTx(ctx, store); rbErr != nil {
-			err = errors.WithStack(fosite.ErrServerError.WithDebug(rbErr.Error()))
+		if rollback {
+			if rbErr := storage.MaybeRollbackTx(ctx, store); rbErr != nil {
+				err = errors.WithStack(fosite.ErrServerError.WithDebug(rbErr.Error()))
+			}
 		}
 	}()
 

diff --git a/handler/oauth2/flow_refresh_test.go b/handler/oauth2/flow_refresh_test.go
@@ -803,6 +803,49 @@ func TestRefreshFlowTransactional_PopulateTokenEndpointResponse(t *testing.T) {
 			},
 			expectError: fosite.ErrServerError,
 		},
+		{
+			description: "should result in a `fosite.ErrInvalidRequest` if transaction fails to commit due to a " +
+				"`fosite.ErrSerializationFailure` error",
+			setup: func() {
+				request.GrantTypes = fosite.Arguments{"refresh_token"}
+				mockTransactional.
+					EXPECT().
+					BeginTX(propagatedContext).
+					Return(propagatedContext, nil).
+					Times(1)
+				mockRevocationStore.
+					EXPECT().
+					GetRefreshTokenSession(propagatedContext, gomock.Any(), nil).
+					Return(request, nil).
+					Times(1)
+				mockRevocationStore.
+					EXPECT().
+					RevokeAccessToken(propagatedContext, gomock.Any()).
+					Return(nil).
+					Times(1)
+				mockRevocationStore.
+					EXPECT().
+					RevokeRefreshToken(propagatedContext, gomock.Any()).
+					Return(nil).
+					Times(1)
+				mockRevocationStore.
+					EXPECT().
+					CreateAccessTokenSession(propagatedContext, gomock.Any(), gomock.Any()).
+					Return(nil).
+					Times(1)
+				mockRevocationStore.
+					EXPECT().
+					CreateRefreshTokenSession(propagatedContext, gomock.Any(), gomock.Any()).
+					Return(nil).
+					Times(1)
+				mockTransactional.
+					EXPECT().
+					Commit(propagatedContext).
+					Return(fosite.ErrSerializationFailure).
+					Times(1)
+			},
+			expectError: fosite.ErrInvalidRequest,
+		},
 	} {
 		t.Run(fmt.Sprintf("scenario=%s", testCase.description), func(t *testing.T) {
 			ctrl := gomock.NewController(t)