From e080e099f2e991bb74b5190d763bd60b36a9a91b Mon Sep 17 00:00:00 2001 From: simeng-li Date: Thu, 14 Mar 2024 17:05:06 +0800 Subject: [PATCH 1/5] fix(schemas): fix the get interation/consent api bug fix the get interation/consent api bug --- .changeset/seven-socks-perform.md | 24 ++++ .../pages/Consent/ScopesListCard/index.tsx | 3 +- .../integration-tests/src/api/interaction.ts | 9 ++ .../integration-tests/src/client/index.ts | 7 +- .../third-party-sign-in/happy-path.test.ts | 134 ++++++++++++++++++ packages/schemas/src/types/consent.ts | 9 +- 6 files changed, 183 insertions(+), 3 deletions(-) create mode 100644 .changeset/seven-socks-perform.md create mode 100644 packages/integration-tests/src/tests/api/interaction/third-party-sign-in/happy-path.test.ts diff --git a/.changeset/seven-socks-perform.md b/.changeset/seven-socks-perform.md new file mode 100644 index 00000000000..8320113b5cb --- /dev/null +++ b/.changeset/seven-socks-perform.md @@ -0,0 +1,24 @@ +--- +"@logto/schemas": patch +--- + +## Fix third-party app get /interaction/consent 500 bug + +### Steps to reproduce + +1. Create a organization scope with empty description, add assign the scope to a third-party app +2. Sign in the third-party app and request for the organization scope +3. Follow the interaction flow till the consent page +4. A internal server error 500 is returned + +### Root cause + +For the get /interaction/consent endpoint, organization scope is returned with other resource scopes in the `missingResourceScopes` property. + +In the `consentInfoResponseGuard`, we use the resource `Scopes` zod guard to validate the `missingResourceScopes` property. However, the description field in resource scope is required. A organization scope with empty description will fail the validation. + +### Fix + +Update the `consentInfoResponseGuard`'s missingResourceScopes property. Use the organization scope zod guard which does not require the description field. + +The alignment of the resource scope and organization scope type will be handled in the next release. diff --git a/packages/experience/src/pages/Consent/ScopesListCard/index.tsx b/packages/experience/src/pages/Consent/ScopesListCard/index.tsx index 15c20614fe2..a4d1d042000 100644 --- a/packages/experience/src/pages/Consent/ScopesListCard/index.tsx +++ b/packages/experience/src/pages/Consent/ScopesListCard/index.tsx @@ -1,5 +1,6 @@ import { ReservedResource, UserScope } from '@logto/core-kit'; import { type ConsentInfoResponse } from '@logto/schemas'; +import { type Nullable } from '@silverhand/essentials'; import classNames from 'classnames'; import { useCallback, useMemo, useState } from 'react'; import { Trans, useTranslation } from 'react-i18next'; @@ -20,7 +21,7 @@ type ScopeGroupProps = { scopes: Array<{ id: string; name: string; - description?: string; + description?: Nullable; // Organization scopes description is nullable }>; }; diff --git a/packages/integration-tests/src/api/interaction.ts b/packages/integration-tests/src/api/interaction.ts index a355b5e6783..555067d96d7 100644 --- a/packages/integration-tests/src/api/interaction.ts +++ b/packages/integration-tests/src/api/interaction.ts @@ -5,6 +5,7 @@ import type { RequestVerificationCodePayload, BindMfaPayload, VerifyMfaPayload, + ConsentInfoResponse, } from '@logto/schemas'; import type { Got } from 'got'; @@ -154,6 +155,14 @@ export const consent = async (api: Got, cookie: string) => }) .json(); +export const getConsentInfo = async (cookie: string) => + api + .get('interaction/consent', { + headers: { cookie }, + followRedirect: false, + }) + .json(); + export const createSingleSignOnAuthorizationUri = async ( cookie: string, payload: SocialAuthorizationUriPayload diff --git a/packages/integration-tests/src/client/index.ts b/packages/integration-tests/src/client/index.ts index 4d71a2c479a..ffda218e5b7 100644 --- a/packages/integration-tests/src/client/index.ts +++ b/packages/integration-tests/src/client/index.ts @@ -84,7 +84,7 @@ export default class MockClient { assert(this.interactionCookie, new Error('Get cookie from authorization endpoint failed')); } - public async processSession(redirectTo: string) { + public async processSession(redirectTo: string, consent = true) { // Note: should redirect to OIDC auth endpoint assert( redirectTo.startsWith(`${this.config.endpoint}/oidc/auth`), @@ -106,6 +106,11 @@ export default class MockClient { this.rawCookies = authResponse.headers['set-cookie'] ?? []; + // Manually handle the consent flow + if (!consent) { + return; + } + const signInCallbackUri = await this.consent(); await this.logto.handleSignInCallback(signInCallbackUri); } diff --git a/packages/integration-tests/src/tests/api/interaction/third-party-sign-in/happy-path.test.ts b/packages/integration-tests/src/tests/api/interaction/third-party-sign-in/happy-path.test.ts new file mode 100644 index 00000000000..44d998e3455 --- /dev/null +++ b/packages/integration-tests/src/tests/api/interaction/third-party-sign-in/happy-path.test.ts @@ -0,0 +1,134 @@ +import { ReservedResource, UserScope } from '@logto/core-kit'; +import { type Application, InteractionEvent, ApplicationType } from '@logto/schemas'; +import { assert } from '@silverhand/essentials'; + +import { deleteUser } from '#src/api/admin-user.js'; +import { assignUserConsentScopes } from '#src/api/application-user-consent-scope.js'; +import { createApplication, deleteApplication } from '#src/api/application.js'; +import { getConsentInfo, putInteraction } from '#src/api/interaction.js'; +import { OrganizationScopeApi } from '#src/api/organization-scope.js'; +import { initClient } from '#src/helpers/client.js'; +import { enableAllPasswordSignInMethods } from '#src/helpers/sign-in-experience.js'; +import { generateNewUser } from '#src/helpers/user.js'; + +describe('consent api', () => { + const applications = new Map(); + const thirdPartyApplicationName = 'consent-third-party-app'; + const redirectUri = 'http://example.com'; + + const bootStrapApplication = async () => { + const thirdPartyApplication = await createApplication( + thirdPartyApplicationName, + ApplicationType.Traditional, + { + isThirdParty: true, + oidcClientMetadata: { + redirectUris: [redirectUri], + postLogoutRedirectUris: [], + }, + } + ); + + applications.set(thirdPartyApplicationName, thirdPartyApplication); + + await assignUserConsentScopes(thirdPartyApplication.id, { + userScopes: [UserScope.Profile], + }); + }; + + beforeAll(async () => { + await Promise.all([enableAllPasswordSignInMethods(), bootStrapApplication()]); + }); + + it('get consent info', async () => { + const application = applications.get(thirdPartyApplicationName); + assert(application, new Error('application.not_found')); + + const { userProfile, user } = await generateNewUser({ username: true, password: true }); + + const client = await initClient( + { + appId: application.id, + appSecret: application.secret, + }, + redirectUri + ); + + await client.successSend(putInteraction, { + event: InteractionEvent.SignIn, + identifier: { + username: userProfile.username, + password: userProfile.password, + }, + }); + + const { redirectTo } = await client.submitInteraction(); + + await client.processSession(redirectTo, false); + + const result = await client.send(getConsentInfo); + + expect(result.application.id).toBe(application.id); + expect(result.user.id).toBe(user.id); + expect(result.redirectUri).toBe(redirectUri); + expect(result.missingOIDCScope).toEqual([UserScope.Profile]); + + await deleteUser(user.id); + }); + + it('get consent info with organization scope', async () => { + const application = applications.get(thirdPartyApplicationName); + assert(application, new Error('application.not_found')); + + const organizationScopeApi = new OrganizationScopeApi(); + + const organizationScope = await organizationScopeApi.create({ + name: 'organization-scope', + }); + + await assignUserConsentScopes(application.id, { + organizationScopes: [organizationScope.id], + userScopes: [UserScope.Organizations], + }); + + const { userProfile, user } = await generateNewUser({ username: true, password: true }); + + const client = await initClient( + { + appId: application.id, + appSecret: application.secret, + scopes: [UserScope.Organizations, UserScope.Profile, organizationScope.name], + }, + redirectUri + ); + + await client.successSend(putInteraction, { + event: InteractionEvent.SignIn, + identifier: { + username: userProfile.username, + password: userProfile.password, + }, + }); + + const { redirectTo } = await client.submitInteraction(); + + await client.processSession(redirectTo, false); + + const result = await client.send(getConsentInfo); + + expect( + result.missingResourceScopes?.find( + ({ resource }) => resource.name === ReservedResource.Organization + ) + ).not.toBeUndefined(); + + await organizationScopeApi.delete(organizationScope.id); + await deleteUser(user.id); + }); + + afterAll(async () => { + for (const application of applications.values()) { + void deleteApplication(application.id); + } + }); +}); diff --git a/packages/schemas/src/types/consent.ts b/packages/schemas/src/types/consent.ts index a872fe5a3d2..5e5088536ca 100644 --- a/packages/schemas/src/types/consent.ts +++ b/packages/schemas/src/types/consent.ts @@ -7,6 +7,7 @@ import { Resources, Scopes, ApplicationSignInExperiences, + OrganizationScopes, } from '../db-entries/index.js'; /** @@ -44,11 +45,17 @@ export const publicOrganizationGuard = Organizations.guard.pick({ id: true, name: true, }); + export const missingResourceScopesGuard = z.object({ // The original resource id has a maximum length of 21 restriction. We need to make it compatible with the logto reserved organization name. // use string here, as we do not care about the resource id length here. resource: Resources.guard.pick({ name: true }).extend({ id: z.string() }), - scopes: Scopes.guard.pick({ id: true, name: true, description: true }).array(), + scopes: Scopes.guard + .pick({ id: true, name: true }) + // The description is optional for organization scopes. Manually extend the schema to make it optional. + // TODO: make the resource scopes description optional at the schema level. + .merge(OrganizationScopes.guard.pick({ description: true }).partial()) + .array(), }); /** From 437d1ed5e992c2e3d67a83e96bacc03bd17070b8 Mon Sep 17 00:00:00 2001 From: simeng-li Date: Thu, 14 Mar 2024 17:09:12 +0800 Subject: [PATCH 2/5] chore: update changeset update changeset --- .changeset/seven-socks-perform.md | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/.changeset/seven-socks-perform.md b/.changeset/seven-socks-perform.md index 8320113b5cb..f5ab6209d90 100644 --- a/.changeset/seven-socks-perform.md +++ b/.changeset/seven-socks-perform.md @@ -2,23 +2,26 @@ "@logto/schemas": patch --- -## Fix third-party app get /interaction/consent 500 bug +## Resolve Third-Party App's /interaction/consent Endpoint 500 Error -### Steps to reproduce +### Reproduction Steps -1. Create a organization scope with empty description, add assign the scope to a third-party app -2. Sign in the third-party app and request for the organization scope -3. Follow the interaction flow till the consent page -4. A internal server error 500 is returned +- Establish an organization scope with an empty description and assign this scope to a third-party application. -### Root cause +- Login to the third-party application and request the organization scope. -For the get /interaction/consent endpoint, organization scope is returned with other resource scopes in the `missingResourceScopes` property. +- Proceed through the interaction flow until reaching the consent page. -In the `consentInfoResponseGuard`, we use the resource `Scopes` zod guard to validate the `missingResourceScopes` property. However, the description field in resource scope is required. A organization scope with empty description will fail the validation. +- An internal server error 500 is returned. -### Fix +### Root Cause -Update the `consentInfoResponseGuard`'s missingResourceScopes property. Use the organization scope zod guard which does not require the description field. +For the /interaction/consent endpoint, the organization scope is returned alongside other resource scopes in the missingResourceScopes property. -The alignment of the resource scope and organization scope type will be handled in the next release. +In the consentInfoResponseGuard, we utilize the resource Scopes zod guard to validate the missingResourceScopes property. However, the description field in the resource scope is mandatory while organization scopes'description is optional. An organization scope with an empty description will not pass the validation. + +### Solution + +Modify the consentInfoResponseGuard's missingResourceScopes property. Use the organization scope zod guard which does not necessitate the description field. + +The alignment of the resource scope and organization scope types will be addressed in the next release. From 8d84b5e166118f4768ec7b08a0a44506851e9c70 Mon Sep 17 00:00:00 2001 From: simeng-li Date: Mon, 18 Mar 2024 13:42:46 +0800 Subject: [PATCH 3/5] fix: update changeset update changeset --- .changeset/seven-socks-perform.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.changeset/seven-socks-perform.md b/.changeset/seven-socks-perform.md index f5ab6209d90..75d9e2761d1 100644 --- a/.changeset/seven-socks-perform.md +++ b/.changeset/seven-socks-perform.md @@ -4,9 +4,9 @@ ## Resolve Third-Party App's /interaction/consent Endpoint 500 Error -### Reproduction Steps +### Reproduction steps -- Establish an organization scope with an empty description and assign this scope to a third-party application. +- Create an organization scope with an empty description and assign this scope to a third-party application. - Login to the third-party application and request the organization scope. @@ -16,12 +16,12 @@ ### Root Cause -For the /interaction/consent endpoint, the organization scope is returned alongside other resource scopes in the missingResourceScopes property. +For the `/interaction/consent` endpoint, the organization scope is returned alongside other resource scopes in the `missingResourceScopes` property. -In the consentInfoResponseGuard, we utilize the resource Scopes zod guard to validate the missingResourceScopes property. However, the description field in the resource scope is mandatory while organization scopes'description is optional. An organization scope with an empty description will not pass the validation. +In the `consentInfoResponseGuard`, we utilize the resource Scopes zod guard to validate the `missingResourceScopes` property. However, the description field in the resource scope is mandatory while organization scopes'description is optional. An organization scope with an empty description will not pass the validation. ### Solution -Modify the consentInfoResponseGuard's missingResourceScopes property. Use the organization scope zod guard which does not necessitate the description field. +Modify the consentInfoResponseGuard's `missingResourceScopes` property. Use the organization scope zod guard which does not necessitate the description field. The alignment of the resource scope and organization scope types will be addressed in the next release. From 3be157e4bb78c302d21eee8e16438f106062df2b Mon Sep 17 00:00:00 2001 From: simeng-li Date: Wed, 20 Mar 2024 11:04:48 +0800 Subject: [PATCH 4/5] refactor(schemas, console): alter the resource scopes description field to nullable (#5504) * refactor(schemas, console): alter the resoruce scopes description field nullable make the resourec scopes description nullable * fix(test): fix the type issue in the integration test fix the type issue in the integration test * fix(console): add the field register add the field register * fix: update the changeset update the changeset --- .changeset/seven-socks-perform.md | 8 +++---- .../CreatePermissionModal/index.tsx | 8 +++---- .../src/tests/api/resource.scope.test.ts | 4 ++-- ...ke-resource-scopes-description-nullable.ts | 22 +++++++++++++++++++ packages/schemas/src/types/consent.ts | 8 +------ packages/schemas/tables/scopes.sql | 2 +- 6 files changed, 33 insertions(+), 19 deletions(-) create mode 100644 packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts diff --git a/.changeset/seven-socks-perform.md b/.changeset/seven-socks-perform.md index 75d9e2761d1..957b5ca8425 100644 --- a/.changeset/seven-socks-perform.md +++ b/.changeset/seven-socks-perform.md @@ -2,7 +2,7 @@ "@logto/schemas": patch --- -## Resolve Third-Party App's /interaction/consent Endpoint 500 Error +## Resolve third-party app's /interaction/consent endpoint 500 error ### Reproduction steps @@ -14,7 +14,7 @@ - An internal server error 500 is returned. -### Root Cause +### Root cause For the `/interaction/consent` endpoint, the organization scope is returned alongside other resource scopes in the `missingResourceScopes` property. @@ -22,6 +22,4 @@ In the `consentInfoResponseGuard`, we utilize the resource Scopes zod guard to v ### Solution -Modify the consentInfoResponseGuard's `missingResourceScopes` property. Use the organization scope zod guard which does not necessitate the description field. - -The alignment of the resource scope and organization scope types will be addressed in the next release. +Alter the resource scopes table to make the description field nullable. Related Scope zod guard and the consentInfoResponseGuard will be updated to reflect this change. Align the resource scopes table with the organization scopes table to ensure consistency. diff --git a/packages/console/src/pages/ApiResourceDetails/ApiResourcePermissions/components/CreatePermissionModal/index.tsx b/packages/console/src/pages/ApiResourceDetails/ApiResourcePermissions/components/CreatePermissionModal/index.tsx index 4efd0b508b8..39326b11b86 100644 --- a/packages/console/src/pages/ApiResourceDetails/ApiResourcePermissions/components/CreatePermissionModal/index.tsx +++ b/packages/console/src/pages/ApiResourceDetails/ApiResourcePermissions/components/CreatePermissionModal/index.tsx @@ -1,5 +1,5 @@ import { noSpaceRegEx } from '@logto/core-kit'; -import type { Scope } from '@logto/schemas'; +import type { Scope, CreateScope } from '@logto/schemas'; import { useContext } from 'react'; import { useForm } from 'react-hook-form'; import { Trans, useTranslation } from 'react-i18next'; @@ -24,7 +24,7 @@ type Props = { onClose: (scope?: Scope) => void; }; -type CreatePermissionFormData = Pick; +type CreatePermissionFormData = Pick; function CreatePermissionModal({ resourceId, totalResourceCount, onClose }: Props) { const { currentPlan } = useContext(SubscriptionDataContext); @@ -118,10 +118,10 @@ function CreatePermissionModal({ resourceId, totalResourceCount, onClose }: Prop error={errors.name?.message} /> - + diff --git a/packages/integration-tests/src/tests/api/resource.scope.test.ts b/packages/integration-tests/src/tests/api/resource.scope.test.ts index 2e65ddbdf39..f098f9907f7 100644 --- a/packages/integration-tests/src/tests/api/resource.scope.test.ts +++ b/packages/integration-tests/src/tests/api/resource.scope.test.ts @@ -5,7 +5,7 @@ import { HTTPError } from 'got'; import { createResource } from '#src/api/index.js'; import { createScope, deleteScope, getScopes, updateScope } from '#src/api/scope.js'; -import { generateScopeName } from '#src/utils.js'; +import { generateName, generateScopeName } from '#src/utils.js'; describe('scopes', () => { it('should get management api resource scopes successfully', async () => { @@ -63,7 +63,7 @@ describe('scopes', () => { expect(scope).toBeTruthy(); const newScopeName = `new_${scope.name}`; - const newScopeDescription = `new_${scope.description}`; + const newScopeDescription = scope.description ? `new_${scope.description}` : generateName(); const updatesScope = await updateScope(resource.id, scope.id, { name: newScopeName, diff --git a/packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts b/packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts new file mode 100644 index 00000000000..d6c6a0c5552 --- /dev/null +++ b/packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts @@ -0,0 +1,22 @@ +import { sql } from 'slonik'; + +import type { AlterationScript } from '../lib/types/alteration.js'; + +const alteration: AlterationScript = { + up: async (pool) => { + // Make the resource scopes description nullable + await pool.query(sql` + alter table scopes + alter column description drop not null; + `); + }, + down: async (pool) => { + // Revert the resource scopes description nullable + await pool.query(sql` + alter table scopes + alter column description set not null; + `); + }, +}; + +export default alteration; diff --git a/packages/schemas/src/types/consent.ts b/packages/schemas/src/types/consent.ts index 5e5088536ca..d1e58468df9 100644 --- a/packages/schemas/src/types/consent.ts +++ b/packages/schemas/src/types/consent.ts @@ -7,7 +7,6 @@ import { Resources, Scopes, ApplicationSignInExperiences, - OrganizationScopes, } from '../db-entries/index.js'; /** @@ -50,12 +49,7 @@ export const missingResourceScopesGuard = z.object({ // The original resource id has a maximum length of 21 restriction. We need to make it compatible with the logto reserved organization name. // use string here, as we do not care about the resource id length here. resource: Resources.guard.pick({ name: true }).extend({ id: z.string() }), - scopes: Scopes.guard - .pick({ id: true, name: true }) - // The description is optional for organization scopes. Manually extend the schema to make it optional. - // TODO: make the resource scopes description optional at the schema level. - .merge(OrganizationScopes.guard.pick({ description: true }).partial()) - .array(), + scopes: Scopes.guard.pick({ id: true, name: true, description: true }).array(), }); /** diff --git a/packages/schemas/tables/scopes.sql b/packages/schemas/tables/scopes.sql index 74f1d4a8919..954009d2c0b 100644 --- a/packages/schemas/tables/scopes.sql +++ b/packages/schemas/tables/scopes.sql @@ -7,7 +7,7 @@ create table scopes ( resource_id varchar(21) not null references resources (id) on update cascade on delete cascade, name varchar(256) not null, - description text not null, + description text, created_at timestamptz not null default(now()), primary key (id), constraint scopes__resource_id_name From 71c2116338a9609e50b6328ca7c32be7256cdf33 Mon Sep 17 00:00:00 2001 From: simeng-li Date: Wed, 20 Mar 2024 13:31:04 +0800 Subject: [PATCH 5/5] fix(console,test): update comments and rebase update comments and rebase the master --- .../experience/src/pages/Consent/ScopesListCard/index.tsx | 2 +- packages/integration-tests/src/client/index.ts | 5 +++++ ...t-1710408335-make-resource-scopes-description-nullable.ts | 2 +- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/experience/src/pages/Consent/ScopesListCard/index.tsx b/packages/experience/src/pages/Consent/ScopesListCard/index.tsx index a4d1d042000..e5bcbd60694 100644 --- a/packages/experience/src/pages/Consent/ScopesListCard/index.tsx +++ b/packages/experience/src/pages/Consent/ScopesListCard/index.tsx @@ -21,7 +21,7 @@ type ScopeGroupProps = { scopes: Array<{ id: string; name: string; - description?: Nullable; // Organization scopes description is nullable + description?: Nullable; // Organization scope description cloud be `null` }>; }; diff --git a/packages/integration-tests/src/client/index.ts b/packages/integration-tests/src/client/index.ts index ffda218e5b7..8026507598c 100644 --- a/packages/integration-tests/src/client/index.ts +++ b/packages/integration-tests/src/client/index.ts @@ -84,6 +84,11 @@ export default class MockClient { assert(this.interactionCookie, new Error('Get cookie from authorization endpoint failed')); } + /** + * + * @param {string} redirectTo the sign-in or register redirect uri + * @param {boolean} [consent=true] whether to automatically consent. Need to manually handle the consent flow if set to false + */ public async processSession(redirectTo: string, consent = true) { // Note: should redirect to OIDC auth endpoint assert( diff --git a/packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts b/packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts index d6c6a0c5552..23c93255690 100644 --- a/packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts +++ b/packages/schemas/alterations/next-1710408335-make-resource-scopes-description-nullable.ts @@ -1,4 +1,4 @@ -import { sql } from 'slonik'; +import { sql } from '@silverhand/slonik'; import type { AlterationScript } from '../lib/types/alteration.js';