From beeff09d6f3a2b3148217dd1ad7e6e028a09df6d Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Mon, 17 Jul 2023 17:09:07 +0200 Subject: [PATCH 1/7] accept null ssoLockedRoles --- .../pages/SingleSignOn/utils/schema.js | 13 ++++++----- .../ee/server/validation/authentication.js | 22 ++++++++++++------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/packages/core/admin/ee/admin/pages/SettingsPage/pages/SingleSignOn/utils/schema.js b/packages/core/admin/ee/admin/pages/SettingsPage/pages/SingleSignOn/utils/schema.js index 89136c0717..515a111c07 100644 --- a/packages/core/admin/ee/admin/pages/SettingsPage/pages/SingleSignOn/utils/schema.js +++ b/packages/core/admin/ee/admin/pages/SettingsPage/pages/SingleSignOn/utils/schema.js @@ -6,11 +6,14 @@ const schema = yup.object().shape({ defaultRole: yup.mixed().when('autoRegister', (value, initSchema) => { return value ? initSchema.required(translatedErrors.required) : initSchema.nullable(); }), - ssoLockedRoles: yup.array().of( - yup.mixed().when('ssoLockedRoles', (value, initSchema) => { - return value ? initSchema.required(translatedErrors.required) : initSchema.nullable(); - }) - ), + ssoLockedRoles: yup + .array() + .nullable() + .of( + yup.mixed().when('ssoLockedRoles', (value, initSchema) => { + return value ? initSchema.required(translatedErrors.required) : initSchema.nullable(); + }) + ), }); export default schema; diff --git a/packages/core/admin/ee/server/validation/authentication.js b/packages/core/admin/ee/server/validation/authentication.js index 351a65fed9..c498a4b10c 100644 --- a/packages/core/admin/ee/server/validation/authentication.js +++ b/packages/core/admin/ee/server/validation/authentication.js @@ -10,14 +10,20 @@ const providerOptionsUpdateSchema = yup.object().shape({ .test('is-valid-role', 'You must submit a valid default role', (roleId) => { return strapi.admin.services.role.exists({ id: roleId }); }), - ssoLockedRoles: yup.array().of( - yup - .strapiID() - .required() - .test('is-valid-role', 'You must submit a valid role for the SSO Locked roles', (roleId) => { - return strapi.admin.services.role.exists({ id: roleId }); - }) - ), + ssoLockedRoles: yup + .array() + .nullable() + .of( + yup + .strapiID() + .test( + 'is-valid-role', + 'You must submit a valid role for the SSO Locked roles', + (roleId) => { + return strapi.admin.services.role.exists({ id: roleId }); + } + ) + ), }); module.exports = { From 52b740ebeee62b576cfd861de802883551bad47e Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 18 Jul 2023 13:22:20 +0200 Subject: [PATCH 2/7] add provider options tests --- .../admin/ee/provider-options.test.api.js | 142 ++++++++++++++++++ 1 file changed, 142 insertions(+) create mode 100644 api-tests/core/admin/ee/provider-options.test.api.js diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js new file mode 100644 index 0000000000..3dd0b17cd6 --- /dev/null +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -0,0 +1,142 @@ +'use strict'; + +const { createStrapiInstance } = require('api-tests/strapi'); +const { createAuthRequest, createRequest } = require('api-tests/request'); +const { createUtils, describeOnCondition } = require('api-tests/utils'); + +const edition = process.env.STRAPI_DISABLE_EE === 'true' ? 'CE' : 'EE'; + +let strapi; +let utils; +const requests = { + public: undefined, + admin: undefined, + noPermissions: undefined, +}; + +const localData = { + restrictedUser: null, + restrictedRole: null, +}; + +const restrictedUser = { + email: 'restricted@user.io', + password: 'Restricted123', +}; + +const restrictedRole = { + name: 'restricted-role', + description: '', +}; + +const createFixtures = async () => { + const role = await utils.createRole(restrictedRole); + const user = await utils.createUserIfNotExists({ + ...restrictedUser, + roles: [role.id], + }); + + localData.restrictedUser = user; + localData.restrictedRole = role; + + return { role, user }; +}; + +const deleteFixtures = async () => { + await utils.deleteUserById(localData.restrictedUser.id); + await utils.deleteRolesById([localData.restrictedRole.id]); +}; + +describeOnCondition(edition === 'EE')('SSO Provider Options', () => { + let hasSSO; + + beforeAll(async () => { + strapi = await createStrapiInstance(); + utils = createUtils(strapi); + // eslint-disable-next-line node/no-extraneous-require + hasSSO = require('@strapi/strapi/lib/utils/ee').features.isEnabled('sso'); + + await createFixtures(); + + requests.public = createRequest({ strapi }); + requests.admin = await createAuthRequest({ strapi }); + requests.noPermissions = await createAuthRequest({ strapi, userInfo: restrictedUser }); + }); + + afterAll(async () => { + await deleteFixtures(); + await strapi.destroy(); + }); + + describe('Get provider options', () => { + test('Get the provider options as public user gives 401', async () => { + const res = await requests.public.get('/admin/providers/options'); + expect(res.status).toEqual(401); + }); + + test('Get the provider options with no permissions gives 403', async () => { + const res = await requests.noPermissions.get('/admin/providers/options'); + expect(res.status).toEqual(403); + }); + + test('Get the provider options as admin succeeds', async () => { + const res = await requests.admin.get('/admin/providers/options'); + if (hasSSO) { + expect(res.status).toBe(200); + const { data } = JSON.parse(res.text); + const keys = Object.keys(data); + expect(keys).toContain('autoRegister'); + expect(keys).toContain('ssoLockedRoles'); + expect(keys).toContain('defaultRole'); + } else { + expect(res.status).toBe(404); + expect(Array.isArray(res.body)).toBeFalsy(); + } + }); + }); + + describe('ssoLockedRoles', () => { + test.each([ + ['empty array', []], + ['null', null], + ['array of roles', [1]], + ])('can be %s', async (name, value) => { + const newData = { + ssoLockedRoles: value, + defaultRole: 1, // TODO: there seems to be a bug with not setting a default role + autoRegister: false, + }; + const res = await requests.admin.put('/admin/providers/options', { + body: newData, + }); + if (hasSSO) { + expect(res.status).toEqual(200); + const parsed = JSON.parse(res.text); + expect(parsed.data).toMatchObject(newData); + } else { + expect(res.status).toBe(404); + expect(Array.isArray(res.body)).toBeFalsy(); + } + }); + + test.each([ + ['invalid role id', [999]], + ['string', '1'], + ['object', { role: 1 }], + ])('cannot be %s', async (name, value) => { + const res = await requests.admin.put('/admin/providers/options', { + body: { + ssoLockedRoles: value, + defaultRole: localData.restrictedRole.id, + autoRegister: false, + }, + }); + if (hasSSO) { + expect(res.status).toEqual(400); + } else { + expect(res.status).toBe(404); + expect(Array.isArray(res.body)).toBeFalsy(); + } + }); + }); +}); From f33ad9cc2966f3de465426c7be45d16aa874159b Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 18 Jul 2023 13:26:22 +0200 Subject: [PATCH 3/7] fix defaultRole and comment --- api-tests/core/admin/ee/provider-options.test.api.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js index 3dd0b17cd6..f06fcff90e 100644 --- a/api-tests/core/admin/ee/provider-options.test.api.js +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -103,7 +103,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { ])('can be %s', async (name, value) => { const newData = { ssoLockedRoles: value, - defaultRole: 1, // TODO: there seems to be a bug with not setting a default role + defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role autoRegister: false, }; const res = await requests.admin.put('/admin/providers/options', { @@ -127,7 +127,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { const res = await requests.admin.put('/admin/providers/options', { body: { ssoLockedRoles: value, - defaultRole: localData.restrictedRole.id, + defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role autoRegister: false, }, }); From 64c995d4ccf5681bac9b10e575ce808f6e17d120 Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 18 Jul 2023 17:32:51 +0200 Subject: [PATCH 4/7] allow null defaultRole on backend --- .../admin/ee/provider-options.test.api.js | 48 ++++++++++++++++++- .../ee/server/validation/authentication.js | 7 ++- 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js index f06fcff90e..bb7488244a 100644 --- a/api-tests/core/admin/ee/provider-options.test.api.js +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -103,7 +103,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { ])('can be %s', async (name, value) => { const newData = { ssoLockedRoles: value, - defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role + defaultRole: null, autoRegister: false, }; const res = await requests.admin.put('/admin/providers/options', { @@ -127,7 +127,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { const res = await requests.admin.put('/admin/providers/options', { body: { ssoLockedRoles: value, - defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role + defaultRole: null, autoRegister: false, }, }); @@ -138,5 +138,49 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { expect(Array.isArray(res.body)).toBeFalsy(); } }); + + describe('autoRegister and defaultRole', () => { + test.each([ + [null, false], + [1, false], + [1, true], + ])('defaultRole can be %s when autoRegister is %s', async (defaultRole, autoRegister) => { + const newData = { + defaultRole, + autoRegister, + }; + const res = await requests.admin.put('/admin/providers/options', { + body: newData, + }); + if (hasSSO) { + expect(res.status).toEqual(200); + const parsed = JSON.parse(res.text); + expect(parsed.data).toMatchObject(newData); + } else { + expect(res.status).toBe(404); + expect(Array.isArray(res.body)).toBeFalsy(); + } + }); + + test.each([ + [null, true], + [{}, true], + [9999, true], + ])('defaultRole cannot be %s when autoRegister is %s', async (defaultRole, autoRegister) => { + const newData = { + defaultRole, + autoRegister, + }; + const res = await requests.admin.put('/admin/providers/options', { + body: newData, + }); + if (hasSSO) { + expect(res.status).toEqual(400); + } else { + expect(res.status).toBe(404); + expect(Array.isArray(res.body)).toBeFalsy(); + } + }); + }); }); }); diff --git a/packages/core/admin/ee/server/validation/authentication.js b/packages/core/admin/ee/server/validation/authentication.js index c498a4b10c..00653c198c 100644 --- a/packages/core/admin/ee/server/validation/authentication.js +++ b/packages/core/admin/ee/server/validation/authentication.js @@ -6,8 +6,13 @@ const providerOptionsUpdateSchema = yup.object().shape({ autoRegister: yup.boolean().required(), defaultRole: yup .strapiID() - .required() + .when('autoRegister', (value, initSchema) => { + return value ? initSchema.required() : initSchema.nullable(); + }) .test('is-valid-role', 'You must submit a valid default role', (roleId) => { + if (roleId === null) { + return true; + } return strapi.admin.services.role.exists({ id: roleId }); }), ssoLockedRoles: yup From ae67e82f26aa6467fe6bc19aabcb405b2831602a Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 18 Jul 2023 17:37:13 +0200 Subject: [PATCH 5/7] Revert "allow null defaultRole on backend" This reverts commit 64c995d4ccf5681bac9b10e575ce808f6e17d120. --- .../admin/ee/provider-options.test.api.js | 48 +------------------ .../ee/server/validation/authentication.js | 7 +-- 2 files changed, 3 insertions(+), 52 deletions(-) diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js index bb7488244a..f06fcff90e 100644 --- a/api-tests/core/admin/ee/provider-options.test.api.js +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -103,7 +103,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { ])('can be %s', async (name, value) => { const newData = { ssoLockedRoles: value, - defaultRole: null, + defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role autoRegister: false, }; const res = await requests.admin.put('/admin/providers/options', { @@ -127,7 +127,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { const res = await requests.admin.put('/admin/providers/options', { body: { ssoLockedRoles: value, - defaultRole: null, + defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role autoRegister: false, }, }); @@ -138,49 +138,5 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { expect(Array.isArray(res.body)).toBeFalsy(); } }); - - describe('autoRegister and defaultRole', () => { - test.each([ - [null, false], - [1, false], - [1, true], - ])('defaultRole can be %s when autoRegister is %s', async (defaultRole, autoRegister) => { - const newData = { - defaultRole, - autoRegister, - }; - const res = await requests.admin.put('/admin/providers/options', { - body: newData, - }); - if (hasSSO) { - expect(res.status).toEqual(200); - const parsed = JSON.parse(res.text); - expect(parsed.data).toMatchObject(newData); - } else { - expect(res.status).toBe(404); - expect(Array.isArray(res.body)).toBeFalsy(); - } - }); - - test.each([ - [null, true], - [{}, true], - [9999, true], - ])('defaultRole cannot be %s when autoRegister is %s', async (defaultRole, autoRegister) => { - const newData = { - defaultRole, - autoRegister, - }; - const res = await requests.admin.put('/admin/providers/options', { - body: newData, - }); - if (hasSSO) { - expect(res.status).toEqual(400); - } else { - expect(res.status).toBe(404); - expect(Array.isArray(res.body)).toBeFalsy(); - } - }); - }); }); }); diff --git a/packages/core/admin/ee/server/validation/authentication.js b/packages/core/admin/ee/server/validation/authentication.js index 00653c198c..c498a4b10c 100644 --- a/packages/core/admin/ee/server/validation/authentication.js +++ b/packages/core/admin/ee/server/validation/authentication.js @@ -6,13 +6,8 @@ const providerOptionsUpdateSchema = yup.object().shape({ autoRegister: yup.boolean().required(), defaultRole: yup .strapiID() - .when('autoRegister', (value, initSchema) => { - return value ? initSchema.required() : initSchema.nullable(); - }) + .required() .test('is-valid-role', 'You must submit a valid default role', (roleId) => { - if (roleId === null) { - return true; - } return strapi.admin.services.role.exists({ id: roleId }); }), ssoLockedRoles: yup From 88f99a54f7f62bf74b8662739422a0d219643c0f Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 18 Jul 2023 17:40:11 +0200 Subject: [PATCH 6/7] Revert "fix defaultRole and comment" This reverts commit f33ad9cc2966f3de465426c7be45d16aa874159b. --- api-tests/core/admin/ee/provider-options.test.api.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js index f06fcff90e..3dd0b17cd6 100644 --- a/api-tests/core/admin/ee/provider-options.test.api.js +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -103,7 +103,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { ])('can be %s', async (name, value) => { const newData = { ssoLockedRoles: value, - defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role + defaultRole: 1, // TODO: there seems to be a bug with not setting a default role autoRegister: false, }; const res = await requests.admin.put('/admin/providers/options', { @@ -127,7 +127,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { const res = await requests.admin.put('/admin/providers/options', { body: { ssoLockedRoles: value, - defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role + defaultRole: localData.restrictedRole.id, autoRegister: false, }, }); From dda8543003c77ae9767c7a427fe4b97dd7d6d70d Mon Sep 17 00:00:00 2001 From: Ben Irvin Date: Tue, 18 Jul 2023 17:48:07 +0200 Subject: [PATCH 7/7] fix defaultRole and comment --- api-tests/core/admin/ee/provider-options.test.api.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api-tests/core/admin/ee/provider-options.test.api.js b/api-tests/core/admin/ee/provider-options.test.api.js index 3dd0b17cd6..f06fcff90e 100644 --- a/api-tests/core/admin/ee/provider-options.test.api.js +++ b/api-tests/core/admin/ee/provider-options.test.api.js @@ -103,7 +103,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { ])('can be %s', async (name, value) => { const newData = { ssoLockedRoles: value, - defaultRole: 1, // TODO: there seems to be a bug with not setting a default role + defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role autoRegister: false, }; const res = await requests.admin.put('/admin/providers/options', { @@ -127,7 +127,7 @@ describeOnCondition(edition === 'EE')('SSO Provider Options', () => { const res = await requests.admin.put('/admin/providers/options', { body: { ssoLockedRoles: value, - defaultRole: localData.restrictedRole.id, + defaultRole: localData.restrictedRole.id, // TODO: there seems to be a bug with not setting a default role autoRegister: false, }, });