From c5da152ddca14ccec0c06f85dcca156bdf24533d Mon Sep 17 00:00:00 2001 From: Gao Sun Date: Thu, 9 Mar 2023 00:07:33 +0800 Subject: [PATCH] refactor: hide internal roles for user tenants introduce internal roles which name starts with #internal: with RLS policies to make them read-only. --- packages/cli/package.json | 1 + packages/cli/src/database.ts | 6 +- packages/cli/src/queries/system.test.ts | 7 ++- packages/cli/src/queries/system.ts | 7 +-- .../Roles/components/CreateRoleForm/index.tsx | 11 +++- packages/core/package.json | 1 + .../middleware/koa-role-rls-error-handler.ts | 32 ++++++++++ .../core/src/queries/applications-roles.ts | 13 ++-- packages/core/src/queries/roles.ts | 6 +- packages/core/src/routes/admin-user-role.ts | 3 + packages/core/src/routes/application.ts | 36 +++++++---- .../interaction/actions/submit-interaction.ts | 4 +- packages/core/src/routes/role.ts | 3 + .../src/tests/api/role.scope.test.ts | 13 ++++ .../src/tests/api/role.test.ts | 12 +++- packages/phrases/src/locales/de/errors.ts | 2 + .../de/translation/admin-console/errors.ts | 4 +- packages/phrases/src/locales/en/errors.ts | 2 + .../en/translation/admin-console/errors.ts | 2 + packages/phrases/src/locales/fr/errors.ts | 2 + .../fr/translation/admin-console/errors.ts | 2 + packages/phrases/src/locales/ko/errors.ts | 2 + .../ko/translation/admin-console/errors.ts | 2 + packages/phrases/src/locales/pt-br/errors.ts | 2 + .../pt-br/translation/admin-console/errors.ts | 2 + packages/phrases/src/locales/pt-pt/errors.ts | 2 + .../pt-pt/translation/admin-console/errors.ts | 2 + packages/phrases/src/locales/tr-tr/errors.ts | 2 + .../tr-tr/translation/admin-console/errors.ts | 2 + packages/phrases/src/locales/zh-cn/errors.ts | 2 + .../zh-cn/translation/admin-console/errors.ts | 2 + ...next-1678284778-restrict-internal-roles.ts | 62 +++++++++++++++++++ packages/schemas/src/index.ts | 1 + packages/schemas/src/seeds/cloud-api.ts | 4 +- packages/schemas/src/seeds/management-api.ts | 16 ++--- packages/schemas/src/types/user.ts | 12 +++- packages/schemas/src/utils/index.ts | 1 + packages/schemas/src/utils/role.ts | 3 + packages/schemas/tables/_after_all.sql | 38 ++++++++++-- pnpm-lock.yaml | 50 +++++---------- 40 files changed, 288 insertions(+), 88 deletions(-) create mode 100644 packages/core/src/middleware/koa-role-rls-error-handler.ts create mode 100644 packages/schemas/alterations/next-1678284778-restrict-internal-roles.ts create mode 100644 packages/schemas/src/utils/index.ts create mode 100644 packages/schemas/src/utils/role.ts diff --git a/packages/cli/package.json b/packages/cli/package.json index 66757f4c4..eb1c0741c 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -57,6 +57,7 @@ "ora": "^6.1.2", "p-limit": "^4.0.0", "p-retry": "^5.1.2", + "pg-protocol": "^1.6.0", "roarr": "^7.11.0", "semver": "^7.3.8", "slonik": "^30.0.0", diff --git a/packages/cli/src/database.ts b/packages/cli/src/database.ts index 3fab9a8ad..716120d64 100644 --- a/packages/cli/src/database.ts +++ b/packages/cli/src/database.ts @@ -2,9 +2,9 @@ import type { SchemaLike } from '@logto/schemas'; import { convertToPrimitiveOrSql } from '@logto/shared'; import { assert } from '@silverhand/essentials'; import decamelize from 'decamelize'; +import { DatabaseError } from 'pg-protocol'; import { createPool, parseDsn, sql, stringifyDsn } from 'slonik'; import { createInterceptors } from 'slonik-interceptor-preset'; -import { z } from 'zod'; import { ConfigKey, getCliConfigWithPrompt, log } from './utils.js'; @@ -36,11 +36,9 @@ export const createPoolAndDatabaseIfNeeded = async () => { try { return await createPoolFromConfig(); } catch (error: unknown) { - const result = z.object({ code: z.string() }).safeParse(error); - // Database does not exist, try to create one // https://www.postgresql.org/docs/14/errcodes-appendix.html - if (!(result.success && result.data.code === '3D000')) { + if (!(error instanceof DatabaseError && error.code === '3D000')) { log.error(error); } diff --git a/packages/cli/src/queries/system.test.ts b/packages/cli/src/queries/system.test.ts index 4fb1116cc..415c47c57 100644 --- a/packages/cli/src/queries/system.test.ts +++ b/packages/cli/src/queries/system.test.ts @@ -1,5 +1,6 @@ import { AlterationStateKey, Systems } from '@logto/schemas'; import { convertToIdentifiers } from '@logto/shared'; +import { DatabaseError } from 'pg-protocol'; import { createMockPool, createMockQueryResult, sql } from 'slonik'; import type { QueryType } from '../test-utils.js'; @@ -21,7 +22,11 @@ const systemsTableExists = async () => createMockQueryResult([{ regclass: true } describe('getCurrentDatabaseAlterationTimestamp()', () => { it('returns 0 if query failed (table not found)', async () => { - mockQuery.mockImplementationOnce(systemsTableExists).mockRejectedValueOnce({ code: '42P01' }); + const error = new DatabaseError('test', 0, 'noData'); + // eslint-disable-next-line @silverhand/fp/no-mutation + error.code = '42P01'; + + mockQuery.mockImplementationOnce(systemsTableExists).mockRejectedValueOnce(error); await expect(getCurrentDatabaseAlterationTimestamp(pool)).resolves.toBe(0); }); diff --git a/packages/cli/src/queries/system.ts b/packages/cli/src/queries/system.ts index 5a9f61680..9b54907eb 100644 --- a/packages/cli/src/queries/system.ts +++ b/packages/cli/src/queries/system.ts @@ -2,9 +2,10 @@ import type { AlterationState, System, SystemKey } from '@logto/schemas'; import { systemGuards, Systems, AlterationStateKey } from '@logto/schemas'; import { convertToIdentifiers } from '@logto/shared'; import type { Nullable } from '@silverhand/essentials'; +import { DatabaseError } from 'pg-protocol'; import type { CommonQueryMethods, DatabaseTransactionConnection } from 'slonik'; import { sql } from 'slonik'; -import { z } from 'zod'; +import type { z } from 'zod'; const { fields, table } = convertToIdentifiers(Systems); @@ -37,11 +38,9 @@ export const getCurrentDatabaseAlterationTimestamp = async (pool: CommonQueryMet return (parsed.success && parsed.data.timestamp) || 0; } catch (error: unknown) { - const result = z.object({ code: z.string() }).safeParse(error); - // Relation does not exist, treat as 0 // https://www.postgresql.org/docs/14/errcodes-appendix.html - if (result.success && result.data.code === '42P01') { + if (error instanceof DatabaseError && error.code === '42P01') { return 0; } diff --git a/packages/console/src/pages/Roles/components/CreateRoleForm/index.tsx b/packages/console/src/pages/Roles/components/CreateRoleForm/index.tsx index 9061d72ea..10d0c98a7 100644 --- a/packages/console/src/pages/Roles/components/CreateRoleForm/index.tsx +++ b/packages/console/src/pages/Roles/components/CreateRoleForm/index.tsx @@ -1,4 +1,5 @@ import type { Role, ScopeResponse } from '@logto/schemas'; +import { internalRolePrefix } from '@logto/schemas'; import { conditional } from '@silverhand/essentials'; import { Controller, useForm } from 'react-hook-form'; import { useTranslation } from 'react-i18next'; @@ -70,9 +71,15 @@ const CreateRoleForm = ({ onClose }: Props) => { + name.startsWith(internalRolePrefix) + ? t('errors.create_internal_role_violation') + : true, + })} placeholder={t('roles.role_name_placeholder')} - hasError={Boolean(errors.name)} + errorMessage={errors.name?.message} /> diff --git a/packages/core/package.json b/packages/core/package.json index d2d4a42f7..355d49e2e 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -65,6 +65,7 @@ "nanoid": "^4.0.0", "oidc-provider": "^8.0.0", "p-retry": "^5.1.2", + "pg-protocol": "^1.6.0", "roarr": "^7.11.0", "semver": "^7.3.8", "slonik": "^30.0.0", diff --git a/packages/core/src/middleware/koa-role-rls-error-handler.ts b/packages/core/src/middleware/koa-role-rls-error-handler.ts new file mode 100644 index 000000000..064542cad --- /dev/null +++ b/packages/core/src/middleware/koa-role-rls-error-handler.ts @@ -0,0 +1,32 @@ +import type { MiddlewareType } from 'koa'; +import { DatabaseError } from 'pg-protocol'; + +import RequestError from '#src/errors/RequestError/index.js'; + +/** + * Provide a user-friendly error message for Row Level Security errors. + * The message is dedicated for providing a hint of internal roles update violation. + * + * Since it's hard to detect the specific violated policy, this function should be ONLY used + * for roles routes. + */ +export default function koaRoleRlsErrorHandler(): MiddlewareType< + StateT, + ContextT, + ResponseBodyT +> { + return async (_, next) => { + try { + await next(); + } catch (error: unknown) { + // https://www.postgresql.org/docs/14/errcodes-appendix.html + if (error instanceof DatabaseError && error.code === '42501') { + throw new RequestError( + { code: 'role.internal_role_violation', status: 403 }, + { original: error } + ); + } + throw error; + } + }; +} diff --git a/packages/core/src/queries/applications-roles.ts b/packages/core/src/queries/applications-roles.ts index 31c064e12..3f46ecd4f 100644 --- a/packages/core/src/queries/applications-roles.ts +++ b/packages/core/src/queries/applications-roles.ts @@ -1,18 +1,21 @@ -import type { ApplicationsRole, CreateApplicationsRole } from '@logto/schemas'; -import { ApplicationsRoles, RolesScopes } from '@logto/schemas'; +import type { ApplicationsRole, CreateApplicationsRole, Role } from '@logto/schemas'; +import { Roles, ApplicationsRoles, RolesScopes } from '@logto/schemas'; import { convertToIdentifiers } from '@logto/shared'; import type { CommonQueryMethods } from 'slonik'; import { sql } from 'slonik'; import { DeletionError } from '#src/errors/SlonikError/index.js'; -const { table, fields } = convertToIdentifiers(ApplicationsRoles); +const { table, fields } = convertToIdentifiers(ApplicationsRoles, true); export const createApplicationsRolesQueries = (pool: CommonQueryMethods) => { const findApplicationsRolesByApplicationId = async (applicationId: string) => - pool.any(sql` - select ${sql.join(Object.values(fields), sql`,`)} + pool.any(sql` + select + ${sql.join(Object.values(fields), sql`,`)}, + to_jsonb(${sql.identifier([Roles.table])}) as role from ${table} + join roles on ${sql.identifier([Roles.table, Roles.fields.id])} = ${fields.roleId} where ${fields.applicationId}=${applicationId} `); diff --git a/packages/core/src/queries/roles.ts b/packages/core/src/queries/roles.ts index 8b8174e5d..b43ef454c 100644 --- a/packages/core/src/queries/roles.ts +++ b/packages/core/src/queries/roles.ts @@ -1,5 +1,5 @@ import type { CreateRole, Role } from '@logto/schemas'; -import { defaultManagementApi, SearchJointMode, Roles } from '@logto/schemas'; +import { internalRolePrefix, SearchJointMode, Roles } from '@logto/schemas'; import type { OmitAutoSetFields } from '@logto/shared'; import { conditionalArraySql, conditionalSql, convertToIdentifiers } from '@logto/shared'; import type { CommonQueryMethods } from 'slonik'; @@ -34,7 +34,7 @@ export const createRolesQueries = (pool: CommonQueryMethods) => { pool.one<{ count: number }>(sql` select count(*) from ${table} - where ${fields.id}<>${defaultManagementApi.role.id} + where (not starts_with(${fields.name}, ${internalRolePrefix})) ${conditionalArraySql( excludeRoleIds, (value) => sql`and ${fields.id} not in (${sql.join(value, sql`, `)})` @@ -57,7 +57,7 @@ export const createRolesQueries = (pool: CommonQueryMethods) => { sql` select ${sql.join(Object.values(fields), sql`, `)} from ${table} - where ${fields.id}<>${defaultManagementApi.role.id} + where (not starts_with(${fields.name}, ${internalRolePrefix})) ${conditionalArraySql( excludeRoleIds, (value) => sql`and ${fields.id} not in (${sql.join(value, sql`, `)})` diff --git a/packages/core/src/routes/admin-user-role.ts b/packages/core/src/routes/admin-user-role.ts index 53beaa1b6..00e4617f0 100644 --- a/packages/core/src/routes/admin-user-role.ts +++ b/packages/core/src/routes/admin-user-role.ts @@ -5,6 +5,7 @@ import { object, string } from 'zod'; import RequestError from '#src/errors/RequestError/index.js'; import koaGuard from '#src/middleware/koa-guard.js'; import koaPagination from '#src/middleware/koa-pagination.js'; +import koaRoleRlsErrorHandler from '#src/middleware/koa-role-rls-error-handler.js'; import assertThat from '#src/utils/assert-that.js'; import { parseSearchParamsForSearch } from '#src/utils/search.js'; @@ -19,6 +20,8 @@ export default function adminUserRoleRoutes( usersRoles: { deleteUsersRolesByUserIdAndRoleId, findUsersRolesByUserId, insertUsersRoles }, } = queries; + router.use('/users/:userId/roles(/.*)?', koaRoleRlsErrorHandler()); + router.get( '/users/:userId/roles', koaPagination(), diff --git a/packages/core/src/routes/application.ts b/packages/core/src/routes/application.ts index aa06dae07..f3ca60e9d 100644 --- a/packages/core/src/routes/application.ts +++ b/packages/core/src/routes/application.ts @@ -1,19 +1,24 @@ import { generateStandardId, buildIdGenerator } from '@logto/core-kit'; +import type { Role } from '@logto/schemas'; import { - defaultManagementApi, - Applications, demoAppApplicationId, buildDemoAppDataForTenant, + Applications, + InternalRole, } from '@logto/schemas'; import { boolean, object, string, z } from 'zod'; +import RequestError from '#src/errors/RequestError/index.js'; import koaGuard from '#src/middleware/koa-guard.js'; import koaPagination from '#src/middleware/koa-pagination.js'; import { buildOidcClientMetadata } from '#src/oidc/utils.js'; +import assertThat from '#src/utils/assert-that.js'; import type { AuthedRouter, RouterInitArgs } from './types.js'; const applicationId = buildIdGenerator(21); +const includesInternalAdminRole = (roles: Readonly>) => + roles.some(({ role: { name } }) => name === InternalRole.Admin); export default function applicationRoutes( ...[router, { queries, id: tenantId }]: RouterInitArgs @@ -28,6 +33,7 @@ export default function applicationRoutes( } = queries.applications; const { findApplicationsRolesByApplicationId, insertApplicationsRoles, deleteApplicationRole } = queries.applicationsRoles; + const { findRoleByRoleName } = queries.roles; router.get('/applications', koaPagination(), async (ctx, next) => { const { limit, offset } = ctx.pagination; @@ -89,7 +95,7 @@ export default function applicationRoutes( ctx.body = { ...application, - isAdmin: applicationsRoles.some(({ roleId }) => roleId === defaultManagementApi.role.id), + isAdmin: includesInternalAdminRole(applicationsRoles), }; return next(); @@ -117,19 +123,27 @@ export default function applicationRoutes( const { isAdmin, ...rest } = body; - // FIXME @sijie temp solution to set admin access to machine to machine app + // User can enable the admin access of Machine-to-Machine apps by switching on a toggle on Admin Console. + // Since those apps sit in the user tenant, we provide an internal role to apply the necessary scopes. + // This role is NOT intended for user assignment. if (isAdmin !== undefined) { - const applicationsRoles = await findApplicationsRolesByApplicationId(id); - const originalIsAdmin = applicationsRoles.some( - ({ roleId }) => roleId === defaultManagementApi.role.id + const [applicationsRoles, internalAdminRole] = await Promise.all([ + findApplicationsRolesByApplicationId(id), + findRoleByRoleName(InternalRole.Admin), + ]); + const usedToBeAdmin = includesInternalAdminRole(applicationsRoles); + + assertThat( + internalAdminRole, + new RequestError('entity.not_exists', { name: InternalRole.Admin }) ); - if (isAdmin && !originalIsAdmin) { + if (isAdmin && !usedToBeAdmin) { await insertApplicationsRoles([ - { id: generateStandardId(), applicationId: id, roleId: defaultManagementApi.role.id }, + { id: generateStandardId(), applicationId: id, roleId: internalAdminRole.id }, ]); - } else if (!isAdmin && originalIsAdmin) { - await deleteApplicationRole(id, defaultManagementApi.role.id); + } else if (!isAdmin && usedToBeAdmin) { + await deleteApplicationRole(id, internalAdminRole.id); } } diff --git a/packages/core/src/routes/interaction/actions/submit-interaction.ts b/packages/core/src/routes/interaction/actions/submit-interaction.ts index a7b41928a..1a81e4d08 100644 --- a/packages/core/src/routes/interaction/actions/submit-interaction.ts +++ b/packages/core/src/routes/interaction/actions/submit-interaction.ts @@ -1,7 +1,7 @@ import type { User, Profile } from '@logto/schemas'; import { + AdminTenantRole, SignInMode, - UserRole, getManagementApiAdminName, defaultTenantId, adminTenantId, @@ -180,7 +180,7 @@ export default async function submitInteraction( ...upsertProfile, }, conditionalArray( - isInAdminTenant && UserRole.User, + isInAdminTenant && AdminTenantRole.User, isCreatingFirstAdminUser && getManagementApiAdminName(defaultTenantId), isCreatingFirstAdminUser && isCloud && getManagementApiAdminName(adminTenantId) ) diff --git a/packages/core/src/routes/role.ts b/packages/core/src/routes/role.ts index 2e51cfd4e..29dcdcb81 100644 --- a/packages/core/src/routes/role.ts +++ b/packages/core/src/routes/role.ts @@ -8,6 +8,7 @@ import { object, string, z } from 'zod'; import RequestError from '#src/errors/RequestError/index.js'; import koaGuard from '#src/middleware/koa-guard.js'; import koaPagination from '#src/middleware/koa-pagination.js'; +import koaRoleRlsErrorHandler from '#src/middleware/koa-role-rls-error-handler.js'; import assertThat from '#src/utils/assert-that.js'; import { parseSearchParamsForSearch } from '#src/utils/search.js'; @@ -39,6 +40,8 @@ export default function roleRoutes( }, } = queries; + router.use('/roles(/.*)?', koaRoleRlsErrorHandler()); + router.get('/roles', koaPagination(), async (ctx, next) => { const { limit, offset } = ctx.pagination; const { searchParams } = ctx.request.URL; diff --git a/packages/integration-tests/src/tests/api/role.scope.test.ts b/packages/integration-tests/src/tests/api/role.scope.test.ts index f94cb6945..029309e47 100644 --- a/packages/integration-tests/src/tests/api/role.scope.test.ts +++ b/packages/integration-tests/src/tests/api/role.scope.test.ts @@ -1,3 +1,6 @@ +import { defaultManagementApi } from '@logto/schemas'; +import { HTTPError } from 'got'; + import { createResource } from '#src/api/index.js'; import { assignScopesToRole, @@ -42,4 +45,14 @@ describe('roles scopes', () => { const newScopes = await getRoleScopes(role.id); expect(newScopes.length).toBe(0); }); + + it('should fail when try to assign a scope to an internal role', async () => { + const resource = await createResource(); + const scope = await createScope(resource.id); + const response = await assignScopesToRole([scope.id], defaultManagementApi.role.id).catch( + (error: unknown) => error + ); + + expect(response instanceof HTTPError && response.response.statusCode).toBe(403); + }); }); diff --git a/packages/integration-tests/src/tests/api/role.test.ts b/packages/integration-tests/src/tests/api/role.test.ts index 98c54b41a..43c2f5571 100644 --- a/packages/integration-tests/src/tests/api/role.test.ts +++ b/packages/integration-tests/src/tests/api/role.test.ts @@ -48,7 +48,13 @@ describe('roles', () => { const createdRole = await createRole(); const response = await createRole(createdRole.name).catch((error: unknown) => error); - expect(response instanceof HTTPError && response.response.statusCode === 422).toBe(true); + expect(response instanceof HTTPError && response.response.statusCode).toBe(422); + }); + + it('should fail when try to create an internal role', async () => { + const response = await createRole('#internal:foo').catch((error: unknown) => error); + + expect(response instanceof HTTPError && response.response.statusCode).toBe(403); }); it('should get role detail successfully', async () => { @@ -82,7 +88,7 @@ describe('roles', () => { const response = await updateRole(role2.id, { name: role1.name, }).catch((error: unknown) => error); - expect(response instanceof HTTPError && response.response.statusCode === 422).toBe(true); + expect(response instanceof HTTPError && response.response.statusCode).toBe(422); }); it('should delete role successfully', async () => { @@ -91,6 +97,6 @@ describe('roles', () => { await deleteRole(role.id); const response = await getRole(role.id).catch((error: unknown) => error); - expect(response instanceof HTTPError && response.response.statusCode === 404).toBe(true); + expect(response instanceof HTTPError && response.response.statusCode).toBe(404); }); }); diff --git a/packages/phrases/src/locales/de/errors.ts b/packages/phrases/src/locales/de/errors.ts index 5f4b67fb5..3a48fa53d 100644 --- a/packages/phrases/src/locales/de/errors.ts +++ b/packages/phrases/src/locales/de/errors.ts @@ -187,6 +187,8 @@ const errors = { user_exists: 'The user id {{userId}} is already been added to this role', // UNTRANSLATED default_role_missing: 'Some of the default roleNames does not exist in database, please ensure to create roles first', // UNTRANSLATED + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', // UNTRANSLATED }, scope: { name_exists: 'The scope name {{name}} is already in use', // UNTRANSLATED diff --git a/packages/phrases/src/locales/de/translation/admin-console/errors.ts b/packages/phrases/src/locales/de/translation/admin-console/errors.ts index 8a52c8a2b..0e3250d8a 100644 --- a/packages/phrases/src/locales/de/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/de/translation/admin-console/errors.ts @@ -17,7 +17,9 @@ const errors = { 'Password requires a minimum of {{min}} characters and contains a mix of letters, numbers, and symbols.', // UNTRANSLATED insecure_contexts: 'Unsichere Kontexte (nicht-HTTPS) werden nicht unterstützt.', unexpected_error: 'Ein unerwarteter Fehler ist aufgetreten', - not_found: '404 not found', // UNTRANSLATED + not_found: '404 not found', // UNTRANSLATED, + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', // UNTRANSLATED }; export default errors; diff --git a/packages/phrases/src/locales/en/errors.ts b/packages/phrases/src/locales/en/errors.ts index eb3f85e6b..b9765de1c 100644 --- a/packages/phrases/src/locales/en/errors.ts +++ b/packages/phrases/src/locales/en/errors.ts @@ -186,6 +186,8 @@ const errors = { user_exists: 'The user id {{userId}} is already been added to this role', default_role_missing: 'Some of the default roleNames does not exist in database, please ensure to create roles first', + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', }, scope: { name_exists: 'The scope name {{name}} is already in use', diff --git a/packages/phrases/src/locales/en/translation/admin-console/errors.ts b/packages/phrases/src/locales/en/translation/admin-console/errors.ts index 9b8d6d7f3..f31378cc0 100644 --- a/packages/phrases/src/locales/en/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/en/translation/admin-console/errors.ts @@ -18,6 +18,8 @@ const errors = { insecure_contexts: 'Insecure contexts (non-HTTPS) are not supported.', unexpected_error: 'An unexpected error occurred', not_found: '404 not found', + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', }; export default errors; diff --git a/packages/phrases/src/locales/fr/errors.ts b/packages/phrases/src/locales/fr/errors.ts index 8569fdb6c..d2b1f476a 100644 --- a/packages/phrases/src/locales/fr/errors.ts +++ b/packages/phrases/src/locales/fr/errors.ts @@ -193,6 +193,8 @@ const errors = { user_exists: 'The user id {{userId}} is already been added to this role', // UNTRANSLATED default_role_missing: 'Some of the default roleNames does not exist in database, please ensure to create roles first', // UNTRANSLATED + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', // UNTRANSLATED }, scope: { name_exists: 'The scope name {{name}} is already in use', // UNTRANSLATED diff --git a/packages/phrases/src/locales/fr/translation/admin-console/errors.ts b/packages/phrases/src/locales/fr/translation/admin-console/errors.ts index 704da9b3e..e9b42248d 100644 --- a/packages/phrases/src/locales/fr/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/fr/translation/admin-console/errors.ts @@ -18,6 +18,8 @@ const errors = { insecure_contexts: 'Les contextes non sécurisés (non HTTPS) ne sont pas pris en charge.', unexpected_error: "Une erreur inattendue s'est produite", not_found: '404 not found', // UNTRANSLATED + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', // UNTRANSLATED }; export default errors; diff --git a/packages/phrases/src/locales/ko/errors.ts b/packages/phrases/src/locales/ko/errors.ts index 9f5906138..2f55db428 100644 --- a/packages/phrases/src/locales/ko/errors.ts +++ b/packages/phrases/src/locales/ko/errors.ts @@ -179,6 +179,8 @@ const errors = { user_exists: '사용자 ID {{userId}}이/가 이미 이 역할에 추가되어 있어요.', default_role_missing: '기본 역할 이름의 일부가 데이터베이스에 존재하지 않아요. 먼저 역할을 생성해 주세요.', + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', // UNTRANSLATED }, scope: { name_exists: '범위 이름 {{name}}이/가 이미 사용 중이에요.', diff --git a/packages/phrases/src/locales/ko/translation/admin-console/errors.ts b/packages/phrases/src/locales/ko/translation/admin-console/errors.ts index 0ee56e5a7..c5b11649d 100644 --- a/packages/phrases/src/locales/ko/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/ko/translation/admin-console/errors.ts @@ -18,6 +18,8 @@ const errors = { insecure_contexts: '비보안 연결(non-HTTPS)는 지원하지 않아요.', unexpected_error: '알 수 없는 오류가 발생했어요.', not_found: '404 not found', // UNTRANSLATED + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', // UNTRANSLATED }; export default errors; diff --git a/packages/phrases/src/locales/pt-br/errors.ts b/packages/phrases/src/locales/pt-br/errors.ts index 5f4d780f5..9f926476f 100644 --- a/packages/phrases/src/locales/pt-br/errors.ts +++ b/packages/phrases/src/locales/pt-br/errors.ts @@ -194,6 +194,8 @@ const errors = { user_exists: 'The user id {{userId}} is already been added to this role', // UNTRANSLATED default_role_missing: 'Some of the default roleNames does not exist in database, please ensure to create roles first', // UNTRANSLATED + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', // UNTRANSLATED }, scope: { name_exists: 'The scope name {{name}} is already in use', // UNTRANSLATED diff --git a/packages/phrases/src/locales/pt-br/translation/admin-console/errors.ts b/packages/phrases/src/locales/pt-br/translation/admin-console/errors.ts index 3cde8f608..197a9455e 100644 --- a/packages/phrases/src/locales/pt-br/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/pt-br/translation/admin-console/errors.ts @@ -18,6 +18,8 @@ const errors = { insecure_contexts: 'Contextos inseguros (não-HTTPS) não são suportados.', unexpected_error: 'Um erro inesperado ocorreu', not_found: '404 not found', // UNTRANSLATED + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', // UNTRANSLATED }; export default errors; diff --git a/packages/phrases/src/locales/pt-pt/errors.ts b/packages/phrases/src/locales/pt-pt/errors.ts index fd43f67c1..717fe9505 100644 --- a/packages/phrases/src/locales/pt-pt/errors.ts +++ b/packages/phrases/src/locales/pt-pt/errors.ts @@ -188,6 +188,8 @@ const errors = { user_exists: 'The user id {{userId}} is already been added to this role', // UNTRANSLATED default_role_missing: 'Some of the default roleNames does not exist in database, please ensure to create roles first', // UNTRANSLATED + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', // UNTRANSLATED }, scope: { name_exists: 'The scope name {{name}} is already in use', // UNTRANSLATED diff --git a/packages/phrases/src/locales/pt-pt/translation/admin-console/errors.ts b/packages/phrases/src/locales/pt-pt/translation/admin-console/errors.ts index 221a87073..340c53370 100644 --- a/packages/phrases/src/locales/pt-pt/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/pt-pt/translation/admin-console/errors.ts @@ -18,6 +18,8 @@ const errors = { insecure_contexts: 'Contextos inseguros (não HTTPS) não são compatíveis.', unexpected_error: 'Um erro inesperado ocorreu', not_found: '404 not found', // UNTRANSLATED + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', // UNTRANSLATED }; export default errors; diff --git a/packages/phrases/src/locales/tr-tr/errors.ts b/packages/phrases/src/locales/tr-tr/errors.ts index a4d9b3bfe..961902a24 100644 --- a/packages/phrases/src/locales/tr-tr/errors.ts +++ b/packages/phrases/src/locales/tr-tr/errors.ts @@ -188,6 +188,8 @@ const errors = { user_exists: 'The user id {{userId}} is already been added to this role', // UNTRANSLATED default_role_missing: 'Some of the default roleNames does not exist in database, please ensure to create roles first', // UNTRANSLATED + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', // UNTRANSLATED }, scope: { name_exists: 'The scope name {{name}} is already in use', // UNTRANSLATED diff --git a/packages/phrases/src/locales/tr-tr/translation/admin-console/errors.ts b/packages/phrases/src/locales/tr-tr/translation/admin-console/errors.ts index 1adf7cec6..e0b6fdae5 100644 --- a/packages/phrases/src/locales/tr-tr/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/tr-tr/translation/admin-console/errors.ts @@ -18,6 +18,8 @@ const errors = { insecure_contexts: 'Güvenli olmayan bağlamlar (HTTPS olmayan) desteklenmez.', unexpected_error: 'Beklenmedik bir hata oluştu', not_found: '404 not found', // UNTRANSLATED + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', // UNTRANSLATED }; export default errors; diff --git a/packages/phrases/src/locales/zh-cn/errors.ts b/packages/phrases/src/locales/zh-cn/errors.ts index 31fdacaa9..bd38727b4 100644 --- a/packages/phrases/src/locales/zh-cn/errors.ts +++ b/packages/phrases/src/locales/zh-cn/errors.ts @@ -168,6 +168,8 @@ const errors = { user_exists: 'The user id {{userId}} is already been added to this role', // UNTRANSLATED default_role_missing: 'Some of the default roleNames does not exist in database, please ensure to create roles first', // UNTRANSLATED + internal_role_violation: + 'You may be trying to update or delete an internal role which is forbidden by Logto. If you are creating a new role, try another name that does not start with "#internal:".', // UNTRANSLATED }, scope: { name_exists: 'The scope name {{name}} is already in use', // UNTRANSLATED diff --git a/packages/phrases/src/locales/zh-cn/translation/admin-console/errors.ts b/packages/phrases/src/locales/zh-cn/translation/admin-console/errors.ts index ea2aa2e2b..a8a4eddbb 100644 --- a/packages/phrases/src/locales/zh-cn/translation/admin-console/errors.ts +++ b/packages/phrases/src/locales/zh-cn/translation/admin-console/errors.ts @@ -16,6 +16,8 @@ const errors = { insecure_contexts: '不支持不安全的上下文（非 HTTPS）。', unexpected_error: '发生未知错误', not_found: '404 not found', // UNTRANSLATED + create_internal_role_violation: + 'You are creating a new internal role which is forbidden by Logto. Try another name that does not start with "#internal:".', // UNTRANSLATED }; export default errors; diff --git a/packages/schemas/alterations/next-1678284778-restrict-internal-roles.ts b/packages/schemas/alterations/next-1678284778-restrict-internal-roles.ts new file mode 100644 index 000000000..5c78ea9e8 --- /dev/null +++ b/packages/schemas/alterations/next-1678284778-restrict-internal-roles.ts @@ -0,0 +1,62 @@ +import { sql } from 'slonik'; + +import type { AlterationScript } from '../lib/types/alteration.js'; + +const alteration: AlterationScript = { + up: async (pool) => { + await pool.query(sql` + update roles + set name = '#internal:admin', description = 'Internal admin role for Logto tenant ' || tenant_id || '.' + where name = 'admin' + and tenant_id != 'admin'; + + update roles + set description = 'Admin tenant admin role for Logto tenant ' || substring(name from 0 for strpos(name, ':admin')) || '.' + where name like '%:admin' + and tenant_id = 'admin'; + + -- Restrict direct role modification + create policy roles_select on roles + for select using (true); + + drop policy roles_modification on roles; + create policy roles_modification on roles + using (not starts_with(name, '#internal:')); + + -- Restrict role - scope modification + create policy roles_scopes_select on roles_scopes + for select using (true); + + drop policy roles_scopes_modification on roles_scopes; + create policy roles_scopes_modification on roles_scopes + using (not starts_with((select roles.name from roles where roles.id = role_id), '#internal:')); + `); + }, + down: async (pool) => { + await pool.query(sql` + update roles + set name = 'admin', description = 'Admin role for Logto.' + where name = '#internal:admin' + and tenant_id != 'admin'; + + update roles + set description = 'Admin role for Logto.' + where name like '%:admin' + and tenant_id = 'admin'; + + drop policy roles_select on roles; + drop policy roles_modification on roles; + + create policy roles_modification on roles + using (true); + + drop policy roles_scopes_select on roles_scopes; + drop policy roles_scopes_modification on roles_scopes; + + create policy roles_scopes_modification on roles_scopes + using (true); + `); + }, +}; + +export default alteration; diff --git a/packages/schemas/src/index.ts b/packages/schemas/src/index.ts index cca97da1a..b7c78a658 100644 --- a/packages/schemas/src/index.ts +++ b/packages/schemas/src/index.ts @@ -4,3 +4,4 @@ export * from './types/index.js'; export * from './api/index.js'; export * from './seeds/index.js'; export * from './consts/index.js'; +export * from './utils/index.js'; diff --git a/packages/schemas/src/seeds/cloud-api.ts b/packages/schemas/src/seeds/cloud-api.ts index 712cee7e2..43242522a 100644 --- a/packages/schemas/src/seeds/cloud-api.ts +++ b/packages/schemas/src/seeds/cloud-api.ts @@ -1,7 +1,7 @@ import { generateStandardId } from '@logto/core-kit'; import type { CreateScope } from '../index.js'; -import { UserRole } from '../types/index.js'; +import { AdminTenantRole } from '../types/index.js'; import type { UpdateAdminData } from './management-api.js'; import { adminTenantId } from './tenant.js'; @@ -34,7 +34,7 @@ export const createCloudApi = (): Readonly<[UpdateAdminData, ...CreateScope[]]> scope: buildScope(CloudScope.CreateTenant, 'Allow creating new tenants.'), role: { tenantId: adminTenantId, - name: UserRole.User, + name: AdminTenantRole.User, }, }, buildScope( diff --git a/packages/schemas/src/seeds/management-api.ts b/packages/schemas/src/seeds/management-api.ts index 11d511cd9..e75ae1de2 100644 --- a/packages/schemas/src/seeds/management-api.ts +++ b/packages/schemas/src/seeds/management-api.ts @@ -1,7 +1,7 @@ import { generateStandardId } from '@logto/core-kit'; import type { CreateResource, CreateRole, CreateScope } from '../db-entries/index.js'; -import { PredefinedScope, UserRole } from '../types/index.js'; +import { PredefinedScope, InternalRole, AdminTenantRole } from '../types/index.js'; import { adminTenantId, defaultTenantId } from './tenant.js'; export type AdminData = { @@ -47,8 +47,8 @@ export const defaultManagementApi = Object.freeze({ tenantId: defaultTenantId, /** @deprecated You should not rely on this constant. Change to something else. */ id: 'admin-role', - name: UserRole.Admin, - description: 'Admin role for Logto.', + name: InternalRole.Admin, + description: `Internal admin role for Logto tenant ${defaultTenantId}.`, }, }) satisfies AdminData; @@ -65,7 +65,7 @@ export function getManagementApiResourceIndicator(tenantId: string, path = 'api' } export const getManagementApiAdminName = (tenantId: TenantId) => - `${tenantId}:${UserRole.Admin}` as const; + `${tenantId}:${AdminTenantRole.Admin}` as const; /** Create a set of admin data for Management API of the given tenant ID. */ export const createAdminData = (tenantId: string): AdminData => { @@ -88,8 +88,8 @@ export const createAdminData = (tenantId: string): AdminData => { role: { tenantId, id: generateStandardId(), - name: UserRole.Admin, - description: 'Admin role for Logto.', + name: InternalRole.Admin, + description: `Internal admin role for Logto tenant ${defaultTenantId}.`, }, }); }; @@ -116,7 +116,7 @@ export const createAdminDataInAdminTenant = (tenantId: string): AdminData => { tenantId: adminTenantId, id: generateStandardId(), name: getManagementApiAdminName(tenantId), - description: 'Admin role for Logto.', + description: `Admin tenant admin role for Logto tenant ${tenantId}.`, }, }); }; @@ -141,7 +141,7 @@ export const createMeApiInAdminTenant = (): AdminData => { role: { tenantId: adminTenantId, id: generateStandardId(), - name: UserRole.User, + name: AdminTenantRole.User, description: 'Default role for admin tenant.', }, }); diff --git a/packages/schemas/src/types/user.ts b/packages/schemas/src/types/user.ts index d4b159aa5..9c5deecee 100644 --- a/packages/schemas/src/types/user.ts +++ b/packages/schemas/src/types/user.ts @@ -22,7 +22,17 @@ export type UserInfo roleName.startsWith(internalRolePrefix); diff --git a/packages/schemas/tables/_after_all.sql b/packages/schemas/tables/_after_all.sql index 6f44eef95..513d9cd5a 100644 --- a/packages/schemas/tables/_after_all.sql +++ b/packages/schemas/tables/_after_all.sql @@ -1,30 +1,56 @@ /* This SQL will run after all other queries. */ +---- Grant CRUD access to the group ---- grant select, insert, update, delete on all tables in schema public to logto_tenant_${database}; --- Security policies for tenants table -- +---- Security policies for tenants table ---- revoke all privileges on table tenants from logto_tenant_${database}; -/* Allow limited select to perform the RLS policy query in `after_each` (using select ... from tenants ...) */ +-- Allow limited select to perform the RLS policy query in `after_each` (using select ... from tenants ...) grant select (id, db_user) on table tenants to logto_tenant_${database}; alter table tenants enable row level security; -/* Create RLS policy to minimize the privilege */ +-- Create RLS policy to minimize the privilege create policy tenants_tenant_id on tenants using (db_user = current_user); --- End -- - -/* Revoke all privileges on systems table for tenant roles */ +---- Revoke all privileges on systems table for tenant roles ---- revoke all privileges on table systems from logto_tenant_${database}; + +---- Create policies to make internal roles read-only ---- + +/** + * Note: + * + * Internal roles have scope preset and they are read-only, but we do not + * limit user or application assignment since it's business logic. + */ + +-- Restrict direct role modification +create policy roles_select on roles + for select using (true); + +drop policy roles_modification on roles; +create policy roles_modification on roles + using (not starts_with(name, '#internal:')); + +-- Restrict role - scope modification +create policy roles_scopes_select on roles_scopes + for select using (true); + +drop policy roles_scopes_modification on roles_scopes; +create policy roles_scopes_modification on roles_scopes + using (not starts_with((select roles.name from roles where roles.id = role_id), '#internal:')); + +---- TODO: Make internal API Resources read-only ---- diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 37db9ba6a..6792b726e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -53,6 +53,7 @@ importers: ora: ^6.1.2 p-limit: ^4.0.0 p-retry: ^5.1.2 + pg-protocol: ^1.6.0 prettier: ^2.8.2 roarr: ^7.11.0 semver: ^7.3.8 @@ -79,6 +80,7 @@ importers: ora: 6.1.2 p-limit: 4.0.0 p-retry: 5.1.2 + pg-protocol: 1.6.0 roarr: 7.11.0 semver: 7.3.8 slonik: 30.1.2 @@ -386,6 +388,7 @@ importers: oidc-provider: ^8.0.0 openapi-types: ^12.0.0 p-retry: ^5.1.2 + pg-protocol: ^1.6.0 prettier: ^2.8.2 roarr: ^7.11.0 semver: ^7.3.8 @@ -439,6 +442,7 @@ importers: nanoid: 4.0.0 oidc-provider: 8.0.0 p-retry: 5.1.2 + pg-protocol: 1.6.0 roarr: 7.11.0 semver: 7.3.8 slonik: 30.1.2 @@ -4271,7 +4275,7 @@ packages: resolution: {integrity: sha512-O2xNmXebtwVekJDD+02udOncjVcMZQuTEQEMpKJ0ZRf5E7/9JJX3izhKUcUifBkyKpljyUM6BTgy2trmviKlpw==} dependencies: '@types/node': 18.11.18 - pg-protocol: 1.5.0 + pg-protocol: 1.6.0 pg-types: 2.2.0 /@types/pluralize/0.0.29: @@ -11353,12 +11357,12 @@ packages: dependencies: obuf: 1.1.2 - /pg-cursor/2.7.3_pg@8.7.3: + /pg-cursor/2.7.3_pg@8.8.0: resolution: {integrity: sha512-vmjXRMD4jZK/oHaaYk6clTypgHNlzCCAqyLCO5d/UeI42egJVE5H4ZfZWACub3jzkHUXXyvibH207zAJg9iBOw==} peerDependencies: pg: ^8 dependencies: - pg: 8.7.3 + pg: 8.8.0 /pg-formatter/1.3.0: resolution: {integrity: sha512-y1kNdgD+QWzhmYCm91z/k7VGyx6BekQg6ww/krFEEhw1IIB4zEk2xaB0pmueTcc59YFetpiHIKECgHEuw6gyvg==} @@ -11376,13 +11380,6 @@ packages: resolution: {integrity: sha512-BM/Thnrw5jm2kKLE5uJkXqqExRUY/toLHda65XgFTBTFYZyopbKjBe29Ii3RbkvlsMoFwD+tHeGaCjjv0gHlyw==} engines: {node: '>=4'} - /pg-pool/3.5.1_pg@8.7.3: - resolution: {integrity: sha512-6iCR0wVrro6OOHFsyavV+i6KYL4lVNyYAB9RD18w66xSzN+d8b66HiwuP30Gp1SH5O9T82fckkzsRjlrhD0ioQ==} - peerDependencies: - pg: '>=8.0' - dependencies: - pg: 8.7.3 - /pg-pool/3.5.2_pg@8.8.0: resolution: {integrity: sha512-His3Fh17Z4eg7oANLob6ZvH8xIVen3phEZh2QuyrIl4dQSDVEabNducv6ysROKpDNPSD+12tONZVWfSgMvDD9w==} peerDependencies: @@ -11390,8 +11387,8 @@ packages: dependencies: pg: 8.8.0 - /pg-protocol/1.5.0: - resolution: {integrity: sha512-muRttij7H8TqRNu/DxrAJQITO4Ac7RmX3Klyr/9mJEOBeIpgnF8f9jAfRz5d3XwQZl5qBjF9gLsUtMPJE0vezQ==} + /pg-protocol/1.6.0: + resolution: {integrity: sha512-M+PDm637OY5WM307051+bsDia5Xej6d9IR4GwJse1qA1DIhiKlksvrneZOYQq42OM+spubpcNYEo2FcKQrDk+Q==} /pg-types/2.2.0: resolution: {integrity: sha512-qTAAlrEsl8s4OiEQY69wDvcMIdQN6wdz5ojQiOy6YRMuynxenON0O5oCpJI6lshc6scgAY8qvJ2On/p+CXY0GA==} @@ -11427,23 +11424,6 @@ packages: postgres-interval: 3.0.0 postgres-range: 1.1.2 - /pg/8.7.3: - resolution: {integrity: sha512-HPmH4GH4H3AOprDJOazoIcpI49XFsHCe8xlrjHkWiapdbHK+HLtbm/GQzXYAZwmPju/kzKhjaSfMACG+8cgJcw==} - engines: {node: '>= 8.0.0'} - peerDependencies: - pg-native: '>=2.0.0' - peerDependenciesMeta: - pg-native: - optional: true - dependencies: - buffer-writer: 2.0.0 - packet-reader: 1.0.0 - pg-connection-string: 2.5.0 - pg-pool: 3.5.1_pg@8.7.3 - pg-protocol: 1.5.0 - pg-types: 2.2.0 - pgpass: 1.0.4 - /pg/8.8.0: resolution: {integrity: sha512-UXYN0ziKj+AeNNP7VDMwrehpACThH7LUl/p8TDFpEUuSejCUIwGSfxpHsPvtM6/WXFy6SU4E5RG4IJV/TZAGjw==} engines: {node: '>= 8.0.0'} @@ -11457,7 +11437,7 @@ packages: packet-reader: 1.0.0 pg-connection-string: 2.5.0 pg-pool: 3.5.2_pg@8.8.0 - pg-protocol: 1.5.0 + pg-protocol: 1.6.0 pg-types: 2.2.0 pgpass: 1.0.4 @@ -13045,11 +13025,11 @@ packages: inline-loops.macro: 1.2.2 is-plain-object: 5.0.0 iso8601-duration: 1.3.0 - pg: 8.7.3 + pg: 8.8.0 pg-connection-string: 2.5.0 pg-copy-streams: 5.1.1 pg-copy-streams-binary: 2.2.0 - pg-cursor: 2.7.3_pg@8.7.3 + pg-cursor: 2.7.3_pg@8.8.0 pg-types: 3.0.1 postgres-array: 2.0.0 postgres-interval: 2.1.0 @@ -13073,11 +13053,11 @@ packages: is-plain-object: 5.0.0 iso8601-duration: 1.3.0 p-defer: 3.0.0 - pg: 8.7.3 + pg: 8.8.0 pg-copy-streams: 6.0.2 pg-copy-streams-binary: 2.2.0 - pg-cursor: 2.7.3_pg@8.7.3 - pg-protocol: 1.5.0 + pg-cursor: 2.7.3_pg@8.8.0 + pg-protocol: 1.6.0 pg-types: 4.0.0 postgres-array: 3.0.1 postgres-interval: 4.0.0