[WIP] fix: correctly respect client expiry #2122
Conversation
| expect(mockFetch).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("registers a new client if the one in storage has no expiry stored", async () => { |
There was a problem hiding this comment.
@NSeydoux I think this is what we'll want, because otherwise someone may have a valid client, but we hadn't stored the expiry, in which case registering a new client resolves that missing expiry.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3280347:
|
| const infoToSave: Record<string, string> = { | ||
| clientId: registeredClient.metadata.client_id, | ||
| idTokenSignedResponseAlg: | ||
| registeredClient.metadata.id_token_signed_response_alg ?? signingAlg, | ||
| }; | ||
| if (registeredClient.metadata.client_secret) { | ||
| infoToSave.clientSecret = registeredClient.metadata.client_secret; | ||
| infoToSave.clientExpiresAt = clientExpiresAt.toString(); |
There was a problem hiding this comment.
I think we may always want to store this, though technically client_secret_expires_at is only returned if a client_secret is returned: https://openid.net/specs/openid-connect-registration-1_0.html#RegistrationResponse
| @@ -143,18 +149,30 @@ export default class ClientRegistrar implements IClientRegistrar { | |||
| } | |||
| ); | |||
|
|
|||
| let clientExpiresAt = 0; | |||
| if ( | |||
| typeof registeredClient.metadata.client_secret_expires_at === "number" | |||
There was a problem hiding this comment.
Note: openid-client doesn't actually verify that client_secret_expires_at is present and that it is in the future, I've added a set of checks in oidc-client-ext to verify these for the browser, but for server we don't currently have these checks.
There was a problem hiding this comment.
In theory here we could receive back a client with a client_secret_expires_at in the past, if the users' clock time on their server is ahead of the IdP's clock time.
Can we assume that users and IdP's can configure their servers to have a synchronised clock time?
| const mockFetch = (jest.fn() as any).mockResolvedValueOnce( | ||
| /* eslint-disable camelcase */ | ||
| new NodeResponse( | ||
| JSON.stringify({ | ||
| client_id: "some id", | ||
| client_secret: "some secret", | ||
| // Expire this client in 5 seconds: | ||
| client_secret_expires_at: currentTime / 1000 + 5, |
There was a problem hiding this comment.
We may want a test for the behaviour when client_secret_expires_at is in the past, as the registerClient would fail in this case, which would likely blow up the code for ClientRegistrar in browser.
ThisIsMissEm
force-pushed
the
fix/redirect-handler
branch
from
bfb6eab to
5da7478
Compare
ThisIsMissEm
force-pushed
the
fix/respect-client-expiry
branch
from
c153424 to
3280347
Compare
|
E2E tests are currently failing because of how the |
| sessionInfo.issuer === undefined | ||
| ) { | ||
| return null; | ||
| } | ||
|
|
||
| // FIXME: verify client is still valid |
There was a problem hiding this comment.
This is actually taken care of by ClientRegistrar — if the client isn't valid, then a new one will be requested.
Valid Client: has clientId & clientExpiresAt == 0 or clientExpiresAt > now
…ferent types This helps with ensuring only the correct options are to a given client type
ThisIsMissEm
force-pushed
the
fix/respect-client-expiry
branch
from
4d46bc5 to
94daee1
Compare
This PR fixes the bug where dynamically registered clients did not expire, despite the server indicating that they should expiry at a given time.