From 062199d0e3087eead27bc3c6d1f4c6a10095feab Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Mon, 2 Sep 2024 15:29:16 +0000 Subject: [PATCH 1/9] feat: write endpoints to update SamlAuthProvidersRoleMappings --- .../update-role-mappings.ee.js | 50 +++++ .../update-role-mappings.ee.test.js | 178 ++++++++++++++++++ packages/backend/src/helpers/error-handler.js | 13 +- .../api/v1/admin/saml-auth-providers.ee.js | 9 + .../saml-auth-providers-role-mapping.js | 16 ++ .../update-role-mappings.ee.js | 23 +++ 6 files changed, 287 insertions(+), 2 deletions(-) create mode 100644 packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js create mode 100644 packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js create mode 100644 packages/backend/test/factories/saml-auth-providers-role-mapping.js create mode 100644 packages/backend/test/mocks/rest/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js new file mode 100644 index 00000000..0aa122dc --- /dev/null +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js @@ -0,0 +1,50 @@ +import isEmpty from 'lodash/isEmpty.js'; +import { renderObject } from '../../../../../helpers/renderer.js'; +import SamlAuthProvider from '../../../../../models/saml-auth-provider.ee.js'; +import SamlAuthProvidersRoleMapping from '../../../../../models/saml-auth-providers-role-mapping.ee.js'; + +export default async (request, response) => { + const samlAuthProviderId = request.params.samlAuthProviderId; + + const samlAuthProvider = await SamlAuthProvider.query() + .findById(samlAuthProviderId) + .throwIfNotFound(); + + const samlAuthProvidersRoleMappings = + await SamlAuthProvidersRoleMapping.transaction(async (trx) => { + await samlAuthProvider + .$relatedQuery('samlAuthProvidersRoleMappings', trx) + .delete(); + + const roleMappings = samlAuthProvidersRoleMappingsParams(request); + + if (isEmpty(roleMappings)) { + return []; + } + + const samlAuthProvidersRoleMappingsData = roleMappings.map( + (samlAuthProvidersRoleMapping) => ({ + ...samlAuthProvidersRoleMapping, + samlAuthProviderId: samlAuthProvider.id, + }) + ); + + const samlAuthProvidersRoleMappings = + await SamlAuthProvidersRoleMapping.query(trx).insertAndFetch( + samlAuthProvidersRoleMappingsData + ); + + return samlAuthProvidersRoleMappings; + }); + + renderObject(response, samlAuthProvidersRoleMappings); +}; + +const samlAuthProvidersRoleMappingsParams = (request) => { + const roleMappings = request.body; + + return roleMappings.map(({ roleId, remoteRoleName }) => ({ + roleId, + remoteRoleName, + })); +}; diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js new file mode 100644 index 00000000..b4e6ab3a --- /dev/null +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js @@ -0,0 +1,178 @@ +import Crypto from 'node:crypto'; +import { vi, describe, it, expect, beforeEach } from 'vitest'; +import request from 'supertest'; +import app from '../../../../../app.js'; +import createAuthTokenByUserId from '../../../../../helpers/create-auth-token-by-user-id.js'; +import { createRole } from '../../../../../../test/factories/role.js'; +import { createUser } from '../../../../../../test/factories/user.js'; +import { createSamlAuthProvider } from '../../../../../../test/factories/saml-auth-provider.ee.js'; +import { createSamlAuthProvidersRoleMapping } from '../../../../../../test/factories/saml-auth-providers-role-mapping.js'; +import createRoleMappingsMock from '../../../../../../test/mocks/rest/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js'; +import * as license from '../../../../../helpers/license.ee.js'; + +describe('PATCH /api/v1/admin/saml-auth-providers/:samlAuthProviderId/role-mappings', () => { + let samlAuthProvider, currentUser, userRole, token; + + beforeEach(async () => { + vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); + + userRole = await createRole({ key: 'admin' }); + currentUser = await createUser({ roleId: userRole.id }); + + samlAuthProvider = await createSamlAuthProvider(); + await createSamlAuthProvidersRoleMapping({ + samlAuthProviderId: samlAuthProvider.id, + }); + await createSamlAuthProvidersRoleMapping({ + samlAuthProviderId: samlAuthProvider.id, + }); + + token = await createAuthTokenByUserId(currentUser.id); + }); + + it('should update role mappings', async () => { + const roleMappings = [ + { + roleId: userRole.id, + remoteRoleName: 'Admin', + }, + ]; + + const response = await request(app) + .patch( + `/api/v1/admin/saml-auth-providers/${samlAuthProvider.id}/role-mappings` + ) + .set('Authorization', token) + .send(roleMappings) + .expect(200); + + const expectedPayload = await createRoleMappingsMock([ + { + roleId: userRole.id, + remoteRoleName: 'Admin', + id: response.body.data[0].id, + samlAuthProviderId: samlAuthProvider.id, + }, + ]); + + expect(response.body).toStrictEqual(expectedPayload); + }); + + it('should delete role mappings when given empty role mappings', async () => { + const existingRoleMappings = await samlAuthProvider.$relatedQuery( + 'samlAuthProvidersRoleMappings' + ); + + expect(existingRoleMappings.length).toBe(2); + + const response = await request(app) + .patch( + `/api/v1/admin/saml-auth-providers/${samlAuthProvider.id}/role-mappings` + ) + .set('Authorization', token) + .send([]) + .expect(200); + + const expectedPayload = await createRoleMappingsMock([]); + + expect(response.body).toStrictEqual({ + ...expectedPayload, + meta: { + ...expectedPayload.meta, + type: 'Object', + }, + }); + }); + + it('should return unprocessable entity response for not existing role UUID', async () => { + const notExistingRoleUUID = Crypto.randomUUID(); + const roleMappings = [ + { + roleId: notExistingRoleUUID, + remoteRoleName: 'Admin', + }, + ]; + + await request(app) + .patch( + `/api/v1/admin/saml-auth-providers/${samlAuthProvider.id}/role-mappings` + ) + .set('Authorization', token) + .send(roleMappings) + .expect(500); + }); + + it('should return unprocessable entity response for invalid data', async () => { + const roleMappings = [ + { + roleId: userRole.id, + remoteRoleName: {}, + }, + ]; + + const response = await request(app) + .patch( + `/api/v1/admin/saml-auth-providers/${samlAuthProvider.id}/role-mappings` + ) + .set('Authorization', token) + .send(roleMappings) + .expect(422); + + expect(response.body).toStrictEqual({ + errors: { + remoteRoleName: ['must be string'], + }, + meta: { + type: 'ModelValidation', + }, + }); + }); + + it('should return not found response for not existing SAML auth provider UUID', async () => { + const notExistingSamlAuthProviderUUID = Crypto.randomUUID(); + const roleMappings = [ + { + roleId: userRole.id, + remoteRoleName: 'Admin', + }, + ]; + + await request(app) + .patch( + `/api/v1/admin/saml-auth-providers/${notExistingSamlAuthProviderUUID}/role-mappings` + ) + .set('Authorization', token) + .send(roleMappings) + .expect(404); + }); + + it('should not delete existing role mapping when error thrown', async () => { + const roleMappings = [ + { + roleId: userRole.id, + remoteRoleName: { + invalid: 'data', + }, + }, + ]; + + const roleMappingsBeforeRequest = await samlAuthProvider.$relatedQuery( + 'samlAuthProvidersRoleMappings' + ); + + await request(app) + .patch( + `/api/v1/admin/saml-auth-providers/${samlAuthProvider.id}/role-mappings` + ) + .set('Authorization', token) + .send(roleMappings) + .expect(422); + + const roleMappingsAfterRequest = await samlAuthProvider.$relatedQuery( + 'samlAuthProvidersRoleMappings' + ); + + expect(roleMappingsBeforeRequest).toStrictEqual(roleMappingsAfterRequest); + expect(roleMappingsAfterRequest.length).toBe(2); + }); +}); diff --git a/packages/backend/src/helpers/error-handler.js b/packages/backend/src/helpers/error-handler.js index ecfa9c18..63c80bbb 100644 --- a/packages/backend/src/helpers/error-handler.js +++ b/packages/backend/src/helpers/error-handler.js @@ -1,8 +1,13 @@ import logger from './logger.js'; import objection from 'objection'; import * as Sentry from './sentry.ee.js'; -const { NotFoundError, DataError, ValidationError, UniqueViolationError } = - objection; +const { + NotFoundError, + DataError, + ForeignKeyViolationError, + ValidationError, + UniqueViolationError, +} = objection; import NotAuthorizedError from '../errors/not-authorized.js'; import HttpError from '../errors/http.js'; import { @@ -29,6 +34,10 @@ const errorHandler = (error, request, response, next) => { renderUniqueViolationError(response, error); } + if (error instanceof ForeignKeyViolationError) { + response.status(500).end(); + } + if (error instanceof DataError) { response.status(400).end(); } diff --git a/packages/backend/src/routes/api/v1/admin/saml-auth-providers.ee.js b/packages/backend/src/routes/api/v1/admin/saml-auth-providers.ee.js index 57e333f8..eb243b82 100644 --- a/packages/backend/src/routes/api/v1/admin/saml-auth-providers.ee.js +++ b/packages/backend/src/routes/api/v1/admin/saml-auth-providers.ee.js @@ -7,6 +7,7 @@ import updateSamlAuthProviderAction from '../../../../controllers/api/v1/admin/s import getSamlAuthProvidersAction from '../../../../controllers/api/v1/admin/saml-auth-providers/get-saml-auth-providers.ee.js'; import getSamlAuthProviderAction from '../../../../controllers/api/v1/admin/saml-auth-providers/get-saml-auth-provider.ee.js'; import getRoleMappingsAction from '../../../../controllers/api/v1/admin/saml-auth-providers/get-role-mappings.ee.js'; +import updateRoleMappingsAction from '../../../../controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js'; const router = Router(); @@ -50,4 +51,12 @@ router.patch( updateSamlAuthProviderAction ); +router.patch( + '/:samlAuthProviderId/role-mappings', + authenticateUser, + authorizeAdmin, + checkIsEnterprise, + updateRoleMappingsAction +); + export default router; diff --git a/packages/backend/test/factories/saml-auth-providers-role-mapping.js b/packages/backend/test/factories/saml-auth-providers-role-mapping.js new file mode 100644 index 00000000..72b23c06 --- /dev/null +++ b/packages/backend/test/factories/saml-auth-providers-role-mapping.js @@ -0,0 +1,16 @@ +import { faker } from '@faker-js/faker'; +import { createRole } from './role.js'; +import SamlAuthProvidersRoleMapping from '../../src/models/saml-auth-providers-role-mapping.ee.js'; +import { createSamlAuthProvider } from './saml-auth-provider.ee.js'; + +export const createSamlAuthProvidersRoleMapping = async (params = {}) => { + params.roleId = params.roleId || (await createRole()).id; + params.samlAuthProviderId = + params.samlAuthProviderId || (await createSamlAuthProvider()).id; + params.remoteRoleName = params.remoteRoleName || faker.person.jobType(); + + const samlAuthProvider = + await SamlAuthProvidersRoleMapping.query().insertAndFetch(params); + + return samlAuthProvider; +}; diff --git a/packages/backend/test/mocks/rest/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js b/packages/backend/test/mocks/rest/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js new file mode 100644 index 00000000..2ff8ca16 --- /dev/null +++ b/packages/backend/test/mocks/rest/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js @@ -0,0 +1,23 @@ +const createRoleMappingsMock = async (roleMappings) => { + const data = roleMappings.map((roleMapping) => { + return { + id: roleMapping.id, + samlAuthProviderId: roleMapping.samlAuthProviderId, + roleId: roleMapping.roleId, + remoteRoleName: roleMapping.remoteRoleName, + }; + }); + + return { + data: data, + meta: { + count: data.length, + currentPage: null, + isArray: true, + totalPages: null, + type: 'SamlAuthProvidersRoleMapping', + }, + }; +}; + +export default createRoleMappingsMock; From 95dc5fb8492dee14ae3eccf33b4b424a9060d169 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Mon, 2 Sep 2024 15:29:38 +0000 Subject: [PATCH 2/9] refactor(RoleMappings): rewrite mutations with REST API endpoints --- ...AdminUpdateSamlAuthProviderRoleMappings.js | 31 +++++++++++++++++ .../src/pages/Authentication/RoleMappings.jsx | 34 +++++++------------ 2 files changed, 44 insertions(+), 21 deletions(-) create mode 100644 packages/web/src/hooks/useAdminUpdateSamlAuthProviderRoleMappings.js diff --git a/packages/web/src/hooks/useAdminUpdateSamlAuthProviderRoleMappings.js b/packages/web/src/hooks/useAdminUpdateSamlAuthProviderRoleMappings.js new file mode 100644 index 00000000..4909e540 --- /dev/null +++ b/packages/web/src/hooks/useAdminUpdateSamlAuthProviderRoleMappings.js @@ -0,0 +1,31 @@ +import { useMutation, useQueryClient } from '@tanstack/react-query'; +import api from 'helpers/api'; + +export default function useAdminUpdateSamlAuthProviderRoleMappings( + samlAuthProviderId, +) { + const queryClient = useQueryClient(); + + const query = useMutation({ + mutationFn: async (payload) => { + const { data } = await api.patch( + `/v1/admin/saml-auth-providers/${samlAuthProviderId}/role-mappings`, + payload, + ); + + return data; + }, + onSuccess: () => { + queryClient.invalidateQueries({ + queryKey: [ + 'admin', + 'samlAuthProviders', + samlAuthProviderId, + 'roleMappings', + ], + }); + }, + }); + + return query; +} diff --git a/packages/web/src/pages/Authentication/RoleMappings.jsx b/packages/web/src/pages/Authentication/RoleMappings.jsx index a2605eef..7a46c35a 100644 --- a/packages/web/src/pages/Authentication/RoleMappings.jsx +++ b/packages/web/src/pages/Authentication/RoleMappings.jsx @@ -1,5 +1,4 @@ import PropTypes from 'prop-types'; -import { useMutation } from '@apollo/client'; import LoadingButton from '@mui/lab/LoadingButton'; import Divider from '@mui/material/Divider'; import Stack from '@mui/material/Stack'; @@ -8,9 +7,9 @@ import useEnqueueSnackbar from 'hooks/useEnqueueSnackbar'; import { useMemo } from 'react'; import Form from 'components/Form'; -import { UPSERT_SAML_AUTH_PROVIDERS_ROLE_MAPPINGS } from 'graphql/mutations/upsert-saml-auth-providers-role-mappings'; import useFormatMessage from 'hooks/useFormatMessage'; import useAdminSamlAuthProviderRoleMappings from 'hooks/useAdminSamlAuthProviderRoleMappings'; +import useAdminUpdateSamlAuthProviderRoleMappings from 'hooks/useAdminUpdateSamlAuthProviderRoleMappings'; import RoleMappingsFieldArray from './RoleMappingsFieldsArray'; function generateFormRoleMappings(roleMappings) { @@ -28,33 +27,26 @@ function RoleMappings({ provider, providerLoading }) { const formatMessage = useFormatMessage(); const enqueueSnackbar = useEnqueueSnackbar(); + const { + mutateAsync: updateSamlAuthProvidersRoleMappings, + isPending: isUpdateSamlAuthProvidersRoleMappingsPending, + } = useAdminUpdateSamlAuthProviderRoleMappings(provider?.id); + const { data, isLoading: isAdminSamlAuthProviderRoleMappingsLoading } = useAdminSamlAuthProviderRoleMappings({ adminSamlAuthProviderId: provider?.id, }); const roleMappings = data?.data; - const [ - upsertSamlAuthProvidersRoleMappings, - { loading: upsertRoleMappingsLoading }, - ] = useMutation(UPSERT_SAML_AUTH_PROVIDERS_ROLE_MAPPINGS); - const handleRoleMappingsUpdate = async (values) => { try { if (provider?.id) { - await upsertSamlAuthProvidersRoleMappings({ - variables: { - input: { - samlAuthProviderId: provider.id, - samlAuthProvidersRoleMappings: values.roleMappings.map( - ({ roleId, remoteRoleName }) => ({ - roleId, - remoteRoleName, - }), - ), - }, - }, - }); + await updateSamlAuthProvidersRoleMappings( + values.roleMappings.map(({ roleId, remoteRoleName }) => ({ + roleId, + remoteRoleName, + })), + ); enqueueSnackbar(formatMessage('roleMappingsForm.successfullySaved'), { variant: 'success', @@ -97,7 +89,7 @@ function RoleMappings({ provider, providerLoading }) { variant="contained" color="primary" sx={{ boxShadow: 2 }} - loading={upsertRoleMappingsLoading} + loading={isUpdateSamlAuthProvidersRoleMappingsPending} > {formatMessage('roleMappingsForm.save')} From 5556aea913235954db533935746af0878b0ccf50 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Mon, 2 Sep 2024 15:32:04 +0000 Subject: [PATCH 3/9] chore: remove upsert-saml-auth-providers-role-mappings mutation --- .../backend/src/graphql/mutation-resolvers.js | 2 - ...rt-saml-auth-providers-role-mappings.ee.js | 42 ------------------- packages/backend/src/graphql/schema.graphql | 13 ------ ...psert-saml-auth-providers-role-mappings.js | 13 ------ 4 files changed, 70 deletions(-) delete mode 100644 packages/backend/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.ee.js delete mode 100644 packages/web/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.js diff --git a/packages/backend/src/graphql/mutation-resolvers.js b/packages/backend/src/graphql/mutation-resolvers.js index 8483637a..60c8b8ea 100644 --- a/packages/backend/src/graphql/mutation-resolvers.js +++ b/packages/backend/src/graphql/mutation-resolvers.js @@ -11,7 +11,6 @@ import updateConnection from './mutations/update-connection.js'; import updateCurrentUser from './mutations/update-current-user.js'; import updateFlowStatus from './mutations/update-flow-status.js'; import updateStep from './mutations/update-step.js'; -import upsertSamlAuthProvidersRoleMappings from './mutations/upsert-saml-auth-providers-role-mappings.ee.js'; // Converted mutations import executeFlow from './mutations/execute-flow.js'; @@ -40,7 +39,6 @@ const mutationResolvers = { updateFlowStatus, updateStep, updateUser, - upsertSamlAuthProvidersRoleMappings, verifyConnection, }; diff --git a/packages/backend/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.ee.js b/packages/backend/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.ee.js deleted file mode 100644 index 0040d48e..00000000 --- a/packages/backend/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.ee.js +++ /dev/null @@ -1,42 +0,0 @@ -import SamlAuthProvider from '../../models/saml-auth-provider.ee.js'; -import SamlAuthProvidersRoleMapping from '../../models/saml-auth-providers-role-mapping.ee.js'; -import isEmpty from 'lodash/isEmpty.js'; - -const upsertSamlAuthProvidersRoleMappings = async ( - _parent, - params, - context -) => { - context.currentUser.can('update', 'SamlAuthProvider'); - - const samlAuthProviderId = params.input.samlAuthProviderId; - - const samlAuthProvider = await SamlAuthProvider.query() - .findById(samlAuthProviderId) - .throwIfNotFound(); - - await samlAuthProvider - .$relatedQuery('samlAuthProvidersRoleMappings') - .delete(); - - if (isEmpty(params.input.samlAuthProvidersRoleMappings)) { - return []; - } - - const samlAuthProvidersRoleMappingsData = - params.input.samlAuthProvidersRoleMappings.map( - (samlAuthProvidersRoleMapping) => ({ - ...samlAuthProvidersRoleMapping, - samlAuthProviderId: samlAuthProvider.id, - }) - ); - - const samlAuthProvidersRoleMappings = - await SamlAuthProvidersRoleMapping.query().insert( - samlAuthProvidersRoleMappingsData - ); - - return samlAuthProvidersRoleMappings; -}; - -export default upsertSamlAuthProvidersRoleMappings; diff --git a/packages/backend/src/graphql/schema.graphql b/packages/backend/src/graphql/schema.graphql index 5598f59a..4958b2f0 100644 --- a/packages/backend/src/graphql/schema.graphql +++ b/packages/backend/src/graphql/schema.graphql @@ -20,9 +20,6 @@ type Mutation { updateFlowStatus(input: UpdateFlowStatusInput): Flow updateStep(input: UpdateStepInput): Step updateUser(input: UpdateUserInput): User - upsertSamlAuthProvidersRoleMappings( - input: UpsertSamlAuthProvidersRoleMappingsInput - ): [SamlAuthProvidersRoleMapping] verifyConnection(input: VerifyConnectionInput): Connection } @@ -247,16 +244,6 @@ input VerifyConnectionInput { id: String! } -input UpsertSamlAuthProvidersRoleMappingsInput { - samlAuthProviderId: String! - samlAuthProvidersRoleMappings: [SamlAuthProviderRoleMappingInput] -} - -input SamlAuthProviderRoleMappingInput { - roleId: String! - remoteRoleName: String! -} - input CreateFlowInput { triggerAppKey: String connectionId: String diff --git a/packages/web/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.js b/packages/web/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.js deleted file mode 100644 index 2d9180e0..00000000 --- a/packages/web/src/graphql/mutations/upsert-saml-auth-providers-role-mappings.js +++ /dev/null @@ -1,13 +0,0 @@ -import { gql } from '@apollo/client'; -export const UPSERT_SAML_AUTH_PROVIDERS_ROLE_MAPPINGS = gql` - mutation UpsertSamlAuthProvidersRoleMappings( - $input: UpsertSamlAuthProvidersRoleMappingsInput - ) { - upsertSamlAuthProvidersRoleMappings(input: $input) { - id - samlAuthProviderId - roleId - remoteRoleName - } - } -`; From fbb6526aac1a20e6d74e9f1722d3888b97fe3fba Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Mon, 2 Sep 2024 15:38:53 +0000 Subject: [PATCH 4/9] refactor(update-role-mappings): move logic to model --- .../update-role-mappings.ee.js | 30 ++------------- .../src/models/saml-auth-provider.ee.js | 37 ++++++++++++++++--- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js index 0aa122dc..c0e6afb8 100644 --- a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.js @@ -1,7 +1,5 @@ -import isEmpty from 'lodash/isEmpty.js'; import { renderObject } from '../../../../../helpers/renderer.js'; import SamlAuthProvider from '../../../../../models/saml-auth-provider.ee.js'; -import SamlAuthProvidersRoleMapping from '../../../../../models/saml-auth-providers-role-mapping.ee.js'; export default async (request, response) => { const samlAuthProviderId = request.params.samlAuthProviderId; @@ -11,31 +9,9 @@ export default async (request, response) => { .throwIfNotFound(); const samlAuthProvidersRoleMappings = - await SamlAuthProvidersRoleMapping.transaction(async (trx) => { - await samlAuthProvider - .$relatedQuery('samlAuthProvidersRoleMappings', trx) - .delete(); - - const roleMappings = samlAuthProvidersRoleMappingsParams(request); - - if (isEmpty(roleMappings)) { - return []; - } - - const samlAuthProvidersRoleMappingsData = roleMappings.map( - (samlAuthProvidersRoleMapping) => ({ - ...samlAuthProvidersRoleMapping, - samlAuthProviderId: samlAuthProvider.id, - }) - ); - - const samlAuthProvidersRoleMappings = - await SamlAuthProvidersRoleMapping.query(trx).insertAndFetch( - samlAuthProvidersRoleMappingsData - ); - - return samlAuthProvidersRoleMappings; - }); + await samlAuthProvider.updateRoleMappings( + samlAuthProvidersRoleMappingsParams(request) + ); renderObject(response, samlAuthProvidersRoleMappings); }; diff --git a/packages/backend/src/models/saml-auth-provider.ee.js b/packages/backend/src/models/saml-auth-provider.ee.js index 431153f9..744da1a8 100644 --- a/packages/backend/src/models/saml-auth-provider.ee.js +++ b/packages/backend/src/models/saml-auth-provider.ee.js @@ -1,5 +1,6 @@ import { URL } from 'node:url'; import { v4 as uuidv4 } from 'uuid'; +import isEmpty from 'lodash/isEmpty.js'; import appConfig from '../config/app.js'; import axios from '../helpers/axios-with-proxy.js'; import Base from './base.js'; @@ -88,7 +89,7 @@ class SamlAuthProvider extends Base { entryPoint: this.entryPoint, issuer: this.issuer, signatureAlgorithm: this.signatureAlgorithm, - logoutUrl: this.remoteLogoutUrl + logoutUrl: this.remoteLogoutUrl, }; } @@ -101,14 +102,16 @@ class SamlAuthProvider extends Base { IssueInstant="${new Date().toISOString()}" Destination="${this.remoteLogoutUrl}"> - ${this.issuer} + ${ + this.issuer + } ${sessionId} `; - const encodedLogoutRequest = Buffer.from(logoutRequest).toString('base64') + const encodedLogoutRequest = Buffer.from(logoutRequest).toString('base64'); - return encodedLogoutRequest + return encodedLogoutRequest; } async terminateRemoteSession(sessionId) { @@ -122,12 +125,36 @@ class SamlAuthProvider extends Base { { headers: { 'Content-Type': 'application/x-www-form-urlencoded', - } + }, } ); return response; } + + async updateRoleMappings(roleMappings) { + return await SamlAuthProvider.transaction(async (trx) => { + await this.$relatedQuery('samlAuthProvidersRoleMappings', trx).delete(); + + if (isEmpty(roleMappings)) { + return []; + } + + const samlAuthProvidersRoleMappingsData = roleMappings.map( + (samlAuthProvidersRoleMapping) => ({ + ...samlAuthProvidersRoleMapping, + samlAuthProviderId: this.id, + }) + ); + + const samlAuthProvidersRoleMappings = + await SamlAuthProvidersRoleMapping.query(trx).insertAndFetch( + samlAuthProvidersRoleMappingsData + ); + + return samlAuthProvidersRoleMappings; + }); + } } export default SamlAuthProvider; From 6fe863eec1210e2315a2af05a14220c7ba23c1d5 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Mon, 2 Sep 2024 15:55:26 +0000 Subject: [PATCH 5/9] test(update-role-mappings): use explicit remote role name --- .../admin/saml-auth-providers/update-role-mappings.ee.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js index b4e6ab3a..50e31975 100644 --- a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js @@ -22,9 +22,11 @@ describe('PATCH /api/v1/admin/saml-auth-providers/:samlAuthProviderId/role-mappi samlAuthProvider = await createSamlAuthProvider(); await createSamlAuthProvidersRoleMapping({ samlAuthProviderId: samlAuthProvider.id, + remoteRoleName: 'Viewer', }); await createSamlAuthProvidersRoleMapping({ samlAuthProviderId: samlAuthProvider.id, + remoteRoleName: 'Editor', }); token = await createAuthTokenByUserId(currentUser.id); From 101483409f3eec8450c078582eafbd4cb9f9c9b2 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Tue, 10 Sep 2024 08:51:39 +0000 Subject: [PATCH 6/9] style(update-role-mappings): add a breakline --- .../admin/saml-auth-providers/update-role-mappings.ee.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js index 50e31975..048918af 100644 --- a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js @@ -20,10 +20,12 @@ describe('PATCH /api/v1/admin/saml-auth-providers/:samlAuthProviderId/role-mappi currentUser = await createUser({ roleId: userRole.id }); samlAuthProvider = await createSamlAuthProvider(); + await createSamlAuthProvidersRoleMapping({ samlAuthProviderId: samlAuthProvider.id, remoteRoleName: 'Viewer', }); + await createSamlAuthProvidersRoleMapping({ samlAuthProviderId: samlAuthProvider.id, remoteRoleName: 'Editor', From fb82e863e0235d58dcd64421c289161d0fd12294 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Tue, 10 Sep 2024 09:30:50 +0000 Subject: [PATCH 7/9] test(update-role-mappings): correct the test case name --- .../admin/saml-auth-providers/update-role-mappings.ee.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js index 048918af..e309564c 100644 --- a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js @@ -88,7 +88,7 @@ describe('PATCH /api/v1/admin/saml-auth-providers/:samlAuthProviderId/role-mappi }); }); - it('should return unprocessable entity response for not existing role UUID', async () => { + it('should return internal server error response for not existing role UUID', async () => { const notExistingRoleUUID = Crypto.randomUUID(); const roleMappings = [ { From be57a82302e282021e65b08bd767259e7fec4166 Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Tue, 10 Sep 2024 09:56:15 +0000 Subject: [PATCH 8/9] test(factories/role): re-create different role if it exists --- packages/backend/test/factories/role.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/backend/test/factories/role.js b/packages/backend/test/factories/role.js index 7db3eff2..28ac9960 100644 --- a/packages/backend/test/factories/role.js +++ b/packages/backend/test/factories/role.js @@ -6,6 +6,12 @@ export const createRole = async (params = {}) => { params.name = params?.name || name; + const existingRole = await Role.query().findOne({ name }).first(); + + if (existingRole) { + return await createRole(); + } + const role = await Role.query().insertAndFetch(params); return role; From fdf53844e1696623b8da7e7cb382368a4ac85c3f Mon Sep 17 00:00:00 2001 From: Ali BARIN Date: Tue, 10 Sep 2024 10:00:40 +0000 Subject: [PATCH 9/9] test(update-role-mappings): use name over key --- .../admin/saml-auth-providers/update-role-mappings.ee.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js index e309564c..f06a3ba6 100644 --- a/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js +++ b/packages/backend/src/controllers/api/v1/admin/saml-auth-providers/update-role-mappings.ee.test.js @@ -16,7 +16,7 @@ describe('PATCH /api/v1/admin/saml-auth-providers/:samlAuthProviderId/role-mappi beforeEach(async () => { vi.spyOn(license, 'hasValidLicense').mockResolvedValue(true); - userRole = await createRole({ key: 'admin' }); + userRole = await createRole({ name: 'Admin' }); currentUser = await createUser({ roleId: userRole.id }); samlAuthProvider = await createSamlAuthProvider();