Skip to content

Commit

Permalink
fix: correctly respect client expiry
Browse files Browse the repository at this point in the history
  • Loading branch information
@ThisIsMissEm
ThisIsMissEm committed Apr 30, 2022
1 parent 5da7478 commit 3280347
Show file tree
Hide file tree
Showing 8 changed files with 365 additions and 22 deletions.
103 changes: 102 additions & 1 deletion packages/browser/src/login/oidc/ClientRegistrar.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ describe("ClientRegistrar", () => {
JSON.stringify({
client_id: "abcd",
client_secret: "1234",
client_secret_expires_at: 0,
redirect_uris: ["https://example.com"],
id_token_signed_response_alg: "RS256",
})
Expand Down Expand Up @@ -170,13 +171,14 @@ describe("ClientRegistrar", () => {
);
});

it("retrieves client id and secret from storage if they are present", async () => {
it("retrieves client id, secret and expiry from storage if they are present", async () => {
const clientRegistrar = getClientRegistrar({
storage: mockStorageUtility(
{
"solidClientAuthenticationUser:mySession": {
clientId: "an id",
clientSecret: "a secret",
clientExpiresAt: "0",
},
},
false
Expand All @@ -193,6 +195,94 @@ describe("ClientRegistrar", () => {
);
expect(client.clientId).toBe("an id");
expect(client.clientSecret).toBe("a secret");
expect(client.clientExpiresAt).toBe(0);
});

it("registers a new client if the one in storage has expired", async () => {
const clientRegistrar = getClientRegistrar({
storage: mockStorageUtility(
{
"solidClientAuthenticationUser:mySession": {
clientId: "an id",
clientSecret: "a secret",
// The stored expiry is in the past:
clientExpiresAt: "5",
},
},
false
),
});

const mockFetch = (jest.fn() as any).mockResolvedValueOnce(
/* eslint-disable camelcase */
new NodeResponse(
JSON.stringify({
client_id: "abcd",
client_secret: "1234",
client_secret_expires_at: 0,
redirect_uris: ["https://example.com"],
})
) as unknown as Response
/* eslint-enable camelcase */
);
global.fetch = mockFetch;

await clientRegistrar.getClient(
{
sessionId: "mySession",
redirectUrl: "https://example.com",
},
{
...IssuerConfigFetcherFetchConfigResponse,
registrationEndpoint: "https://some.issuer/register",
}
);

expect(mockFetch).toHaveBeenCalled();
});

it("registers a new client if the one in storage has no expiry stored", async () => {
// We do this because the client may actually have an expiry, but when we
// stored it, we didn't store the expiry.
const clientRegistrar = getClientRegistrar({
storage: mockStorageUtility(
{
"solidClientAuthenticationUser:mySession": {
clientId: "an id",
clientSecret: "a secret",
// Note: clientExpiresAt is not present
},
},
false
),
});

const mockFetch = (jest.fn() as any).mockResolvedValueOnce(
/* eslint-disable camelcase */
new NodeResponse(
JSON.stringify({
client_id: "abcd",
client_secret: "1234",
client_secret_expires_at: 0,
redirect_uris: ["https://example.com"],
})
) as unknown as Response
/* eslint-enable camelcase */
);
global.fetch = mockFetch;

await clientRegistrar.getClient(
{
sessionId: "mySession",
redirectUrl: "https://example.com",
},
{
...IssuerConfigFetcherFetchConfigResponse,
registrationEndpoint: "https://some.issuer/register",
}
);

expect(mockFetch).toHaveBeenCalled();
});

it("passes the registration token if provided", async () => {
Expand All @@ -206,6 +296,7 @@ describe("ClientRegistrar", () => {
JSON.stringify({
client_id: "abcd",
client_secret: "1234",
client_secret_expires_at: 0,
redirect_uris: ["https://example.com"],
})
) as unknown as Response
Expand Down Expand Up @@ -250,6 +341,7 @@ describe("ClientRegistrar", () => {
JSON.stringify({
client_id: "abcd",
client_secret: "1234",
client_secret_expires_at: 0,
redirect_uris: ["https://example.com"],
})
) as unknown as Response
Expand All @@ -276,12 +368,18 @@ describe("ClientRegistrar", () => {
});

it("saves dynamic registration information", async () => {
const currentTime = 1651281010000;
jest.useFakeTimers();
jest.setSystemTime(currentTime);

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,
redirect_uris: ["https://example.com"],
})
) as unknown as Response
Expand Down Expand Up @@ -311,6 +409,9 @@ describe("ClientRegistrar", () => {
await expect(
myStorage.getForUser("mySession", "clientSecret", { secure: false })
).resolves.toBe("some secret");
await expect(
myStorage.getForUser("mySession", "clientExpiresAt", { secure: false })
).resolves.toBe("1651281015000");
});
});
});
30 changes: 20 additions & 10 deletions packages/browser/src/login/oidc/ClientRegistrar.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export default class ClientRegistrar implements IClientRegistrar {
const [
storedClientId,
storedClientSecret,
storedClientExpiry,
// storedClientName,
] = await Promise.all([
this.storageUtility.getForUser(options.sessionId, "clientId", {
Expand All @@ -55,18 +56,23 @@ export default class ClientRegistrar implements IClientRegistrar {
this.storageUtility.getForUser(options.sessionId, "clientSecret", {
secure: false,
}),
// this.storageUtility.getForUser(options.sessionId, "clientName", {
// // FIXME: figure out how to persist secure storage at reload
// secure: false,
// }),
this.storageUtility.getForUser(options.sessionId, "clientExpiresAt", {
secure: false,
}),
]);
if (storedClientId) {
return {
clientId: storedClientId,
clientSecret: storedClientSecret,
clientType: "dynamic",
};

if (storedClientId && storedClientExpiry) {
const parsedClientExpiry = parseInt(storedClientExpiry, 10);
if (parsedClientExpiry === 0 || parsedClientExpiry > Date.now()) {
return {
clientId: storedClientId,
clientSecret: storedClientSecret,
clientExpiresAt: parsedClientExpiry,
clientType: "dynamic",
};
}
}

const extendedOptions = { ...options };
// If registration access token is stored, use that.
extendedOptions.registrationAccessToken =
Expand All @@ -88,6 +94,10 @@ export default class ClientRegistrar implements IClientRegistrar {
if (registeredClient.clientSecret) {
infoToSave.clientSecret = registeredClient.clientSecret;
}
if (registeredClient.clientExpiresAt) {
infoToSave.clientExpiresAt =
registeredClient.clientExpiresAt.toString();
}
if (registeredClient.idTokenSignedResponseAlg) {
infoToSave.idTokenSignedResponseAlg =
registeredClient.idTokenSignedResponseAlg;
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/login/oidc/IClient.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export interface IClient {
clientId: string;
clientSecret?: string;
clientName?: string;
clientExpiresAt?: number;
idTokenSignedResponseAlg?: string;
clientType: ClientType;
}
86 changes: 85 additions & 1 deletion packages/node/src/login/oidc/ClientRegistrar.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ describe("ClientRegistrar", () => {
"solidClientAuthenticationUser:mySession": {
clientId: "an id",
clientSecret: "a secret",
clientExpiresAt: "0",
clientName: "my client name",
idTokenSignedResponseAlg: "ES256",
},
Expand All @@ -102,16 +103,99 @@ describe("ClientRegistrar", () => {
expect(client.clientId).toBe("an id");
expect(client.clientSecret).toBe("a secret");
expect(client.clientName).toBe("my client name");
expect(client.clientExpiresAt).toBe(0);
expect(client.idTokenSignedResponseAlg).toBe("ES256");
});

it("registers a new client if the one in storage has expired", async () => {
const { Issuer } = jest.requireMock("openid-client") as any;
const mockedIssuer = {
metadata: mockDefaultIssuerMetadata(),
Client: {
register: (jest.fn() as any).mockResolvedValueOnce({
metadata: mockDefaultClientConfig(),
}),
},
};
Issuer.mockReturnValue(mockedIssuer);

const clientRegistrar = getClientRegistrar({
storage: mockStorageUtility(
{
"solidClientAuthenticationUser:mySession": {
clientId: "an id",
clientSecret: "a secret",
// The expiry is in the past:
clientExpiresAt: "5",
clientName: "my client name",
idTokenSignedResponseAlg: "ES256",
},
},
false
),
});

await clientRegistrar.getClient(
{
sessionId: "mySession",
redirectUrl: "https://example.com",
},
{
...IssuerConfigFetcherFetchConfigResponse,
}
);

expect(mockedIssuer.Client.register).toHaveBeenCalled();
});

it("registers a new client if the one in storage has no expiry stored", async () => {
const { Issuer } = jest.requireMock("openid-client") as any;
const mockedIssuer = {
metadata: mockDefaultIssuerMetadata(),
Client: {
register: (jest.fn() as any).mockResolvedValueOnce({
metadata: mockDefaultClientConfig(),
}),
},
};
Issuer.mockReturnValue(mockedIssuer);

const clientRegistrar = getClientRegistrar({
storage: mockStorageUtility(
{
"solidClientAuthenticationUser:mySession": {
clientId: "an id",
clientSecret: "a secret",
// Note: no clientExpiresAt has been stored
clientName: "my client name",
idTokenSignedResponseAlg: "ES256",
},
},
false
),
});

await clientRegistrar.getClient(
{
sessionId: "mySession",
redirectUrl: "https://example.com",
},
{
...IssuerConfigFetcherFetchConfigResponse,
}
);

expect(mockedIssuer.Client.register).toHaveBeenCalled();
});

it("negotiates signing alg if not found in storage", async () => {
const clientRegistrar = getClientRegistrar({
storage: mockStorageUtility(
{
"solidClientAuthenticationUser:mySession": {
clientId: "an id",
clientSecret: "a secret",
clientExpiresAt: "0",
clientName: "my client name",
},
},
Expand Down Expand Up @@ -181,7 +265,7 @@ describe("ClientRegistrar", () => {
);
});

it("throws if the issuer doesn't avertise for supported signing algorithms", async () => {
it("throws if the issuer doesn't advertise for supported signing algorithms", async () => {
// Sets up the mock-up for DCR
const { Issuer } = jest.requireMock("openid-client") as any;
const mockedIssuer = {
Expand Down
Loading

0 comments on commit 3280347

Please sign in to comment.