More tests and cleanups

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

@@ -10,20 +10,36 @@ import { parseInt } from 'lodash';
 
 const WHITELISTED_PATHS_FOR_NON_ADMINS = [ "/Me", "/Schemas", "/ResourceTypes", "/ServiceProviderConfig" ];
 
-async function isAuthorizedAction(mreq: RequestWithLogin, installAdmin: InstallAdmin): Promise<boolean> {
+interface RequestContext {
-  const isAdmin = await installAdmin.isAdminReq(mreq);
+  path: string;
-  const isScimUser = Boolean(process.env.GRIST_SCIM_EMAIL && mreq.user?.loginEmail === process.env.GRIST_SCIM_EMAIL);
+  isAdmin: boolean;
-  return isAdmin || isScimUser || WHITELISTED_PATHS_FOR_NON_ADMINS.includes(mreq.path);
+  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, {
-    egress: async (resource: any) => {
+    egress: async (resource: any, context: RequestContext) => {
+      checkAccess(context);
+
       const { filter } = resource;
       const id = parseInt(resource.id, 10);
-      if (id) {
+      if (!isNaN(id)) {
         const user = await dbManager.getUser(id);
         if (!user) {
           throw new SCIMMY.Types.Error(404, null!, `User with ID ${id} not found`);
@@ -33,18 +49,18 @@ const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin)
       const scimmyUsers = (await dbManager.getUsers()).map(user => toSCIMMYUser(user));
       return filter ? filter.match(scimmyUsers) : scimmyUsers;
     },
-    ingress: async (resource: any, data: any) => {
+    ingress: async (resource: any, data: any, context: RequestContext) => {
+      checkAccess(context);
+
       try {
         const id = parseInt(resource.id, 10);
-        if (id) {
+        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 maybeExistingUser = await dbManager.getExistingUserByLogin(userProfileToInsert.email);
-        if (maybeExistingUser !== undefined) {
-          throw new SCIMMY.Types.Error(409, 'uniqueness', 'An existing user with the passed email exist.');
-        }
         const newUser = await dbManager.getUserByLoginWithRetry(userProfileToInsert.email, {
           profile: userProfileToInsert
         });
@@ -57,29 +73,20 @@ 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);
-          }
-        }
-
         throw ex;
       }
     },
-    degress: async (resource: any) => {
+    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) {
-        console.error('Error deleting user', ex);
         if (ex instanceof ApiError) {
           throw new SCIMMY.Types.Error(ex.status, null!, ex.message);
         }
@@ -102,10 +109,15 @@ const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin)
         throw new Error('Anonymous user cannot access SCIM resources');
       }
 
-      if (!await isAuthorizedAction(mreq, installAdmin)) {
+      return String(mreq.userId); // SCIMMYRouters requires the userId to be a string.
-        throw new SCIMMY.Types.Error(403, null!, 'Resource disallowed for non-admin users');
+    },
-      }
+    context: async (mreq: RequestWithLogin): Promise<RequestContext> => {
-      return String(mreq.userId); // HACK: SCIMMYRouters requires the userId to be a string.
+      const isAdmin = await installAdmin.isAdminReq(mreq);
+      const isScimUser = Boolean(
+        process.env.GRIST_SCIM_EMAIL && mreq.user?.loginEmail === process.env.GRIST_SCIM_EMAIL
+      );
+      const path = mreq.path;
+      return { isAdmin, isScimUser, path };
     }
   }) as express.Router; // Have to cast it into express.Router. See https://github.com/scimmyjs/scimmy-routers/issues/24
 

test/server/lib/Scim.ts

@@ -18,7 +18,7 @@ function scimConfigForUser(user: string) {
 
 const chimpy = scimConfigForUser('Chimpy');
 const kiwi = scimConfigForUser('Kiwi');
-const anon = configForUser('Anonymous');
+const anon = scimConfigForUser('Anonymous');
 
 const USER_CONFIG_BY_NAME = {
   chimpy,
@@ -89,13 +89,14 @@ describe('Scim', () => {
 
   function checkEndpointNotAccessibleForNonAdminUsers(
     method: 'get' | 'post' | 'put' | 'patch' | 'delete',
-    path: string
+    path: string,
+    validBody: object = {}
   ) {
     function makeCallWith(user: keyof UserConfigByName) {
       if (method === 'get' || method === 'delete') {
         return axios[method](scimUrl(path), USER_CONFIG_BY_NAME[user]);
       }
-      return axios[method](scimUrl(path), {}, USER_CONFIG_BY_NAME[user]);
+      return axios[method](scimUrl(path), validBody, USER_CONFIG_BY_NAME[user]);
     }
     it('should return 401 for anonymous', async function () {
       const res: any = await makeCallWith('anon');
@@ -104,7 +105,12 @@ describe('Scim', () => {
 
     it('should return 401 for kiwi', async function () {
       const res: any = await makeCallWith('kiwi');
-      assert.equal(res.status, 401);
+      assert.deepEqual(res.data, {
+        schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
+        status: '403',
+        detail: 'You are not authorized to access this resource'
+      });
+      assert.equal(res.status, 403);
     });
   }
 
@@ -127,6 +133,27 @@ describe('Scim', () => {
       const res = await axios.get(scimUrl('/Me'), anon);
       assert.equal(res.status, 401);
     });
+
+    it.skip('should allow operation like PATCH for kiwi', async function () {
+      // SKIPPING this test: only the GET verb is currently implemented by SCIMMY for the /Me endpoint.
+      // Issue created here: https://github.com/scimmyjs/scimmy/issues/47
+      const patchBody = {
+        schemas: ['urn:ietf:params:scim:api:messages:2.0:PatchOp'],
+        Operations: [{
+          op: "replace",
+          path: 'locale',
+          value: 'fr',
+        }],
+      };
+      const res = await axios.patch(scimUrl('/Me'), patchBody, kiwi);
+      assert.equal(res.status, 200);
+      assert.deepEqual(res.data, {
+        ...personaToSCIMMYUserWithId('kiwi'),
+        locale: 'fr',
+        preferredLanguage: 'en',
+      });
+    });
+
   });
 
   describe('/Users/{id}', function () {
@@ -170,12 +197,15 @@ describe('Scim', () => {
 
   describe('POST /Users/.search', function () {
     const SEARCH_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:SearchRequest';
-    it('should return all users for chimpy order by userName in descending order', async function () {
+
-      const res = await axios.post(scimUrl('/Users/.search'), {
+    const searchExample = {
       schemas: [SEARCH_SCHEMA],
       sortBy: 'userName',
       sortOrder: 'descending',
-      }, chimpy);
+    };
+
+    it('should return all users for chimpy order by userName in descending order', async function () {
+      const res = await axios.post(scimUrl('/Users/.search'), searchExample, chimpy);
       assert.equal(res.status, 200);
       assert.isAbove(res.data.totalResults, 0, 'should have retrieved some users');
       const users = res.data.Resources.map((r: any) => r.userName);
@@ -198,54 +228,70 @@ describe('Scim', () => {
         "should have retrieved only chimpy's username and not other attribute");
     });
 
-    checkEndpointNotAccessibleForNonAdminUsers('post', '/Users/.search');
+    checkEndpointNotAccessibleForNonAdminUsers('post', '/Users/.search', searchExample);
   });
 
   describe('POST /Users', function () { // Create a new users
+    async function withUserName(userName: string, cb: (userName: string) => Promise<void>) {
+      try {
+        await cb(userName);
+      } finally {
+        const user = await server.dbManager.getExistingUserByLogin(userName + "@getgrist.com");
+        if (user) {
+          await cleanupUser(user.id);
+        }
+      }
+    }
     it('should create a new user', async function () {
-      const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId('newuser1'), chimpy);
+      await withUserName('newuser1', async (userName) => {
+        const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId(userName), chimpy);
         assert.equal(res.status, 201);
-      const newUserId = await getOrCreateUserId('newuser1');
+        const newUserId = await getOrCreateUserId(userName);
-      assert.deepEqual(res.data, toSCIMUserWithId('newuser1', newUserId));
+        assert.deepEqual(res.data, toSCIMUserWithId(userName, newUserId));
+      });
     });
 
     it('should allow creating a new user given only their email passed as username', async function () {
+      await withUserName('new.user2', async (userName) => {
         const res = await axios.post(scimUrl('/Users'), {
           schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'],
           userName: 'new.user2@getgrist.com',
         }, chimpy);
         assert.equal(res.status, 201);
-      assert.equal(res.data.userName, 'new.user2@getgrist.com');
+        assert.equal(res.data.userName, userName + '@getgrist.com');
-      assert.equal(res.data.displayName, 'new.user2');
+        assert.equal(res.data.displayName, userName);
+      });
     });
 
     it('should reject when passed email differs from username', async function () {
+      await withUserName('username', async (userName) => {
         const res = await axios.post(scimUrl('/Users'), {
           schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'],
-        userName: 'username@getgrist.com',
+          userName: userName + '@getgrist.com',
           emails: [{ value: 'emails.value@getgrist.com' }],
         }, chimpy);
-      assert.equal(res.status, 400);
         assert.deepEqual(res.data, {
           schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
           status: '400',
           detail: 'Email and userName must be the same',
           scimType: 'invalidValue'
         });
+        assert.equal(res.status, 400);
+      });
     });
 
     it('should disallow creating a user with the same email', async function () {
       const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId('chimpy'), chimpy);
-      assert.equal(res.status, 409);
       assert.deepEqual(res.data, {
         schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
         status: '409',
         detail: 'An existing user with the passed email exist.',
         scimType: 'uniqueness'
       });
+      assert.equal(res.status, 409);
     });
 
-    checkEndpointNotAccessibleForNonAdminUsers('post', '/Users');
+    checkEndpointNotAccessibleForNonAdminUsers('post', '/Users', toSCIMUserWithoutId('some-user'));
   });
 
   describe('PUT /Users/{id}', function () {
@@ -286,34 +332,34 @@ describe('Scim', () => {
         userName: userToUpdateEmailLocalPart + '@getgrist.com',
         emails: [{ value: 'whatever@getgrist.com', primary: true }],
       }, chimpy);
-      assert.equal(res.status, 400);
       assert.deepEqual(res.data, {
         schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
         status: '400',
         detail: 'Email and userName must be the same',
         scimType: 'invalidValue'
       });
+      assert.equal(res.status, 400);
     });
 
-    it('should disallow updating a user with the same email', async function () {
+    it('should disallow updating a user with the same email as another user\'s', async function () {
       const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), toSCIMUserWithoutId('chimpy'), chimpy);
-      assert.equal(res.status, 409);
       assert.deepEqual(res.data, {
         schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
         status: '409',
-        detail: 'SQLITE_CONSTRAINT: UNIQUE constraint failed: logins.email',
+        detail: 'An existing user with the passed email exist.',
         scimType: 'uniqueness'
       });
+      assert.equal(res.status, 409);
     });
 
     it('should return 404 when the user is not found', async function () {
-      const res = await axios.put(scimUrl('/Users/1000'), toSCIMUserWithoutId('chimpy'), chimpy);
+      const res = await axios.put(scimUrl('/Users/1000'), toSCIMUserWithoutId('whoever'), chimpy);
-      assert.equal(res.status, 404);
       assert.deepEqual(res.data, {
         schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
         status: '404',
         detail: 'unable to find user to update'
       });
+      assert.equal(res.status, 404);
     });
 
     it('should deduce the name from the displayEmail when not provided', async function () {
@@ -346,7 +392,7 @@ describe('Scim', () => {
       });
     });
 
-    checkEndpointNotAccessibleForNonAdminUsers('put', '/Users/1');
+    checkEndpointNotAccessibleForNonAdminUsers('put', '/Users/1', toSCIMUserWithoutId('chimpy'));
   });
 
   describe('PATCH /Users/{id}', function () {
@@ -359,9 +405,7 @@ describe('Scim', () => {
       await cleanupUser(userToPatchId);
     });
 
-    it('should replace values of an existing user', async function () {
+    const validPatchBody = (newName: string) => ({
-      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",
@@ -372,7 +416,11 @@ describe('Scim', () => {
           path: "locale",
           value: 'fr'
         }],
-      }, chimpy);
+    });
+
+    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}`), validPatchBody(newName), chimpy);
       assert.equal(res.status, 200);
       const refreshedUser = await axios.get(scimUrl(`/Users/${userToPatchId}`), chimpy);
       assert.deepEqual(refreshedUser.data, {
@@ -384,7 +432,7 @@ describe('Scim', () => {
       });
     });
 
-    checkEndpointNotAccessibleForNonAdminUsers('patch', '/Users/1');
+    checkEndpointNotAccessibleForNonAdminUsers('patch', '/Users/1', validPatchBody('new name2'));
   });
 
   describe('DELETE /Users/{id}', function () {
@@ -407,17 +455,200 @@ describe('Scim', () => {
 
     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'
       });
+      assert.equal(res.status, 404);
     });
     checkEndpointNotAccessibleForNonAdminUsers('delete', '/Users/1');
   });
 
-  it('should fix the 401 response for authenticated users', function () {
+  describe('POST /Bulk', function () {
-    throw new Error("This is a reminder :)");
+    let usersToCleanupEmails: string[];
+
+    beforeEach(async function () {
+      usersToCleanupEmails = [];
+      usersToCleanupEmails.push('bulk-user1@getgrist.com');
+      usersToCleanupEmails.push('bulk-user2@getgrist.com');
+    });
+
+    afterEach(async function () {
+      for (const email of usersToCleanupEmails) {
+        const user = await server.dbManager.getExistingUserByLogin(email);
+        if (user) {
+          await cleanupUser(user.id);
+        }
+      }
+    });
+
+    it('should return statuses for each operation', async function () {
+      const putOnUnknownResource = { method: 'PUT', path: '/Users/1000', value: toSCIMUserWithoutId('chimpy') };
+      const validCreateOperation = {
+        method: 'POST', path: '/Users/', data: toSCIMUserWithoutId('bulk-user3'), bulkId: '1'
+      };
+      usersToCleanupEmails.push('bulk-user3');
+      const createOperationWithUserNameConflict = {
+        method: 'POST', path: '/Users/', data: toSCIMUserWithoutId('chimpy'), bulkId: '2'
+      };
+      const res = await axios.post(scimUrl('/Bulk'), {
+        schemas: ['urn:ietf:params:scim:api:messages:2.0:BulkRequest'],
+        Operations: [
+          putOnUnknownResource,
+          validCreateOperation,
+          createOperationWithUserNameConflict,
+        ],
+      }, chimpy);
+      assert.equal(res.status, 200);
+      assert.deepEqual(res.data, {
+        schemas: [ "urn:ietf:params:scim:api:messages:2.0:BulkResponse" ],
+        Operations: [
+          {
+            method: "PUT",
+            location: "/api/scim/v2/Users/1000",
+            status: "400",
+            response: {
+              schemas: [
+                "urn:ietf:params:scim:api:messages:2.0:Error"
+              ],
+              status: "400",
+              scimType: "invalidSyntax",
+              detail: "Expected 'data' to be a single complex value in BulkRequest operation #1"
+            }
+          }, {
+            method: "POST",
+            bulkId: "1",
+            location: "/api/scim/v2/Users/26",
+            status: "201"
+          }, {
+            method: "POST",
+            bulkId: "2",
+            status: "409",
+            response: {
+              schemas: [
+                "urn:ietf:params:scim:api:messages:2.0:Error"
+              ],
+              status: "409",
+              scimType: "uniqueness",
+              detail: "An existing user with the passed email exist."
+            }
+          }
+        ]
+      });
+    });
+
+    it('should return 400 when no operations are provided', async function () {
+      const res = await axios.post(scimUrl('/Bulk'), {
+        schemas: ['urn:ietf:params:scim:api:messages:2.0:BulkRequest'],
+        Operations: [],
+      }, chimpy);
+      assert.equal(res.status, 400);
+      assert.deepEqual(res.data, {
+        schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ],
+        status: '400',
+        detail: "BulkRequest request body must contain 'Operations' attribute with at least one operation",
+        scimType: 'invalidValue'
+      });
+    });
+
+    it('should disallow accessing resources to kiwi', async function () {
+      const creationOperation = {
+        method: 'POST', path: '/Users', data: toSCIMUserWithoutId('bulk-user4'), bulkId: '1'
+      };
+      usersToCleanupEmails.push('bulk-user4');
+      const selfPutOperation = { method: 'PUT', path: '/Me', value: toSCIMUserWithoutId('kiwi') };
+      const res = await axios.post(scimUrl('/Bulk'), {
+        schemas: ['urn:ietf:params:scim:api:messages:2.0:BulkRequest'],
+        Operations: [
+          creationOperation,
+          selfPutOperation,
+        ],
+      }, kiwi);
+      assert.equal(res.status, 200);
+      assert.deepEqual(res.data, {
+        schemas: [ "urn:ietf:params:scim:api:messages:2.0:BulkResponse" ],
+        Operations: [
+          {
+            method: "POST",
+            bulkId: "1",
+            status: "403",
+            response: {
+              detail: "You are not authorized to access this resource",
+              schemas: [ "urn:ietf:params:scim:api:messages:2.0:Error" ],
+              status: "403"
+            }
+          }, {
+            // When writing this test, the SCIMMY implementation does not yet support PUT operations on /Me.
+            // This reflects the current behavior, but it may change in the future.
+            // Change this test if the behavior changes.
+            // It is probably fine to allow altering oneself even for non-admins.
+            method: "PUT",
+            location: "/Me",
+            status: "400",
+            response: {
+              schemas: [
+                "urn:ietf:params:scim:api:messages:2.0:Error"
+              ],
+              status: "400",
+              detail: "Invalid 'path' value '/Me' in BulkRequest operation #2",
+              scimType: "invalidValue"
+            }
+          }
+        ]
+      });
+    });
+
+    it('should disallow accessing resources to anonymous', async function () {
+      const creationOperation = {
+        method: 'POST', path: '/Users', data: toSCIMUserWithoutId('bulk-user5'), bulkId: '1'
+      };
+      usersToCleanupEmails.push('bulk-user5');
+      const res = await axios.post(scimUrl('/Bulk'), {
+        schemas: ['urn:ietf:params:scim:api:messages:2.0:BulkRequest'],
+        Operations: [creationOperation],
+      }, anon);
+      assert.equal(res.status, 401);
+    });
+  });
+
+  it('should allow fetching the Scim schema when autenticated', async function () {
+    const res = await axios.get(scimUrl('/Schemas'), kiwi);
+    assert.equal(res.status, 200);
+    assert.deepInclude(res.data, {
+      schemas: ['urn:ietf:params:scim:api:messages:2.0:ListResponse'],
+    });
+    assert.property(res.data, 'Resources');
+    assert.deepInclude(res.data.Resources[0], {
+      schemas: ['urn:ietf:params:scim:schemas:core:2.0:Schema'],
+      id: 'urn:ietf:params:scim:schemas:core:2.0:User',
+      name: 'User',
+      description: 'User Account',
+    });
+  });
+
+  it('should allow fetching the Scim resource types when autenticated', async function () {
+    const res = await axios.get(scimUrl('/ResourceTypes'), kiwi);
+    assert.equal(res.status, 200);
+    assert.deepInclude(res.data, {
+      schemas: ['urn:ietf:params:scim:api:messages:2.0:ListResponse'],
+    });
+    assert.property(res.data, 'Resources');
+    assert.deepInclude(res.data.Resources[0], {
+      schemas: ['urn:ietf:params:scim:schemas:core:2.0:ResourceType'],
+      name: 'User',
+      endpoint: '/Users',
+    });
+  });
+
+  it('should allow fetching the Scim service provider config when autenticated', async function () {
+    const res = await axios.get(scimUrl('/ServiceProviderConfig'), kiwi);
+    assert.equal(res.status, 200);
+    assert.deepInclude(res.data, {
+      schemas: ['urn:ietf:params:scim:schemas:core:2.0:ServiceProviderConfig'],
+    });
+    assert.property(res.data, 'patch');
+    assert.property(res.data, 'bulk');
+    assert.property(res.data, 'filter');
   });
 });