Move logic for Users to its own controller

app/server/lib/FlexServer.ts

@@ -890,11 +890,13 @@ export class FlexServer implements GristServer {
 
   public addScimApi() {
     if (this._check('scim', 'api', 'homedb', 'json', 'api-mw')) { return; }
+
     const scimRouter = isAffirmative(process.env.GRIST_ENABLE_SCIM) ?
       buildScimRouter(this._dbManager, this._installAdmin) :
       () => {
         throw new ApiError('SCIM API is not enabled', 501);
       };
+
     this.app.use('/api/scim', scimRouter);
   }
 

app/server/lib/scim/v2/ScimTypes.ts

@@ -0,0 +1,6 @@
+export interface RequestContext {
+  path: string;
+  isAdmin: boolean;
+  isScimUser: boolean;
+}
+

app/server/lib/scim/v2/ScimUserController.ts

@@ -0,0 +1,121 @@
+import { ApiError } from 'app/common/ApiError';
+import { HomeDBManager, Scope } from 'app/gen-server/lib/homedb/HomeDBManager';
+import SCIMMY from 'scimmy';
+import { toSCIMMYUser, toUserProfile } from './ScimUserUtils';
+import { RequestContext } from './ScimTypes';
+
+class ScimUserController {
+  private static _getIdFromResource(resource: any) {
+    const id = parseInt(resource.id, 10);
+    if (Number.isNaN(id)) {
+      throw new SCIMMY.Types.Error(400, 'invalidValue', 'Invalid passed user ID');
+    }
+    return id;
+  }
+
+  constructor(
+    private _dbManager: HomeDBManager,
+    private _checkAccess: (context: RequestContext) => void
+  ) {}
+
+  public async getSingleUser(resource: any, context: RequestContext) {
+    this._checkAccess(context);
+
+    const id = ScimUserController._getIdFromResource(resource);
+    const user = await this._dbManager.getUser(id);
+    if (!user) {
+      throw new SCIMMY.Types.Error(404, null!, `User with ID ${id} not found`);
+    }
+    return toSCIMMYUser(user);
+  }
+
+  public async getUsers(resource: any, context: RequestContext) {
+    this._checkAccess(context);
+
+    const { filter } = resource;
+    const scimmyUsers = (await this._dbManager.getUsers()).map(user => toSCIMMYUser(user));
+    return filter ? filter.match(scimmyUsers) : scimmyUsers;
+  }
+
+  public async createUser(data: any, context: RequestContext) {
+    this._checkAccess(context);
+
+    try {
+      await this._checkEmailIsUnique(data.userName);
+      const userProfile = toUserProfile(data);
+      const newUser = await this._dbManager.getUserByLoginWithRetry(userProfile.email, {
+        profile: userProfile
+      });
+      return toSCIMMYUser(newUser);
+    } catch (ex) {
+      return this._toScimError(ex);
+    }
+  }
+
+  public async overrideUser(resource: any, data: any, context: RequestContext) {
+    this._checkAccess(context);
+
+    try {
+      const id = ScimUserController._getIdFromResource(resource);
+      await this._checkEmailIsUnique(data.userName, id);
+      const updatedUser = await this._dbManager.overrideUser(id, toUserProfile(data));
+      return toSCIMMYUser(updatedUser);
+    } catch (ex) {
+      return this._toScimError(ex);
+    }
+  }
+
+  public async deleteUser(resource: any, context: RequestContext) {
+    this._checkAccess(context);
+
+    const id = ScimUserController._getIdFromResource(resource);
+    try {
+      const fakeScope: Scope = { userId: id };
+      // FIXME: deleteUser should probably better not requiring a scope.
+      await this._dbManager.deleteUser(fakeScope, id);
+    } catch (ex) {
+      return this._toScimError(ex);
+    }
+  }
+
+  private async _toScimError(ex: Error) {
+    if (ex instanceof ApiError) {
+      if (ex.status === 409) {
+        throw new SCIMMY.Types.Error(ex.status, 'uniqueness', ex.message);
+      }
+      throw new SCIMMY.Types.Error(ex.status, null!, ex.message);
+    }
+    throw ex;
+  }
+
+  private async _checkEmailIsUnique(email: string, id?: number) {
+    const existingUser = await this._dbManager.getExistingUserByLogin(email);
+    if (existingUser !== undefined && existingUser.id !== id) {
+      throw new SCIMMY.Types.Error(409, 'uniqueness', 'An existing user with the passed email exist.');
+    }
+  }
+}
+
+export const getScimUserConfig = (
+  dbManager: HomeDBManager, checkAccess: (context: RequestContext) => void
+) => {
+  const controller = new ScimUserController(dbManager, checkAccess);
+
+  return {
+    egress: async (resource: any, context: RequestContext) => {
+      if (resource.id) {
+        return await controller.getSingleUser(resource, context);
+      }
+      return await controller.getUsers(resource, context);
+    },
+    ingress: async (resource: any, data: any, context: RequestContext) => {
+      if (resource.id) {
+        return await controller.overrideUser(resource, data, context);
+      }
+      return await controller.createUser(data, context);
+    },
+    degress: async (resource: any, context: RequestContext) => {
+      return await controller.deleteUser(resource, context);
+    }
+  };
+};

app/server/lib/scim/v2/ScimV2Api.ts

@@ -1,100 +1,25 @@
 import * as express from 'express';
-import { HomeDBManager, Scope } from 'app/gen-server/lib/homedb/HomeDBManager';
+import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager';
 import SCIMMY from "scimmy";
 import SCIMMYRouters from "scimmy-routers";
-import { RequestWithLogin } from '../../Authorizer';
+import { RequestWithLogin } from 'app/server/lib/Authorizer';
-import { InstallAdmin } from '../../InstallAdmin';
+import { InstallAdmin } from 'app/server/lib/InstallAdmin';
-import { toSCIMMYUser, toUserProfile } from './ScimUserUtils';
+import { RequestContext } from './ScimTypes';
-import { ApiError } from 'app/common/ApiError';
+import { getScimUserConfig } from './ScimUserController';
-import { parseInt } from 'lodash';
 
 const WHITELISTED_PATHS_FOR_NON_ADMINS = [ "/Me", "/Schemas", "/ResourceTypes", "/ServiceProviderConfig" ];
 
-interface RequestContext {
-  path: string;
-  isAdmin: boolean;
-  isScimUser: boolean;
-}
-
-function checkAccess(context: RequestContext) {
-  const {isAdmin, isScimUser, path } = context;
-  if (!isAdmin && !isScimUser && !WHITELISTED_PATHS_FOR_NON_ADMINS.includes(path)) {
-    throw new SCIMMY.Types.Error(403, null!, 'You are not authorized to access this resource');
-  }
-}
-
-async function checkEmailIsUnique(dbManager: HomeDBManager, email: string, id?: number) {
-  const existingUser = await dbManager.getExistingUserByLogin(email);
-  if (existingUser !== undefined && existingUser.id !== id) {
-    throw new SCIMMY.Types.Error(409, 'uniqueness', 'An existing user with the passed email exist.');
-  }
-}
-
 const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin) => {
   const v2 = express.Router();
 
-  SCIMMY.Resources.declare(SCIMMY.Resources.User, {
+  function checkAccess(context: RequestContext) {
-    egress: async (resource: any, context: RequestContext) => {
+    const {isAdmin, isScimUser, path } = context;
-      checkAccess(context);
+    if (!isAdmin && !isScimUser && !WHITELISTED_PATHS_FOR_NON_ADMINS.includes(path)) {
-
+      throw new SCIMMY.Types.Error(403, null!, 'You are not authorized to access this resource');
-      const { filter } = resource;
-      const id = parseInt(resource.id, 10);
-      if (!isNaN(id)) {
-        const user = await dbManager.getUser(id);
-        if (!user) {
-          throw new SCIMMY.Types.Error(404, null!, `User with ID ${id} not found`);
-        }
-        return toSCIMMYUser(user);
-      }
-      const scimmyUsers = (await dbManager.getUsers()).map(user => toSCIMMYUser(user));
-      return filter ? filter.match(scimmyUsers) : scimmyUsers;
-    },
-    ingress: async (resource: any, data: any, context: RequestContext) => {
-      checkAccess(context);
-
-      try {
-        const id = parseInt(resource.id, 10);
-        if (!isNaN(id)) {
-          await checkEmailIsUnique(dbManager, data.userName, id);
-          const updatedUser = await dbManager.overrideUser(id, toUserProfile(data));
-          return toSCIMMYUser(updatedUser);
-        }
-        await checkEmailIsUnique(dbManager, data.userName);
-        const userProfileToInsert = toUserProfile(data);
-        const newUser = await dbManager.getUserByLoginWithRetry(userProfileToInsert.email, {
-          profile: userProfileToInsert
-        });
-        return toSCIMMYUser(newUser);
-      } catch (ex) {
-        if (ex instanceof ApiError) {
-          if (ex.status === 409) {
-            throw new SCIMMY.Types.Error(ex.status, 'uniqueness', ex.message);
-          }
-          throw new SCIMMY.Types.Error(ex.status, null!, ex.message);
-        }
-
-        throw ex;
-      }
-    },
-    degress: async (resource: any, context: RequestContext) => {
-      checkAccess(context);
-
-      const id = parseInt(resource.id, 10);
-      if (isNaN(id)) {
-        throw new SCIMMY.Types.Error(400, null!, 'Invalid ID');
-      }
-      const fakeScope: Scope = { userId: id }; // FIXME: deleteUser should probably better not requiring a scope.
-      try {
-        await dbManager.deleteUser(fakeScope, id);
-      } catch (ex) {
-        if (ex instanceof ApiError) {
-          throw new SCIMMY.Types.Error(ex.status, null!, ex.message);
-        }
-
-        throw new SCIMMY.Types.Error(500, 'serverError', ex.message);
-      }
     }
-  });
+  }
+
+  SCIMMY.Resources.declare(SCIMMY.Resources.User, getScimUserConfig(dbManager, checkAccess));
 
   const scimmyRouter = new SCIMMYRouters({
     type: 'bearer',

test/server/lib/Scim.ts

@@ -416,6 +416,17 @@ describe('Scim', () => {
         });
       });
 
+      it('should return 400 when the user id is malformed', async function () {
+        const res = await axios.put(scimUrl('/Users/not-an-id'), toSCIMUserWithoutId('whoever'), chimpy);
+        assert.deepEqual(res.data, {
+          schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
+          status: '400',
+          detail: 'Invalid passed user ID',
+          scimType: 'invalidValue'
+        });
+        assert.equal(res.status, 400);
+      });
+
       it('should normalize the passed email for the userName and keep the case for email.value', async function () {
         const newEmail = 'my-EMAIL@getgrist.com';
         const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), {