Repository: ory/fosite (go)

Issue:
openid session storage should be deleted when the authcode is exchanged
### Preflight checklist

- [X] I could not find a solution in the existing issues, docs, nor discussions.
- [X] I agree to follow this project's [Code of Conduct](https://github.com/ory/fosite/blob/master/CODE_OF_CONDUCT.md).
- [X] I have read and am following this repository's [Contribution Guidelines](https://github.com/ory/fosite/blob/master/CONTRIBUTING.md).
- [ ] I have joined the [Ory Community Slack](https://slack.ory.sh).
- [ ] I am signed up to the [Ory Security Patch Newsletter](https://ory.us10.list-manage.com/subscribe?u=ffb1a878e4ec6c0ed312a3480&id=f605a41b53).

### Ory Network Project

_No response_

### Describe the bug

I work on a project that use fosite as a golang package and implements the fosite storage interfaces in a custom way. The `DeleteOpenIDConnectSession` interface has never been called and more recently has been marked deprecated. However, this is a problem for the fosite storage interfaces.

The storage interfaces are very nicely designed and gives implementors lots of flexibility in how the underlying storage works. In our project, for several technical reasons (including that we are not using SQL), each storage interface gets stored in the underlying datastore separately, with no connection to each other. Because the openid sessions are never deleted, we have no choice but to allow them to linger for longer than is actually necessary, using up storage in the underlying datastore.

If I understand the code correctly, there is no reason to keep the openid session data after the authcode is successfully exchanged [in `PopulateTokenEndpointResponse` of flow_explicit_token.go](https://github.com/ory/fosite/blob/f4114878826c6d26b6751a1ea61f69ebbc25d4f2/handler/openid/flow_explicit_token.go#L20), so that function could call `DeleteOpenIDConnectSession` before it returns.

I understand that other implementations of the storage interface might choose to store the openid session data along with the authcode session data (e.g. together in a SQL table), and therefore incur less overhead in the underlying datastore. It's nice that the fosite storage interface gives that flexibility. However, the storage interfaces should not require or assume that style of implementation, and none of the other fosite storage interfaces require it except for the openid interface.

Might you consider accepting a PR which removes the deprecation notice on `DeleteOpenIDConnectSession` and adds a call to that function in the appropriate place? Storage interfaces which do not need it could simply implement it as an empty (no-op) function, which they are most likely already doing anyways.

### Reproducing the bug

In the source code, you can see that the `DeleteOpenIDConnectSession` is never called, and is marked deprecated.

### Relevant log output

_No response_

### Relevant configuration

_No response_

### Version

v0.44.0

### On which operating system are you observing this issue?

None

### In which environment are you deploying?

None

### Additional Context

_No response_

Pull request:
fix: call DeleteOpenIDConnectSession during successful authcode exchange
Fixes https://github.com/ory/fosite/issues/790. Please see description and comments in https://github.com/ory/fosite/issues/790 for more information about why this is a necessary and safe change.

```
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`.
```

## Related Issue or Design Document

Fixes #790.

## Checklist

- [X] I have read the [contributing guidelines](../blob/master/CONTRIBUTING.md) and signed the CLA.
- [X] I have referenced an issue containing the design document if my change introduces a new feature.
- [X] I have read the [security policy](../security/policy).
- [X] I confirm that this pull request does not address a security vulnerability. 
      If this pull request addresses a security vulnerability, 
      I confirm that I got approval (please contact [security@ory.sh](mailto:security@ory.sh)) from the maintainers to push the changes.
- [X] I have added tests that prove my fix is effective or that my feature works.
- [X] I have added the necessary documentation within the code base (if appropriate).

## Further comments

<!--
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution
you did and what alternatives you considered, etc...
-->


Patch:
diff --git a/handler/openid/flow_explicit_token.go b/handler/openid/flow_explicit_token.go
index 0b416d2..5988913 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/storage.go b/handler/openid/storage.go
index d29f737..2b9dae9 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 3c6a5ad..079b80b 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()


Test patch:
diff --git a/handler/openid/flow_explicit_token_test.go b/handler/openid/flow_explicit_token_test.go
index 56325e5..8c6b571 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)