Skip to content

Commit 778407e

Browse files
gao-sungao-sun
authored
Merge pull request #6292 from logto-io/gao-core-app-secrets
feat(core): multiple app secrets
2 parents 9854be2 + 94296ac commit 778407e

20 files changed

+1106
-101
lines changed

‎packages/core/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@
8484
"pg-protocol": "^1.6.0",
8585
"pluralize": "^8.0.0",
8686
"qrcode": "^1.5.3",
87+
"raw-body": "^2.5.2",
8788
"redis": "^4.6.14",
8889
"roarr": "^7.11.0",
8990
"samlify": "2.8.11",

‎packages/core/src/event-listeners/index.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,12 @@
1-
import { ConsoleLog } from '@logto/shared';
2-
import chalk from 'chalk';
31
import type Provider from 'oidc-provider';
42

53
import type Queries from '#src/tenants/Queries.js';
4+
import { getConsoleLogFromContext } from '#src/utils/console.js';
65

76
import { grantListener, grantRevocationListener } from './grant.js';
87
import { interactionEndedListener, interactionStartedListener } from './interaction.js';
98
import { recordActiveUsers } from './record-active-users.js';
109

11-
const consoleLog = new ConsoleLog(chalk.magenta('oidc'));
12-
1310
/**
1411
* @see {@link https://github.com/panva/node-oidc-provider/blob/v7.x/docs/README.md#im-getting-a-client-authentication-failed-error-with-no-details Getting auth error with no details?}
1512
* @see {@link https://github.com/panva/node-oidc-provider/blob/v7.x/docs/events.md OIDC Provider events}
@@ -29,8 +26,8 @@ export const addOidcEventListeners = (provider: Provider, queries: Queries) => {
2926
});
3027
provider.addListener('interaction.started', interactionStartedListener);
3128
provider.addListener('interaction.ended', interactionEndedListener);
32-
provider.addListener('server_error', (_, error) => {
33-
consoleLog.error('server_error:', error);
29+
provider.addListener('server_error', (ctx, error) => {
30+
getConsoleLogFromContext(ctx).error('server_error:', error);
3431
});
3532

3633
// Record token usage.

‎packages/core/src/middleware/koa-app-secret-transpilation.test.ts

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
import { type Context } from 'koa';
2+
import { type IRouterParamContext } from 'koa-router';
3+
4+
import { MockQueries } from '#src/test-utils/tenant.js';
5+
import { createContextWithRouteParameters } from '#src/utils/test-utils.js';
6+
7+
import koaAppSecretTranspilation from './koa-app-secret-transpilation.js';
8+
9+
const { jest } = import.meta;
10+
11+
describe('koaAppSecretTranspilation middleware', () => {
12+
const next = jest.fn();
13+
const findByCredentials = jest.fn();
14+
const queries = new MockQueries({ applicationSecrets: { findByCredentials } });
15+
16+
type Values = {
17+
authorization?: string;
18+
body?: Record<string, unknown>;
19+
query?: Record<string, unknown>;
20+
};
21+
22+
const expectNothingChanged = (
23+
ctx: Context & IRouterParamContext,
24+
{ authorization, body, query }: Values = {},
25+
calledCount = 0
26+
) => {
27+
expect(ctx.headers.authorization).toBe(authorization);
28+
expect(ctx.request.body).toStrictEqual(body);
29+
expect(ctx.query).toStrictEqual(query);
30+
expect(findByCredentials).toHaveBeenCalledTimes(calledCount);
31+
};
32+
33+
afterEach(() => {
34+
jest.clearAllMocks();
35+
});
36+
37+
it('should do nothing if no credentials are found', async () => {
38+
const ctx = createContextWithRouteParameters();
39+
await koaAppSecretTranspilation(queries)(ctx, next);
40+
expectNothingChanged(ctx);
41+
});
42+
43+
it('should do nothing if Authorization header is not valid', async () => {
44+
const ctx = createContextWithRouteParameters();
45+
46+
for (const authorization of [
47+
'Bearer',
48+
'Bearer invalid',
49+
'Basic invalid',
50+
`Basic ${Buffer.from('\u0019:1').toString('base64')}`,
51+
]) {
52+
ctx.headers.authorization = authorization;
53+
// eslint-disable-next-line no-await-in-loop
54+
await koaAppSecretTranspilation(queries)(ctx, next);
55+
expectNothingChanged(ctx, { authorization });
56+
}
57+
});
58+
59+
it('should do nothing if params are not valid', async () => {
60+
const ctx = createContextWithRouteParameters();
61+
62+
ctx.method = 'POST';
63+
for (const body of [
64+
{},
65+
{ client_id: 1 },
66+
{ client_secret: 1 },
67+
{ client_id: '1', client_secret: 1 },
68+
]) {
69+
ctx.request.body = body;
70+
// eslint-disable-next-line no-await-in-loop
71+
await koaAppSecretTranspilation(queries)(ctx, next);
72+
expectNothingChanged(ctx, { body });
73+
}
74+
75+
ctx.method = 'GET';
76+
for (const query of [
77+
{},
78+
{ client_id: 1 },
79+
{ client_secret: 1 },
80+
{ client_id: '1', client_secret: 1 },
81+
]) {
82+
ctx.request.body = undefined;
83+
// @ts-expect-error
84+
ctx.query = query;
85+
// eslint-disable-next-line no-await-in-loop
86+
await koaAppSecretTranspilation(queries)(ctx, next);
87+
expectNothingChanged(ctx, { query });
88+
}
89+
});
90+
91+
it('should do nothing if client credentials have the wrong combination', async () => {
92+
const ctx = createContextWithRouteParameters();
93+
94+
for (const [authorization, body] of [
95+
['Basic ' + Buffer.from('1:').toString('base64'), { client_id: '2', client_secret: '1' }],
96+
['Basic ' + Buffer.from('1:1').toString('base64'), { client_id: '1', client_secret: '1' }],
97+
] as const) {
98+
ctx.headers.authorization = authorization;
99+
ctx.method = 'POST';
100+
ctx.request.body = body;
101+
// eslint-disable-next-line no-await-in-loop
102+
await koaAppSecretTranspilation(queries)(ctx, next);
103+
expectNothingChanged(ctx, { authorization, body });
104+
}
105+
});
106+
107+
it('should do nothing if client credentials cannot be found', async () => {
108+
const ctx = createContextWithRouteParameters();
109+
const authorization = 'Basic ' + Buffer.from('1:1').toString('base64');
110+
ctx.headers.authorization = authorization;
111+
await koaAppSecretTranspilation(queries)(ctx, next);
112+
expectNothingChanged(ctx, { authorization }, 1);
113+
});
114+
115+
it('should throw an error if client credentials are expired', async () => {
116+
const ctx = createContextWithRouteParameters();
117+
const authorization = 'Basic ' + Buffer.from('1:1').toString('base64');
118+
ctx.headers.authorization = authorization;
119+
findByCredentials.mockResolvedValueOnce({ expiresAt: new Date(Date.now() - 1000) });
120+
await expect(koaAppSecretTranspilation(queries)(ctx, next)).rejects.toThrowError(
121+
'invalid_request'
122+
);
123+
expectNothingChanged(ctx, { authorization }, 1);
124+
});
125+
126+
it('should rewrite the client secret in the header', async () => {
127+
const ctx = createContextWithRouteParameters();
128+
const authorization = 'Basic ' + Buffer.from('1:1').toString('base64');
129+
ctx.headers.authorization = authorization;
130+
findByCredentials.mockResolvedValueOnce({ originalSecret: '2' });
131+
await koaAppSecretTranspilation(queries)(ctx, next);
132+
expect(ctx.headers.authorization).toBe('Basic ' + Buffer.from('1:2').toString('base64'));
133+
});
134+
135+
it('should rewrite the client secret in the body', async () => {
136+
const ctx = createContextWithRouteParameters();
137+
const body = { client_id: '1', client_secret: '1' };
138+
ctx.method = 'POST';
139+
ctx.request.body = body;
140+
findByCredentials.mockResolvedValueOnce({ originalSecret: '2' });
141+
await koaAppSecretTranspilation(queries)(ctx, next);
142+
expect(ctx.request.body.client_secret).toBe('2');
143+
});
144+
145+
it('should rewrite the client secret in the query', async () => {
146+
const ctx = createContextWithRouteParameters();
147+
const query = { client_id: '1', client_secret: '1' };
148+
ctx.method = 'GET';
149+
ctx.query = query;
150+
findByCredentials.mockResolvedValueOnce({ originalSecret: '2' });
151+
await koaAppSecretTranspilation(queries)(ctx, next);
152+
expect(ctx.query.client_secret).toBe('2');
153+
});
154+
});

‎packages/core/src/middleware/koa-app-secret-transpilation.ts

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
import type { Nullable } from '@silverhand/essentials';
2+
import type { MiddlewareType } from 'koa';
3+
import { errors } from 'oidc-provider';
4+
5+
import type Queries from '#src/tenants/Queries.js';
6+
7+
const noVSCHAR = /[^\u0020-\u007E]/;
8+
9+
function decodeAuthToken(token: string) {
10+
const authToken = decodeURIComponent(token.replaceAll('+', '%20'));
11+
if (noVSCHAR.test(authToken)) {
12+
return;
13+
}
14+
return authToken;
15+
}
16+
17+
/** @import { getConstantClientMetadata } from '#src/oidc/utils.js'; */
18+
/**
19+
* Create a middleware function that reads the app secret from the request and check if it matches
20+
* the app secret in the `application_secrets` table. If it matches, the secret will be transpiled
21+
* to the one in the `applications` table in order to be recognized by `oidc-provider`.
22+
*
23+
* If the app secret is not found in the `application_secrets` table, the middleware will keep
24+
* everything as is and let the `oidc-provider` handle it.
25+
*
26+
* @remarks
27+
* We have to transpile the app secret because the `oidc-provider` only accepts one secret per
28+
* client.
29+
*
30+
* @remarks
31+
* The way to read the app secret from the request is duplicated from the original `oidc-provider`
32+
* implementation as the client should not be aware of this process. If we change the way to
33+
* authenticate the client in the future, we should update this middleware accordingly.
34+
*
35+
* @see {@link getConstantClientMetadata} for client authentication method based on application
36+
* type.
37+
* @see {@link https://github.com/panva/node-oidc-provider/blob/v8.4.6/lib/shared/token_auth.js#L74 | oidc-provider} for
38+
* the original implementation.
39+
*/
40+
export default function koaAppSecretTranspilation<StateT, ContextT, ResponseBodyT>(
41+
queries: Queries
42+
): MiddlewareType<StateT, ContextT, Nullable<ResponseBodyT>> {
43+
return async (ctx, next) => {
44+
type ClientCredentials = Partial<{
45+
clientId: string;
46+
clientSecret: string;
47+
}>;
48+
const getCredentialsFromHeader = (): ClientCredentials => {
49+
const parts = ctx.headers.authorization?.split(' ');
50+
51+
if (parts?.length !== 2 || parts[0]?.toLowerCase() !== 'basic' || !parts[1]) {
52+
return {};
53+
}
54+
55+
const [part0, part1] = Buffer.from(parts[1], 'base64').toString().split(':');
56+
57+
return {
58+
clientId: part0 && decodeAuthToken(part0),
59+
clientSecret: part1 && decodeAuthToken(part1),
60+
};
61+
};
62+
63+
const getCredentialsFromParams = (): ClientCredentials => {
64+
const params: unknown = ctx.method === 'POST' ? ctx.request.body : ctx.query;
65+
66+
if (typeof params !== 'object' || params === null) {
67+
return {};
68+
}
69+
70+
const clientId =
71+
'client_id' in params && typeof params.client_id === 'string'
72+
? params.client_id
73+
: undefined;
74+
const clientSecret =
75+
'client_secret' in params && typeof params.client_secret === 'string'
76+
? params.client_secret
77+
: undefined;
78+
return { clientId, clientSecret };
79+
};
80+
81+
const getCredentials = (
82+
header: ClientCredentials,
83+
params: ClientCredentials
84+
): ClientCredentials => {
85+
if (header.clientId && params.clientId && header.clientId !== params.clientId) {
86+
return {};
87+
}
88+
89+
// Only authorization header is allowed to be used for client authentication if the client ID
90+
// is provided in the header.
91+
if (header.clientId && (!header.clientSecret || params.clientSecret)) {
92+
return {};
93+
}
94+
95+
const clientId = header.clientId ?? params.clientId;
96+
const clientSecret = header.clientSecret ?? params.clientSecret;
97+
98+
return { clientId, clientSecret };
99+
};
100+
101+
const header = getCredentialsFromHeader();
102+
const params = getCredentialsFromParams();
103+
const { clientId, clientSecret } = getCredentials(header, params);
104+
if (!clientId || !clientSecret) {
105+
return next();
106+
}
107+
108+
const result = await queries.applicationSecrets.findByCredentials(clientId, clientSecret);
109+
110+
// Fall back to the original client secret logic to provide backward compatibility.
111+
if (!result) {
112+
return next();
113+
}
114+
115+
if (result.expiresAt && result.expiresAt < Date.now()) {
116+
throw new errors.InvalidRequest('client secret has expired');
117+
}
118+
119+
// All checks passed. Rewrite the client secret to the one in the `applications` table.
120+
if (header.clientSecret) {
121+
ctx.headers.authorization = `Basic ${Buffer.from(
122+
`${clientId}:${result.originalSecret}`
123+
).toString('base64')}`;
124+
} else if (ctx.method === 'POST') {
125+
ctx.request.body.client_secret = result.originalSecret;
126+
} else {
127+
ctx.query.client_secret = result.originalSecret;
128+
}
129+
130+
return next();
131+
};
132+
}

‎packages/core/src/middleware/koa-oidc-error-handler.ts

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -90,17 +90,10 @@ export default function koaOidcErrorHandler<StateT, ContextT>(): Middleware<Stat
9090
throw error;
9191
}
9292

93-
// Report oidc exceptions to ApplicationInsights
94-
void appInsights.trackException(error, buildAppInsightsTelemetry(ctx));
95-
9693
// Mimic oidc-provider's error handling, thus we can use the unified logic below.
9794
// See https://github.com/panva/node-oidc-provider/blob/37d0a6cfb3c618141a44cbb904ce45659438f821/lib/shared/error_handler.js
9895
ctx.status = error.statusCode || 500;
9996
ctx.body = errorOut(error);
100-
101-
if (!EnvSet.values.isUnitTest && (!EnvSet.values.isProduction || ctx.status >= 500)) {
102-
getConsoleLogFromContext(ctx).error(error);
103-
}
10497
}
10598

10699
// This is the only way we can check if the error is handled by the oidc-provider, because
@@ -115,6 +108,7 @@ export default function koaOidcErrorHandler<StateT, ContextT>(): Middleware<Stat
115108

116109
if (parsed.success) {
117110
const { data } = parsed;
111+
118112
const code = isSessionNotFound(data.error_description)
119113
? 'session.not_found'
120114
: `oidc.${data.error}`;
@@ -126,6 +120,12 @@ export default function koaOidcErrorHandler<StateT, ContextT>(): Middleware<Stat
126120
error_uri: uri,
127121
...ctx.body,
128122
};
123+
124+
if (!EnvSet.values.isUnitTest && (!EnvSet.values.isProduction || ctx.status >= 500)) {
125+
getConsoleLogFromContext(ctx).error(ctx.body);
126+
}
127+
128+
void appInsights.trackException(ctx.body, buildAppInsightsTelemetry(ctx));
129129
}
130130
}
131131
};

‎packages/core/src/middleware/koa-slonik-error-handler.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
InvalidInputError,
66
CheckIntegrityConstraintViolationError,
77
UniqueIntegrityConstraintViolationError,
8+
ForeignKeyIntegrityConstraintViolationError,
89
} from '@silverhand/slonik';
910
import type { Middleware } from 'koa';
1011

@@ -42,6 +43,13 @@ export default function koaSlonikErrorHandler<StateT, ContextT>(): Middleware<St
4243
status: 422,
4344
});
4445
}
46+
47+
if (error.constraint === 'application_secrets_pkey') {
48+
throw new RequestError({
49+
code: 'application.secret_name_exists',
50+
status: 422,
51+
});
52+
}
4553
}
4654

4755
if (error instanceof CheckIntegrityConstraintViolationError) {
@@ -51,6 +59,13 @@ export default function koaSlonikErrorHandler<StateT, ContextT>(): Middleware<St
5159
});
5260
}
5361

62+
if (error instanceof ForeignKeyIntegrityConstraintViolationError) {
63+
throw new RequestError({
64+
code: 'entity.relation_foreign_key_not_found',
65+
status: 404,
66+
});
67+
}
68+
5469
if (error instanceof InsertionError) {
5570
throw new RequestError({
5671
code: 'entity.create_failed',

0 commit comments

Comments
 (0)