fix: call DeleteOpenIDConnectSession during successful authcode exchange

[cfryanr]

Fixes #790. Please see description and comments in #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

• I have read the contributing guidelines and signed the CLA.
• I have referenced an issue containing the design document if my change introduces a new feature.
• I have read the security policy.
• 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 [email protected]) from the maintainers to push the changes.
• I have added tests that prove my fix is effective or that my feature works.
• I have added the necessary documentation within the code base (if appropriate).

Further comments

[cfryanr]

The format check seems to have failed complaining about almost every copyright date in the repo. Please advise if I should update any copyright dates in this PR.

[aeneasr]

Nice! I recently found the same issue on our prod system. I think we can also delete the PKCE entry if it's not already being deleted

[james-d-elliott]

Small aside, this makes me think there is an issue with the tests since there is only one mock EXPECT added for DeleteOpenIDConnectSession.

[cfryanr]

Thanks for accepting the PR.

I think we can also delete the PKCE entry if it's not already being deleted

I believe those are already correctly deleted here https://github.com/ory/fosite/blob/v0.46.0/handler/pkce/handler.go#L133.

Small aside, this makes me think there is an issue with the tests since there is only one mock EXPECT added for DeleteOpenIDConnectSession.

I added two EXPECTs in the PR. One for each pre-existing happy path unit test.

[aeneasr]

You're right - those are indeed already covered!

[james-d-elliott]

I added two EXPECTs in the PR. One for each pre-existing happy path unit test.

Oh your right my bad. I swore when I looked at the "should pass" test it didn't have one. Was really confusing why it was passing without it. But it's there!