Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(core,schemas): remove feature guard of organization api resource #5743

Merged
merged 1 commit into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions packages/core/src/oidc/extra-token-claims.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,6 @@
ctx: KoaContextWithOIDC,
token: unknown
): Promise<UnknownObject | undefined> => {
const { isDevFeaturesEnabled } = EnvSet.values;

if (!isDevFeaturesEnabled) {
return;
}

const organizationId = ctx.oidc.params?.organization_id;
const resource = ctx.oidc.params?.resource;

Expand Down Expand Up @@ -121,7 +115,7 @@
? { tokenType: LogtoJwtTokenKeyType.ClientCredentials }
: {
tokenType: LogtoJwtTokenKeyType.AccessToken,
// TODO (LOG-8555): the newly added `UserProfile` type includes undefined fields and can not be directly assigned to `Json` type. And the `undefined` fields should be removed by zod guard.

Check warning on line 118 in packages/core/src/oidc/extra-token-claims.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/core/src/oidc/extra-token-claims.ts#L118

[no-warning-comments] Unexpected 'todo' comment: 'TODO (LOG-8555): the newly added...'.
// `context` parameter is only eligible for user's access token for now.
// eslint-disable-next-line no-restricted-syntax
context: { user: logtoUserInfo as Record<string, Json> },
Expand Down
16 changes: 6 additions & 10 deletions packages/core/src/oidc/grants/refresh-token.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import dpopValidate from 'oidc-provider/lib/helpers/validate_dpop.js';
import validatePresence from 'oidc-provider/lib/helpers/validate_presence.js';
import instance from 'oidc-provider/lib/helpers/weak_cache.js';

import { EnvSet } from '#src/env-set/index.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';

Expand Down Expand Up @@ -140,15 +140,11 @@ export const buildHandler: (
// 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.
if (!refreshToken.scopes.has(UserScope.Organizations)) {
throw new InsufficientScope('refresh token missing required scope', UserScope.Organizations);
}
// Does not allow requesting resource token when requesting organization token (yet).
if (!EnvSet.values.isDevFeaturesEnabled && params.resource) {
throw new InvalidRequest('resource is not allowed when requesting organization token');
}
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 === */

Expand Down
8 changes: 3 additions & 5 deletions packages/core/src/oidc/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import { trySafe, type Nullable } from '@silverhand/essentials';
import { type ResourceServer } from 'oidc-provider';

import { EnvSet } from '#src/env-set/index.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';

Expand Down Expand Up @@ -125,7 +125,7 @@
includeOrganizationResourceScopes = true,
includeResourceScopes = true,
}: { includeOrganizationResourceScopes?: boolean; includeResourceScopes?: boolean } = {}
) => {

Check warning on line 128 in packages/core/src/oidc/resource.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/core/src/oidc/resource.ts#L128

[max-params] Async arrow function has too many parameters (5). Maximum allowed is 4.
const {
applications: {
getApplicationUserConsentOrganizationScopes,
Expand All @@ -148,7 +148,7 @@
)
);
}
// FIXME: @simeng double check if it's necessary

Check warning on line 151 in packages/core/src/oidc/resource.ts

View workflow job for this annotation

GitHub Actions / ESLint Report Analysis

packages/core/src/oidc/resource.ts#L151

[no-warning-comments] Unexpected 'fixme' comment: 'FIXME: @simeng double check if it's...'.
// Return all the scopes for the reserved resources
// eslint-disable-next-line @typescript-eslint/switch-exhaustiveness-check
default: {
Expand All @@ -164,10 +164,8 @@
const userConsentResource = userConsentResources.find(
({ resource }) => resource.indicator === indicator
);
const userConsentOrganizationResources = EnvSet.values.isDevFeaturesEnabled
? includeOrganizationResourceScopes
? await getApplicationUserConsentOrganizationResourceScopes(applicationId)
: []
const userConsentOrganizationResources = includeOrganizationResourceScopes
? await getApplicationUserConsentOrganizationResourceScopes(applicationId)

Check warning on line 168 in packages/core/src/oidc/resource.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/oidc/resource.ts#L167-L168

Added lines #L167 - L168 were not covered by tests
: [];
const userConsentOrganizationResource = userConsentOrganizationResources.find(
({ resource }) => resource.indicator === indicator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
} from '@logto/schemas';
import { object, string, nativeEnum } from 'zod';

import { EnvSet } from '#src/env-set/index.js';
import koaGuard from '#src/middleware/koa-guard.js';

import type { ManagementApiRouter, RouterInitArgs } from '../types.js';
Expand Down Expand Up @@ -53,15 +52,11 @@
body,
} = ctx.guard;

// TODO @wangsijie: Remove this when feature is enabled in production
const { organizationResourceScopes, ...rest } = body;
const theBody = EnvSet.values.isDevFeaturesEnabled ? body : rest;

await validateThirdPartyApplicationById(applicationId);

await validateApplicationUserConsentScopes(theBody, tenantId);
await validateApplicationUserConsentScopes(body, tenantId);

Check warning on line 57 in packages/core/src/routes/applications/application-user-consent-scope.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/routes/applications/application-user-consent-scope.ts#L57

Added line #L57 was not covered by tests

await assignApplicationUserConsentScopes(applicationId, theBody);
await assignApplicationUserConsentScopes(applicationId, body);

Check warning on line 59 in packages/core/src/routes/applications/application-user-consent-scope.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/routes/applications/application-user-consent-scope.ts#L59

Added line #L59 was not covered by tests

ctx.status = 201;

Expand All @@ -75,9 +70,7 @@
params: object({
applicationId: string(),
}),
response: EnvSet.values.isDevFeaturesEnabled
? applicationUserConsentScopesResponseGuard
: applicationUserConsentScopesResponseGuard.omit({ organizationResourceScopes: true }),
response: applicationUserConsentScopesResponseGuard,
status: [200, 404],
}),
async (ctx, next) => {
Expand All @@ -94,18 +87,12 @@
getApplicationUserConsentScopes(applicationId),
]);

ctx.body = EnvSet.values.isDevFeaturesEnabled
? {
organizationScopes,
resourceScopes,
organizationResourceScopes,
userScopes,
}
: {
organizationScopes,
resourceScopes,
userScopes,
};
ctx.body = {
organizationScopes,
resourceScopes,
organizationResourceScopes,
userScopes,
};

Check warning on line 95 in packages/core/src/routes/applications/application-user-consent-scope.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/routes/applications/application-user-consent-scope.ts#L90-L95

Added lines #L90 - L95 were not covered by tests

return next();
}
Expand Down
26 changes: 7 additions & 19 deletions packages/core/src/routes/interaction/consent/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import { errors } from 'oidc-provider';
import { z } from 'zod';

import { EnvSet } from '#src/env-set/index.js';
import { consent, getMissingScopes } from '#src/libraries/session.js';
import koaGuard from '#src/middleware/koa-guard.js';
import type TenantContext from '#src/tenants/TenantContext.js';
Expand Down Expand Up @@ -90,12 +89,13 @@

// Find the organizations granted by the user
// The user may send consent request multiple times, so we need to find the organizations again
const [, organizations] = EnvSet.values.isDevFeaturesEnabled
? await queries.applications.userConsentOrganizations.getEntities(Organizations, {
applicationId,
userId,
})
: [0, []];
const [, organizations] = await queries.applications.userConsentOrganizations.getEntities(
Organizations,
{
applicationId,
userId,
}
);

Check warning on line 98 in packages/core/src/routes/interaction/consent/index.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/routes/interaction/consent/index.ts#L92-L98

Added lines #L92 - L98 were not covered by tests

// The missingResourceScopes from the prompt details are from `getResourceServerInfo`,
// which contains resource scopes and organization resource scopes.
Expand All @@ -111,10 +111,6 @@

const organizationsWithMissingResourceScopes = await Promise.all(
organizations.map(async ({ name, id }) => {
if (!EnvSet.values.isDevFeaturesEnabled) {
return { name, id };
}

const missingResourceScopes = await filterAndParseMissingResourceScopes({
resourceScopes: allMissingResourceScopes,
queries,
Expand All @@ -136,10 +132,6 @@
const resourceScopesToGrant: Record<string, string[]> = Object.fromEntries(
organizationsWithMissingResourceScopes.reduce<Array<[string, string[]]>>(
(entries, { missingResourceScopes }) => {
if (!missingResourceScopes) {
return entries;
}

const organizationEntries: Array<[string, string[]]> = missingResourceScopes.map(
({ resource, scopes }) => [resource.indicator, scopes.map(({ name }) => name)]
);
Expand Down Expand Up @@ -261,10 +253,6 @@

const organizationsWithMissingResourceScopes = await Promise.all(
organizations.map(async ({ name, id }) => {
if (!EnvSet.values.isDevFeaturesEnabled) {
return { name, id };
}

const missingResourceScopes = await filterAndParseMissingResourceScopes({
resourceScopes: allMissingResourceScopes,
queries,
Expand Down
5 changes: 0 additions & 5 deletions packages/core/src/routes/interaction/consent/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { ReservedResource } from '@logto/core-kit';
import { type MissingResourceScopes, type Scope, missingResourceScopesGuard } from '@logto/schemas';
import { errors } from 'oidc-provider';

import { EnvSet } from '#src/env-set/index.js';
import {
filterResourceScopesForTheThirdPartyApplication,
findResourceScopes,
Expand Down Expand Up @@ -108,10 +107,6 @@ export const filterAndParseMissingResourceScopes = async ({
await Promise.all(
Object.entries(resourceScopes).map(
async ([resourceIndicator, missingScopes]): Promise<[string, string[]]> => {
if (!EnvSet.values.isDevFeaturesEnabled) {
return [resourceIndicator, missingScopes];
}

// Fetch the list of scopes, `findFromOrganizations` is set to false,
// so it will only search the user resource scopes.
const scopes = await findResourceScopes({
Expand Down
29 changes: 4 additions & 25 deletions packages/core/src/routes/organization/roles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,10 @@
type CreateOrganizationRole,
OrganizationRoles,
organizationRoleWithScopesGuard,
organizationRoleWithScopesGuardDeprecated,
} from '@logto/schemas';
import { generateStandardId } from '@logto/shared';
import { z } from 'zod';

import { EnvSet } from '#src/env-set/index.js';
import koaGuard from '#src/middleware/koa-guard.js';
import koaPagination from '#src/middleware/koa-pagination.js';
import koaQuotaGuard from '#src/middleware/koa-quota-guard.js';
Expand Down Expand Up @@ -45,10 +43,7 @@
koaPagination(),
koaGuard({
query: z.object({ q: z.string().optional() }),
// TODO @wangsijie - Remove this once the feature is ready
response: EnvSet.values.isDevFeaturesEnabled
? organizationRoleWithScopesGuard.array()
: organizationRoleWithScopesGuardDeprecated.array(),
response: organizationRoleWithScopesGuard.array(),
status: [200],
}),
async (ctx, next) => {
Expand All @@ -70,19 +65,6 @@
resourceScopeIds: string[];
};

// TODO @wangsijie - Remove this once the feature is ready
const originalCreateCard: z.ZodType<
Omit<CreateOrganizationRolePayload, 'resourceScopeIds'> & { resourceScopeIds?: string[] },
z.ZodTypeDef,
unknown
> = OrganizationRoles.createGuard
.omit({
id: true,
})
.extend({
organizationScopeIds: z.array(z.string()).default([]),
});

const createGuard: z.ZodType<CreateOrganizationRolePayload, z.ZodTypeDef, unknown> =
OrganizationRoles.createGuard
.omit({
Expand All @@ -96,7 +78,7 @@
router.post(
'/',
koaGuard({
body: EnvSet.values.isDevFeaturesEnabled ? createGuard : originalCreateCard,
body: createGuard,
response: OrganizationRoles.guard,
status: [201, 422],
}),
Expand All @@ -110,8 +92,7 @@
);
}

// TODO @wangsijie - Remove this once the feature is ready
if (EnvSet.values.isDevFeaturesEnabled && resourceScopeIds && resourceScopeIds.length > 0) {
if (resourceScopeIds.length > 0) {

Check warning on line 95 in packages/core/src/routes/organization/roles.ts

View check run for this annotation

Codecov / codecov/patch

packages/core/src/routes/organization/roles.ts#L95

Added line #L95 was not covered by tests
await rolesResourceScopes.insert(
...resourceScopeIds.map<[string, string]>((id) => [role.id, id])
);
Expand All @@ -124,9 +105,7 @@
);

router.addRelationRoutes(rolesScopes, 'scopes');
if (EnvSet.values.isDevFeaturesEnabled) {
router.addRelationRoutes(rolesResourceScopes, 'resource-scopes');
}
router.addRelationRoutes(rolesResourceScopes, 'resource-scopes');

originalRouter.use(router.routes());
}
12 changes: 0 additions & 12 deletions packages/schemas/src/types/organization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,6 @@ export type OrganizationRoleWithScopes = OrganizationRole & {
resourceScopes: ResourceScopeEntity[];
};

// TODO @wangsijie - Remove this once the feature is ready
export const organizationRoleWithScopesGuardDeprecated: ToZodObject<
Omit<OrganizationRoleWithScopes, 'resourceScopes'>
> = OrganizationRoles.guard.extend({
scopes: z
.object({
id: z.string(),
name: z.string(),
})
.array(),
});

export const organizationRoleWithScopesGuard: ToZodObject<OrganizationRoleWithScopes> =
OrganizationRoles.guard.extend({
scopes: z
Expand Down
Loading