From 41718cb0dab7556ec57367524ae751cd443e4abb Mon Sep 17 00:00:00 2001
From: fflorent <florent.git@zeteo.me>
Date: Thu, 5 Sep 2024 21:27:27 +0200
Subject: [PATCH] SCIM: Implement DELETE
---
app/server/lib/scim/v2/ScimV2Api.ts | 27 ++++++++--
test/server/lib/Scim.ts | 78 +++++++++++++++++++++++++----
2 files changed, 91 insertions(+), 14 deletions(-)
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<keyof UserConfigByName>;
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');
});