SCIM: Implement DELETE

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);
+      }
     }
   });
 

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');
   });