fix: call DeleteOpenIDConnectSession during successful authcode exchange

ory · Feb 12, 2024 · f27124d · f27124d
1 parent f411487
commit f27124d
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 4 deletions.
diff --git a/handler/openid/flow_explicit_token.go b/handler/openid/flow_explicit_token.go
@@ -22,7 +22,9 @@ func (c *OpenIDConnectExplicitHandler) PopulateTokenEndpointResponse(ctx context
 		return errorsx.WithStack(fosite.ErrUnknownRequest)
 	}
 
-	authorize, err := c.OpenIDConnectRequestStorage.GetOpenIDConnectSession(ctx, requester.GetRequestForm().Get("code"), requester)
+	authorizeCode := requester.GetRequestForm().Get("code")
+
+	authorize, err := c.OpenIDConnectRequestStorage.GetOpenIDConnectSession(ctx, authorizeCode, requester)
 	if errors.Is(err, ErrNoSessionFound) {
 		return errorsx.WithStack(fosite.ErrUnknownRequest.WithWrap(err).WithDebug(err.Error()))
 	} else if err != nil {
@@ -47,6 +49,11 @@ func (c *OpenIDConnectExplicitHandler) PopulateTokenEndpointResponse(ctx context
 		return errorsx.WithStack(fosite.ErrServerError.WithDebug("Failed to generate id token because subject is an empty string."))
 	}
 
+	err = c.OpenIDConnectRequestStorage.DeleteOpenIDConnectSession(ctx, authorizeCode)
+	if err != nil {
+		return errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error()))
+	}
+
 	claims.AccessTokenHash = c.GetAccessTokenHash(ctx, requester, responder)
 
 	// The response type `id_token` is only required when performing the implicit or hybrid flow, see:

diff --git a/handler/openid/flow_explicit_token_test.go b/handler/openid/flow_explicit_token_test.go
@@ -102,6 +102,7 @@ func TestExplicit_PopulateTokenEndpointResponse(t *testing.T) {
 				storedReq.GrantedScope = fosite.Arguments{"openid"}
 				storedReq.Form.Set("nonce", "1111111111111111")
 				store.EXPECT().GetOpenIDConnectSession(gomock.Any(), "foobar", req).Return(storedReq, nil)
+				store.EXPECT().DeleteOpenIDConnectSession(gomock.Any(), "foobar").Return(nil)
 			},
 			check: func(t *testing.T, aresp *fosite.AccessResponse) {
 				assert.NotEmpty(t, aresp.GetExtra("id_token"))
@@ -132,6 +133,7 @@ func TestExplicit_PopulateTokenEndpointResponse(t *testing.T) {
 				storedReq.GrantedScope = fosite.Arguments{"openid"}
 				storedReq.Form.Set("nonce", "1111111111111111")
 				store.EXPECT().GetOpenIDConnectSession(gomock.Any(), "foobar", req).Return(storedReq, nil)
+				store.EXPECT().DeleteOpenIDConnectSession(gomock.Any(), "foobar").Return(nil)
 			},
 			check: func(t *testing.T, aresp *fosite.AccessResponse) {
 				assert.NotEmpty(t, aresp.GetExtra("id_token"))
@@ -173,6 +175,25 @@ func TestExplicit_PopulateTokenEndpointResponse(t *testing.T) {
 			},
 			expectErr: fosite.ErrServerError,
 		},
+		{
+			description: "should fail because storage returns error when deleting openid session",
+			setup: func(store *internal.MockOpenIDConnectRequestStorage, req *fosite.AccessRequest) {
+				req.Client = &fosite.DefaultClient{
+					GrantTypes: fosite.Arguments{"authorization_code"},
+				}
+				req.GrantTypes = fosite.Arguments{"authorization_code"}
+				req.Form.Set("code", "foobar")
+				storedSession := &DefaultSession{
+					Claims: &jwt.IDTokenClaims{Subject: "peter"},
+				}
+				storedReq := fosite.NewAuthorizeRequest()
+				storedReq.Session = storedSession
+				storedReq.GrantedScope = fosite.Arguments{"openid"}
+				store.EXPECT().GetOpenIDConnectSession(gomock.Any(), "foobar", req).Return(storedReq, nil)
+				store.EXPECT().DeleteOpenIDConnectSession(gomock.Any(), "foobar").Return(errors.New("delete openid session err"))
+			},
+			expectErr: fosite.ErrServerError,
+		},
 	} {
 		t.Run(fmt.Sprintf("case=%d/description=%s", k, c.description), func(t *testing.T) {
 			ctrl := gomock.NewController(t)

diff --git a/handler/openid/storage.go b/handler/openid/storage.go
@@ -22,7 +22,6 @@ type OpenIDConnectRequestStorage interface {
 	// - or an arbitrary error if an error occurred.
 	GetOpenIDConnectSession(ctx context.Context, authorizeCode string, requester fosite.Requester) (fosite.Requester, error)
 
-	// Deprecated: DeleteOpenIDConnectSession is not called from anywhere.
-	// Originally, it should remove an open id connect session from the store.
+	// DeleteOpenIDConnectSession removes an open id connect session from the store.
 	DeleteOpenIDConnectSession(ctx context.Context, authorizeCode string) error
 }
diff --git a/storage/memory.go b/storage/memory.go
@@ -165,7 +165,6 @@ func (s *MemoryStore) GetOpenIDConnectSession(_ context.Context, authorizeCode s
 	return cl, nil
 }
 
-// DeleteOpenIDConnectSession is not really called from anywhere and it is deprecated.
 func (s *MemoryStore) DeleteOpenIDConnectSession(_ context.Context, authorizeCode string) error {
 	s.idSessionsMutex.Lock()
 	defer s.idSessionsMutex.Unlock()