From cf15973b137ada73d8715c9663c0ee1c7884f303 Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Wed, 27 May 2026 13:57:51 +0800 Subject: [PATCH 1/7] feat(core): enforce application access during oidc flows --- .../application-access-control.test.ts | 10 + .../libraries/application-access-control.ts | 4 +- .../src/libraries/session/consent.test.ts | 50 ++++- .../core/src/libraries/session/consent.ts | 16 +- .../libraries/session/session-context.test.ts | 13 +- .../middleware/koa-app-access-control.test.ts | 69 +++++++ .../src/middleware/koa-app-access-control.ts | 33 ++++ .../core/src/middleware/koa-auto-consent.ts | 15 +- .../src/middleware/koa-consent-guard.test.ts | 55 +++--- .../core/src/middleware/koa-consent-guard.ts | 14 +- .../src/oidc/application-access-control.ts | 20 ++ packages/core/src/oidc/grants/index.ts | 12 +- .../src/oidc/grants/refresh-token.test.ts | 21 +- .../core/src/oidc/grants/refresh-token.ts | 18 +- .../oidc/grants/token-exchange/index.test.ts | 22 ++- .../src/oidc/grants/token-exchange/index.ts | 13 +- packages/core/src/oidc/init.test.ts | 48 ++++- packages/core/src/oidc/init.ts | 19 +- .../src/routes/interaction/consent/index.ts | 1 + packages/core/src/tenants/Tenant.ts | 8 +- .../integration-tests/src/api/application.ts | 11 ++ .../integration-tests/src/client/index.ts | 4 + .../api/application-access-control.test.ts | 187 ++++++++++++++++++ 23 files changed, 590 insertions(+), 73 deletions(-) create mode 100644 packages/core/src/middleware/koa-app-access-control.test.ts create mode 100644 packages/core/src/middleware/koa-app-access-control.ts create mode 100644 packages/core/src/oidc/application-access-control.ts create mode 100644 packages/integration-tests/src/tests/api/application-access-control.test.ts diff --git a/packages/core/src/libraries/application-access-control.test.ts b/packages/core/src/libraries/application-access-control.test.ts index 78db123765d6..13e18605de3d 100644 --- a/packages/core/src/libraries/application-access-control.test.ts +++ b/packages/core/src/libraries/application-access-control.test.ts @@ -1,4 +1,5 @@ import { + accountCenterApplicationId, createDefaultApplicationAccessControl, type Application, type ApplicationAccessControl, @@ -86,6 +87,15 @@ describe('assertUserHasApplicationAccess()', () => { expect(findApplicationAccessControl).not.toHaveBeenCalled(); }); + it('allows built-in applications without querying the application', async () => { + await expect( + createLibrary().assertUserHasApplicationAccess(accountCenterApplicationId, userId) + ).resolves.not.toThrow(); + + expect(findApplicationById).not.toHaveBeenCalled(); + expect(findApplicationAccessControl).not.toHaveBeenCalled(); + }); + it('allows without querying rule tables when app-level access control is disabled', async () => { findApplicationById.mockResolvedValueOnce(disabledApplication); diff --git a/packages/core/src/libraries/application-access-control.ts b/packages/core/src/libraries/application-access-control.ts index c38fa244311a..5874b8c984cb 100644 --- a/packages/core/src/libraries/application-access-control.ts +++ b/packages/core/src/libraries/application-access-control.ts @@ -1,4 +1,4 @@ -import type { ApplicationAccessControl } from '@logto/schemas'; +import { isBuiltInApplicationId, type ApplicationAccessControl } from '@logto/schemas'; import { EnvSet } from '#src/env-set/index.js'; import RequestError from '#src/errors/RequestError/index.js'; @@ -25,7 +25,7 @@ export const createApplicationAccessControlLibrary = (queries: Queries) => { } = queries; const assertUserHasApplicationAccess = async (applicationId: string, userId: string) => { - if (!EnvSet.values.isDevFeaturesEnabled) { + if (!EnvSet.values.isDevFeaturesEnabled || isBuiltInApplicationId(applicationId)) { return; } diff --git a/packages/core/src/libraries/session/consent.test.ts b/packages/core/src/libraries/session/consent.test.ts index d4587b6988bf..99b6db993efe 100644 --- a/packages/core/src/libraries/session/consent.test.ts +++ b/packages/core/src/libraries/session/consent.test.ts @@ -39,6 +39,11 @@ const userQueries = { findUserById: jest.fn(async (): Promise => mockUser), updateUserById: jest.fn(async (..._args: unknown[]) => ({ id: 'id' })), }; +const applicationAccessControl = { + assertUserHasApplicationAccess: jest.fn(async () => { + await Promise.resolve(); + }), +}; // @ts-expect-error const queries: Queries = { users: userQueries }; @@ -59,7 +64,13 @@ describe('consent', () => { it('should update with new grantId if not exist', async () => { const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); - await consent({ ctx: context, provider, queries, interactionDetails: baseInteractionDetails }); + await consent({ + ctx: context, + applicationAccessControl, + provider, + queries, + interactionDetails: baseInteractionDetails, + }); expect(grantSave).toHaveBeenCalled(); @@ -85,7 +96,13 @@ describe('consent', () => { const provider = createMockProvider(jest.fn().mockResolvedValue(interactionDetails), Grant); - await consent({ ctx: context, provider, queries, interactionDetails }); + await consent({ + ctx: context, + applicationAccessControl, + provider, + queries, + interactionDetails, + }); expect(grantSave).toHaveBeenCalled(); @@ -109,7 +126,13 @@ describe('consent', () => { })); const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); - await consent({ ctx: context, provider, queries, interactionDetails: baseInteractionDetails }); + await consent({ + ctx: context, + applicationAccessControl, + provider, + queries, + interactionDetails: baseInteractionDetails, + }); expect(userQueries.updateUserById).toHaveBeenCalledWith(mockUser.id, { applicationId: baseInteractionDetails.params.client_id, @@ -120,6 +143,7 @@ describe('consent', () => { const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); await consent({ ctx: context, + applicationAccessControl, provider, queries, interactionDetails: baseInteractionDetails, @@ -137,4 +161,24 @@ describe('consent', () => { ); expect(grantAddResourceScope).toHaveBeenCalledWith('resource2', 'resource2_scope1'); }); + + it('should assert user application access before saving grant', async () => { + const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); + + await consent({ + ctx: context, + applicationAccessControl, + provider, + queries, + interactionDetails: baseInteractionDetails, + }); + + expect(applicationAccessControl.assertUserHasApplicationAccess).toHaveBeenCalledWith( + 'clientId', + mockUser.id + ); + expect( + applicationAccessControl.assertUserHasApplicationAccess.mock.invocationCallOrder[0] + ).toBeLessThan(grantSave.mock.invocationCallOrder[0] ?? 0); + }); }); diff --git a/packages/core/src/libraries/session/consent.ts b/packages/core/src/libraries/session/consent.ts index fde56fcf37b4..7785b116d778 100644 --- a/packages/core/src/libraries/session/consent.ts +++ b/packages/core/src/libraries/session/consent.ts @@ -2,8 +2,10 @@ import { jsonObjectGuard } from '@logto/schemas'; import { conditional } from '@silverhand/essentials'; import type { Context } from 'koa'; import type { PromptDetail, Provider } from 'oidc-provider'; +import { errors } from 'oidc-provider'; import { z } from 'zod'; +import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -85,6 +87,7 @@ const saveInteractionLastSubmissionToSession = async ( export const consent = async ({ ctx, + applicationAccessControl, provider, queries, interactionDetails, @@ -93,6 +96,7 @@ export const consent = async ({ resourceScopesToReject = {}, }: { ctx: Context; + applicationAccessControl: Libraries['applicationAccessControl']; provider: Provider; queries: Queries; interactionDetails: Awaited>; @@ -103,19 +107,25 @@ export const consent = async ({ const { session, grantId, - params: { client_id }, + params: { client_id: clientId }, } = interactionDetails; assertThat(session, 'session.not_found'); + assertThat( + clientId && typeof clientId === 'string', + new errors.InvalidClient('client must be available') + ); const { accountId } = session; + await applicationAccessControl.assertUserHasApplicationAccess(clientId, accountId); + const grant = conditional(grantId && (await provider.Grant.find(grantId))) ?? - new provider.Grant({ accountId, clientId: String(client_id) }); + new provider.Grant({ accountId, clientId }); await Promise.all([ - saveUserFirstConsentedAppId(queries, accountId, String(client_id)), + saveUserFirstConsentedAppId(queries, accountId, clientId), saveInteractionLastSubmissionToSession(queries, interactionDetails), ]); diff --git a/packages/core/src/libraries/session/session-context.test.ts b/packages/core/src/libraries/session/session-context.test.ts index 3258a4415ad5..703f8624a9a7 100644 --- a/packages/core/src/libraries/session/session-context.test.ts +++ b/packages/core/src/libraries/session/session-context.test.ts @@ -25,6 +25,11 @@ const userQueries = { findUserById: jest.fn(async (): Promise => mockUser), updateUserById: jest.fn(async (..._args: unknown[]) => ({ id: 'id' })), }; +const applicationAccessControl = { + assertUserHasApplicationAccess: jest.fn(async () => { + await Promise.resolve(); + }), +}; const queries = { users: userQueries, @@ -51,7 +56,13 @@ describe('saveInteractionLastSubmissionToSession', () => { const provider = createMockProvider(jest.fn().mockResolvedValue(interactionDetails), Grant); - await consent({ ctx: context, provider, queries, interactionDetails }); + await consent({ + ctx: context, + applicationAccessControl, + provider, + queries, + interactionDetails, + }); expect(oidcSessionExtensionsInsert).toHaveBeenCalledWith({ sessionUid: 'sessionUid', diff --git a/packages/core/src/middleware/koa-app-access-control.test.ts b/packages/core/src/middleware/koa-app-access-control.test.ts new file mode 100644 index 000000000000..043261f64ae0 --- /dev/null +++ b/packages/core/src/middleware/koa-app-access-control.test.ts @@ -0,0 +1,69 @@ +import RequestError from '#src/errors/RequestError/index.js'; +import { MockTenant } from '#src/test-utils/tenant.js'; +import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; + +import koaAppAccessControl from './koa-app-access-control.js'; + +const { jest } = import.meta; + +describe('koaAppAccessControl middleware', () => { + const assertUserHasApplicationAccess = jest.fn(async () => { + await Promise.resolve(); + }); + const mockTenant = new MockTenant(undefined, undefined, undefined, { + applicationAccessControl: { assertUserHasApplicationAccess }, + }); + const next = jest.fn(); + + afterEach(() => { + jest.clearAllMocks(); + }); + + const createContext = ( + interactionDetails: Partial>> + ) => ({ + ...createContextWithRouteParameters({ + url: `/consent`, + }), + interactionDetails: interactionDetails as Awaited< + ReturnType + >, + }); + + it('should assert application access before next middleware', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + // @ts-expect-error + session: { accountId: 'user-id' }, + }); + const guard = koaAppAccessControl(mockTenant.libraries); + + await guard(ctx, next); + + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); + expect(next).toHaveBeenCalled(); + }); + + it('should throw when access is denied', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + // @ts-expect-error + session: { accountId: 'user-id' }, + }); + assertUserHasApplicationAccess.mockRejectedValueOnce(new RequestError('oidc.access_denied')); + const guard = koaAppAccessControl(mockTenant.libraries); + + await expect(guard(ctx, next)).rejects.toMatchObject({ code: 'oidc.access_denied' }); + expect(next).not.toHaveBeenCalled(); + }); + + it('should throw an error if session is not found', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + session: undefined, + }); + const guard = koaAppAccessControl(mockTenant.libraries); + + await expect(guard(ctx, next)).rejects.toThrow(new RequestError({ code: 'session.not_found' })); + }); +}); diff --git a/packages/core/src/middleware/koa-app-access-control.ts b/packages/core/src/middleware/koa-app-access-control.ts new file mode 100644 index 000000000000..c5e4ff764507 --- /dev/null +++ b/packages/core/src/middleware/koa-app-access-control.ts @@ -0,0 +1,33 @@ +import { type MiddlewareType } from 'koa'; +import { errors } from 'oidc-provider'; + +import RequestError from '#src/errors/RequestError/index.js'; +import type { WithInteractionDetailsContext } from '#src/middleware/koa-interaction-details.js'; +import type Libraries from '#src/tenants/Libraries.js'; +import assertThat from '#src/utils/assert-that.js'; + +export default function koaAppAccessControl< + StateT, + ContextT extends WithInteractionDetailsContext, + ResponseBodyT, +>(libraries: Libraries): MiddlewareType { + return async (ctx, next) => { + const { + params: { client_id: clientId }, + session, + } = ctx.interactionDetails; + + assertThat(session, new RequestError({ code: 'session.not_found' })); + assertThat( + clientId && typeof clientId === 'string', + new errors.InvalidClient('client must be available') + ); + + await libraries.applicationAccessControl.assertUserHasApplicationAccess( + clientId, + session.accountId + ); + + return next(); + }; +} diff --git a/packages/core/src/middleware/koa-auto-consent.ts b/packages/core/src/middleware/koa-auto-consent.ts index 8c626aad3abb..54af64e145a0 100644 --- a/packages/core/src/middleware/koa-auto-consent.ts +++ b/packages/core/src/middleware/koa-auto-consent.ts @@ -1,10 +1,11 @@ import { buildBuiltInApplicationDataForTenant, isBuiltInApplicationId } from '@logto/schemas'; import { type MiddlewareType } from 'koa'; -import { type IRouterParamContext } from 'koa-router'; import type { Provider } from 'oidc-provider'; import { errors } from 'oidc-provider'; import { consent, getMissingScopes } from '#src/libraries/session/index.js'; +import type { WithInteractionDetailsContext } from '#src/middleware/koa-interaction-details.js'; +import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -12,12 +13,17 @@ import assertThat from '#src/utils/assert-that.js'; * Automatically consent for the first party apps. */ -export default function koaAutoConsent( +export default function koaAutoConsent< + StateT, + ContextT extends WithInteractionDetailsContext, + ResponseBodyT, +>( provider: Provider, - query: Queries + query: Queries, + libraries: Libraries ): MiddlewareType { return async (ctx, next) => { - const interactionDetails = await provider.interactionDetails(ctx.req, ctx.res); + const { interactionDetails } = ctx; const { params: { client_id: clientId }, prompt, @@ -44,6 +50,7 @@ export default function koaAutoConsent { const provider = new Provider('https://logto.test'); - const interactionDetails = jest.spyOn(provider, 'interactionDetails'); const checkOneTimeToken = jest.fn().mockResolvedValue({ token: 'token_value', @@ -50,30 +49,34 @@ describe('koaConsentGuard middleware', () => { jest.clearAllMocks(); }); + const createContext = ( + interactionDetails: Partial>> + ) => ({ + ...createContextWithRouteParameters({ + url: `/consent`, + }), + interactionDetails: interactionDetails as Awaited< + ReturnType + >, + }); + it('should throw an error if session is not found', async () => { - // @ts-expect-error - interactionDetails.mockResolvedValue({ + const ctx = createContext({ params: { one_time_token: 'token', login_hint: 'foo@example.com' }, session: undefined, }); - const ctx = createContextWithRouteParameters({ - url: `/consent`, - }); - const guard = koaConsentGuard(provider, mockTenant.libraries, mockTenant.queries); + const guard = koaConsentGuard(mockTenant.libraries, mockTenant.queries); await expect(guard(ctx, next)).rejects.toThrow(new RequestError({ code: 'session.not_found' })); }); it('should not block if token or login_hint are not provided', async () => { - interactionDetails.mockResolvedValue({ + const ctx = createContext({ params: { one_time_token: '', login_hint: '' }, // @ts-expect-error session: { accountId: 'foo' }, }); - const ctx = createContextWithRouteParameters({ - url: `/consent`, - }); - const guard = koaConsentGuard(provider, mockTenant.libraries, mockTenant.queries); + const guard = koaConsentGuard(mockTenant.libraries, mockTenant.queries); await guard(ctx, next); expect(mockTenant.queries.users.findUserById).not.toHaveBeenCalled(); @@ -81,15 +84,12 @@ describe('koaConsentGuard middleware', () => { }); it('should redirect to switch account page if email does not match', async () => { - interactionDetails.mockResolvedValue({ + const ctx = createContext({ params: { one_time_token: 'abcdefg', login_hint: 'bar@example.com' }, // @ts-expect-error session: { accountId: 'bar' }, }); - const ctx = createContextWithRouteParameters({ - url: `/consent`, - }); - const guard = koaConsentGuard(provider, mockTenant.libraries, mockTenant.queries); + const guard = koaConsentGuard(mockTenant.libraries, mockTenant.queries); await guard(ctx, jest.fn()); expect(ctx.redirect).toHaveBeenCalledWith( @@ -98,7 +98,7 @@ describe('koaConsentGuard middleware', () => { }); it('should provision user to organizations on consent, if a valid one-time token is provided and there are organizations in token context', async () => { - interactionDetails.mockResolvedValue({ + const ctx = createContext({ params: { one_time_token: 'token_value', login_hint: 'foo@example.com' }, // @ts-expect-error session: { accountId: 'foo' }, @@ -111,10 +111,7 @@ describe('koaConsentGuard middleware', () => { jitOrganizationIds: ['org_id'], }, }); - const ctx = createContextWithRouteParameters({ - url: `/consent`, - }); - const guard = koaConsentGuard(provider, mockTenant.libraries, mockTenant.queries); + const guard = koaConsentGuard(mockTenant.libraries, mockTenant.queries); await guard(ctx, next); @@ -130,22 +127,19 @@ describe('koaConsentGuard middleware', () => { }); it('should call next middleware if validations pass', async () => { - const ctx = createContextWithRouteParameters({ - url: `/consent`, - }); - interactionDetails.mockResolvedValue({ + const ctx = createContext({ params: { one_time_token: 'token_value', login_hint: 'foo@example.com' }, // @ts-expect-error session: { accountId: 'foo' }, }); - const guard = koaConsentGuard(provider, mockTenant.libraries, mockTenant.queries); + const guard = koaConsentGuard(mockTenant.libraries, mockTenant.queries); await guard(ctx, next); expect(next).toHaveBeenCalled(); }); it('should navigate to `/one-time-token` route with error message in URL params, if the one-time token is not valid', async () => { - interactionDetails.mockResolvedValue({ + const ctx = createContext({ params: { one_time_token: 'token_value', login_hint: 'foo@example.com' }, // @ts-expect-error session: { accountId: 'foo' }, @@ -153,10 +147,7 @@ describe('koaConsentGuard middleware', () => { checkOneTimeToken.mockImplementationOnce(() => { throw new RequestError('one_time_token.token_expired'); }); - const ctx = createContextWithRouteParameters({ - url: `/consent`, - }); - const guard = koaConsentGuard(provider, mockTenant.libraries, mockTenant.queries); + const guard = koaConsentGuard(mockTenant.libraries, mockTenant.queries); await guard(ctx, next); expect(ctx.redirect).toHaveBeenCalledWith( diff --git a/packages/core/src/middleware/koa-consent-guard.ts b/packages/core/src/middleware/koa-consent-guard.ts index 037ef5058910..efdbf7352268 100644 --- a/packages/core/src/middleware/koa-consent-guard.ts +++ b/packages/core/src/middleware/koa-consent-guard.ts @@ -1,9 +1,8 @@ import { experience, OneTimeTokenStatus } from '@logto/schemas'; import { type MiddlewareType } from 'koa'; -import { type IRouterParamContext } from 'koa-router'; -import { type Provider } from 'oidc-provider'; import RequestError from '#src/errors/RequestError/index.js'; +import type { WithInteractionDetailsContext } from '#src/middleware/koa-interaction-details.js'; import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -14,19 +13,14 @@ import assertThat from '#src/utils/assert-that.js'; */ export default function koaConsentGuard< StateT, - ContextT extends IRouterParamContext, + ContextT extends WithInteractionDetailsContext, ResponseBodyT, ->( - provider: Provider, - libraries: Libraries, - queries: Queries -): MiddlewareType { +>(libraries: Libraries, queries: Queries): MiddlewareType { return async (ctx, next) => { - const interactionDetails = await provider.interactionDetails(ctx.req, ctx.res); const { params: { one_time_token: token, login_hint: loginHint }, session, - } = interactionDetails; + } = ctx.interactionDetails; assertThat(session, new RequestError({ code: 'session.not_found' })); diff --git a/packages/core/src/oidc/application-access-control.ts b/packages/core/src/oidc/application-access-control.ts new file mode 100644 index 000000000000..5aea3d684c8f --- /dev/null +++ b/packages/core/src/oidc/application-access-control.ts @@ -0,0 +1,20 @@ +import { errors } from 'oidc-provider'; + +import RequestError from '#src/errors/RequestError/index.js'; +import type Libraries from '#src/tenants/Libraries.js'; + +export const assertUserHasApplicationAccessForOidc = async ( + applicationAccessControl: Libraries['applicationAccessControl'], + applicationId: string, + userId: string +) => { + try { + await applicationAccessControl.assertUserHasApplicationAccess(applicationId, userId); + } catch (error: unknown) { + if (error instanceof RequestError && error.code === 'oidc.access_denied') { + throw new errors.AccessDenied(error.message); + } + + throw error; + } +}; diff --git a/packages/core/src/oidc/grants/index.ts b/packages/core/src/oidc/grants/index.ts index 6285c1b426fa..9bf0dc5a1fd0 100644 --- a/packages/core/src/oidc/grants/index.ts +++ b/packages/core/src/oidc/grants/index.ts @@ -3,13 +3,19 @@ import type { Provider } from 'oidc-provider'; import instance from 'oidc-provider/lib/helpers/weak_cache.js'; import { type EnvSet } from '#src/env-set/index.js'; +import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import * as clientCredentials from './client-credentials.js'; import * as refreshToken from './refresh-token.js'; import * as tokenExchange from './token-exchange/index.js'; -export const registerGrants = (oidc: Provider, envSet: EnvSet, queries: Queries) => { +export const registerGrants = ( + oidc: Provider, + envSet: EnvSet, + queries: Queries, + libraries: Libraries +) => { const { features: { resourceIndicators }, } = instance(oidc).configuration(); @@ -26,7 +32,7 @@ export const registerGrants = (oidc: Provider, envSet: EnvSet, queries: Queries) // Override the default grants oidc.registerGrantType( GrantType.RefreshToken, - refreshToken.buildHandler(envSet, queries), + refreshToken.buildHandler(envSet, queries, libraries.applicationAccessControl), ...getParameterConfig(refreshToken.parameters) ); oidc.registerGrantType( @@ -36,7 +42,7 @@ export const registerGrants = (oidc: Provider, envSet: EnvSet, queries: Queries) ); oidc.registerGrantType( GrantType.TokenExchange, - tokenExchange.buildHandler(envSet, queries), + tokenExchange.buildHandler(envSet, queries, libraries.applicationAccessControl), ...getParameterConfig(tokenExchange.parameters) ); }; diff --git a/packages/core/src/oidc/grants/refresh-token.test.ts b/packages/core/src/oidc/grants/refresh-token.test.ts index 8ec1f459f928..84d8f60da7fc 100644 --- a/packages/core/src/oidc/grants/refresh-token.test.ts +++ b/packages/core/src/oidc/grants/refresh-token.test.ts @@ -3,6 +3,7 @@ import { type KoaContextWithOIDC, errors, type Adapter } from 'oidc-provider'; import Sinon from 'sinon'; import { mockApplication } from '#src/__mocks__/index.js'; +import RequestError from '#src/errors/RequestError/index.js'; import { createOidcContext } from '#src/test-utils/oidc-provider.js'; import { MockTenant } from '#src/test-utils/tenant.js'; @@ -12,9 +13,12 @@ const { jest } = import.meta; // eslint-disable-next-line @typescript-eslint/no-empty-function const noop = async () => {}; +const assertUserHasApplicationAccess = jest.fn(async () => { + await Promise.resolve(); +}); const mockHandler = (tenant = new MockTenant()) => { - return buildHandler(tenant.envSet, tenant.queries); + return buildHandler(tenant.envSet, tenant.queries, { assertUserHasApplicationAccess }); }; const clientId = 'some_client_id'; @@ -159,6 +163,10 @@ afterAll(() => { // check and basic token validation. Comprehensive token validation should be done in // integration tests. describe('refresh token grant', () => { + afterEach(() => { + assertUserHasApplicationAccess.mockClear(); + }); + it('should throw when client is not available', async () => { const ctx = createOidcContext({ ...validOidcContext, client: undefined }); await expect(mockHandler()(ctx, noop)).rejects.toThrow(errors.InvalidClient); @@ -263,6 +271,17 @@ describe('refresh token grant', () => { await expect(mockHandler()(ctx, noop)).rejects.toThrow(errors.InvalidGrant); }); + it('should throw before refresh token rotation when the user has no application access', async () => { + const ctx = createPreparedContext(); + const tenant = new MockTenant(); + const accessError = new RequestError('oidc.access_denied'); + assertUserHasApplicationAccess.mockRejectedValueOnce(accessError); + + await expect(mockHandler(tenant)(ctx, noop)).rejects.toThrow(errors.AccessDenied); + + expect(validRefreshToken.consume).not.toHaveBeenCalled(); + }); + it('should throw if the user is not a member of the organization', async () => { const ctx = createPreparedContext(); const tenant = new MockTenant(); diff --git a/packages/core/src/oidc/grants/refresh-token.ts b/packages/core/src/oidc/grants/refresh-token.ts index 673a4d90dcf9..6d1f0d907170 100644 --- a/packages/core/src/oidc/grants/refresh-token.ts +++ b/packages/core/src/oidc/grants/refresh-token.ts @@ -31,6 +31,8 @@ import validatePresence from 'oidc-provider/lib/helpers/validate_presence.js'; import instance from 'oidc-provider/lib/helpers/weak_cache.js'; import { type EnvSet } from '#src/env-set/index.js'; +import { assertUserHasApplicationAccessForOidc } from '#src/oidc/application-access-control.js'; +import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -63,12 +65,14 @@ const requiredParameters = Object.freeze(['refresh_token']) satisfies ReadonlyAr // We have to disable the rules because the original implementation is written in JavaScript and // uses mutable variables. -/* eslint-disable @silverhand/fp/no-let, @typescript-eslint/no-non-null-assertion, @silverhand/fp/no-mutation, unicorn/no-array-method-this-argument */ -export const buildHandler: ( +/* eslint-disable complexity, @silverhand/fp/no-let, @typescript-eslint/no-non-null-assertion, @silverhand/fp/no-mutation, unicorn/no-array-method-this-argument */ +type Handler = ( envSet: EnvSet, - queries: Queries - // eslint-disable-next-line complexity -) => Parameters[1] = (envSet, queries) => async (ctx, next) => { + queries: Queries, + applicationAccessControl: Libraries['applicationAccessControl'] +) => Parameters[1]; + +export const buildHandler: Handler = (envSet, queries, appAccess) => async (ctx, next) => { const { client, params, requestParamScopes, provider } = ctx.oidc; const { RefreshToken, Account, AccessToken, Grant, IdToken } = provider; @@ -159,6 +163,8 @@ export const buildHandler: ( throw new InvalidGrant('refresh token already used'); } + await assertUserHasApplicationAccessForOidc(appAccess, client.clientId, account.accountId); + const { organizationId } = await checkOrganizationAccess(ctx, queries, account); /* === RFC 0001 === */ @@ -322,4 +328,4 @@ export const buildHandler: ( await next(); }; -/* eslint-enable @silverhand/fp/no-let, @typescript-eslint/no-non-null-assertion, @silverhand/fp/no-mutation, unicorn/no-array-method-this-argument */ +/* eslint-enable complexity, @silverhand/fp/no-let, @typescript-eslint/no-non-null-assertion, @silverhand/fp/no-mutation, unicorn/no-array-method-this-argument */ diff --git a/packages/core/src/oidc/grants/token-exchange/index.test.ts b/packages/core/src/oidc/grants/token-exchange/index.test.ts index f36eda20d4c7..fd8ff46c13b5 100644 --- a/packages/core/src/oidc/grants/token-exchange/index.test.ts +++ b/packages/core/src/oidc/grants/token-exchange/index.test.ts @@ -2,6 +2,7 @@ import { type SubjectToken } from '@logto/schemas'; import { type KoaContextWithOIDC, errors } from 'oidc-provider'; import Sinon from 'sinon'; +import RequestError from '#src/errors/RequestError/index.js'; import { createOidcContext } from '#src/test-utils/oidc-provider.js'; import { MockTenant } from '#src/test-utils/tenant.js'; @@ -28,9 +29,12 @@ const mockQueries = { updateSubjectTokenById, }, }; +const assertUserHasApplicationAccess = jest.fn(async () => { + await Promise.resolve(); +}); const mockTenant = new MockTenant(undefined, mockQueries); const mockHandler = (tenant = mockTenant) => { - return buildHandler(tenant.envSet, tenant.queries); + return buildHandler(tenant.envSet, tenant.queries, { assertUserHasApplicationAccess }); }; const clientId = 'some_client_id'; @@ -102,6 +106,7 @@ describe('token exchange', () => { afterEach(() => { findSubjectToken.mockClear(); updateSubjectTokenById.mockClear(); + assertUserHasApplicationAccess.mockClear(); }); it('should throw when client is not available', async () => { @@ -155,6 +160,21 @@ describe('token exchange', () => { await expect(mockHandler()(ctx, noop)).rejects.toThrow(errors.InvalidGrant); }); + it('should throw before creating token continuation when the user has no application access', async () => { + const ctx = createPreparedContext(); + findSubjectToken.mockResolvedValueOnce(createValidSubjectToken()); + Sinon.stub(ctx.oidc.provider.Account, 'findAccount').resolves({ accountId }); + const tenant = new MockTenant(undefined, mockQueries); + const accessError = new RequestError('oidc.access_denied'); + assertUserHasApplicationAccess.mockRejectedValueOnce(accessError); + const noopStub = Sinon.stub().resolves(); + + await expect(mockHandler(tenant)(ctx, noopStub)).rejects.toThrow(errors.AccessDenied); + + expect(updateSubjectTokenById).not.toHaveBeenCalled(); + expect(noopStub.callCount).toBe(0); + }); + // The handler returns void so we cannot check the return value, and it's also not // straightforward to assert the token is issued correctly. Here we just do the sanity // check and basic token validation. Comprehensive token validation should be done in diff --git a/packages/core/src/oidc/grants/token-exchange/index.ts b/packages/core/src/oidc/grants/token-exchange/index.ts index 3b1126e96267..b6703f259a0c 100644 --- a/packages/core/src/oidc/grants/token-exchange/index.ts +++ b/packages/core/src/oidc/grants/token-exchange/index.ts @@ -14,6 +14,8 @@ import validatePresence from 'oidc-provider/lib/helpers/validate_presence.js'; import instance from 'oidc-provider/lib/helpers/weak_cache.js'; import { type EnvSet } from '#src/env-set/index.js'; +import { assertUserHasApplicationAccessForOidc } from '#src/oidc/application-access-control.js'; +import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -54,10 +56,13 @@ const requiredParameters = Object.freeze([ ] as const) satisfies ReadonlyArray<(typeof parameters)[number]>; /* eslint-disable @silverhand/fp/no-mutation, @typescript-eslint/no-unsafe-assignment */ -export const buildHandler: ( +type Handler = ( envSet: EnvSet, - queries: Queries -) => Parameters['1'] = (envSet, queries) => async (ctx, next) => { + queries: Queries, + applicationAccessControl: Libraries['applicationAccessControl'] +) => Parameters['1']; + +export const buildHandler: Handler = (envSet, queries, appAccess) => async (ctx, next) => { const { client, params, requestParamScopes, provider } = ctx.oidc; const { Account, AccessToken, Grant } = provider; @@ -93,6 +98,8 @@ export const buildHandler: ( ctx.oidc.entity('Account', account); + await assertUserHasApplicationAccessForOidc(appAccess, client.clientId, account.accountId); + // Pre-generate grant ID to avoid a separate DB write just to obtain it. // oidc-provider's BaseModel.save() skips ID generation when jti is already set. const grantId = nanoid(); diff --git a/packages/core/src/oidc/init.test.ts b/packages/core/src/oidc/init.test.ts index ce193e380b36..e718892753dd 100644 --- a/packages/core/src/oidc/init.test.ts +++ b/packages/core/src/oidc/init.test.ts @@ -1,10 +1,11 @@ import assert from 'node:assert'; import { GrantType, type Scope } from '@logto/schemas'; -import type { KoaContextWithOIDC } from 'oidc-provider'; +import { errors, type KoaContextWithOIDC } from 'oidc-provider'; import instance from 'oidc-provider/lib/helpers/weak_cache.js'; import { mockResource } from '#src/__mocks__/index.js'; +import RequestError from '#src/errors/RequestError/index.js'; import { mockEnvSet } from '#src/test-utils/env-set.js'; import { createOidcContext } from '#src/test-utils/oidc-provider.js'; import { MockTenant } from '#src/test-utils/tenant.js'; @@ -168,6 +169,51 @@ describe('oidc provider init', () => { expect(findUserScopesForResourceIndicator).toHaveBeenCalledTimes(2); }); + it('should translate application access denial to an OIDC access denied error when loading existing grant', async () => { + const assertUserHasApplicationAccess = jest + .fn() + .mockRejectedValueOnce(new RequestError('oidc.access_denied')); + const tenant = new MockTenant(); + + tenant.setPartial('libraries', { + applicationAccessControl: { assertUserHasApplicationAccess }, + }); + + const provider = createProvider(tenant); + const configuration = instance(provider).configuration(); + const ctx = createOidcContext({ + provider, + account: { accountId }, + client: { clientId }, + result: { consent: { grantId: 'grant_id' } }, + } as Partial); + + await expect(configuration.loadExistingGrant(ctx)).rejects.toThrow(errors.AccessDenied); + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith(clientId, accountId); + }); + + it('should defer application access check to consent prompt when no existing grant is loaded', async () => { + const assertUserHasApplicationAccess = jest + .fn() + .mockRejectedValueOnce(new RequestError('oidc.access_denied')); + const tenant = new MockTenant(); + + tenant.setPartial('libraries', { + applicationAccessControl: { assertUserHasApplicationAccess }, + }); + + const provider = createProvider(tenant); + const configuration = instance(provider).configuration(); + const ctx = createOidcContext({ + provider, + account: { accountId }, + client: { clientId }, + } as Partial); + + await expect(configuration.loadExistingGrant(ctx)).resolves.toBeUndefined(); + expect(assertUserHasApplicationAccess).not.toHaveBeenCalled(); + }); + it('should reflect updated resource data outside token exchange read path', async () => { const findResourceByIndicator = jest .fn() diff --git a/packages/core/src/oidc/init.ts b/packages/core/src/oidc/init.ts index b39cb44701c8..d8c781daec75 100644 --- a/packages/core/src/oidc/init.ts +++ b/packages/core/src/oidc/init.ts @@ -45,6 +45,7 @@ import { i18next } from '#src/utils/i18n.js'; import { type SubscriptionLibrary } from '../libraries/subscription.js'; import koaTokenUsageGuard from '../middleware/koa-token-usage-guard.js'; +import { assertUserHasApplicationAccessForOidc } from './application-access-control.js'; import defaults from './defaults.js'; import { deviceFlowConfig, defaultDeviceCodeTtl } from './device-flow.js'; import { @@ -263,6 +264,22 @@ export default function initOidc( } }, }, + loadExistingGrant: async (ctx) => { + const { account, client, provider, result, session } = ctx.oidc; + const grantId = result?.consent?.grantId ?? (client && session?.grantIdFor(client.clientId)); + + if (grantId && account && client) { + await assertUserHasApplicationAccessForOidc( + libraries.applicationAccessControl, + client.clientId, + account.accountId + ); + } + + if (grantId) { + return provider.Grant.find(String(grantId)); + } + }, extraParams: Object.values(ExtraParamsKey), extraTokenClaims: async (ctx, token) => { const [tokenExchangeClaims, organizationApiResourceClaims, jwtCustomizedClaims] = @@ -416,7 +433,7 @@ export default function initOidc( }); addOidcEventListeners(tenantId, oidc, queries); - registerGrants(oidc, envSet, queries); + registerGrants(oidc, envSet, queries, libraries); // Provide audit log context for event listeners oidc.use(koaAuditLog(queries)); diff --git a/packages/core/src/routes/interaction/consent/index.ts b/packages/core/src/routes/interaction/consent/index.ts index cba53a3e4fa6..079876e85a37 100644 --- a/packages/core/src/routes/interaction/consent/index.ts +++ b/packages/core/src/routes/interaction/consent/index.ts @@ -179,6 +179,7 @@ export default function consentRoutes( const redirectTo = await consent({ ctx, + applicationAccessControl: libraries.applicationAccessControl, provider, queries, interactionDetails, diff --git a/packages/core/src/tenants/Tenant.ts b/packages/core/src/tenants/Tenant.ts index 44bd2b288b8d..8ee752cf7092 100644 --- a/packages/core/src/tenants/Tenant.ts +++ b/packages/core/src/tenants/Tenant.ts @@ -13,6 +13,7 @@ import { AdminApps, EnvSet, UserApps } from '#src/env-set/index.js'; import { createCloudConnectionLibrary } from '#src/libraries/cloud-connection.js'; import { createConnectorLibrary } from '#src/libraries/connector.js'; import { createLogtoConfigLibrary } from '#src/libraries/logto-config.js'; +import koaAppAccessControl from '#src/middleware/koa-app-access-control.js'; import koaAutoConsent from '#src/middleware/koa-auto-consent.js'; import koaConnectorErrorHandler from '#src/middleware/koa-connector-error-handler.js'; import koaConsoleRedirectProxy from '#src/middleware/koa-console-redirect-proxy.js'; @@ -20,6 +21,7 @@ import koaDeviceFlowShortcut from '#src/middleware/koa-device-flow-shortcut.js'; import koaErrorHandler from '#src/middleware/koa-error-handler.js'; import koaExperienceSsr from '#src/middleware/koa-experience-ssr.js'; import koaI18next from '#src/middleware/koa-i18next.js'; +import koaInteractionDetails from '#src/middleware/koa-interaction-details.js'; import koaOidcErrorHandler from '#src/middleware/koa-oidc-error-handler.js'; import koaSecurityHeaders, { koaExperienceSecurityHeaders, @@ -242,8 +244,10 @@ export default class Tenant implements TenantContext { mount( `/${experience.routes.consent}`, compose([ - koaConsentGuard(provider, libraries, queries), - koaAutoConsent(provider, queries), + koaInteractionDetails(provider), + koaConsentGuard(libraries, queries), + koaAutoConsent(provider, queries, libraries), + koaAppAccessControl(libraries), ]) ), koaSpaProxy({ mountedApps, queries }), diff --git a/packages/integration-tests/src/api/application.ts b/packages/integration-tests/src/api/application.ts index b33296ec761c..a1e979c04630 100644 --- a/packages/integration-tests/src/api/application.ts +++ b/packages/integration-tests/src/api/application.ts @@ -3,6 +3,7 @@ import { type Application, type ApplicationSecret, type CreateApplication, + type ApplicationAccessControl, type CreateApplicationSecret, type OidcClientMetadata, type OrganizationWithRoles, @@ -70,6 +71,16 @@ export const updateApplication = async ( }) .json(); +export const replaceApplicationAccessControl = async ( + applicationId: string, + accessControl: ApplicationAccessControl +) => + authedAdminApi + .put(`applications/${applicationId}/access-control`, { + json: accessControl, + }) + .json(); + export const deleteApplication = async (applicationId: string) => authedAdminApi.delete(`applications/${applicationId}`); diff --git a/packages/integration-tests/src/client/index.ts b/packages/integration-tests/src/client/index.ts index 53cb75ca8664..987faca3d415 100644 --- a/packages/integration-tests/src/client/index.ts +++ b/packages/integration-tests/src/client/index.ts @@ -186,6 +186,10 @@ export default class MockClient { this.rawCookies = cookie.split(';').map((value) => value.trim()); } + public assignRawCookies(cookies: string[]) { + this.rawCookies = cookies; + } + public async send( api: (cookie: string, ...args: Args) => Promise, ...payload: Args diff --git a/packages/integration-tests/src/tests/api/application-access-control.test.ts b/packages/integration-tests/src/tests/api/application-access-control.test.ts new file mode 100644 index 000000000000..e24020a9b680 --- /dev/null +++ b/packages/integration-tests/src/tests/api/application-access-control.test.ts @@ -0,0 +1,187 @@ +import { ReservedScope } from '@logto/core-kit'; +import { + ApplicationType, + createDefaultApplicationAccessControl, + SignInIdentifier, +} from '@logto/schemas'; +import { assert } from '@silverhand/essentials'; +import ky from 'ky'; + +import { oidcApi } from '#src/api/api.js'; +import { + createApplication, + deleteApplication, + replaceApplicationAccessControl, + updateApplication, +} from '#src/api/application.js'; +import { demoAppRedirectUri, logtoUrl } from '#src/constants.js'; +import { initExperienceClient } from '#src/helpers/client.js'; +import { + createDefaultTenantUserWithPassword, + deleteDefaultTenantUser, +} from '#src/helpers/profile.js'; +import { devFeatureTest, generateTestName } from '#src/utils.js'; + +const signInAndGetRefreshToken = async ({ + appId, + username, + password, +}: { + appId: string; + username: string; + password: string; +}) => { + const client = await initExperienceClient({ + config: { + appId, + scopes: [ReservedScope.OfflineAccess], + }, + redirectUri: demoAppRedirectUri, + }); + + const { verificationId } = await client.verifyPassword({ + identifier: { + type: SignInIdentifier.Username, + value: username, + }, + password, + }); + + await client.identifyUser({ verificationId }); + + const { redirectTo } = await client.submitInteraction(); + await client.processSession(redirectTo); + + return client.getRefreshToken(); +}; + +devFeatureTest.describe('application access control OIDC enforcement', () => { + it('denies refresh token exchange after app access is revoked', async () => { + const [{ user, username, password }, allowedUser] = await Promise.all([ + createDefaultTenantUserWithPassword(), + createDefaultTenantUserWithPassword(), + ]); + + const application = await createApplication(generateTestName(), ApplicationType.SPA, { + oidcClientMetadata: { + redirectUris: [demoAppRedirectUri], + postLogoutRedirectUris: [], + }, + customClientMetadata: { + alwaysIssueRefreshToken: true, + }, + }); + + try { + await replaceApplicationAccessControl(application.id, { + ...createDefaultApplicationAccessControl(), + userIds: [user.id], + }); + await updateApplication(application.id, { appLevelAccessControlEnabled: true }); + + const refreshToken = await signInAndGetRefreshToken({ + appId: application.id, + username, + password, + }); + assert(refreshToken, new Error('Refresh token should be issued')); + + await replaceApplicationAccessControl(application.id, { + ...createDefaultApplicationAccessControl(), + userIds: [allowedUser.user.id], + }); + + const response = await oidcApi.post('token', { + body: new URLSearchParams({ + client_id: application.id, + grant_type: 'refresh_token', + refresh_token: refreshToken, + }), + throwHttpErrors: false, + }); + + expect(response.status).toBe(400); + await expect(response.json()).resolves.toMatchObject({ + code: 'oidc.access_denied', + error: 'access_denied', + }); + } finally { + await Promise.allSettled([ + deleteApplication(application.id), + deleteDefaultTenantUser(user.id), + deleteDefaultTenantUser(allowedUser.user.id), + ]); + } + }); + + it('denies the consent step when the signed-in user has no app access', async () => { + const [{ user, username, password }, allowedUser] = await Promise.all([ + createDefaultTenantUserWithPassword(), + createDefaultTenantUserWithPassword(), + ]); + + const application = await createApplication(generateTestName(), ApplicationType.SPA, { + oidcClientMetadata: { + redirectUris: [demoAppRedirectUri], + postLogoutRedirectUris: [], + }, + }); + + try { + await replaceApplicationAccessControl(application.id, { + ...createDefaultApplicationAccessControl(), + userIds: [allowedUser.user.id], + }); + await updateApplication(application.id, { appLevelAccessControlEnabled: true }); + + const client = await initExperienceClient({ + config: { + appId: application.id, + }, + redirectUri: demoAppRedirectUri, + }); + + const { verificationId } = await client.verifyPassword({ + identifier: { + type: SignInIdentifier.Username, + value: username, + }, + password, + }); + + await client.identifyUser({ verificationId }); + + const { redirectTo } = await client.submitInteraction(); + const authResponse = await ky.get(redirectTo, { + headers: { + cookie: client.interactionCookie, + }, + redirect: 'manual', + throwHttpErrors: false, + }); + + expect(authResponse.status).toBe(303); + expect(authResponse.headers.get('location')).toBe(`/consent?app_id=${application.id}`); + client.assignRawCookies(authResponse.headers.getSetCookie()); + + const consentResponse = await ky.get(`${logtoUrl}/consent`, { + headers: { + cookie: client.interactionCookie, + }, + redirect: 'manual', + throwHttpErrors: false, + }); + + expect(consentResponse.status).toBe(400); + await expect(consentResponse.json()).resolves.toMatchObject({ + code: 'oidc.access_denied', + }); + } finally { + await Promise.allSettled([ + deleteApplication(application.id), + deleteDefaultTenantUser(user.id), + deleteDefaultTenantUser(allowedUser.user.id), + ]); + } + }); +}); From 6ee5bc70d2816d9dcbfda1649b07c410ccaa8db4 Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Fri, 29 May 2026 15:14:28 +0800 Subject: [PATCH 2/7] refactor(core): clarify oidc app access checks --- packages/core/src/oidc/application-access-control.ts | 5 +++++ packages/core/src/oidc/init.ts | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/core/src/oidc/application-access-control.ts b/packages/core/src/oidc/application-access-control.ts index 5aea3d684c8f..78952943f831 100644 --- a/packages/core/src/oidc/application-access-control.ts +++ b/packages/core/src/oidc/application-access-control.ts @@ -3,6 +3,11 @@ import { errors } from 'oidc-provider'; import RequestError from '#src/errors/RequestError/index.js'; import type Libraries from '#src/tenants/Libraries.js'; +/** + * Use this wrapper for app-access checks inside oidc-provider hooks and grant handlers. + * The application-access-control library throws Logto `RequestError`s, while oidc-provider + * expects its own errors to produce the correct OAuth response. + */ export const assertUserHasApplicationAccessForOidc = async ( applicationAccessControl: Libraries['applicationAccessControl'], applicationId: string, diff --git a/packages/core/src/oidc/init.ts b/packages/core/src/oidc/init.ts index d8c781daec75..b54786683746 100644 --- a/packages/core/src/oidc/init.ts +++ b/packages/core/src/oidc/init.ts @@ -266,7 +266,8 @@ export default function initOidc( }, loadExistingGrant: async (ctx) => { const { account, client, provider, result, session } = ctx.oidc; - const grantId = result?.consent?.grantId ?? (client && session?.grantIdFor(client.clientId)); + // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- Keep oidc-provider's default loadExistingGrant fallback semantics. + const grantId = result?.consent?.grantId || (client && session?.grantIdFor(client.clientId)); if (grantId && account && client) { await assertUserHasApplicationAccessForOidc( From 4de43f7a6a1904a709623c18561f3afe9ca16d77 Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Fri, 29 May 2026 18:49:40 +0800 Subject: [PATCH 3/7] refactor(core): centralize consent app access checks --- .../src/libraries/session/consent.test.ts | 29 ------------------- .../core/src/libraries/session/consent.ts | 5 ---- .../libraries/session/session-context.test.ts | 6 ---- .../core/src/middleware/koa-auto-consent.ts | 8 +---- .../src/routes/interaction/consent/index.ts | 3 +- packages/core/src/tenants/Tenant.ts | 2 +- 6 files changed, 4 insertions(+), 49 deletions(-) diff --git a/packages/core/src/libraries/session/consent.test.ts b/packages/core/src/libraries/session/consent.test.ts index 99b6db993efe..134941180e22 100644 --- a/packages/core/src/libraries/session/consent.test.ts +++ b/packages/core/src/libraries/session/consent.test.ts @@ -39,11 +39,6 @@ const userQueries = { findUserById: jest.fn(async (): Promise => mockUser), updateUserById: jest.fn(async (..._args: unknown[]) => ({ id: 'id' })), }; -const applicationAccessControl = { - assertUserHasApplicationAccess: jest.fn(async () => { - await Promise.resolve(); - }), -}; // @ts-expect-error const queries: Queries = { users: userQueries }; @@ -66,7 +61,6 @@ describe('consent', () => { const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); await consent({ ctx: context, - applicationAccessControl, provider, queries, interactionDetails: baseInteractionDetails, @@ -98,7 +92,6 @@ describe('consent', () => { await consent({ ctx: context, - applicationAccessControl, provider, queries, interactionDetails, @@ -128,7 +121,6 @@ describe('consent', () => { const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); await consent({ ctx: context, - applicationAccessControl, provider, queries, interactionDetails: baseInteractionDetails, @@ -143,7 +135,6 @@ describe('consent', () => { const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); await consent({ ctx: context, - applicationAccessControl, provider, queries, interactionDetails: baseInteractionDetails, @@ -161,24 +152,4 @@ describe('consent', () => { ); expect(grantAddResourceScope).toHaveBeenCalledWith('resource2', 'resource2_scope1'); }); - - it('should assert user application access before saving grant', async () => { - const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); - - await consent({ - ctx: context, - applicationAccessControl, - provider, - queries, - interactionDetails: baseInteractionDetails, - }); - - expect(applicationAccessControl.assertUserHasApplicationAccess).toHaveBeenCalledWith( - 'clientId', - mockUser.id - ); - expect( - applicationAccessControl.assertUserHasApplicationAccess.mock.invocationCallOrder[0] - ).toBeLessThan(grantSave.mock.invocationCallOrder[0] ?? 0); - }); }); diff --git a/packages/core/src/libraries/session/consent.ts b/packages/core/src/libraries/session/consent.ts index 7785b116d778..de97031dfcf9 100644 --- a/packages/core/src/libraries/session/consent.ts +++ b/packages/core/src/libraries/session/consent.ts @@ -5,7 +5,6 @@ import type { PromptDetail, Provider } from 'oidc-provider'; import { errors } from 'oidc-provider'; import { z } from 'zod'; -import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -87,7 +86,6 @@ const saveInteractionLastSubmissionToSession = async ( export const consent = async ({ ctx, - applicationAccessControl, provider, queries, interactionDetails, @@ -96,7 +94,6 @@ export const consent = async ({ resourceScopesToReject = {}, }: { ctx: Context; - applicationAccessControl: Libraries['applicationAccessControl']; provider: Provider; queries: Queries; interactionDetails: Awaited>; @@ -118,8 +115,6 @@ export const consent = async ({ const { accountId } = session; - await applicationAccessControl.assertUserHasApplicationAccess(clientId, accountId); - const grant = conditional(grantId && (await provider.Grant.find(grantId))) ?? new provider.Grant({ accountId, clientId }); diff --git a/packages/core/src/libraries/session/session-context.test.ts b/packages/core/src/libraries/session/session-context.test.ts index 703f8624a9a7..d4e4935ec29a 100644 --- a/packages/core/src/libraries/session/session-context.test.ts +++ b/packages/core/src/libraries/session/session-context.test.ts @@ -25,11 +25,6 @@ const userQueries = { findUserById: jest.fn(async (): Promise => mockUser), updateUserById: jest.fn(async (..._args: unknown[]) => ({ id: 'id' })), }; -const applicationAccessControl = { - assertUserHasApplicationAccess: jest.fn(async () => { - await Promise.resolve(); - }), -}; const queries = { users: userQueries, @@ -58,7 +53,6 @@ describe('saveInteractionLastSubmissionToSession', () => { await consent({ ctx: context, - applicationAccessControl, provider, queries, interactionDetails, diff --git a/packages/core/src/middleware/koa-auto-consent.ts b/packages/core/src/middleware/koa-auto-consent.ts index 54af64e145a0..29e66fbba9e3 100644 --- a/packages/core/src/middleware/koa-auto-consent.ts +++ b/packages/core/src/middleware/koa-auto-consent.ts @@ -5,7 +5,6 @@ import { errors } from 'oidc-provider'; import { consent, getMissingScopes } from '#src/libraries/session/index.js'; import type { WithInteractionDetailsContext } from '#src/middleware/koa-interaction-details.js'; -import type Libraries from '#src/tenants/Libraries.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -17,11 +16,7 @@ export default function koaAutoConsent< StateT, ContextT extends WithInteractionDetailsContext, ResponseBodyT, ->( - provider: Provider, - query: Queries, - libraries: Libraries -): MiddlewareType { +>(provider: Provider, query: Queries): MiddlewareType { return async (ctx, next) => { const { interactionDetails } = ctx; const { @@ -50,7 +45,6 @@ export default function koaAutoConsent< const redirectTo = await consent({ ctx, - applicationAccessControl: libraries.applicationAccessControl, provider, queries: query, interactionDetails, diff --git a/packages/core/src/routes/interaction/consent/index.ts b/packages/core/src/routes/interaction/consent/index.ts index 079876e85a37..a6b31a63ed99 100644 --- a/packages/core/src/routes/interaction/consent/index.ts +++ b/packages/core/src/routes/interaction/consent/index.ts @@ -17,6 +17,7 @@ import { errors } from 'oidc-provider'; import { z } from 'zod'; import { consent, getMissingScopes } from '#src/libraries/session/index.js'; +import koaAppAccessControl from '#src/middleware/koa-app-access-control.js'; import koaGuard from '#src/middleware/koa-guard.js'; import type { WithInteractionDetailsContext } from '#src/middleware/koa-interaction-details.js'; import type TenantContext from '#src/tenants/TenantContext.js'; @@ -45,6 +46,7 @@ export default function consentRoutes( }), status: [200], }), + koaAppAccessControl(libraries), async (ctx, next) => { const { interactionDetails, @@ -179,7 +181,6 @@ export default function consentRoutes( const redirectTo = await consent({ ctx, - applicationAccessControl: libraries.applicationAccessControl, provider, queries, interactionDetails, diff --git a/packages/core/src/tenants/Tenant.ts b/packages/core/src/tenants/Tenant.ts index 8ee752cf7092..32a14459bfc4 100644 --- a/packages/core/src/tenants/Tenant.ts +++ b/packages/core/src/tenants/Tenant.ts @@ -246,8 +246,8 @@ export default class Tenant implements TenantContext { compose([ koaInteractionDetails(provider), koaConsentGuard(libraries, queries), - koaAutoConsent(provider, queries, libraries), koaAppAccessControl(libraries), + koaAutoConsent(provider, queries), ]) ), koaSpaProxy({ mountedApps, queries }), From aa4d77da2f2de8f46a2996f19fd08c30a07178dc Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Fri, 29 May 2026 23:15:13 +0800 Subject: [PATCH 4/7] refactor(core): dedupe app access checks per interaction --- .../middleware/koa-app-access-control.test.ts | 46 +++++++-- .../src/middleware/koa-app-access-control.ts | 22 +++++ .../src/oidc/application-access-control.ts | 48 +++++++++- packages/core/src/oidc/init.test.ts | 93 ++++++++++++++++++- packages/core/src/oidc/init.ts | 17 +++- 5 files changed, 212 insertions(+), 14 deletions(-) diff --git a/packages/core/src/middleware/koa-app-access-control.test.ts b/packages/core/src/middleware/koa-app-access-control.test.ts index 043261f64ae0..77a91a87f67d 100644 --- a/packages/core/src/middleware/koa-app-access-control.test.ts +++ b/packages/core/src/middleware/koa-app-access-control.test.ts @@ -1,10 +1,14 @@ +import type { Provider } from 'oidc-provider'; + import RequestError from '#src/errors/RequestError/index.js'; +import { markAppLevelAccessControlChecked } from '#src/oidc/application-access-control.js'; import { MockTenant } from '#src/test-utils/tenant.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; import koaAppAccessControl from './koa-app-access-control.js'; const { jest } = import.meta; +type InteractionDetails = Awaited>; describe('koaAppAccessControl middleware', () => { const assertUserHasApplicationAccess = jest.fn(async () => { @@ -19,16 +23,20 @@ describe('koaAppAccessControl middleware', () => { jest.clearAllMocks(); }); - const createContext = ( - interactionDetails: Partial>> - ) => ({ - ...createContextWithRouteParameters({ - url: `/consent`, - }), - interactionDetails: interactionDetails as Awaited< - ReturnType - >, - }); + const createContext = (interactionDetails: Partial) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- minimal interaction details stub for middleware testing + const details = { + persist: jest.fn(), + ...interactionDetails, + } as InteractionDetails; + + return { + ...createContextWithRouteParameters({ + url: `/consent`, + }), + interactionDetails: details, + }; + }; it('should assert application access before next middleware', async () => { const ctx = createContext({ @@ -41,6 +49,23 @@ describe('koaAppAccessControl middleware', () => { await guard(ctx, next); expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); + expect(ctx.interactionDetails.persist).toHaveBeenCalled(); + expect(next).toHaveBeenCalled(); + }); + + it('should skip duplicated application access check in the same interaction', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + // @ts-expect-error + session: { accountId: 'user-id' }, + result: markAppLevelAccessControlChecked(undefined, 'app-id', 'user-id'), + }); + const guard = koaAppAccessControl(mockTenant.libraries); + + await guard(ctx, next); + + expect(assertUserHasApplicationAccess).not.toHaveBeenCalled(); + expect(ctx.interactionDetails.persist).not.toHaveBeenCalled(); expect(next).toHaveBeenCalled(); }); @@ -54,6 +79,7 @@ describe('koaAppAccessControl middleware', () => { const guard = koaAppAccessControl(mockTenant.libraries); await expect(guard(ctx, next)).rejects.toMatchObject({ code: 'oidc.access_denied' }); + expect(ctx.interactionDetails.persist).not.toHaveBeenCalled(); expect(next).not.toHaveBeenCalled(); }); diff --git a/packages/core/src/middleware/koa-app-access-control.ts b/packages/core/src/middleware/koa-app-access-control.ts index c5e4ff764507..eab6cc4f0889 100644 --- a/packages/core/src/middleware/koa-app-access-control.ts +++ b/packages/core/src/middleware/koa-app-access-control.ts @@ -3,6 +3,10 @@ import { errors } from 'oidc-provider'; import RequestError from '#src/errors/RequestError/index.js'; import type { WithInteractionDetailsContext } from '#src/middleware/koa-interaction-details.js'; +import { + hasAppLevelAccessControlChecked, + markAppLevelAccessControlChecked, +} from '#src/oidc/application-access-control.js'; import type Libraries from '#src/tenants/Libraries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -23,11 +27,29 @@ export default function koaAppAccessControl< new errors.InvalidClient('client must be available') ); + if ( + hasAppLevelAccessControlChecked(ctx.interactionDetails.result, clientId, session.accountId) || + hasAppLevelAccessControlChecked( + ctx.interactionDetails.lastSubmission, + clientId, + session.accountId + ) + ) { + return next(); + } + await libraries.applicationAccessControl.assertUserHasApplicationAccess( clientId, session.accountId ); + ctx.interactionDetails.result = markAppLevelAccessControlChecked( + ctx.interactionDetails.result ?? ctx.interactionDetails.lastSubmission, + clientId, + session.accountId + ); + await ctx.interactionDetails.persist(); + return next(); }; } diff --git a/packages/core/src/oidc/application-access-control.ts b/packages/core/src/oidc/application-access-control.ts index 78952943f831..a775077c439b 100644 --- a/packages/core/src/oidc/application-access-control.ts +++ b/packages/core/src/oidc/application-access-control.ts @@ -1,8 +1,54 @@ -import { errors } from 'oidc-provider'; +import { errors, type InteractionResults } from 'oidc-provider'; import RequestError from '#src/errors/RequestError/index.js'; import type Libraries from '#src/tenants/Libraries.js'; +const appLevelAccessControlInteractionResultKey = 'appLevelAccessControl'; + +const isObjectRecord = (value: unknown): value is Record => + typeof value === 'object' && value !== null; + +export const hasAppLevelAccessControlChecked = ( + result: unknown, + applicationId: string, + userId: string +) => { + if (!isObjectRecord(result)) { + return false; + } + + const marker = result[appLevelAccessControlInteractionResultKey]; + + if (!isObjectRecord(marker)) { + return false; + } + + return ( + marker.checked === true && marker.applicationId === applicationId && marker.userId === userId + ); +}; + +export const markAppLevelAccessControlChecked = ( + result: InteractionResults | undefined, + applicationId: string, + userId: string +): InteractionResults => ({ + ...result, + [appLevelAccessControlInteractionResultKey]: { + checked: true, + applicationId, + userId, + }, +}); + +export const markAppLevelAccessControlCheckedForOidcContext = ( + oidc: { result?: InteractionResults }, + applicationId: string, + userId: string +) => { + Reflect.set(oidc, 'result', markAppLevelAccessControlChecked(oidc.result, applicationId, userId)); +}; + /** * Use this wrapper for app-access checks inside oidc-provider hooks and grant handlers. * The application-access-control library throws Logto `RequestError`s, while oidc-provider diff --git a/packages/core/src/oidc/init.test.ts b/packages/core/src/oidc/init.test.ts index e718892753dd..cfb62d8bb862 100644 --- a/packages/core/src/oidc/init.test.ts +++ b/packages/core/src/oidc/init.test.ts @@ -10,6 +10,10 @@ import { mockEnvSet } from '#src/test-utils/env-set.js'; import { createOidcContext } from '#src/test-utils/oidc-provider.js'; import { MockTenant } from '#src/test-utils/tenant.js'; +import { + hasAppLevelAccessControlChecked, + markAppLevelAccessControlChecked, +} from './application-access-control.js'; import initOidc from './init.js'; const { jest } = import.meta; @@ -48,6 +52,18 @@ const createProvider = (tenant: MockTenant) => tenant.subscription ); +const createTestClient = (): KoaContextWithOIDC['oidc']['client'] => { + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- minimal client stub for OIDC context testing + return { clientId } as KoaContextWithOIDC['oidc']['client']; +}; + +const mockGrantFound = (provider: KoaContextWithOIDC['oidc']['provider']) => { + // eslint-disable-next-line @typescript-eslint/consistent-type-assertions -- minimal grant stub for OIDC context testing + const grant = {} as Awaited>; + + return jest.spyOn(provider.Grant, 'find').mockResolvedValueOnce(grant); +}; + const createContext = ( provider: KoaContextWithOIDC['oidc']['provider'], grantType: GrantType, @@ -184,7 +200,7 @@ describe('oidc provider init', () => { const ctx = createOidcContext({ provider, account: { accountId }, - client: { clientId }, + client: createTestClient(), result: { consent: { grantId: 'grant_id' } }, } as Partial); @@ -192,6 +208,81 @@ describe('oidc provider init', () => { expect(assertUserHasApplicationAccess).toHaveBeenCalledWith(clientId, accountId); }); + it('should check application access for consent prompt without existing marker when loading existing grant', async () => { + const assertUserHasApplicationAccess = jest.fn(); + const tenant = new MockTenant(); + + tenant.setPartial('libraries', { + applicationAccessControl: { assertUserHasApplicationAccess }, + }); + + const provider = createProvider(tenant); + const findGrant = mockGrantFound(provider); + const configuration = instance(provider).configuration(); + const ctx = createOidcContext({ + provider, + account: { accountId, claims: async () => ({ sub: accountId }) }, + client: createTestClient(), + prompts: new Set(['consent']), + result: { consent: { grantId: 'grant_id' } }, + } as Partial); + + await expect(configuration.loadExistingGrant(ctx)).resolves.toBeDefined(); + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith(clientId, accountId); + expect(findGrant).toHaveBeenCalledWith('grant_id'); + }); + + it('should skip duplicated application access check when loading existing grant', async () => { + const assertUserHasApplicationAccess = jest.fn(); + const tenant = new MockTenant(); + + tenant.setPartial('libraries', { + applicationAccessControl: { assertUserHasApplicationAccess }, + }); + + const provider = createProvider(tenant); + const findGrant = mockGrantFound(provider); + const configuration = instance(provider).configuration(); + const ctx = createOidcContext({ + provider, + account: { accountId, claims: async () => ({ sub: accountId }) }, + client: createTestClient(), + result: markAppLevelAccessControlChecked( + { consent: { grantId: 'grant_id' } }, + clientId, + accountId + ), + } as Partial); + + await expect(configuration.loadExistingGrant(ctx)).resolves.toBeDefined(); + expect(assertUserHasApplicationAccess).not.toHaveBeenCalled(); + expect(findGrant).toHaveBeenCalledWith('grant_id'); + }); + + it('should mark application access as checked after loading existing grant', async () => { + const assertUserHasApplicationAccess = jest.fn(); + const tenant = new MockTenant(); + + tenant.setPartial('libraries', { + applicationAccessControl: { assertUserHasApplicationAccess }, + }); + + const provider = createProvider(tenant); + const findGrant = mockGrantFound(provider); + const configuration = instance(provider).configuration(); + const ctx = createOidcContext({ + provider, + account: { accountId, claims: async () => ({ sub: accountId }) }, + client: createTestClient(), + result: { consent: { grantId: 'grant_id' } }, + } as Partial); + + await expect(configuration.loadExistingGrant(ctx)).resolves.toBeDefined(); + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith(clientId, accountId); + expect(hasAppLevelAccessControlChecked(ctx.oidc.result, clientId, accountId)).toBe(true); + expect(findGrant).toHaveBeenCalledWith('grant_id'); + }); + it('should defer application access check to consent prompt when no existing grant is loaded', async () => { const assertUserHasApplicationAccess = jest .fn() diff --git a/packages/core/src/oidc/init.ts b/packages/core/src/oidc/init.ts index b54786683746..5967bbfd6233 100644 --- a/packages/core/src/oidc/init.ts +++ b/packages/core/src/oidc/init.ts @@ -45,7 +45,11 @@ import { i18next } from '#src/utils/i18n.js'; import { type SubscriptionLibrary } from '../libraries/subscription.js'; import koaTokenUsageGuard from '../middleware/koa-token-usage-guard.js'; -import { assertUserHasApplicationAccessForOidc } from './application-access-control.js'; +import { + assertUserHasApplicationAccessForOidc, + hasAppLevelAccessControlChecked, + markAppLevelAccessControlCheckedForOidcContext, +} from './application-access-control.js'; import defaults from './defaults.js'; import { deviceFlowConfig, defaultDeviceCodeTtl } from './device-flow.js'; import { @@ -268,13 +272,22 @@ export default function initOidc( const { account, client, provider, result, session } = ctx.oidc; // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing -- Keep oidc-provider's default loadExistingGrant fallback semantics. const grantId = result?.consent?.grantId || (client && session?.grantIdFor(client.clientId)); + const shouldCheckApplicationAccess = + account && + client && + !hasAppLevelAccessControlChecked(result, client.clientId, account.accountId); - if (grantId && account && client) { + if (grantId && account && client && shouldCheckApplicationAccess) { await assertUserHasApplicationAccessForOidc( libraries.applicationAccessControl, client.clientId, account.accountId ); + markAppLevelAccessControlCheckedForOidcContext( + ctx.oidc, + client.clientId, + account.accountId + ); } if (grantId) { From 33eee8bca7ed1db51efeca9f5514e8e29bb1132e Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Mon, 1 Jun 2026 11:45:55 +0800 Subject: [PATCH 5/7] refactor(core): limit app access marker persistence --- .../middleware/koa-app-access-control.test.ts | 49 +++++++++++++++++++ .../src/middleware/koa-app-access-control.ts | 34 ++++++++++--- .../core/src/middleware/koa-auto-consent.ts | 22 +++++---- .../src/routes/interaction/consent/index.ts | 2 +- packages/core/src/tenants/Tenant.ts | 12 ++++- 5 files changed, 100 insertions(+), 19 deletions(-) diff --git a/packages/core/src/middleware/koa-app-access-control.test.ts b/packages/core/src/middleware/koa-app-access-control.test.ts index 77a91a87f67d..ea2d411ce0d6 100644 --- a/packages/core/src/middleware/koa-app-access-control.test.ts +++ b/packages/core/src/middleware/koa-app-access-control.test.ts @@ -48,11 +48,60 @@ describe('koaAppAccessControl middleware', () => { await guard(ctx, next); + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); + expect(ctx.interactionDetails.persist).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalled(); + }); + + it('should mark application access checked when configured', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + // @ts-expect-error + session: { accountId: 'user-id' }, + }); + const guard = koaAppAccessControl(mockTenant.libraries, { markInteractionResult: true }); + + await guard(ctx, next); + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); expect(ctx.interactionDetails.persist).toHaveBeenCalled(); expect(next).toHaveBeenCalled(); }); + it('should mark application access checked when the mark predicate returns true', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + // @ts-expect-error + session: { accountId: 'user-id' }, + }); + const guard = koaAppAccessControl(mockTenant.libraries, { + markInteractionResult: async () => true, + }); + + await guard(ctx, next); + + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); + expect(ctx.interactionDetails.persist).toHaveBeenCalled(); + expect(next).toHaveBeenCalled(); + }); + + it('should not mark application access checked when the mark predicate returns false', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + // @ts-expect-error + session: { accountId: 'user-id' }, + }); + const guard = koaAppAccessControl(mockTenant.libraries, { + markInteractionResult: async () => false, + }); + + await guard(ctx, next); + + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); + expect(ctx.interactionDetails.persist).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalled(); + }); + it('should skip duplicated application access check in the same interaction', async () => { const ctx = createContext({ params: { client_id: 'app-id' }, diff --git a/packages/core/src/middleware/koa-app-access-control.ts b/packages/core/src/middleware/koa-app-access-control.ts index eab6cc4f0889..b033f2526dcd 100644 --- a/packages/core/src/middleware/koa-app-access-control.ts +++ b/packages/core/src/middleware/koa-app-access-control.ts @@ -10,11 +10,29 @@ import { import type Libraries from '#src/tenants/Libraries.js'; import assertThat from '#src/utils/assert-that.js'; +type KoaAppAccessControlOptions = { + readonly markInteractionResult?: boolean | ((ctx: ContextT) => boolean | Promise); +}; + +const shouldMarkInteractionResult = async ( + ctx: ContextT, + option: KoaAppAccessControlOptions['markInteractionResult'] +) => { + if (typeof option === 'function') { + return option(ctx); + } + + return option === true; +}; + export default function koaAppAccessControl< StateT, ContextT extends WithInteractionDetailsContext, ResponseBodyT, ->(libraries: Libraries): MiddlewareType { +>( + libraries: Libraries, + options: KoaAppAccessControlOptions = {} +): MiddlewareType { return async (ctx, next) => { const { params: { client_id: clientId }, @@ -43,12 +61,14 @@ export default function koaAppAccessControl< session.accountId ); - ctx.interactionDetails.result = markAppLevelAccessControlChecked( - ctx.interactionDetails.result ?? ctx.interactionDetails.lastSubmission, - clientId, - session.accountId - ); - await ctx.interactionDetails.persist(); + if (await shouldMarkInteractionResult(ctx, options.markInteractionResult)) { + ctx.interactionDetails.result = markAppLevelAccessControlChecked( + ctx.interactionDetails.result ?? ctx.interactionDetails.lastSubmission, + clientId, + session.accountId + ); + await ctx.interactionDetails.persist(); + } return next(); }; diff --git a/packages/core/src/middleware/koa-auto-consent.ts b/packages/core/src/middleware/koa-auto-consent.ts index 29e66fbba9e3..cf79f668e5b2 100644 --- a/packages/core/src/middleware/koa-auto-consent.ts +++ b/packages/core/src/middleware/koa-auto-consent.ts @@ -12,6 +12,18 @@ import assertThat from '#src/utils/assert-that.js'; * Automatically consent for the first party apps. */ +export const shouldAutoConsentApplication = async (clientId: string, query: Queries) => { + const { + applications: { findApplicationById }, + } = query; + + const application = isBuiltInApplicationId(clientId) + ? buildBuiltInApplicationDataForTenant('', clientId) + : await findApplicationById(clientId); + + return !application.isThirdParty; +}; + export default function koaAutoConsent< StateT, ContextT extends WithInteractionDetailsContext, @@ -24,20 +36,12 @@ export default function koaAutoConsent< prompt, } = interactionDetails; - const { - applications: { findApplicationById }, - } = query; - assertThat( clientId && typeof clientId === 'string', new errors.InvalidClient('client must be available') ); - const application = isBuiltInApplicationId(clientId) - ? buildBuiltInApplicationDataForTenant('', clientId) - : await findApplicationById(clientId); - - const shouldAutoConsent = !application.isThirdParty; + const shouldAutoConsent = await shouldAutoConsentApplication(clientId, query); if (shouldAutoConsent) { const { missingOIDCScope: missingOIDCScopes, missingResourceScopes: resourceScopesToGrant } = diff --git a/packages/core/src/routes/interaction/consent/index.ts b/packages/core/src/routes/interaction/consent/index.ts index a6b31a63ed99..70b7eda0b241 100644 --- a/packages/core/src/routes/interaction/consent/index.ts +++ b/packages/core/src/routes/interaction/consent/index.ts @@ -46,7 +46,7 @@ export default function consentRoutes( }), status: [200], }), - koaAppAccessControl(libraries), + koaAppAccessControl(libraries, { markInteractionResult: true }), async (ctx, next) => { const { interactionDetails, diff --git a/packages/core/src/tenants/Tenant.ts b/packages/core/src/tenants/Tenant.ts index 32a14459bfc4..7c44f408ec44 100644 --- a/packages/core/src/tenants/Tenant.ts +++ b/packages/core/src/tenants/Tenant.ts @@ -14,7 +14,7 @@ import { createCloudConnectionLibrary } from '#src/libraries/cloud-connection.js import { createConnectorLibrary } from '#src/libraries/connector.js'; import { createLogtoConfigLibrary } from '#src/libraries/logto-config.js'; import koaAppAccessControl from '#src/middleware/koa-app-access-control.js'; -import koaAutoConsent from '#src/middleware/koa-auto-consent.js'; +import koaAutoConsent, { shouldAutoConsentApplication } from '#src/middleware/koa-auto-consent.js'; import koaConnectorErrorHandler from '#src/middleware/koa-connector-error-handler.js'; import koaConsoleRedirectProxy from '#src/middleware/koa-console-redirect-proxy.js'; import koaDeviceFlowShortcut from '#src/middleware/koa-device-flow-shortcut.js'; @@ -246,7 +246,15 @@ export default class Tenant implements TenantContext { compose([ koaInteractionDetails(provider), koaConsentGuard(libraries, queries), - koaAppAccessControl(libraries), + koaAppAccessControl(libraries, { + markInteractionResult: async (ctx) => { + const { client_id: clientId } = ctx.interactionDetails.params; + + return ( + typeof clientId === 'string' && shouldAutoConsentApplication(clientId, queries) + ); + }, + }), koaAutoConsent(provider, queries), ]) ), From 9e1b502a4a5c6ab0695820a50f5c1c3322a2468a Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Mon, 1 Jun 2026 21:22:04 +0800 Subject: [PATCH 6/7] refactor(core): write app access marker with consent result --- .../src/libraries/session/consent.test.ts | 31 ++++++++++++ .../core/src/libraries/session/consent.ts | 15 +++++- .../middleware/koa-app-access-control.test.ts | 49 ------------------- .../src/middleware/koa-app-access-control.ts | 34 +------------ .../core/src/middleware/koa-auto-consent.ts | 3 +- .../src/oidc/grants/token-exchange/index.ts | 2 +- .../src/routes/interaction/consent/index.ts | 3 +- packages/core/src/tenants/Tenant.ts | 12 +---- 8 files changed, 54 insertions(+), 95 deletions(-) diff --git a/packages/core/src/libraries/session/consent.test.ts b/packages/core/src/libraries/session/consent.test.ts index 134941180e22..aa1cd6617450 100644 --- a/packages/core/src/libraries/session/consent.test.ts +++ b/packages/core/src/libraries/session/consent.test.ts @@ -3,6 +3,7 @@ import { generateStandardId } from '@logto/shared'; import type { Provider } from 'oidc-provider'; import { mockUser } from '#src/__mocks__/user.js'; +import { markAppLevelAccessControlChecked } from '#src/oidc/application-access-control.js'; import type Queries from '#src/tenants/Queries.js'; import { GrantMock, createMockProvider } from '#src/test-utils/oidc-provider.js'; import { createContextWithRouteParameters } from '#src/utils/test-utils.js'; @@ -112,6 +113,36 @@ describe('consent', () => { ); }); + it('should mark app-level access control checked when configured', async () => { + const provider = createMockProvider(jest.fn().mockResolvedValue(baseInteractionDetails), Grant); + await consent({ + ctx: context, + provider, + queries, + interactionDetails: baseInteractionDetails, + markAppLevelAccessControlChecked: true, + }); + + expect(provider.interactionResult).toHaveBeenCalledWith( + context.req, + context.res, + { + ...baseInteractionDetails.result, + ...markAppLevelAccessControlChecked( + { + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + consent: { grantId: expect.any(String) }, + }, + 'clientId', + mockUser.id + ), + }, + { + mergeWithLastSubmission: true, + } + ); + }); + it('should save first consented app id', async () => { userQueries.findUserById.mockImplementationOnce(async () => ({ ...mockUser, diff --git a/packages/core/src/libraries/session/consent.ts b/packages/core/src/libraries/session/consent.ts index de97031dfcf9..9e800a5ce63f 100644 --- a/packages/core/src/libraries/session/consent.ts +++ b/packages/core/src/libraries/session/consent.ts @@ -5,6 +5,7 @@ import type { PromptDetail, Provider } from 'oidc-provider'; import { errors } from 'oidc-provider'; import { z } from 'zod'; +import { markAppLevelAccessControlChecked } from '#src/oidc/application-access-control.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; @@ -92,6 +93,7 @@ export const consent = async ({ missingOIDCScopes = [], resourceScopesToGrant = {}, resourceScopesToReject = {}, + markAppLevelAccessControlChecked: shouldMarkAppLevelAccessControlChecked = false, }: { ctx: Context; provider: Provider; @@ -100,6 +102,7 @@ export const consent = async ({ missingOIDCScopes?: string[]; resourceScopesToGrant?: Record; resourceScopesToReject?: Record; + markAppLevelAccessControlChecked?: boolean; }) => { const { session, @@ -138,7 +141,17 @@ export const consent = async ({ } const finalGrantId = await grant.save(); + const result = { + consent: { grantId: finalGrantId }, + }; // Configure consent - return updateInteractionResult(ctx, provider, { consent: { grantId: finalGrantId } }, true); + return updateInteractionResult( + ctx, + provider, + shouldMarkAppLevelAccessControlChecked + ? markAppLevelAccessControlChecked(result, clientId, accountId) + : result, + true + ); }; diff --git a/packages/core/src/middleware/koa-app-access-control.test.ts b/packages/core/src/middleware/koa-app-access-control.test.ts index ea2d411ce0d6..29961f47eaaa 100644 --- a/packages/core/src/middleware/koa-app-access-control.test.ts +++ b/packages/core/src/middleware/koa-app-access-control.test.ts @@ -53,55 +53,6 @@ describe('koaAppAccessControl middleware', () => { expect(next).toHaveBeenCalled(); }); - it('should mark application access checked when configured', async () => { - const ctx = createContext({ - params: { client_id: 'app-id' }, - // @ts-expect-error - session: { accountId: 'user-id' }, - }); - const guard = koaAppAccessControl(mockTenant.libraries, { markInteractionResult: true }); - - await guard(ctx, next); - - expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); - expect(ctx.interactionDetails.persist).toHaveBeenCalled(); - expect(next).toHaveBeenCalled(); - }); - - it('should mark application access checked when the mark predicate returns true', async () => { - const ctx = createContext({ - params: { client_id: 'app-id' }, - // @ts-expect-error - session: { accountId: 'user-id' }, - }); - const guard = koaAppAccessControl(mockTenant.libraries, { - markInteractionResult: async () => true, - }); - - await guard(ctx, next); - - expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); - expect(ctx.interactionDetails.persist).toHaveBeenCalled(); - expect(next).toHaveBeenCalled(); - }); - - it('should not mark application access checked when the mark predicate returns false', async () => { - const ctx = createContext({ - params: { client_id: 'app-id' }, - // @ts-expect-error - session: { accountId: 'user-id' }, - }); - const guard = koaAppAccessControl(mockTenant.libraries, { - markInteractionResult: async () => false, - }); - - await guard(ctx, next); - - expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); - expect(ctx.interactionDetails.persist).not.toHaveBeenCalled(); - expect(next).toHaveBeenCalled(); - }); - it('should skip duplicated application access check in the same interaction', async () => { const ctx = createContext({ params: { client_id: 'app-id' }, diff --git a/packages/core/src/middleware/koa-app-access-control.ts b/packages/core/src/middleware/koa-app-access-control.ts index b033f2526dcd..840166af848e 100644 --- a/packages/core/src/middleware/koa-app-access-control.ts +++ b/packages/core/src/middleware/koa-app-access-control.ts @@ -3,36 +3,15 @@ import { errors } from 'oidc-provider'; import RequestError from '#src/errors/RequestError/index.js'; import type { WithInteractionDetailsContext } from '#src/middleware/koa-interaction-details.js'; -import { - hasAppLevelAccessControlChecked, - markAppLevelAccessControlChecked, -} from '#src/oidc/application-access-control.js'; +import { hasAppLevelAccessControlChecked } from '#src/oidc/application-access-control.js'; import type Libraries from '#src/tenants/Libraries.js'; import assertThat from '#src/utils/assert-that.js'; -type KoaAppAccessControlOptions = { - readonly markInteractionResult?: boolean | ((ctx: ContextT) => boolean | Promise); -}; - -const shouldMarkInteractionResult = async ( - ctx: ContextT, - option: KoaAppAccessControlOptions['markInteractionResult'] -) => { - if (typeof option === 'function') { - return option(ctx); - } - - return option === true; -}; - export default function koaAppAccessControl< StateT, ContextT extends WithInteractionDetailsContext, ResponseBodyT, ->( - libraries: Libraries, - options: KoaAppAccessControlOptions = {} -): MiddlewareType { +>(libraries: Libraries): MiddlewareType { return async (ctx, next) => { const { params: { client_id: clientId }, @@ -61,15 +40,6 @@ export default function koaAppAccessControl< session.accountId ); - if (await shouldMarkInteractionResult(ctx, options.markInteractionResult)) { - ctx.interactionDetails.result = markAppLevelAccessControlChecked( - ctx.interactionDetails.result ?? ctx.interactionDetails.lastSubmission, - clientId, - session.accountId - ); - await ctx.interactionDetails.persist(); - } - return next(); }; } diff --git a/packages/core/src/middleware/koa-auto-consent.ts b/packages/core/src/middleware/koa-auto-consent.ts index cf79f668e5b2..858f3ad5a1ef 100644 --- a/packages/core/src/middleware/koa-auto-consent.ts +++ b/packages/core/src/middleware/koa-auto-consent.ts @@ -12,7 +12,7 @@ import assertThat from '#src/utils/assert-that.js'; * Automatically consent for the first party apps. */ -export const shouldAutoConsentApplication = async (clientId: string, query: Queries) => { +const shouldAutoConsentApplication = async (clientId: string, query: Queries) => { const { applications: { findApplicationById }, } = query; @@ -54,6 +54,7 @@ export default function koaAutoConsent< interactionDetails, missingOIDCScopes, resourceScopesToGrant, + markAppLevelAccessControlChecked: true, }); ctx.redirect(redirectTo); diff --git a/packages/core/src/oidc/grants/token-exchange/index.ts b/packages/core/src/oidc/grants/token-exchange/index.ts index b6703f259a0c..409f257f1879 100644 --- a/packages/core/src/oidc/grants/token-exchange/index.ts +++ b/packages/core/src/oidc/grants/token-exchange/index.ts @@ -60,7 +60,7 @@ type Handler = ( envSet: EnvSet, queries: Queries, applicationAccessControl: Libraries['applicationAccessControl'] -) => Parameters['1']; +) => Parameters[1]; export const buildHandler: Handler = (envSet, queries, appAccess) => async (ctx, next) => { const { client, params, requestParamScopes, provider } = ctx.oidc; diff --git a/packages/core/src/routes/interaction/consent/index.ts b/packages/core/src/routes/interaction/consent/index.ts index 70b7eda0b241..dc1d1b391d66 100644 --- a/packages/core/src/routes/interaction/consent/index.ts +++ b/packages/core/src/routes/interaction/consent/index.ts @@ -46,7 +46,7 @@ export default function consentRoutes( }), status: [200], }), - koaAppAccessControl(libraries, { markInteractionResult: true }), + koaAppAccessControl(libraries), async (ctx, next) => { const { interactionDetails, @@ -187,6 +187,7 @@ export default function consentRoutes( missingOIDCScopes: missingOIDCScope, resourceScopesToGrant, resourceScopesToReject, + markAppLevelAccessControlChecked: true, }); ctx.body = { redirectTo }; diff --git a/packages/core/src/tenants/Tenant.ts b/packages/core/src/tenants/Tenant.ts index 7c44f408ec44..32a14459bfc4 100644 --- a/packages/core/src/tenants/Tenant.ts +++ b/packages/core/src/tenants/Tenant.ts @@ -14,7 +14,7 @@ import { createCloudConnectionLibrary } from '#src/libraries/cloud-connection.js import { createConnectorLibrary } from '#src/libraries/connector.js'; import { createLogtoConfigLibrary } from '#src/libraries/logto-config.js'; import koaAppAccessControl from '#src/middleware/koa-app-access-control.js'; -import koaAutoConsent, { shouldAutoConsentApplication } from '#src/middleware/koa-auto-consent.js'; +import koaAutoConsent from '#src/middleware/koa-auto-consent.js'; import koaConnectorErrorHandler from '#src/middleware/koa-connector-error-handler.js'; import koaConsoleRedirectProxy from '#src/middleware/koa-console-redirect-proxy.js'; import koaDeviceFlowShortcut from '#src/middleware/koa-device-flow-shortcut.js'; @@ -246,15 +246,7 @@ export default class Tenant implements TenantContext { compose([ koaInteractionDetails(provider), koaConsentGuard(libraries, queries), - koaAppAccessControl(libraries, { - markInteractionResult: async (ctx) => { - const { client_id: clientId } = ctx.interactionDetails.params; - - return ( - typeof clientId === 'string' && shouldAutoConsentApplication(clientId, queries) - ); - }, - }), + koaAppAccessControl(libraries), koaAutoConsent(provider, queries), ]) ), From a582d6970dc168027b24b8f1b574081e56018759 Mon Sep 17 00:00:00 2001 From: Charles Zhao Date: Tue, 2 Jun 2026 10:55:56 +0800 Subject: [PATCH 7/7] fix(core): recheck app access on consent submit --- .../middleware/koa-app-access-control.test.ts | 16 ++++++++++++++++ .../src/middleware/koa-app-access-control.ts | 7 +------ packages/core/src/oidc/init.ts | 2 +- 3 files changed, 18 insertions(+), 7 deletions(-) diff --git a/packages/core/src/middleware/koa-app-access-control.test.ts b/packages/core/src/middleware/koa-app-access-control.test.ts index 29961f47eaaa..6a6b67585481 100644 --- a/packages/core/src/middleware/koa-app-access-control.test.ts +++ b/packages/core/src/middleware/koa-app-access-control.test.ts @@ -69,6 +69,22 @@ describe('koaAppAccessControl middleware', () => { expect(next).toHaveBeenCalled(); }); + it('should not skip application access check with last submission marker', async () => { + const ctx = createContext({ + params: { client_id: 'app-id' }, + // @ts-expect-error + session: { accountId: 'user-id' }, + lastSubmission: markAppLevelAccessControlChecked(undefined, 'app-id', 'user-id'), + }); + const guard = koaAppAccessControl(mockTenant.libraries); + + await guard(ctx, next); + + expect(assertUserHasApplicationAccess).toHaveBeenCalledWith('app-id', 'user-id'); + expect(ctx.interactionDetails.persist).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalled(); + }); + it('should throw when access is denied', async () => { const ctx = createContext({ params: { client_id: 'app-id' }, diff --git a/packages/core/src/middleware/koa-app-access-control.ts b/packages/core/src/middleware/koa-app-access-control.ts index 840166af848e..9899db92693e 100644 --- a/packages/core/src/middleware/koa-app-access-control.ts +++ b/packages/core/src/middleware/koa-app-access-control.ts @@ -25,12 +25,7 @@ export default function koaAppAccessControl< ); if ( - hasAppLevelAccessControlChecked(ctx.interactionDetails.result, clientId, session.accountId) || - hasAppLevelAccessControlChecked( - ctx.interactionDetails.lastSubmission, - clientId, - session.accountId - ) + hasAppLevelAccessControlChecked(ctx.interactionDetails.result, clientId, session.accountId) ) { return next(); } diff --git a/packages/core/src/oidc/init.ts b/packages/core/src/oidc/init.ts index 5967bbfd6233..65560f9a8b37 100644 --- a/packages/core/src/oidc/init.ts +++ b/packages/core/src/oidc/init.ts @@ -277,7 +277,7 @@ export default function initOidc( client && !hasAppLevelAccessControlChecked(result, client.clientId, account.accountId); - if (grantId && account && client && shouldCheckApplicationAccess) { + if (grantId && shouldCheckApplicationAccess) { await assertUserHasApplicationAccessForOidc( libraries.applicationAccessControl, client.clientId,