From c0b30f65bca686b3583384c611278fa8079f022a Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 12 Feb 2024 23:57:46 -0800 Subject: [PATCH] fix: call DeleteOpenIDConnectSession during successful authcode exchange (#793) Remove deprecation of `DeleteOpenIDConnectSession` storage interface function and call it during authorization code exchange. This function was not previously called. Implementors of the openid storage interface who which to preserve the old behavior should implement this function as a no-op which returns `nil`. Fixes #790 --- handler/openid/flow_explicit_token.go | 9 ++++++++- handler/openid/flow_explicit_token_test.go | 21 +++++++++++++++++++++ handler/openid/storage.go | 3 +-- storage/memory.go | 1 - 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/handler/openid/flow_explicit_token.go b/handler/openid/flow_explicit_token.go index 0b416d2c2..5988913e6 100644 --- 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 index 56325e5d9..8c6b571ab 100644 --- 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 index d29f73717..2b9dae9ba 100644 --- 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 index 3c6a5ad45..079b80beb 100644 --- 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()