Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: call DeleteOpenIDConnectSession during successful authcode exchange #793

Merged
merged 1 commit into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion handler/openid/flow_explicit_token.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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:
Expand Down
21 changes: 21 additions & 0 deletions handler/openid/flow_explicit_token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"))
Expand Down Expand Up @@ -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"))
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions handler/openid/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
1 change: 0 additions & 1 deletion storage/memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
Loading