From 362ecd2c60356a4d0526c46148a0f8071cb0e0ef Mon Sep 17 00:00:00 2001 From: wangsijie Date: Tue, 9 Jul 2024 15:04:31 +0800 Subject: [PATCH] refactor(core): refactor organizations in grants --- .../src/oidc/grants/client-credentials.ts | 28 ++--- .../src/oidc/grants/refresh-token.test.ts | 40 ++++-- .../core/src/oidc/grants/refresh-token.ts | 102 +++------------- .../src/oidc/grants/token-exchange/index.ts | 35 +----- packages/core/src/oidc/grants/utils.ts | 114 +++++++++++++++++- .../api/oidc/refresh-token-grant.test.ts | 18 +-- 6 files changed, 183 insertions(+), 154 deletions(-) diff --git a/packages/core/src/oidc/grants/client-credentials.ts b/packages/core/src/oidc/grants/client-credentials.ts index dab884391d25..91f61b745c9d 100644 --- a/packages/core/src/oidc/grants/client-credentials.ts +++ b/packages/core/src/oidc/grants/client-credentials.ts @@ -19,7 +19,6 @@ * The commit hash of the original file is `0c52469f08b0a4a1854d90a96546a3f7aa090e5e`. */ -import { buildOrganizationUrn } from '@logto/core-kit'; import { cond } from '@silverhand/essentials'; import type Provider from 'oidc-provider'; import { errors } from 'oidc-provider'; @@ -30,9 +29,7 @@ import { type EnvSet } from '#src/env-set/index.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; -import { getSharedResourceServerData, reversedResourceAccessTokenTtl } from '../resource.js'; - -import { handleDPoP } from './utils.js'; +import { handleDPoP, handleOrganizationToken } from './utils.js'; const { AccessDenied, InvalidClient, InvalidGrant, InvalidScope, InvalidTarget } = errors; @@ -130,26 +127,17 @@ export const buildHandler: ( // If it's present, the flow falls into the `checkResource` and `if (resourceServer)` block above. if (organizationId && !resourceServer) { /* === RFC 0006 === */ - const audience = buildOrganizationUrn(organizationId); const availableScopes = await queries.organizations.relations.appsRoles .getApplicationScopes(organizationId, client.clientId) .then((scope) => scope.map(({ name }) => name)); - /** The intersection of the available scopes and the requested scopes. */ - const issuedScopes = availableScopes.filter((scope) => scopes.includes(scope)).join(' '); - - token.aud = audience; - // Note: the original implementation uses `new provider.ResourceServer` to create the resource - // server. But it's not available in the typings. The class is actually very simple and holds - // no provider-specific context. So we just create the object manually. - // See https://github.com/panva/node-oidc-provider/blob/cf2069cbb31a6a855876e95157372d25dde2511c/lib/helpers/resource_server.js - token.resourceServer = { - ...getSharedResourceServerData(envSet), - accessTokenTTL: reversedResourceAccessTokenTtl, - audience, - scope: availableScopes.join(' '), - }; - token.scope = issuedScopes; + await handleOrganizationToken({ + envSet, + availableScopes, + accessToken: token, + organizationId, + scope: new Set(scopes), + }); /* === End RFC 0006 === */ } diff --git a/packages/core/src/oidc/grants/refresh-token.test.ts b/packages/core/src/oidc/grants/refresh-token.test.ts index 69d8e21b77f2..8ec1f459f928 100644 --- a/packages/core/src/oidc/grants/refresh-token.test.ts +++ b/packages/core/src/oidc/grants/refresh-token.test.ts @@ -191,16 +191,6 @@ describe('refresh token grant', () => { ); }); - it('should throw when refresh token has no organization scope', async () => { - const ctx = createOidcContext(validOidcContext); - stubRefreshToken(ctx, { - scopes: new Set(), - }); - await expect(mockHandler()(ctx, noop)).rejects.toMatchError( - new errors.InsufficientScope('refresh token missing required scope', UserScope.Organizations) - ); - }); - it('should throw when refresh token has no grant id or the grant cannot be found', async () => { const ctx = createOidcContext(validOidcContext); const findRefreshToken = stubRefreshToken(ctx, { @@ -311,6 +301,36 @@ describe('refresh token grant', () => { ); }); + it('should throw when refresh token has no organization scope', async () => { + const ctx = createOidcContext({ + ...validOidcContext, + params: { + ...validOidcContext.params, + scope: '', + }, + }); + const tenant = new MockTenant(); + stubRefreshToken(ctx, { + scopes: new Set(), + }); + stubGrant(ctx); + Sinon.stub(tenant.queries.organizations.relations.users, 'exists').resolves(true); + Sinon.stub(tenant.queries.applications, 'findApplicationById').resolves(mockApplication); + Sinon.stub(tenant.queries.organizations.relations.usersRoles, 'getUserScopes').resolves([ + { tenantId: 'default', id: 'foo', name: 'foo', description: 'foo' }, + { tenantId: 'default', id: 'bar', name: 'bar', description: 'bar' }, + { tenantId: 'default', id: 'baz', name: 'baz', description: 'baz' }, + ]); + Sinon.stub(tenant.queries.organizations, 'getMfaStatus').resolves({ + isMfaRequired: false, + hasMfaConfigured: false, + }); + + await expect(mockHandler(tenant)(ctx, noop)).rejects.toMatchError( + new errors.InsufficientScope('refresh token missing required scope', UserScope.Organizations) + ); + }); + it('should not explode when everything looks fine', 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 e52e2a8eb20e..b71c214872d6 100644 --- a/packages/core/src/oidc/grants/refresh-token.ts +++ b/packages/core/src/oidc/grants/refresh-token.ts @@ -19,8 +19,8 @@ * The commit hash of the original file is `cf2069cbb31a6a855876e95157372d25dde2511c`. */ -import { UserScope, buildOrganizationUrn } from '@logto/core-kit'; -import { isKeyInObject, cond } from '@silverhand/essentials'; +import { UserScope } from '@logto/core-kit'; +import { isKeyInObject } from '@silverhand/essentials'; import type Provider from 'oidc-provider'; import { errors } from 'oidc-provider'; import difference from 'oidc-provider/lib/helpers/_/difference.js'; @@ -34,14 +34,7 @@ import { type EnvSet } from '#src/env-set/index.js'; import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; -import { - getSharedResourceServerData, - isThirdPartyApplication, - reversedResourceAccessTokenTtl, - isOrganizationConsentedToApplication, -} from '../resource.js'; - -import { handleDPoP } from './utils.js'; +import { checkOrganizationAccess, handleDPoP, handleOrganizationToken } from './utils.js'; const { InvalidClient, InvalidGrant, InvalidScope, InsufficientScope, AccessDenied } = errors; @@ -72,7 +65,7 @@ export const buildHandler: ( // eslint-disable-next-line complexity ) => Parameters[1] = (envSet, queries) => async (ctx, next) => { const { client, params, requestParamScopes, provider } = ctx.oidc; - const { RefreshToken, Account, AccessToken, Grant, ReplayDetection, IdToken } = provider; + const { RefreshToken, Account, AccessToken, Grant, IdToken } = provider; assertThat(params, new InvalidGrant('parameters must be available')); assertThat(client, new InvalidClient('client must be available')); @@ -83,11 +76,7 @@ export const buildHandler: ( const { rotateRefreshToken, conformIdTokenClaims, - features: { - mTLS: { getCertificate }, - userinfo, - resourceIndicators, - }, + features: { userinfo, resourceIndicators }, } = providerInstance.configuration(); // @gao: I believe the presence of the param is validated by required parameters of this grant. @@ -107,18 +96,6 @@ export const buildHandler: ( throw new InvalidGrant('refresh token is expired'); } - /* === RFC 0001 === */ - // The value type is `unknown`, which will swallow other type inferences. So we have to cast it - // to `Boolean` first. - const organizationId = cond(Boolean(params.organization_id) && String(params.organization_id)); - if ( - organizationId && // Validate if the refresh token has the required scope from RFC 0001. - !refreshToken.scopes.has(UserScope.Organizations) - ) { - throw new InsufficientScope('refresh token missing required scope', UserScope.Organizations); - } - /* === End RFC 0001 === */ - if (!refreshToken.grantId) { throw new InvalidGrant('grantId not found'); } @@ -177,45 +154,14 @@ export const buildHandler: ( throw new InvalidGrant('refresh token already used'); } - /* === RFC 0001 === */ - if (organizationId) { - // Check membership - if ( - !(await queries.organizations.relations.users.exists({ - organizationId, - userId: account.accountId, - })) - ) { - const error = new AccessDenied('user is not a member of the organization'); - error.statusCode = 403; - throw error; - } - - // Check if the organization is granted (third-party application only) by the user - if ( - (await isThirdPartyApplication(queries, client.clientId)) && - !(await isOrganizationConsentedToApplication( - queries, - client.clientId, - account.accountId, - organizationId - )) - ) { - const error = new AccessDenied('organization access is not granted to the application'); - error.statusCode = 403; - throw error; - } + const { organizationId } = await checkOrganizationAccess(ctx, queries, account); - // Check if the organization requires MFA and the user has MFA enabled - const { isMfaRequired, hasMfaConfigured } = await queries.organizations.getMfaStatus( - organizationId, - account.accountId - ); - if (isMfaRequired && !hasMfaConfigured) { - const error = new AccessDenied('organization requires MFA but user has no MFA configured'); - error.statusCode = 403; - throw error; - } + /* === RFC 0001 === */ + if ( + organizationId && // Validate if the refresh token has the required scope from RFC 0001. + !refreshToken.scopes.has(UserScope.Organizations) + ) { + throw new InsufficientScope('refresh token missing required scope', UserScope.Organizations); } /* === End RFC 0001 === */ @@ -280,27 +226,17 @@ export const buildHandler: ( // the logic is handled in `getResourceServerInfo` and `extraTokenClaims`, see the init file of oidc-provider. if (organizationId && !params.resource) { /* === RFC 0001 === */ - const audience = buildOrganizationUrn(organizationId); /** All available scopes for the user in the organization. */ const availableScopes = await queries.organizations.relations.usersRoles .getUserScopes(organizationId, account.accountId) .then((scopes) => scopes.map(({ name }) => name)); - - /** The intersection of the available scopes and the requested scopes. */ - const issuedScopes = availableScopes.filter((name) => scope.has(name)).join(' '); - - at.aud = audience; - // Note: the original implementation uses `new provider.ResourceServer` to create the resource - // server. But it's not available in the typings. The class is actually very simple and holds - // no provider-specific context. So we just create the object manually. - // See https://github.com/panva/node-oidc-provider/blob/cf2069cbb31a6a855876e95157372d25dde2511c/lib/helpers/resource_server.js - at.resourceServer = { - ...getSharedResourceServerData(envSet), - accessTokenTTL: reversedResourceAccessTokenTtl, - audience, - scope: availableScopes.join(' '), - }; - at.scope = issuedScopes; + await handleOrganizationToken({ + envSet, + availableScopes, + accessToken: at, + organizationId, + scope, + }); /* === End RFC 0001 === */ } else { const resource = await resolveResource( diff --git a/packages/core/src/oidc/grants/token-exchange/index.ts b/packages/core/src/oidc/grants/token-exchange/index.ts index c5436c566a3d..6ef8b7d31b91 100644 --- a/packages/core/src/oidc/grants/token-exchange/index.ts +++ b/packages/core/src/oidc/grants/token-exchange/index.ts @@ -6,7 +6,7 @@ import { buildOrganizationUrn } from '@logto/core-kit'; import { GrantType } from '@logto/schemas'; -import { cond, trySafe } from '@silverhand/essentials'; +import { trySafe } from '@silverhand/essentials'; import type Provider from 'oidc-provider'; import { errors } from 'oidc-provider'; import resolveResource from 'oidc-provider/lib/helpers/resolve_resource.js'; @@ -22,7 +22,7 @@ import { getSharedResourceServerData, reversedResourceAccessTokenTtl, } from '../../resource.js'; -import { handleDPoP } from '../utils.js'; +import { checkOrganizationAccess, handleDPoP } from '../utils.js'; import { handleActorToken } from './actor-token.js'; import { TokenExchangeTokenType, type TokenExchangeAct } from './types.js'; @@ -96,36 +96,7 @@ export const buildHandler: ( ctx.oidc.entity('Account', account); - /* === RFC 0001 === */ - // The value type is `unknown`, which will swallow other type inferences. So we have to cast it - // to `Boolean` first. - const organizationId = cond(Boolean(params.organization_id) && String(params.organization_id)); - - if (organizationId) { - // Check membership - if ( - !(await queries.organizations.relations.users.exists({ - organizationId, - userId: account.accountId, - })) - ) { - const error = new AccessDenied('user is not a member of the organization'); - error.statusCode = 403; - throw error; - } - - // Check if the organization requires MFA and the user has MFA enabled - const { isMfaRequired, hasMfaConfigured } = await queries.organizations.getMfaStatus( - organizationId, - account.accountId - ); - if (isMfaRequired && !hasMfaConfigured) { - const error = new AccessDenied('organization requires MFA but user has no MFA configured'); - error.statusCode = 403; - throw error; - } - } - /* === End RFC 0001 === */ + const { organizationId } = await checkOrganizationAccess(ctx, queries, account); const accessToken = new AccessToken({ accountId: account.accountId, diff --git a/packages/core/src/oidc/grants/utils.ts b/packages/core/src/oidc/grants/utils.ts index c0226916a3f3..3f8ba61c0034 100644 --- a/packages/core/src/oidc/grants/utils.ts +++ b/packages/core/src/oidc/grants/utils.ts @@ -1,13 +1,24 @@ +import { buildOrganizationUrn } from '@logto/core-kit'; +import { cond } from '@silverhand/essentials'; import type Provider from 'oidc-provider'; -import { errors, type KoaContextWithOIDC } from 'oidc-provider'; +import { type Account, errors, type KoaContextWithOIDC } from 'oidc-provider'; import certificateThumbprint from 'oidc-provider/lib/helpers/certificate_thumbprint.js'; import epochTime from 'oidc-provider/lib/helpers/epoch_time.js'; import dpopValidate from 'oidc-provider/lib/helpers/validate_dpop.js'; import instance from 'oidc-provider/lib/helpers/weak_cache.js'; +import { type EnvSet } from '#src/env-set/index.js'; +import type Queries from '#src/tenants/Queries.js'; import assertThat from '#src/utils/assert-that.js'; -const { InvalidGrant, InvalidClient } = errors; +import { + getSharedResourceServerData, + isOrganizationConsentedToApplication, + isThirdPartyApplication, + reversedResourceAccessTokenTtl, +} from '../resource.js'; + +const { InvalidGrant, InvalidClient, AccessDenied } = errors; export const handleDPoP = async ( ctx: KoaContextWithOIDC, @@ -64,3 +75,102 @@ export const handleDPoP = async ( throw new InvalidGrant('failed jkt verification'); } }; + +/** + * Implement access check for RFC 0001 + */ +export const checkOrganizationAccess = async ( + ctx: KoaContextWithOIDC, + queries: Queries, + account: Account +): Promise<{ organizationId?: string }> => { + const { client, params } = ctx.oidc; + + assertThat(params, new InvalidGrant('parameters must be available')); + assertThat(client, new InvalidClient('client must be available')); + + const organizationId = cond(Boolean(params.organization_id) && String(params.organization_id)); + + if (organizationId) { + // Check membership + if ( + !(await queries.organizations.relations.users.exists({ + organizationId, + userId: account.accountId, + })) + ) { + const error = new AccessDenied('user is not a member of the organization'); + // eslint-disable-next-line @silverhand/fp/no-mutation + error.statusCode = 403; + throw error; + } + + // Check if the organization is granted (third-party application only) by the user + if ( + (await isThirdPartyApplication(queries, client.clientId)) && + !(await isOrganizationConsentedToApplication( + queries, + client.clientId, + account.accountId, + organizationId + )) + ) { + const error = new AccessDenied('organization access is not granted to the application'); + // eslint-disable-next-line @silverhand/fp/no-mutation + error.statusCode = 403; + throw error; + } + + // Check if the organization requires MFA and the user has MFA enabled + const { isMfaRequired, hasMfaConfigured } = await queries.organizations.getMfaStatus( + organizationId, + account.accountId + ); + if (isMfaRequired && !hasMfaConfigured) { + const error = new AccessDenied('organization requires MFA but user has no MFA configured'); + // eslint-disable-next-line @silverhand/fp/no-mutation + error.statusCode = 403; + throw error; + } + } + + return { organizationId }; +}; + +/** + * Implement organization token for RFC 0001 + */ +export const handleOrganizationToken = async ({ + envSet, + availableScopes, + accessToken: at, + organizationId, + scope, +}: { + envSet: EnvSet; + availableScopes: string[]; + accessToken: InstanceType | InstanceType; + organizationId: string; + scope: Set; +}): Promise => { + /* eslint-disable @silverhand/fp/no-mutation */ + const audience = buildOrganizationUrn(organizationId); + + /** The intersection of the available scopes and the requested scopes. */ + const issuedScopes = availableScopes.filter((name) => scope.has(name)).join(' '); + + at.aud = audience; + // Note: the original implementation uses `new provider.ResourceServer` to create the resource + // server. But it's not available in the typings. The class is actually very simple and holds + // no provider-specific context. So we just create the object manually. + // See https://github.com/panva/node-oidc-provider/blob/cf2069cbb31a6a855876e95157372d25dde2511c/lib/helpers/resource_server.js + at.resourceServer = { + ...getSharedResourceServerData(envSet), + accessTokenTTL: reversedResourceAccessTokenTtl, + audience, + scope: availableScopes.join(' '), + }; + at.scope = issuedScopes; + + /* eslint-enable @silverhand/fp/no-mutation */ +}; diff --git a/packages/integration-tests/src/tests/api/oidc/refresh-token-grant.test.ts b/packages/integration-tests/src/tests/api/oidc/refresh-token-grant.test.ts index 0577f4d3d1d4..c0cbdfc0a06e 100644 --- a/packages/integration-tests/src/tests/api/oidc/refresh-token-grant.test.ts +++ b/packages/integration-tests/src/tests/api/oidc/refresh-token-grant.test.ts @@ -242,13 +242,6 @@ describe('`refresh_token` grant (for organization tokens)', () => { expect(response.access_token).not.toContain('.'); }); - it('should return error when organizations scope is not requested', async () => { - const client = await initClient({ scopes: [] }); - await expect(client.fetchOrganizationToken('1')).rejects.toMatchError( - grantErrorContaining('oidc.insufficient_scope', 'refresh token missing required scope', 403) - ); - }); - it('should return access denied when organization id is invalid', async () => { const client = await initClient(); await expect(client.fetchOrganizationToken('1')).rejects.toMatchError(accessDeniedError); @@ -273,6 +266,17 @@ describe('`refresh_token` grant (for organization tokens)', () => { await expect(client.fetchOrganizationToken(org.id)).rejects.toMatchError(accessDeniedError); }); + it('should return error when organizations scope is not requested', async () => { + const org = await organizationApi.create({ name: 'org' }); + await organizationApi.addUsers(org.id, [userId]); + + const client = await initClient({ scopes: [] }); + await expect(client.fetchOrganizationToken(org.id)).rejects.toMatchError( + grantErrorContaining('oidc.insufficient_scope', 'refresh token missing required scope', 403) + ); + await organizationApi.deleteUser(org.id, userId); + }); + it('should issue organization scopes even organization resource is not requested (handled by SDK)', async () => { const { orgs } = await initOrganizations();