diff --git a/app/server/lib/scim/v2/ScimV2Api.ts b/app/server/lib/scim/v2/ScimV2Api.ts index ebd0c6db..69aeac3d 100644 --- a/app/server/lib/scim/v2/ScimV2Api.ts +++ b/app/server/lib/scim/v2/ScimV2Api.ts @@ -1,11 +1,12 @@ import * as express from 'express'; -import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; +import { HomeDBManager, Scope } from 'app/gen-server/lib/homedb/HomeDBManager'; import SCIMMY from "scimmy"; import SCIMMYRouters from "scimmy-routers"; import { RequestWithLogin } from '../../Authorizer'; import { InstallAdmin } from '../../InstallAdmin'; import { toSCIMMYUser, toUserProfile } from './ScimUserUtils'; import { ApiError } from 'app/common/ApiError'; +import { parseInt } from 'lodash'; const WHITELISTED_PATHS_FOR_NON_ADMINS = [ "/Me", "/Schemas", "/ResourceTypes", "/ServiceProviderConfig" ]; @@ -20,7 +21,8 @@ const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin) SCIMMY.Resources.declare(SCIMMY.Resources.User, { egress: async (resource: any) => { - const { id, filter } = resource; + const { filter } = resource; + const id = parseInt(resource.id, 10); if (id) { const user = await dbManager.getUser(id); if (!user) { @@ -33,7 +35,7 @@ const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin) }, ingress: async (resource: any, data: any) => { try { - const { id } = resource; + const id = parseInt(resource.id, 10); if (id) { const updatedUser = await dbManager.overrideUser(id, toUserProfile(data)); return toSCIMMYUser(updatedUser); @@ -55,9 +57,13 @@ const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin) throw new SCIMMY.Types.Error(ex.status, null!, ex.message); } + // FIXME: Remove this part and find another way to detect a constraint error. if (ex.code?.startsWith('SQLITE')) { switch (ex.code) { case 'SQLITE_CONSTRAINT': + // Return a 409 error if a conflict is detected (e.g. email already exists) + // "uniqueness" is an error code expected by the SCIM RFC for this case. + // FIXME: the emails are unique in the database, but this is not enforced in the schema. throw new SCIMMY.Types.Error(409, 'uniqueness', ex.message); default: throw new SCIMMY.Types.Error(500, 'serverError', ex.message); @@ -67,8 +73,19 @@ const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin) throw ex; } }, - degress: () => { - return null; + degress: async (resource: any) => { + const id = parseInt(resource.id, 10); + const fakeScope: Scope = { userId: id }; // FIXME: deleteUser should probably better not requiring a scope. + try { + await dbManager.deleteUser(fakeScope, id); + } catch (ex) { + console.error('Error deleting user', ex); + if (ex instanceof ApiError) { + throw new SCIMMY.Types.Error(ex.status, null!, ex.message); + } + + throw new SCIMMY.Types.Error(500, 'serverError', ex.message); + } } }); diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts index cbfc5806..64e7531f 100644 --- a/test/server/lib/Scim.ts +++ b/test/server/lib/Scim.ts @@ -44,7 +44,7 @@ describe('Scim', () => { homeUrl = await server.start(); const userNames = Object.keys(USER_CONFIG_BY_NAME) as Array; for (const user of userNames) { - userIdByName[user] = await getUserId(user); + userIdByName[user] = await getOrCreateUserId(user); } }); @@ -77,10 +77,16 @@ describe('Scim', () => { }; } - async function getUserId(user: string) { + async function getOrCreateUserId(user: string) { return (await server.dbManager.getUserByLogin(user + '@getgrist.com'))!.id; } + async function cleanupUser(userId: number) { + if (await server.dbManager.getUser(userId)) { + await server.dbManager.deleteUser({ userId: userId }, userId); + } + } + function checkEndpointNotAccessibleForNonAdminUsers( method: 'get' | 'post' | 'put' | 'patch' | 'delete', path: string @@ -199,7 +205,7 @@ describe('Scim', () => { it('should create a new user', async function () { const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId('newuser1'), chimpy); assert.equal(res.status, 201); - const newUserId = await getUserId('newuser1'); + const newUserId = await getOrCreateUserId('newuser1'); assert.deepEqual(res.data, toSCIMUserWithId('newuser1', newUserId)); }); @@ -247,10 +253,10 @@ describe('Scim', () => { const userToUpdateEmailLocalPart = 'user-to-update'; beforeEach(async function () { - userToUpdateId = await getUserId(userToUpdateEmailLocalPart); + userToUpdateId = await getOrCreateUserId(userToUpdateEmailLocalPart); }); afterEach(async function () { - await server.dbManager.deleteUser({ userId: userToUpdateId }, userToUpdateId); + await cleanupUser(userToUpdateId); }); it('should update an existing user', async function () { @@ -344,15 +350,69 @@ describe('Scim', () => { }); describe('PATCH /Users/{id}', function () { - it('should update the user of id=1', async function () { - throw new Error("This is a reminder :)"); + let userToPatchId: number; + const userToPatchEmailLocalPart = 'user-to-patch'; + beforeEach(async function () { + userToPatchId = await getOrCreateUserId(userToPatchEmailLocalPart); }); + afterEach(async function () { + await cleanupUser(userToPatchId); + }); + + it('should replace values of an existing user', async function () { + const newName = 'User to Patch new Name'; + const res = await axios.patch(scimUrl(`/Users/${userToPatchId}`), { + schemas: ['urn:ietf:params:scim:api:messages:2.0:PatchOp'], + Operations: [{ + op: "replace", + path: "displayName", + value: newName, + }, { + op: "replace", + path: "locale", + value: 'fr' + }], + }, chimpy); + assert.equal(res.status, 200); + const refreshedUser = await axios.get(scimUrl(`/Users/${userToPatchId}`), chimpy); + assert.deepEqual(refreshedUser.data, { + ...toSCIMUserWithId(userToPatchEmailLocalPart, userToPatchId), + displayName: newName, + name: { formatted: newName }, + locale: 'fr', + preferredLanguage: 'fr', + }); + }); + checkEndpointNotAccessibleForNonAdminUsers('patch', '/Users/1'); }); describe('DELETE /Users/{id}', function () { - it('should delete the user of id=1', async function () { - throw new Error("This is a reminder :)"); + let userToDeleteId: number; + const userToDeleteEmailLocalPart = 'user-to-delete'; + + beforeEach(async function () { + userToDeleteId = await getOrCreateUserId(userToDeleteEmailLocalPart); + }); + afterEach(async function () { + await cleanupUser(userToDeleteId); + }); + + it('should delete some user', async function () { + const res = await axios.delete(scimUrl(`/Users/${userToDeleteId}`), chimpy); + assert.equal(res.status, 204); + const refreshedUser = await axios.get(scimUrl(`/Users/${userToDeleteId}`), chimpy); + assert.equal(refreshedUser.status, 404); + }); + + it('should return 404 when the user is not found', async function () { + const res = await axios.delete(scimUrl('/Users/1000'), chimpy); + assert.equal(res.status, 404); + assert.deepEqual(res.data, { + schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ], + status: '404', + detail: 'user not found' + }); }); checkEndpointNotAccessibleForNonAdminUsers('delete', '/Users/1'); });