From 103b995c3261cfbb9f89baee1e94b8cfc5a81135 Mon Sep 17 00:00:00 2001 From: fflorent Date: Sat, 7 Sep 2024 11:00:10 +0200 Subject: [PATCH] An unknown error should return a 500 --- app/server/lib/scim/v2/ScimUserController.ts | 73 ++++++++++---------- test/server/lib/Scim.ts | 49 +++++++++---- 2 files changed, 72 insertions(+), 50 deletions(-) diff --git a/app/server/lib/scim/v2/ScimUserController.ts b/app/server/lib/scim/v2/ScimUserController.ts index 2106d984..fc0f5522 100644 --- a/app/server/lib/scim/v2/ScimUserController.ts +++ b/app/server/lib/scim/v2/ScimUserController.ts @@ -19,73 +19,70 @@ class ScimUserController { ) {} 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); + return this._runAndHandleErrors(context, async () => { + 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; + return this._runAndHandleErrors(context, async () => { + 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 { + return this._runAndHandleErrors(context, async () => { 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 { + return this._runAndHandleErrors(context, async () => { 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 { + return this._runAndHandleErrors(context, async () => { + const id = ScimUserController._getIdFromResource(resource); 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); + private async _runAndHandleErrors(context: RequestContext, cb: () => Promise): Promise { + try { + this._checkAccess(context); + return await cb(); + } 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 new SCIMMY.Types.Error(ex.status, null!, ex.message); + if (ex instanceof SCIMMY.Types.Error) { + throw ex; + } + // By default, return a 500 error + throw new SCIMMY.Types.Error(500, null!, ex.message); } - throw ex; } private async _checkEmailIsUnique(email: string, id?: number) { diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts index 934e53af..e6ff2abc 100644 --- a/test/server/lib/Scim.ts +++ b/test/server/lib/Scim.ts @@ -1,6 +1,8 @@ import axios from 'axios'; import capitalize from 'lodash/capitalize'; import { assert } from 'chai'; +import Sinon from 'sinon'; + import { TestServer } from 'test/gen-server/apiUtils'; import { configForUser } from 'test/gen-server/testUtils'; import * as testUtils from 'test/server/testUtils'; @@ -115,7 +117,7 @@ describe('Scim', () => { } } - function checkEndpointNotAccessibleForNonAdminUsers( + function checkCommonErrors( method: 'get' | 'post' | 'put' | 'patch' | 'delete', path: string, validBody: object = {} @@ -127,12 +129,12 @@ describe('Scim', () => { 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'); + const res = await makeCallWith('anon'); assert.equal(res.status, 401); }); it('should return 403 for kiwi', async function () { - const res: any = await makeCallWith('kiwi'); + const res = await makeCallWith('kiwi'); assert.deepEqual(res.data, { schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ], status: '403', @@ -140,6 +142,30 @@ describe('Scim', () => { }); assert.equal(res.status, 403); }); + + it('should return a 500 in case of unknown Error', async function () { + const sandbox = Sinon.createSandbox(); + try { + const error = new Error('Some unexpected Error'); + + // Stub all the dbManager methods called by the controller + sandbox.stub(getDbManager(), 'getUsers').throws(error); + sandbox.stub(getDbManager(), 'getUser').throws(error); + sandbox.stub(getDbManager(), 'getUserByLoginWithRetry').throws(error); + sandbox.stub(getDbManager(), 'overrideUser').throws(error); + sandbox.stub(getDbManager(), 'deleteUser').throws(error); + + const res = await makeCallWith('chimpy'); + assert.deepEqual(res.data, { + schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ], + status: '500', + detail: error.message + }); + assert.equal(res.status, 500); + } finally { + sandbox.restore(); + } + }); } describe('/Me', function () { @@ -181,10 +207,9 @@ describe('Scim', () => { preferredLanguage: 'en', }); }); - }); - describe('/Users/{id}', function () { + describe('GET /Users/{id}', function () { it('should return the user of id=1 for chimpy', async function () { const res = await axios.get(scimUrl('/Users/1'), chimpy); @@ -208,7 +233,7 @@ describe('Scim', () => { }); }); - checkEndpointNotAccessibleForNonAdminUsers('get', '/Users/1'); + checkCommonErrors('get', '/Users/1'); }); describe('GET /Users', function () { @@ -220,7 +245,7 @@ describe('Scim', () => { assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('kiwi')); }); - checkEndpointNotAccessibleForNonAdminUsers('get', '/Users'); + checkCommonErrors('get', '/Users'); }); describe('POST /Users/.search', function () { @@ -261,7 +286,7 @@ describe('Scim', () => { "should have retrieved only chimpy's username and not other attribute"); }); - checkEndpointNotAccessibleForNonAdminUsers('post', '/Users/.search', searchExample); + checkCommonErrors('post', '/Users/.search', searchExample); }); describe('POST /Users', function () { // Create a new users @@ -331,7 +356,7 @@ describe('Scim', () => { assert.equal(res.status, 409); }); - checkEndpointNotAccessibleForNonAdminUsers('post', '/Users', toSCIMUserWithoutId('some-user')); + checkCommonErrors('post', '/Users', toSCIMUserWithoutId('some-user')); }); describe('PUT /Users/{id}', function () { @@ -443,7 +468,7 @@ describe('Scim', () => { }); }); - checkEndpointNotAccessibleForNonAdminUsers('put', '/Users/1', toSCIMUserWithoutId('chimpy')); + checkCommonErrors('put', '/Users/1', toSCIMUserWithoutId('chimpy')); }); describe('PATCH /Users/{id}', function () { @@ -483,7 +508,7 @@ describe('Scim', () => { }); }); - checkEndpointNotAccessibleForNonAdminUsers('patch', '/Users/1', validPatchBody('new name2')); + checkCommonErrors('patch', '/Users/1', validPatchBody('new name2')); }); describe('DELETE /Users/{id}', function () { @@ -513,7 +538,7 @@ describe('Scim', () => { }); assert.equal(res.status, 404); }); - checkEndpointNotAccessibleForNonAdminUsers('delete', '/Users/1'); + checkCommonErrors('delete', '/Users/1'); }); describe('POST /Bulk', function () {