From 92b4a65da030f7f5fc02140581730fc0133a5ae6 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Thu, 29 Aug 2024 10:13:48 +0200 Subject: [PATCH 01/15] WIP --- app/server/lib/scim/index.ts | 24 +++++++++++++++++++++ app/server/lib/scim/v2/users.ts | 37 +++++++++++++++++++++++++++++++++ package.json | 1 + yarn.lock | 15 ++++++++++++- 4 files changed, 76 insertions(+), 1 deletion(-) create mode 100644 app/server/lib/scim/index.ts create mode 100644 app/server/lib/scim/v2/users.ts diff --git a/app/server/lib/scim/index.ts b/app/server/lib/scim/index.ts new file mode 100644 index 00000000..54031c35 --- /dev/null +++ b/app/server/lib/scim/index.ts @@ -0,0 +1,24 @@ +import * as express from 'express'; +import { buildUsersRoute, checkPermissionToUsersEndpoint } from './v2/users'; +import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; +import SCIMMY from "scimmy"; +import SCIMMYRouters from "scimmy-routers"; + +type SCIMMYResource = typeof SCIMMY.Types.Resource; + +const buildScimRouter = (dbManager: HomeDBManager) => { + const v2 = express.Router(); + v2.use('/Users', checkPermissionToUsersEndpoint, buildUsersRoute(dbManager)); + + SCIMMY.Resources.User.ingress(handler) + SCIMMY.Resources.declare(SCIMMY.Resources.User) + .ingress((resource: SCIMMYResource, data) => { + + + }); + const scim = express.Router(); + scim.use('/v2', v2); + return scim; +}; + +export { buildScimRouter }; diff --git a/app/server/lib/scim/v2/users.ts b/app/server/lib/scim/v2/users.ts new file mode 100644 index 00000000..1b4175bd --- /dev/null +++ b/app/server/lib/scim/v2/users.ts @@ -0,0 +1,37 @@ +import express, { NextFunction, Request, Response } from 'express'; +import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; +import { expressWrap } from '../../expressWrap'; +import { integerParam } from '../../requestUtils'; +import { ApiError } from 'app/common/ApiError'; +import { RequestWithLogin } from '../../Authorizer'; + +function checkPermissionToUsersEndpoint(req: Request, res: Response, next: NextFunction) { + const mreq = req as RequestWithLogin; + const adminEmail = process.env.GRIST_DEFAULT_EMAIL; + if (!adminEmail || mreq.user?.loginEmail !== adminEmail) { + throw new ApiError('Permission denied', 403); + } + return next(); +} + +const buildUsersRoute = (dbManager: HomeDBManager) => { + const userRoute = express.Router(); + + async function findUserOrFail(userId: number) { + const user = await dbManager.getUser(userId); + if (!user) { + throw new ApiError('User not found', 404); + } + return user; + } + + + userRoute.get('/:id', expressWrap(async (req, res) => { + const userId = integerParam(req.params.id, 'id'); + const user = await findUserOrFail(userId); + res.status(200).json(user); + })); + return userRoute; +}; + +export { buildUsersRoute, checkPermissionToUsersEndpoint }; diff --git a/package.json b/package.json index 773f1ed7..9a52c5b7 100644 --- a/package.json +++ b/package.json @@ -189,6 +189,7 @@ "redis": "3.1.1", "redlock": "3.1.2", "saml2-js": "4.0.2", + "scimmy-routers": "^1.2.0", "short-uuid": "3.1.1", "slugify": "1.6.6", "swagger-ui-dist": "5.11.0", diff --git a/yarn.lock b/yarn.lock index 4a9ea139..35d640ae 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3801,7 +3801,7 @@ express-rate-limit@7.2.0: resolved "https://registry.yarnpkg.com/express-rate-limit/-/express-rate-limit-7.2.0.tgz#06ce387dd5388f429cab8263c514fc07bf90a445" integrity sha512-T7nul1t4TNyfZMJ7pKRKkdeVJWa2CqB8NA1P8BwYaoDI5QSBZARv5oMS43J7b7I5P+4asjVXjb7ONuwDKucahg== -express@4.19.2: +express@4.19.2, express@^4.18.2: version "4.19.2" resolved "https://registry.yarnpkg.com/express/-/express-4.19.2.tgz#e25437827a3aa7f2a827bc8171bbbb664a356465" integrity sha512-5T6nhjsT+EOMzuck8JjBHARTHfMht0POzlA60WV2pMD3gyXw2LZnZ+ueGdNxG+0calOJcWKbpFcuzLZ91YWq9Q== @@ -7346,6 +7346,19 @@ schema-utils@^3.2.0: ajv "^6.12.5" ajv-keywords "^3.5.2" +scimmy-routers@^1.2.0: + version "1.2.0" + resolved "https://registry.yarnpkg.com/scimmy-routers/-/scimmy-routers-1.2.0.tgz#42090c616f127aefce194ebf4a3c8f4a4f62e30b" + integrity sha512-+dT8yRglz2gMu0X1LlUYTi/PDgW6Zzu1YRWiv362I/wA64kud24XjYIoufNiW5OhskvBPQGT/P1aYOffcmxxsQ== + dependencies: + express "^4.18.2" + scimmy "^1.2.0" + +scimmy@^1.2.0: + version "1.2.3" + resolved "https://registry.yarnpkg.com/scimmy/-/scimmy-1.2.3.tgz#56ca9dbf11749b272e18090c923f81dbad4bc911" + integrity sha512-16oXCvnieVeKxTDQqi275bLuyOCwXci8Jywm2/M+4dWNNYoduUz0Crj1nFY0ETYMsuYvCdareWov6/Mebu92xA== + selenium-webdriver@^4.20.0: version "4.20.0" resolved "https://registry.yarnpkg.com/selenium-webdriver/-/selenium-webdriver-4.20.0.tgz#14941ab4a59e8956a5e4b4491a8ba2bd6619d1ac" From 98f50d3c8158d27fe271a7e8d56f88118a1cd334 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Mon, 2 Sep 2024 17:50:42 +0200 Subject: [PATCH 02/15] SCIM: Implement egress + tests --- app/gen-server/lib/homedb/HomeDBManager.ts | 6 +- app/gen-server/lib/homedb/UsersManager.ts | 6 +- app/server/MergedServer.ts | 1 + app/server/lib/FlexServer.ts | 7 ++ app/server/lib/scim/index.ts | 20 ++---- app/server/lib/scim/v2/ScimUserUtils.ts | 31 +++++++++ app/server/lib/scim/v2/ScimV2Api.ts | 76 ++++++++++++++++++++++ app/server/lib/scim/v2/users.ts | 37 ----------- package.json | 3 +- test/gen-server/seed.ts | 1 + test/server/lib/Authorizer.ts | 1 + yarn.lock | 16 ++--- 12 files changed, 142 insertions(+), 63 deletions(-) create mode 100644 app/server/lib/scim/v2/ScimUserUtils.ts create mode 100644 app/server/lib/scim/v2/ScimV2Api.ts delete mode 100644 app/server/lib/scim/v2/users.ts diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index 58e6a561..08f77e2d 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -441,6 +441,10 @@ export class HomeDBManager extends EventEmitter { return this._usersManager.getUser(userId, options); } + public async getUsers() { + return this._usersManager.getUsers(); + } + public async getFullUser(userId: number) { return this._usersManager.getFullUser(userId); } @@ -3271,7 +3275,7 @@ export class HomeDBManager extends EventEmitter { // Get the user objects which map to non-null values in the userDelta. const userIds = Object.keys(userDelta).filter(userId => userDelta[userId]) .map(userIdStr => parseInt(userIdStr, 10)); - const users = await this._usersManager.getUsers(userIds, manager); + const users = await this._usersManager.getUsersByIds(userIds, manager); // Add unaffected users to the delta so that we have a record of where they are. groups.forEach(grp => { diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index e2be5b1d..81bde779 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -657,10 +657,14 @@ export class UsersManager { return [this.getSupportUserId(), this.getAnonymousUserId(), this.getEveryoneUserId()]; } + public async getUsers() { + return await User.find({relations: ["logins"]}); + } + /** * Returns a Promise for an array of User entites for the given userIds. */ - public async getUsers(userIds: number[], optManager?: EntityManager): Promise<User[]> { + public async getUsersByIds(userIds: number[], optManager?: EntityManager): Promise<User[]> { if (userIds.length === 0) { return []; } diff --git a/app/server/MergedServer.ts b/app/server/MergedServer.ts index 2ac994d1..cfa53b48 100644 --- a/app/server/MergedServer.ts +++ b/app/server/MergedServer.ts @@ -161,6 +161,7 @@ export class MergedServer { await this.flexServer.addLandingPages(); // todo: add support for home api to standalone app this.flexServer.addHomeApi(); + this.flexServer.addScimApi(); this.flexServer.addBillingApi(); this.flexServer.addNotifier(); this.flexServer.addAuditLogger(); diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 45297904..5f641549 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -87,6 +87,7 @@ import * as path from 'path'; import * as serveStatic from "serve-static"; import {ConfigBackendAPI} from "app/server/lib/ConfigBackendAPI"; import {IGristCoreConfig} from "app/server/lib/configCore"; +import { buildScimRouter } from './scim'; // Health checks are a little noisy in the logs, so we don't show them all. // We show the first N health checks: @@ -887,6 +888,12 @@ export class FlexServer implements GristServer { new ApiServer(this, this.app, this._dbManager, this._widgetRepository = buildWidgetRepository(this)); } + public addScimApi() { + if (this._check('scim', 'api', 'homedb', 'json', 'api-mw')) { return; } + this.app.use('/api/scim', buildScimRouter(this._dbManager, this._installAdmin)); + } + + public addBillingApi() { if (this._check('billing-api', 'homedb', 'json', 'api-mw')) { return; } this._getBilling(); diff --git a/app/server/lib/scim/index.ts b/app/server/lib/scim/index.ts index 54031c35..2ee49303 100644 --- a/app/server/lib/scim/index.ts +++ b/app/server/lib/scim/index.ts @@ -1,21 +1,11 @@ import * as express from 'express'; -import { buildUsersRoute, checkPermissionToUsersEndpoint } from './v2/users'; + +import { buildScimRouterv2 } from './v2/ScimV2Api'; import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; -import SCIMMY from "scimmy"; -import SCIMMYRouters from "scimmy-routers"; +import { InstallAdmin } from '../InstallAdmin'; -type SCIMMYResource = typeof SCIMMY.Types.Resource; - -const buildScimRouter = (dbManager: HomeDBManager) => { - const v2 = express.Router(); - v2.use('/Users', checkPermissionToUsersEndpoint, buildUsersRoute(dbManager)); - - SCIMMY.Resources.User.ingress(handler) - SCIMMY.Resources.declare(SCIMMY.Resources.User) - .ingress((resource: SCIMMYResource, data) => { - - - }); +const buildScimRouter = (dbManager: HomeDBManager, installAdmin: InstallAdmin) => { + const v2 = buildScimRouterv2(dbManager, installAdmin); const scim = express.Router(); scim.use('/v2', v2); return scim; diff --git a/app/server/lib/scim/v2/ScimUserUtils.ts b/app/server/lib/scim/v2/ScimUserUtils.ts new file mode 100644 index 00000000..b9db4bf0 --- /dev/null +++ b/app/server/lib/scim/v2/ScimUserUtils.ts @@ -0,0 +1,31 @@ +import { User } from "app/gen-server/entity/User.js"; +import { SCIMMY } from "scimmy-routers"; + +/** + * Converts a user from your database to a SCIMMY user + */ +export function toSCIMMYUser(user: User) { + if (!user.logins) { + throw new Error("User must have at least one login"); + } + const preferredLanguage = user.options?.locale ?? "en"; + return new SCIMMY.Schemas.User({ + id: String(user.id), + userName: user.loginEmail, + displayName: user.name, + name: { + formatted: user.name, + }, + preferredLanguage, + locale: preferredLanguage, // Assume locale is the same as preferredLanguage + photos: user.picture ? [{ + value: user.picture, + type: "photo", + primary: true + }] : undefined, + emails: [{ + value: user.loginEmail, + primary: true, + }], + }); +} diff --git a/app/server/lib/scim/v2/ScimV2Api.ts b/app/server/lib/scim/v2/ScimV2Api.ts new file mode 100644 index 00000000..05026251 --- /dev/null +++ b/app/server/lib/scim/v2/ScimV2Api.ts @@ -0,0 +1,76 @@ +import * as express from 'express'; +import { HomeDBManager } 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 } from './ScimUserUtils'; + +const WHITELISTED_PATHS_FOR_NON_ADMINS = [ "/Me", "/Schemas", "/ResourceTypes", "/ServiceProviderConfig" ]; + +async function isAuthorizedAction(mreq: RequestWithLogin, installAdmin: InstallAdmin): Promise<boolean> { + const isAdmin = await installAdmin.isAdminReq(mreq) + const isScimUser = Boolean(process.env.GRIST_SCIM_EMAIL && mreq.user?.loginEmail === process.env.GRIST_SCIM_EMAIL); + return isAdmin || isScimUser || WHITELISTED_PATHS_FOR_NON_ADMINS.includes(mreq.path); +} + +const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin) => { + const v2 = express.Router(); + + SCIMMY.Resources.declare(SCIMMY.Resources.User, { + egress: async (resource: any) => { + const { id, filter } = resource; + if (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) => { + try { + const { id } = resource; + if (id) { + return null; + } + return []; + } catch (ex) { + // FIXME: remove this + if (Math.random() > 1) { + return null; + } + throw ex; + } + }, + degress: () => { + return null; + } + }); + + const scimmyRouter = new SCIMMYRouters({ + type: 'bearer', + handler: async (request: express.Request) => { + const mreq = request as RequestWithLogin; + if (mreq.userId === undefined) { + // Note that any Error thrown here is automatically converted into a 403 response by SCIMMYRouters. + throw new Error('You are not authorized to access this resource!'); + } + + if (mreq.userId === dbManager.getAnonymousUserId()) { + throw new Error('Anonymous user cannot access SCIM resources'); + } + + if (!await isAuthorizedAction(mreq, installAdmin)) { + throw new SCIMMY.Types.Error(403, null!, 'Resource disallowed for non-admin users'); + } + return String(mreq.userId); // HACK: SCIMMYRouters requires the userId to be a string. + } + }) as express.Router; // Have to cast it into express.Router. See https://github.com/scimmyjs/scimmy-routers/issues/24 + + return v2.use('/', scimmyRouter); +}; + +export { buildScimRouterv2 }; diff --git a/app/server/lib/scim/v2/users.ts b/app/server/lib/scim/v2/users.ts deleted file mode 100644 index 1b4175bd..00000000 --- a/app/server/lib/scim/v2/users.ts +++ /dev/null @@ -1,37 +0,0 @@ -import express, { NextFunction, Request, Response } from 'express'; -import { HomeDBManager } from 'app/gen-server/lib/homedb/HomeDBManager'; -import { expressWrap } from '../../expressWrap'; -import { integerParam } from '../../requestUtils'; -import { ApiError } from 'app/common/ApiError'; -import { RequestWithLogin } from '../../Authorizer'; - -function checkPermissionToUsersEndpoint(req: Request, res: Response, next: NextFunction) { - const mreq = req as RequestWithLogin; - const adminEmail = process.env.GRIST_DEFAULT_EMAIL; - if (!adminEmail || mreq.user?.loginEmail !== adminEmail) { - throw new ApiError('Permission denied', 403); - } - return next(); -} - -const buildUsersRoute = (dbManager: HomeDBManager) => { - const userRoute = express.Router(); - - async function findUserOrFail(userId: number) { - const user = await dbManager.getUser(userId); - if (!user) { - throw new ApiError('User not found', 404); - } - return user; - } - - - userRoute.get('/:id', expressWrap(async (req, res) => { - const userId = integerParam(req.params.id, 'id'); - const user = await findUserOrFail(userId); - res.status(200).json(user); - })); - return userRoute; -}; - -export { buildUsersRoute, checkPermissionToUsersEndpoint }; diff --git a/package.json b/package.json index 9a52c5b7..78aabe41 100644 --- a/package.json +++ b/package.json @@ -189,7 +189,8 @@ "redis": "3.1.1", "redlock": "3.1.2", "saml2-js": "4.0.2", - "scimmy-routers": "^1.2.0", + "scimmy": "1.2.3", + "scimmy-routers": "1.2.1", "short-uuid": "3.1.1", "slugify": "1.6.6", "swagger-ui-dist": "5.11.0", diff --git a/test/gen-server/seed.ts b/test/gen-server/seed.ts index c3e7073f..a4acbdf0 100644 --- a/test/gen-server/seed.ts +++ b/test/gen-server/seed.ts @@ -612,6 +612,7 @@ export async function createServer(port: number, initDb = createInitialDb): Prom flexServer.addAccessMiddleware(); flexServer.addApiMiddleware(); flexServer.addHomeApi(); + flexServer.addScimApi(); flexServer.addApiErrorHandlers(); await initDb(flexServer.getHomeDBManager().connection); flexServer.summary(); diff --git a/test/server/lib/Authorizer.ts b/test/server/lib/Authorizer.ts index 449bf138..9f35c939 100644 --- a/test/server/lib/Authorizer.ts +++ b/test/server/lib/Authorizer.ts @@ -35,6 +35,7 @@ async function activateServer(home: FlexServer, docManager: DocManager) { await home.addLandingPages(); home.addHomeApi(); home.addAuditLogger(); + home.addScimApi(); await home.addTelemetry(); await home.addDoc(); home.addApiErrorHandlers(); diff --git a/yarn.lock b/yarn.lock index 35d640ae..4a8d7a96 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3801,7 +3801,7 @@ express-rate-limit@7.2.0: resolved "https://registry.yarnpkg.com/express-rate-limit/-/express-rate-limit-7.2.0.tgz#06ce387dd5388f429cab8263c514fc07bf90a445" integrity sha512-T7nul1t4TNyfZMJ7pKRKkdeVJWa2CqB8NA1P8BwYaoDI5QSBZARv5oMS43J7b7I5P+4asjVXjb7ONuwDKucahg== -express@4.19.2, express@^4.18.2: +express@4.19.2, express@^4.19.2: version "4.19.2" resolved "https://registry.yarnpkg.com/express/-/express-4.19.2.tgz#e25437827a3aa7f2a827bc8171bbbb664a356465" integrity sha512-5T6nhjsT+EOMzuck8JjBHARTHfMht0POzlA60WV2pMD3gyXw2LZnZ+ueGdNxG+0calOJcWKbpFcuzLZ91YWq9Q== @@ -7346,15 +7346,15 @@ schema-utils@^3.2.0: ajv "^6.12.5" ajv-keywords "^3.5.2" -scimmy-routers@^1.2.0: - version "1.2.0" - resolved "https://registry.yarnpkg.com/scimmy-routers/-/scimmy-routers-1.2.0.tgz#42090c616f127aefce194ebf4a3c8f4a4f62e30b" - integrity sha512-+dT8yRglz2gMu0X1LlUYTi/PDgW6Zzu1YRWiv362I/wA64kud24XjYIoufNiW5OhskvBPQGT/P1aYOffcmxxsQ== +scimmy-routers@1.2.1: + version "1.2.1" + resolved "https://registry.yarnpkg.com/scimmy-routers/-/scimmy-routers-1.2.1.tgz#a535ced83f051b2a0374e8df02da6e17424e8be0" + integrity sha512-PDX4/mwOScSd+TjUPX0k3gH6v50WeYVgK1bisZ8f3p3eyJ0Qy4qYebDo6gzHqYCBjXNQniQxGSQvtlvG22NLdA== dependencies: - express "^4.18.2" - scimmy "^1.2.0" + express "^4.19.2" + scimmy "^1.2.3" -scimmy@^1.2.0: +scimmy@1.2.3, scimmy@^1.2.3: version "1.2.3" resolved "https://registry.yarnpkg.com/scimmy/-/scimmy-1.2.3.tgz#56ca9dbf11749b272e18090c923f81dbad4bc911" integrity sha512-16oXCvnieVeKxTDQqi275bLuyOCwXci8Jywm2/M+4dWNNYoduUz0Crj1nFY0ETYMsuYvCdareWov6/Mebu92xA== From c38107aadcc468c5643956266a67f418b13a7ab5 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Tue, 3 Sep 2024 23:53:25 +0200 Subject: [PATCH 03/15] Implement ingress --- app/gen-server/lib/homedb/HomeDBManager.ts | 4 +++ app/gen-server/lib/homedb/UsersManager.ts | 27 ++++++++++++++- app/server/lib/scim/v2/ScimUserUtils.ts | 25 +++++++++++--- app/server/lib/scim/v2/ScimV2Api.ts | 38 +++++++++++++++++----- 4 files changed, 80 insertions(+), 14 deletions(-) diff --git a/app/gen-server/lib/homedb/HomeDBManager.ts b/app/gen-server/lib/homedb/HomeDBManager.ts index 08f77e2d..b31011e7 100644 --- a/app/gen-server/lib/homedb/HomeDBManager.ts +++ b/app/gen-server/lib/homedb/HomeDBManager.ts @@ -541,6 +541,10 @@ export class HomeDBManager extends EventEmitter { return this._usersManager.deleteUser(scope, userIdToDelete, name); } + public async overrideUser(userId: number, props: UserProfile) { + return this._usersManager.overrideUser(userId, props); + } + /** * Returns a QueryResult for the given organization. The orgKey * can be a string (the domain from url) or the id of an org. If it is diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index 81bde779..6993d72e 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -386,7 +386,7 @@ export class UsersManager { // Set the user's name if our provider knows it. Otherwise use their username // from email, for lack of something better. If we don't have a profile at this // time, then leave the name blank in the hopes of learning it when the user logs in. - user.name = (profile && (profile.name || email.split('@')[0])) || ''; + user.name = (profile && this._getNameOrDeduceFromEmail(profile.name, email)) || ''; needUpdate = true; } if (!user.picture && profile && profile.picture) { @@ -561,6 +561,27 @@ export class UsersManager { .filter(fullProfile => fullProfile); } + /** + * Update users with passed property. Optional user properties that are missing will be reset to their default value. + */ + public async overrideUser(userId: number, props: UserProfile): Promise<User> { + return await this._connection.transaction(async manager => { + const user = await this.getUser(userId, {includePrefs: true}); + if (!user) { throw new ApiError("unable to find user to update", 404); } + const login = user.logins[0]; + user.name = this._getNameOrDeduceFromEmail(props.name, props.email); + user.picture = props.picture || ''; + user.options = {...(user.options || {}), locale: props.locale ?? undefined}; + if (props.email) { + login.email = normalizeEmail(props.email); + login.displayEmail = props.email; + } + await manager.save([user, login]); + + return (await this.getUser(userId))!; + }); + } + /** * ================================== * @@ -748,6 +769,10 @@ export class UsersManager { return id; } + private _getNameOrDeduceFromEmail(name: string, email: string) { + return name || email.split('@')[0]; + } + // This deals with the problem posed by receiving a PermissionDelta specifying a // role for both alice@x and Alice@x. We do not distinguish between such emails. // If there are multiple indistinguishabe emails, we preserve just one of them, diff --git a/app/server/lib/scim/v2/ScimUserUtils.ts b/app/server/lib/scim/v2/ScimUserUtils.ts index b9db4bf0..0ddaffe9 100644 --- a/app/server/lib/scim/v2/ScimUserUtils.ts +++ b/app/server/lib/scim/v2/ScimUserUtils.ts @@ -1,5 +1,7 @@ +import { normalizeEmail } from "app/common/emails"; +import { UserProfile } from "app/common/LoginSessionAPI"; import { User } from "app/gen-server/entity/User.js"; -import { SCIMMY } from "scimmy-routers"; +import SCIMMY from "scimmy"; /** * Converts a user from your database to a SCIMMY user @@ -8,7 +10,7 @@ export function toSCIMMYUser(user: User) { if (!user.logins) { throw new Error("User must have at least one login"); } - const preferredLanguage = user.options?.locale ?? "en"; + const locale = user.options?.locale ?? "en"; return new SCIMMY.Schemas.User({ id: String(user.id), userName: user.loginEmail, @@ -16,16 +18,29 @@ export function toSCIMMYUser(user: User) { name: { formatted: user.name, }, - preferredLanguage, - locale: preferredLanguage, // Assume locale is the same as preferredLanguage + locale, + preferredLanguage: locale, // Assume preferredLanguage is the same as locale photos: user.picture ? [{ value: user.picture, type: "photo", primary: true }] : undefined, emails: [{ - value: user.loginEmail, + value: user.logins[0].displayEmail, primary: true, }], }); } + +export function toUserProfile(scimUser: any, existingUser?: User): UserProfile { + const emailValue = scimUser.emails?.[0]?.value; + if (emailValue && normalizeEmail(emailValue) !== normalizeEmail(scimUser.userName)) { + throw new SCIMMY.Types.Error(400, 'invalidValue', 'Email and userName must be the same'); + } + return { + name: scimUser.displayName ?? existingUser?.name, + picture: scimUser.photos?.[0]?.value, + locale: scimUser.locale, + email: emailValue ?? scimUser.userName ?? existingUser?.loginEmail, + }; +} diff --git a/app/server/lib/scim/v2/ScimV2Api.ts b/app/server/lib/scim/v2/ScimV2Api.ts index 05026251..ebd0c6db 100644 --- a/app/server/lib/scim/v2/ScimV2Api.ts +++ b/app/server/lib/scim/v2/ScimV2Api.ts @@ -4,12 +4,13 @@ import SCIMMY from "scimmy"; import SCIMMYRouters from "scimmy-routers"; import { RequestWithLogin } from '../../Authorizer'; import { InstallAdmin } from '../../InstallAdmin'; -import { toSCIMMYUser } from './ScimUserUtils'; +import { toSCIMMYUser, toUserProfile } from './ScimUserUtils'; +import { ApiError } from 'app/common/ApiError'; const WHITELISTED_PATHS_FOR_NON_ADMINS = [ "/Me", "/Schemas", "/ResourceTypes", "/ServiceProviderConfig" ]; async function isAuthorizedAction(mreq: RequestWithLogin, installAdmin: InstallAdmin): Promise<boolean> { - const isAdmin = await installAdmin.isAdminReq(mreq) + const isAdmin = await installAdmin.isAdminReq(mreq); const isScimUser = Boolean(process.env.GRIST_SCIM_EMAIL && mreq.user?.loginEmail === process.env.GRIST_SCIM_EMAIL); return isAdmin || isScimUser || WHITELISTED_PATHS_FOR_NON_ADMINS.includes(mreq.path); } @@ -30,18 +31,39 @@ 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) => { + ingress: async (resource: any, data: any) => { try { const { id } = resource; if (id) { - return null; + const updatedUser = await dbManager.overrideUser(id, toUserProfile(data)); + return toSCIMMYUser(updatedUser); } - return []; + 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 + }); + return toSCIMMYUser(newUser!); } catch (ex) { - // FIXME: remove this - if (Math.random() > 1) { - return null; + 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); } + + if (ex.code?.startsWith('SQLITE')) { + switch (ex.code) { + case 'SQLITE_CONSTRAINT': + throw new SCIMMY.Types.Error(409, 'uniqueness', ex.message); + default: + throw new SCIMMY.Types.Error(500, 'serverError', ex.message); + } + } + throw ex; } }, From 6f49f83b6645f01a826764e4c55440ab6ad34f87 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Tue, 3 Sep 2024 23:53:37 +0200 Subject: [PATCH 04/15] Add tests --- test/server/lib/Scim.ts | 363 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 363 insertions(+) create mode 100644 test/server/lib/Scim.ts diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts new file mode 100644 index 00000000..cbfc5806 --- /dev/null +++ b/test/server/lib/Scim.ts @@ -0,0 +1,363 @@ +import axios from 'axios'; +import capitalize from 'lodash/capitalize'; +import { assert } from 'chai'; +import { TestServer } from 'test/gen-server/apiUtils'; +import { configForUser } from 'test/gen-server/testUtils'; +import * as testUtils from 'test/server/testUtils'; + +function scimConfigForUser(user: string) { + const config = configForUser(user); + return { + ...config, + headers: { + ...config.headers, + 'Content-Type': 'application/scim+json' + } + }; +} + +const chimpy = scimConfigForUser('Chimpy'); +const kiwi = scimConfigForUser('Kiwi'); +const anon = configForUser('Anonymous'); + +const USER_CONFIG_BY_NAME = { + chimpy, + kiwi, + anon, +}; + +type UserConfigByName = typeof USER_CONFIG_BY_NAME; + +describe('Scim', () => { + let oldEnv: testUtils.EnvironmentSnapshot; + let server: TestServer; + let homeUrl: string; + const userIdByName: {[name in keyof UserConfigByName]?: number} = {}; + + const scimUrl = (path: string) => (homeUrl + '/api/scim/v2' + path); + + before(async function () { + oldEnv = new testUtils.EnvironmentSnapshot(); + process.env.GRIST_DEFAULT_EMAIL = 'chimpy@getgrist.com'; + process.env.TYPEORM_DATABASE = ':memory:'; + server = new TestServer(this); + 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); + } + }); + + after(async () => { + oldEnv.restore(); + await server.stop(); + }); + + function personaToSCIMMYUserWithId(user: keyof UserConfigByName) { + return toSCIMUserWithId(user, userIdByName[user]!); + } + + function toSCIMUserWithId(user: string, id: number) { + return { + ...toSCIMUserWithoutId(user), + id: String(id), + meta: { resourceType: 'User', location: '/api/scim/v2/Users/' + id }, + }; + } + + function toSCIMUserWithoutId(user: string) { + return { + schemas: [ 'urn:ietf:params:scim:schemas:core:2.0:User' ], + userName: user + '@getgrist.com', + name: { formatted: capitalize(user) }, + displayName: capitalize(user), + preferredLanguage: 'en', + locale: 'en', + emails: [ { value: user + '@getgrist.com', primary: true } ] + }; + } + + async function getUserId(user: string) { + return (await server.dbManager.getUserByLogin(user + '@getgrist.com'))!.id; + } + + function checkEndpointNotAccessibleForNonAdminUsers( + method: 'get' | 'post' | 'put' | 'patch' | 'delete', + path: string + ) { + 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]); + } + it('should return 401 for anonymous', async function () { + const res: any = await makeCallWith('anon'); + assert.equal(res.status, 401); + }); + + it('should return 401 for kiwi', async function () { + const res: any = await makeCallWith('kiwi'); + assert.equal(res.status, 401); + }); + } + + describe('/Me', function () { + async function checkGetMeAs(user: keyof UserConfigByName, expected: any) { + const res = await axios.get(scimUrl('/Me'), USER_CONFIG_BY_NAME[user]); + assert.equal(res.status, 200); + assert.deepInclude(res.data, expected); + } + + it(`should return the current user for chimpy`, async function () { + return checkGetMeAs('chimpy', personaToSCIMMYUserWithId('chimpy')); + }); + + it(`should return the current user for kiwi`, async function () { + return checkGetMeAs('kiwi', personaToSCIMMYUserWithId('kiwi')); + }); + + it('should return 401 for anonymous', async function () { + const res = await axios.get(scimUrl('/Me'), anon); + assert.equal(res.status, 401); + }); + }); + + describe('/Users/{id}', function () { + + it('should return the user of id=1 for chimpy', async function () { + const res = await axios.get(scimUrl('/Users/1'), chimpy); + + assert.equal(res.status, 200); + assert.deepInclude(res.data, { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + id: '1', + displayName: 'Chimpy', + userName: 'chimpy@getgrist.com' + }); + }); + + it('should return 404 when the user is not found', async function () { + const res = await axios.get(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 with ID 1000 not found' + }); + }); + + checkEndpointNotAccessibleForNonAdminUsers('get', '/Users/1'); + }); + + describe('GET /Users', function () { + it('should return all users for chimpy', async function () { + const res = await axios.get(scimUrl('/Users'), chimpy); + assert.equal(res.status, 200); + assert.isAbove(res.data.totalResults, 0, 'should have retrieved some users'); + assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('chimpy')); + assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('kiwi')); + }); + + checkEndpointNotAccessibleForNonAdminUsers('get', '/Users'); + }); + + 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'), { + schemas: [SEARCH_SCHEMA], + sortBy: 'userName', + sortOrder: 'descending', + }, 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); + assert.include(users, 'chimpy@getgrist.com'); + assert.include(users, 'kiwi@getgrist.com'); + const indexOfChimpy = users.indexOf('chimpy@getgrist.com'); + const indexOfKiwi = users.indexOf('kiwi@getgrist.com'); + assert.isBelow(indexOfKiwi, indexOfChimpy, 'kiwi should come before chimpy'); + }); + + it('should filter the users by userName', async function () { + const res = await axios.post(scimUrl('/Users/.search'), { + schemas: [SEARCH_SCHEMA], + attributes: ['userName'], + filter: 'userName sw "chimpy"', + }, chimpy); + assert.equal(res.status, 200); + assert.equal(res.data.totalResults, 1); + assert.deepEqual(res.data.Resources[0], { id: String(userIdByName['chimpy']), userName: 'chimpy@getgrist.com' }, + "should have retrieved only chimpy's username and not other attribute"); + }); + + checkEndpointNotAccessibleForNonAdminUsers('post', '/Users/.search'); + }); + + describe('POST /Users', function () { // Create a new users + 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'); + assert.deepEqual(res.data, toSCIMUserWithId('newuser1', newUserId)); + }); + + it('should allow creating a new user given only their email passed as username', async function () { + 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.displayName, 'new.user2'); + }); + + it('should reject when passed email differs from username', async function () { + const res = await axios.post(scimUrl('/Users'), { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + 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' + }); + }); + + 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' + }); + }); + + checkEndpointNotAccessibleForNonAdminUsers('post', '/Users'); + }); + + describe('PUT /Users/{id}', function () { + let userToUpdateId: number; + const userToUpdateEmailLocalPart = 'user-to-update'; + + beforeEach(async function () { + userToUpdateId = await getUserId(userToUpdateEmailLocalPart); + }); + afterEach(async function () { + await server.dbManager.deleteUser({ userId: userToUpdateId }, userToUpdateId); + }); + + it('should update an existing user', async function () { + const userToUpdateProperties = { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + userName: userToUpdateEmailLocalPart + '-now-updated@getgrist.com', + displayName: 'User to Update', + photos: [{ value: 'https://example.com/photo.jpg', type: 'photo', primary: true }], + locale: 'fr', + }; + const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), userToUpdateProperties, chimpy); + assert.equal(res.status, 200); + const refreshedUser = await axios.get(scimUrl(`/Users/${userToUpdateId}`), chimpy); + assert.deepEqual(refreshedUser.data, { + ...userToUpdateProperties, + id: String(userToUpdateId), + meta: { resourceType: 'User', location: `/api/scim/v2/Users/${userToUpdateId}` }, + emails: [ { value: userToUpdateProperties.userName, primary: true } ], + name: { formatted: userToUpdateProperties.displayName }, + preferredLanguage: 'fr', + }); + }); + + it('should reject when passed email differs from username', async function () { + const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + 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' + }); + }); + + it('should disallow updating a user with the same email', 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', + scimType: 'uniqueness' + }); + }); + + it('should return 404 when the user is not found', async function () { + const res = await axios.put(scimUrl('/Users/1000'), toSCIMUserWithoutId('chimpy'), 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' + }); + }); + + it('should deduce the name from the displayEmail when not provided', async function () { + const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + userName: 'my-email@getgrist.com', + }, chimpy); + assert.equal(res.status, 200); + assert.deepInclude(res.data, { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + id: String(userToUpdateId), + userName: 'my-email@getgrist.com', + displayName: 'my-email', + }); + }); + + 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}`), { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + userName: newEmail, + }, chimpy); + assert.equal(res.status, 200); + assert.deepInclude(res.data, { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + id: String(userToUpdateId), + userName: newEmail.toLowerCase(), + displayName: 'my-EMAIL', + emails: [{ value: newEmail, primary: true }] + }); + }); + + checkEndpointNotAccessibleForNonAdminUsers('put', '/Users/1'); + }); + + describe('PATCH /Users/{id}', function () { + it('should update the user of id=1', async function () { + throw new Error("This is a reminder :)"); + }); + 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 :)"); + }); + checkEndpointNotAccessibleForNonAdminUsers('delete', '/Users/1'); + }); + + it('should fix the 401 response for authenticated users', function () { + throw new Error("This is a reminder :)"); + }); +}); 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 05/15] 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'); }); From 9c81ddbba9731a32dd4c1a22e0c323737157f7b7 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Fri, 6 Sep 2024 17:02:14 +0200 Subject: [PATCH 06/15] More tests and cleanups --- app/server/lib/scim/v2/ScimV2Api.ts | 74 +++--- test/server/lib/Scim.ts | 341 +++++++++++++++++++++++----- 2 files changed, 329 insertions(+), 86 deletions(-) diff --git a/app/server/lib/scim/v2/ScimV2Api.ts b/app/server/lib/scim/v2/ScimV2Api.ts index 69aeac3d..2d1add37 100644 --- a/app/server/lib/scim/v2/ScimV2Api.ts +++ b/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> { - const isAdmin = await installAdmin.isAdminReq(mreq); - const isScimUser = Boolean(process.env.GRIST_SCIM_EMAIL && mreq.user?.loginEmail === process.env.GRIST_SCIM_EMAIL); - return isAdmin || isScimUser || WHITELISTED_PATHS_FOR_NON_ADMINS.includes(mreq.path); +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, { - 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)) { - throw new SCIMMY.Types.Error(403, null!, 'Resource disallowed for non-admin users'); - } - return String(mreq.userId); // HACK: SCIMMYRouters requires the userId to be a string. + return String(mreq.userId); // SCIMMYRouters requires the userId to be a string. + }, + context: async (mreq: RequestWithLogin): Promise<RequestContext> => { + 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 diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts index 64e7531f..feb964e6 100644 --- a/test/server/lib/Scim.ts +++ b/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'; + + const searchExample = { + schemas: [SEARCH_SCHEMA], + sortBy: 'userName', + sortOrder: 'descending', + }; + it('should return all users for chimpy order by userName in descending order', async function () { - const res = await axios.post(scimUrl('/Users/.search'), { - schemas: [SEARCH_SCHEMA], - sortBy: 'userName', - sortOrder: 'descending', - }, chimpy); + 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); - assert.equal(res.status, 201); - const newUserId = await getOrCreateUserId('newuser1'); - assert.deepEqual(res.data, toSCIMUserWithId('newuser1', newUserId)); + await withUserName('newuser1', async (userName) => { + const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId(userName), chimpy); + assert.equal(res.status, 201); + const newUserId = await getOrCreateUserId(userName); + assert.deepEqual(res.data, toSCIMUserWithId(userName, newUserId)); + }); }); it('should allow creating a new user given only their email passed as username', async function () { - 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.displayName, 'new.user2'); + 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, userName + '@getgrist.com'); + assert.equal(res.data.displayName, userName); + }); }); it('should reject when passed email differs from username', async function () { - const res = await axios.post(scimUrl('/Users'), { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - 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' + 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', + emails: [{ value: 'emails.value@getgrist.com' }], + }, chimpy); + 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); - assert.equal(res.status, 404); + const res = await axios.put(scimUrl('/Users/1000'), toSCIMUserWithoutId('whoever'), chimpy); 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,20 +405,22 @@ describe('Scim', () => { 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, - }, { + const validPatchBody = (newName: string) => ({ + schemas: ['urn:ietf:params:scim:api:messages:2.0:PatchOp'], + Operations: [{ + op: "replace", + path: "displayName", + value: newName, + }, { op: "replace", 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 () { - throw new Error("This is a reminder :)"); + describe('POST /Bulk', function () { + 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'); }); }); From 4ba322d1cb834219144f1c360134774b765c18f0 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Fri, 6 Sep 2024 19:11:13 +0200 Subject: [PATCH 07/15] Rebase fix + check GRIST_SCIM_USER --- app/server/lib/scim/v2/ScimV2Api.ts | 2 +- test/server/lib/Scim.ts | 22 +++++++++++++++++++--- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/server/lib/scim/v2/ScimV2Api.ts b/app/server/lib/scim/v2/ScimV2Api.ts index 2d1add37..df94f29f 100644 --- a/app/server/lib/scim/v2/ScimV2Api.ts +++ b/app/server/lib/scim/v2/ScimV2Api.ts @@ -64,7 +64,7 @@ const buildScimRouterv2 = (dbManager: HomeDBManager, installAdmin: InstallAdmin) const newUser = await dbManager.getUserByLoginWithRetry(userProfileToInsert.email, { profile: userProfileToInsert }); - return toSCIMMYUser(newUser!); + return toSCIMMYUser(newUser); } catch (ex) { if (ex instanceof ApiError) { if (ex.status === 409) { diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts index feb964e6..124bc6f2 100644 --- a/test/server/lib/Scim.ts +++ b/test/server/lib/Scim.ts @@ -18,6 +18,7 @@ function scimConfigForUser(user: string) { const chimpy = scimConfigForUser('Chimpy'); const kiwi = scimConfigForUser('Kiwi'); +const charon = scimConfigForUser('Charon'); const anon = scimConfigForUser('Anonymous'); const USER_CONFIG_BY_NAME = { @@ -36,9 +37,12 @@ describe('Scim', () => { const scimUrl = (path: string) => (homeUrl + '/api/scim/v2' + path); + testUtils.setTmpLogLevel('error'); + before(async function () { oldEnv = new testUtils.EnvironmentSnapshot(); process.env.GRIST_DEFAULT_EMAIL = 'chimpy@getgrist.com'; + process.env.GRIST_SCIM_EMAIL = 'charon@getgrist.com'; process.env.TYPEORM_DATABASE = ':memory:'; server = new TestServer(this); homeUrl = await server.start(); @@ -216,6 +220,11 @@ describe('Scim', () => { assert.isBelow(indexOfKiwi, indexOfChimpy, 'kiwi should come before chimpy'); }); + it('should also allow access for user Charon (the one refered in GRIST_SCIM_EMAIL)', async function () { + const res = await axios.post(scimUrl('/Users/.search'), searchExample, charon); + assert.equal(res.status, 200); + }); + it('should filter the users by userName', async function () { const res = await axios.post(scimUrl('/Users/.search'), { schemas: [SEARCH_SCHEMA], @@ -263,6 +272,13 @@ describe('Scim', () => { }); }); + it('should also allow user Charon to create a user (the one refered in GRIST_SCIM_EMAIL)', async function () { + await withUserName('new.user.by.charon', async (userName) => { + const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId(userName), charon); + assert.equal(res.status, 201); + }); + }); + it('should reject when passed email differs from username', async function () { await withUserName('username', async (userName) => { const res = await axios.post(scimUrl('/Users'), { @@ -470,8 +486,6 @@ describe('Scim', () => { beforeEach(async function () { usersToCleanupEmails = []; - usersToCleanupEmails.push('bulk-user1@getgrist.com'); - usersToCleanupEmails.push('bulk-user2@getgrist.com'); }); afterEach(async function () { @@ -501,6 +515,8 @@ describe('Scim', () => { ], }, chimpy); assert.equal(res.status, 200); + + const newUserID = await getOrCreateUserId('bulk-user3'); assert.deepEqual(res.data, { schemas: [ "urn:ietf:params:scim:api:messages:2.0:BulkResponse" ], Operations: [ @@ -519,7 +535,7 @@ describe('Scim', () => { }, { method: "POST", bulkId: "1", - location: "/api/scim/v2/Users/26", + location: "/api/scim/v2/Users/" + newUserID, status: "201" }, { method: "POST", From 7c8c2f20579555dad82f95036c4f2cdd3c68c6de Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Fri, 6 Sep 2024 20:31:13 +0200 Subject: [PATCH 08/15] Add GRIST_ENABLE_SCIM env variable --- app/server/lib/FlexServer.ts | 7 +- test/server/lib/Scim.ts | 1139 +++++++++++++++++----------------- 2 files changed, 588 insertions(+), 558 deletions(-) diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 5f641549..94a3715b 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -890,7 +890,12 @@ export class FlexServer implements GristServer { public addScimApi() { if (this._check('scim', 'api', 'homedb', 'json', 'api-mw')) { return; } - this.app.use('/api/scim', buildScimRouter(this._dbManager, this._installAdmin)); + 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); } diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts index 124bc6f2..6fb1a804 100644 --- a/test/server/lib/Scim.ts +++ b/test/server/lib/Scim.ts @@ -30,261 +30,347 @@ const USER_CONFIG_BY_NAME = { type UserConfigByName = typeof USER_CONFIG_BY_NAME; describe('Scim', () => { - let oldEnv: testUtils.EnvironmentSnapshot; - let server: TestServer; - let homeUrl: string; - const userIdByName: {[name in keyof UserConfigByName]?: number} = {}; - - const scimUrl = (path: string) => (homeUrl + '/api/scim/v2' + path); - testUtils.setTmpLogLevel('error'); - before(async function () { - oldEnv = new testUtils.EnvironmentSnapshot(); - process.env.GRIST_DEFAULT_EMAIL = 'chimpy@getgrist.com'; - process.env.GRIST_SCIM_EMAIL = 'charon@getgrist.com'; - process.env.TYPEORM_DATABASE = ':memory:'; - server = new TestServer(this); - homeUrl = await server.start(); - const userNames = Object.keys(USER_CONFIG_BY_NAME) as Array<keyof UserConfigByName>; - for (const user of userNames) { - userIdByName[user] = await getOrCreateUserId(user); - } - }); + const setupTestServer = (env: NodeJS.ProcessEnv) => { + let homeUrl: string; + let oldEnv: testUtils.EnvironmentSnapshot; + let server: TestServer; - after(async () => { - oldEnv.restore(); - await server.stop(); - }); + before(async function () { + oldEnv = new testUtils.EnvironmentSnapshot(); + process.env.TYPEORM_DATABASE = ':memory:'; + Object.assign(process.env, env); + server = new TestServer(this); + homeUrl = await server.start(); + }); - function personaToSCIMMYUserWithId(user: keyof UserConfigByName) { - return toSCIMUserWithId(user, userIdByName[user]!); - } + after(async () => { + oldEnv.restore(); + await server.stop(); + }); - function toSCIMUserWithId(user: string, id: number) { return { - ...toSCIMUserWithoutId(user), - id: String(id), - meta: { resourceType: 'User', location: '/api/scim/v2/Users/' + id }, + scimUrl: (path: string) => (homeUrl + '/api/scim/v2' + path), + getDbManager: () => server.dbManager, }; - } + }; - function toSCIMUserWithoutId(user: string) { - return { - schemas: [ 'urn:ietf:params:scim:schemas:core:2.0:User' ], - userName: user + '@getgrist.com', - name: { formatted: capitalize(user) }, - displayName: capitalize(user), - preferredLanguage: 'en', - locale: 'en', - emails: [ { value: user + '@getgrist.com', primary: true } ] - }; - } + describe('when disabled', function () { + const { scimUrl } = setupTestServer({}); - 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, - 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), validBody, USER_CONFIG_BY_NAME[user]); - } - it('should return 401 for anonymous', async function () { - const res: any = await makeCallWith('anon'); - assert.equal(res.status, 401); - }); - - it('should return 401 for kiwi', async function () { - const res: any = await makeCallWith('kiwi'); - 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); - }); - } - - describe('/Me', function () { - async function checkGetMeAs(user: keyof UserConfigByName, expected: any) { - const res = await axios.get(scimUrl('/Me'), USER_CONFIG_BY_NAME[user]); - assert.equal(res.status, 200); - assert.deepInclude(res.data, expected); - } - - it(`should return the current user for chimpy`, async function () { - return checkGetMeAs('chimpy', personaToSCIMMYUserWithId('chimpy')); - }); - - it(`should return the current user for kiwi`, async function () { - return checkGetMeAs('kiwi', personaToSCIMMYUserWithId('kiwi')); - }); - - it('should return 401 for anonymous', async function () { - 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 () { - - it('should return the user of id=1 for chimpy', async function () { - const res = await axios.get(scimUrl('/Users/1'), chimpy); - - assert.equal(res.status, 200); - assert.deepInclude(res.data, { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - id: '1', - displayName: 'Chimpy', - userName: 'chimpy@getgrist.com' - }); - }); - - it('should return 404 when the user is not found', async function () { - const res = await axios.get(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 with ID 1000 not found' - }); - }); - - checkEndpointNotAccessibleForNonAdminUsers('get', '/Users/1'); - }); - - describe('GET /Users', function () { - it('should return all users for chimpy', async function () { + it('should return 501 for /api/scim/v2/Users', async function () { const res = await axios.get(scimUrl('/Users'), chimpy); - assert.equal(res.status, 200); - assert.isAbove(res.data.totalResults, 0, 'should have retrieved some users'); - assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('chimpy')); - assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('kiwi')); + assert.equal(res.status, 501); + assert.deepEqual(res.data, { error: 'SCIM API is not enabled' }); }); - - checkEndpointNotAccessibleForNonAdminUsers('get', '/Users'); }); - describe('POST /Users/.search', function () { - const SEARCH_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:SearchRequest'; + describe('when enabled using GRIST_ENABLE_SCIM=1', function () { + const { scimUrl, getDbManager } = setupTestServer({ + GRIST_ENABLE_SCIM: '1', + GRIST_DEFAULT_EMAIL: 'chimpy@getgrist.com', + GRIST_SCIM_EMAIL: 'charon@getgrist.com', + }); + const userIdByName: {[name in keyof UserConfigByName]?: number} = {}; - const searchExample = { - schemas: [SEARCH_SCHEMA], - sortBy: 'userName', - sortOrder: 'descending', - }; - - 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); - assert.include(users, 'chimpy@getgrist.com'); - assert.include(users, 'kiwi@getgrist.com'); - const indexOfChimpy = users.indexOf('chimpy@getgrist.com'); - const indexOfKiwi = users.indexOf('kiwi@getgrist.com'); - assert.isBelow(indexOfKiwi, indexOfChimpy, 'kiwi should come before chimpy'); + before(async function () { + const userNames = Object.keys(USER_CONFIG_BY_NAME) as Array<keyof UserConfigByName>; + for (const user of userNames) { + userIdByName[user] = await getOrCreateUserId(user); + } }); - it('should also allow access for user Charon (the one refered in GRIST_SCIM_EMAIL)', async function () { - const res = await axios.post(scimUrl('/Users/.search'), searchExample, charon); - assert.equal(res.status, 200); + function personaToSCIMMYUserWithId(user: keyof UserConfigByName) { + return toSCIMUserWithId(user, userIdByName[user]!); + } + + function toSCIMUserWithId(user: string, id: number) { + return { + ...toSCIMUserWithoutId(user), + id: String(id), + meta: { resourceType: 'User', location: '/api/scim/v2/Users/' + id }, + }; + } + + function toSCIMUserWithoutId(user: string) { + return { + schemas: [ 'urn:ietf:params:scim:schemas:core:2.0:User' ], + userName: user + '@getgrist.com', + name: { formatted: capitalize(user) }, + displayName: capitalize(user), + preferredLanguage: 'en', + locale: 'en', + emails: [ { value: user + '@getgrist.com', primary: true } ] + }; + } + + async function getOrCreateUserId(user: string) { + return (await getDbManager().getUserByLogin(user + '@getgrist.com'))!.id; + } + + async function cleanupUser(userId: number) { + if (await getDbManager().getUser(userId)) { + await getDbManager().deleteUser({ userId: userId }, userId); + } + } + + function checkEndpointNotAccessibleForNonAdminUsers( + method: 'get' | 'post' | 'put' | 'patch' | 'delete', + 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), validBody, USER_CONFIG_BY_NAME[user]); + } + it('should return 401 for anonymous', async function () { + const res: any = await makeCallWith('anon'); + assert.equal(res.status, 401); + }); + + it('should return 403 for kiwi', async function () { + const res: any = await makeCallWith('kiwi'); + 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); + }); + } + + describe('/Me', function () { + async function checkGetMeAs(user: keyof UserConfigByName, expected: any) { + const res = await axios.get(scimUrl('/Me'), USER_CONFIG_BY_NAME[user]); + assert.equal(res.status, 200); + assert.deepInclude(res.data, expected); + } + + it(`should return the current user for chimpy`, async function () { + return checkGetMeAs('chimpy', personaToSCIMMYUserWithId('chimpy')); + }); + + it(`should return the current user for kiwi`, async function () { + return checkGetMeAs('kiwi', personaToSCIMMYUserWithId('kiwi')); + }); + + it('should return 401 for anonymous', async function () { + 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', + }); + }); + }); - it('should filter the users by userName', async function () { - const res = await axios.post(scimUrl('/Users/.search'), { + describe('/Users/{id}', function () { + + it('should return the user of id=1 for chimpy', async function () { + const res = await axios.get(scimUrl('/Users/1'), chimpy); + + assert.equal(res.status, 200); + assert.deepInclude(res.data, { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + id: '1', + displayName: 'Chimpy', + userName: 'chimpy@getgrist.com' + }); + }); + + it('should return 404 when the user is not found', async function () { + const res = await axios.get(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 with ID 1000 not found' + }); + }); + + checkEndpointNotAccessibleForNonAdminUsers('get', '/Users/1'); + }); + + describe('GET /Users', function () { + it('should return all users for chimpy', async function () { + const res = await axios.get(scimUrl('/Users'), chimpy); + assert.equal(res.status, 200); + assert.isAbove(res.data.totalResults, 0, 'should have retrieved some users'); + assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('chimpy')); + assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('kiwi')); + }); + + checkEndpointNotAccessibleForNonAdminUsers('get', '/Users'); + }); + + describe('POST /Users/.search', function () { + const SEARCH_SCHEMA = 'urn:ietf:params:scim:api:messages:2.0:SearchRequest'; + + const searchExample = { schemas: [SEARCH_SCHEMA], - attributes: ['userName'], - filter: 'userName sw "chimpy"', - }, chimpy); - assert.equal(res.status, 200); - assert.equal(res.data.totalResults, 1); - assert.deepEqual(res.data.Resources[0], { id: String(userIdByName['chimpy']), userName: 'chimpy@getgrist.com' }, - "should have retrieved only chimpy's username and not other attribute"); + sortBy: 'userName', + sortOrder: 'descending', + }; + + 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); + assert.include(users, 'chimpy@getgrist.com'); + assert.include(users, 'kiwi@getgrist.com'); + const indexOfChimpy = users.indexOf('chimpy@getgrist.com'); + const indexOfKiwi = users.indexOf('kiwi@getgrist.com'); + assert.isBelow(indexOfKiwi, indexOfChimpy, 'kiwi should come before chimpy'); + }); + + it('should also allow access for user Charon (the one refered in GRIST_SCIM_EMAIL)', async function () { + const res = await axios.post(scimUrl('/Users/.search'), searchExample, charon); + assert.equal(res.status, 200); + }); + + it('should filter the users by userName', async function () { + const res = await axios.post(scimUrl('/Users/.search'), { + schemas: [SEARCH_SCHEMA], + attributes: ['userName'], + filter: 'userName sw "chimpy"', + }, chimpy); + assert.equal(res.status, 200); + assert.equal(res.data.totalResults, 1); + assert.deepEqual(res.data.Resources[0], { id: String(userIdByName['chimpy']), userName: 'chimpy@getgrist.com' }, + "should have retrieved only chimpy's username and not other attribute"); + }); + + checkEndpointNotAccessibleForNonAdminUsers('post', '/Users/.search', searchExample); }); - 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); + 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 getDbManager().getExistingUserByLogin(userName + "@getgrist.com"); + if (user) { + await cleanupUser(user.id); + } } } - } - it('should create a new user', async function () { - await withUserName('newuser1', async (userName) => { - const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId(userName), chimpy); - assert.equal(res.status, 201); - const newUserId = await getOrCreateUserId(userName); - assert.deepEqual(res.data, toSCIMUserWithId(userName, newUserId)); + it('should create a new user', async function () { + await withUserName('newuser1', async (userName) => { + const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId(userName), chimpy); + assert.equal(res.status, 201); + const newUserId = await getOrCreateUserId(userName); + 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, userName + '@getgrist.com'); + assert.equal(res.data.displayName, userName); + }); + }); + + it('should also allow user Charon to create a user (the one refered in GRIST_SCIM_EMAIL)', async function () { + await withUserName('new.user.by.charon', async (userName) => { + const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId(userName), charon); + assert.equal(res.status, 201); + }); + }); + + 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', + emails: [{ value: 'emails.value@getgrist.com' }], + }, chimpy); + 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.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', toSCIMUserWithoutId('some-user')); }); - 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'), { + describe('PUT /Users/{id}', function () { + let userToUpdateId: number; + const userToUpdateEmailLocalPart = 'user-to-update'; + + beforeEach(async function () { + userToUpdateId = await getOrCreateUserId(userToUpdateEmailLocalPart); + }); + afterEach(async function () { + await cleanupUser(userToUpdateId); + }); + + it('should update an existing user', async function () { + const userToUpdateProperties = { 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, userName + '@getgrist.com'); - assert.equal(res.data.displayName, userName); + userName: userToUpdateEmailLocalPart + '-now-updated@getgrist.com', + displayName: 'User to Update', + photos: [{ value: 'https://example.com/photo.jpg', type: 'photo', primary: true }], + locale: 'fr', + }; + const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), userToUpdateProperties, chimpy); + assert.equal(res.status, 200); + const refreshedUser = await axios.get(scimUrl(`/Users/${userToUpdateId}`), chimpy); + assert.deepEqual(refreshedUser.data, { + ...userToUpdateProperties, + id: String(userToUpdateId), + meta: { resourceType: 'User', location: `/api/scim/v2/Users/${userToUpdateId}` }, + emails: [ { value: userToUpdateProperties.userName, primary: true } ], + name: { formatted: userToUpdateProperties.displayName }, + preferredLanguage: 'fr', + }); }); - }); - it('should also allow user Charon to create a user (the one refered in GRIST_SCIM_EMAIL)', async function () { - await withUserName('new.user.by.charon', async (userName) => { - const res = await axios.post(scimUrl('/Users'), toSCIMUserWithoutId(userName), charon); - assert.equal(res.status, 201); - }); - }); - - it('should reject when passed email differs from username', async function () { - await withUserName('username', async (userName) => { - const res = await axios.post(scimUrl('/Users'), { + it('should reject when passed email differs from username', async function () { + const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), { schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - userName: userName + '@getgrist.com', - emails: [{ value: 'emails.value@getgrist.com' }], + userName: userToUpdateEmailLocalPart + '@getgrist.com', + emails: [{ value: 'whatever@getgrist.com', primary: true }], }, chimpy); assert.deepEqual(res.data, { schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ], @@ -294,377 +380,316 @@ describe('Scim', () => { }); 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.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' + 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.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); }); - assert.equal(res.status, 409); - }); - checkEndpointNotAccessibleForNonAdminUsers('post', '/Users', toSCIMUserWithoutId('some-user')); - }); - - describe('PUT /Users/{id}', function () { - let userToUpdateId: number; - const userToUpdateEmailLocalPart = 'user-to-update'; - - beforeEach(async function () { - userToUpdateId = await getOrCreateUserId(userToUpdateEmailLocalPart); - }); - afterEach(async function () { - await cleanupUser(userToUpdateId); - }); - - it('should update an existing user', async function () { - const userToUpdateProperties = { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - userName: userToUpdateEmailLocalPart + '-now-updated@getgrist.com', - displayName: 'User to Update', - photos: [{ value: 'https://example.com/photo.jpg', type: 'photo', primary: true }], - locale: 'fr', - }; - const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), userToUpdateProperties, chimpy); - assert.equal(res.status, 200); - const refreshedUser = await axios.get(scimUrl(`/Users/${userToUpdateId}`), chimpy); - assert.deepEqual(refreshedUser.data, { - ...userToUpdateProperties, - id: String(userToUpdateId), - meta: { resourceType: 'User', location: `/api/scim/v2/Users/${userToUpdateId}` }, - emails: [ { value: userToUpdateProperties.userName, primary: true } ], - name: { formatted: userToUpdateProperties.displayName }, - preferredLanguage: 'fr', + it('should return 404 when the user is not found', async function () { + const res = await axios.put(scimUrl('/Users/1000'), toSCIMUserWithoutId('whoever'), chimpy); + 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 reject when passed email differs from username', async function () { - const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - userName: userToUpdateEmailLocalPart + '@getgrist.com', - emails: [{ value: 'whatever@getgrist.com', primary: true }], - }, chimpy); - 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' + it('should deduce the name from the displayEmail when not provided', async function () { + const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + userName: 'my-email@getgrist.com', + }, chimpy); + assert.equal(res.status, 200); + assert.deepInclude(res.data, { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + id: String(userToUpdateId), + userName: 'my-email@getgrist.com', + displayName: 'my-email', + }); }); - assert.equal(res.status, 400); - }); - 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.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' + 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}`), { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + userName: newEmail, + }, chimpy); + assert.equal(res.status, 200); + assert.deepInclude(res.data, { + schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], + id: String(userToUpdateId), + userName: newEmail.toLowerCase(), + displayName: 'my-EMAIL', + emails: [{ value: newEmail, primary: true }] + }); }); - assert.equal(res.status, 409); + + checkEndpointNotAccessibleForNonAdminUsers('put', '/Users/1', toSCIMUserWithoutId('chimpy')); }); - it('should return 404 when the user is not found', async function () { - const res = await axios.put(scimUrl('/Users/1000'), toSCIMUserWithoutId('whoever'), chimpy); - assert.deepEqual(res.data, { - schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ], - status: '404', - detail: 'unable to find user to update' + describe('PATCH /Users/{id}', function () { + let userToPatchId: number; + const userToPatchEmailLocalPart = 'user-to-patch'; + beforeEach(async function () { + userToPatchId = await getOrCreateUserId(userToPatchEmailLocalPart); }); - assert.equal(res.status, 404); - }); - - it('should deduce the name from the displayEmail when not provided', async function () { - const res = await axios.put(scimUrl(`/Users/${userToUpdateId}`), { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - userName: 'my-email@getgrist.com', - }, chimpy); - assert.equal(res.status, 200); - assert.deepInclude(res.data, { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - id: String(userToUpdateId), - userName: 'my-email@getgrist.com', - displayName: 'my-email', + afterEach(async function () { + await cleanupUser(userToPatchId); }); - }); - 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}`), { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - userName: newEmail, - }, chimpy); - assert.equal(res.status, 200); - assert.deepInclude(res.data, { - schemas: ['urn:ietf:params:scim:schemas:core:2.0:User'], - id: String(userToUpdateId), - userName: newEmail.toLowerCase(), - displayName: 'my-EMAIL', - emails: [{ value: newEmail, primary: true }] - }); - }); - - checkEndpointNotAccessibleForNonAdminUsers('put', '/Users/1', toSCIMUserWithoutId('chimpy')); - }); - - describe('PATCH /Users/{id}', function () { - let userToPatchId: number; - const userToPatchEmailLocalPart = 'user-to-patch'; - beforeEach(async function () { - userToPatchId = await getOrCreateUserId(userToPatchEmailLocalPart); - }); - afterEach(async function () { - await cleanupUser(userToPatchId); - }); - - const validPatchBody = (newName: string) => ({ - schemas: ['urn:ietf:params:scim:api:messages:2.0:PatchOp'], - Operations: [{ - op: "replace", - path: "displayName", - value: newName, - }, { + const validPatchBody = (newName: string) => ({ + schemas: ['urn:ietf:params:scim:api:messages:2.0:PatchOp'], + Operations: [{ op: "replace", - path: "locale", - value: 'fr' - }], - }); - - 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, { - ...toSCIMUserWithId(userToPatchEmailLocalPart, userToPatchId), - displayName: newName, - name: { formatted: newName }, - locale: 'fr', - preferredLanguage: 'fr', + path: "displayName", + value: newName, + }, { + op: "replace", + path: "locale", + value: 'fr' + }], }); - }); - checkEndpointNotAccessibleForNonAdminUsers('patch', '/Users/1', validPatchBody('new name2')); - }); - - describe('DELETE /Users/{id}', function () { - 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.deepEqual(res.data, { - schemas: [ 'urn:ietf:params:scim:api:messages:2.0:Error' ], - status: '404', - detail: 'user not found' + 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, { + ...toSCIMUserWithId(userToPatchEmailLocalPart, userToPatchId), + displayName: newName, + name: { formatted: newName }, + locale: 'fr', + preferredLanguage: 'fr', + }); }); - assert.equal(res.status, 404); - }); - checkEndpointNotAccessibleForNonAdminUsers('delete', '/Users/1'); - }); - describe('POST /Bulk', function () { - let usersToCleanupEmails: string[]; - - beforeEach(async function () { - usersToCleanupEmails = []; + checkEndpointNotAccessibleForNonAdminUsers('patch', '/Users/1', validPatchBody('new name2')); }); - afterEach(async function () { - for (const email of usersToCleanupEmails) { - const user = await server.dbManager.getExistingUserByLogin(email); - if (user) { - await cleanupUser(user.id); + describe('DELETE /Users/{id}', function () { + 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.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'); + }); + + describe('POST /Bulk', function () { + let usersToCleanupEmails: string[]; + + beforeEach(async function () { + usersToCleanupEmails = []; + }); + + afterEach(async function () { + for (const email of usersToCleanupEmails) { + const user = await getDbManager().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); + 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); - const newUserID = await getOrCreateUserId('bulk-user3'); - 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" - ], + const newUserID = await getOrCreateUserId('bulk-user3'); + 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", - scimType: "invalidSyntax", - detail: "Expected 'data' to be a single complex value in BulkRequest operation #1" - } - }, { - method: "POST", - bulkId: "1", - location: "/api/scim/v2/Users/" + newUserID, - status: "201" - }, { - method: "POST", - bulkId: "2", - status: "409", - response: { - schemas: [ - "urn:ietf:params:scim:api:messages:2.0:Error" - ], + 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/" + newUserID, + status: "201" + }, { + method: "POST", + bulkId: "2", status: "409", - scimType: "uniqueness", - detail: "An existing user with the passed email exist." + 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 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" - ], + 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", - detail: "Invalid 'path' value '/Me' in BulkRequest operation #2", - scimType: "invalidValue" + 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 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 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'], + 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', + }); }); - 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'], + 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'); }); - 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'); }); }); From 8162028fffd3582c8977ebc879d5c7161ae79bca Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Fri, 6 Sep 2024 22:10:32 +0200 Subject: [PATCH 09/15] Move logic for Users to its own controller --- app/server/lib/FlexServer.ts | 2 + app/server/lib/scim/v2/ScimTypes.ts | 6 + app/server/lib/scim/v2/ScimUserController.ts | 121 +++++++++++++++++++ app/server/lib/scim/v2/ScimV2Api.ts | 99 ++------------- test/server/lib/Scim.ts | 11 ++ 5 files changed, 152 insertions(+), 87 deletions(-) create mode 100644 app/server/lib/scim/v2/ScimTypes.ts create mode 100644 app/server/lib/scim/v2/ScimUserController.ts diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index 94a3715b..48bd61fb 100644 --- a/app/server/lib/FlexServer.ts +++ b/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); } diff --git a/app/server/lib/scim/v2/ScimTypes.ts b/app/server/lib/scim/v2/ScimTypes.ts new file mode 100644 index 00000000..d70aaa0d --- /dev/null +++ b/app/server/lib/scim/v2/ScimTypes.ts @@ -0,0 +1,6 @@ +export interface RequestContext { + path: string; + isAdmin: boolean; + isScimUser: boolean; +} + diff --git a/app/server/lib/scim/v2/ScimUserController.ts b/app/server/lib/scim/v2/ScimUserController.ts new file mode 100644 index 00000000..2106d984 --- /dev/null +++ b/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); + } + }; +}; diff --git a/app/server/lib/scim/v2/ScimV2Api.ts b/app/server/lib/scim/v2/ScimV2Api.ts index df94f29f..00d896a5 100644 --- a/app/server/lib/scim/v2/ScimV2Api.ts +++ b/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 { InstallAdmin } from '../../InstallAdmin'; -import { toSCIMMYUser, toUserProfile } from './ScimUserUtils'; -import { ApiError } from 'app/common/ApiError'; -import { parseInt } from 'lodash'; +import { RequestWithLogin } from 'app/server/lib/Authorizer'; +import { InstallAdmin } from 'app/server/lib/InstallAdmin'; +import { RequestContext } from './ScimTypes'; +import { getScimUserConfig } from './ScimUserController'; 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, { - egress: async (resource: any, context: RequestContext) => { - checkAccess(context); - - 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); - } + 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'); } - }); + } + + SCIMMY.Resources.declare(SCIMMY.Resources.User, getScimUserConfig(dbManager, checkAccess)); const scimmyRouter = new SCIMMYRouters({ type: 'bearer', diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts index 6fb1a804..934e53af 100644 --- a/test/server/lib/Scim.ts +++ b/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}`), { From 103b995c3261cfbb9f89baee1e94b8cfc5a81135 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Sat, 7 Sep 2024 11:00:10 +0200 Subject: [PATCH 10/15] 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<T>(context: RequestContext, cb: () => Promise<T>): Promise<T> { + 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 () { From 590a5b40fb154219e99be6b3f7c74a214462475b Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Sat, 7 Sep 2024 11:18:02 +0200 Subject: [PATCH 11/15] Add a test for pagination --- test/server/lib/Scim.ts | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/server/lib/Scim.ts b/test/server/lib/Scim.ts index e6ff2abc..3a9c3966 100644 --- a/test/server/lib/Scim.ts +++ b/test/server/lib/Scim.ts @@ -128,6 +128,7 @@ describe('Scim', () => { } return axios[method](scimUrl(path), validBody, USER_CONFIG_BY_NAME[user]); } + it('should return 401 for anonymous', async function () { const res = await makeCallWith('anon'); assert.equal(res.status, 401); @@ -245,6 +246,25 @@ describe('Scim', () => { assert.deepInclude(res.data.Resources, personaToSCIMMYUserWithId('kiwi')); }); + it('should handle pagination', async function () { + const endpointPaginated = '/Users?count=1&sortBy=id'; + { + const firstPage = await axios.get(scimUrl(endpointPaginated), chimpy); + assert.equal(firstPage.status, 200); + assert.lengthOf(firstPage.data.Resources, 1); + const firstPageResourceId = parseInt(firstPage.data.Resources[0].id); + assert.equal(firstPageResourceId, 1); + } + + { + const secondPage = await axios.get(scimUrl(endpointPaginated + '&startIndex=2'), chimpy); + assert.equal(secondPage.status, 200); + assert.lengthOf(secondPage.data.Resources, 1); + const secondPageResourceId = parseInt(secondPage.data.Resources[0].id); + assert.equal(secondPageResourceId, 2); + } + }); + checkCommonErrors('get', '/Users'); }); From 1d90cea869f07f73f27eab8eec1956f08c5533fe Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Sat, 7 Sep 2024 11:59:12 +0200 Subject: [PATCH 12/15] Add tests for the new UsersManager methods --- app/gen-server/lib/homedb/UsersManager.ts | 9 ++-- test/gen-server/lib/homedb/UsersManager.ts | 63 ++++++++++++++++++++++ 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/app/gen-server/lib/homedb/UsersManager.ts b/app/gen-server/lib/homedb/UsersManager.ts index 6993d72e..f2800bf9 100644 --- a/app/gen-server/lib/homedb/UsersManager.ts +++ b/app/gen-server/lib/homedb/UsersManager.ts @@ -582,6 +582,11 @@ export class UsersManager { }); } + public async getUsers() { + return await User.find({relations: ["logins"]}); + } + + /** * ================================== * @@ -678,10 +683,6 @@ export class UsersManager { return [this.getSupportUserId(), this.getAnonymousUserId(), this.getEveryoneUserId()]; } - public async getUsers() { - return await User.find({relations: ["logins"]}); - } - /** * Returns a Promise for an array of User entites for the given userIds. */ diff --git a/test/gen-server/lib/homedb/UsersManager.ts b/test/gen-server/lib/homedb/UsersManager.ts index d2306ead..b0af8f26 100644 --- a/test/gen-server/lib/homedb/UsersManager.ts +++ b/test/gen-server/lib/homedb/UsersManager.ts @@ -20,6 +20,7 @@ import { assert } from 'chai'; import Sinon, { SinonSandbox, SinonSpy } from 'sinon'; import { EntityManager } from 'typeorm'; import winston from 'winston'; +import omit from 'lodash/omit'; import {delay} from 'app/common/delay'; @@ -948,6 +949,68 @@ describe('UsersManager', function () { } }); }); + + describe('overrideUser()', function () { + it('should reject when user is not found', async function () { + disableLoggingLevel('debug'); + + const promise = db.overrideUser(NON_EXISTING_USER_ID, { + email: 'whatever@getgrist.com', + name: 'whatever', + }); + + await assert.isRejected(promise, 'unable to find user to update'); + }); + + it('should update user information', async function () { + const localPart = 'overrideuser-updates-user-info'; + const newLocalPart = 'overrideuser-updates-user-info-new'; + const user = await createUniqueUser(localPart); + const newInfo: UserProfile = { + name: 'new name', + email: makeEmail(newLocalPart).toUpperCase(), + picture: 'https://mypic.com/me.png', + locale: 'fr-FR', + }; + + await db.overrideUser(user.id, newInfo); + + const updatedUser = await getOrCreateUser(newLocalPart); + assert.deepInclude(updatedUser, { + id: user.id, + name: newInfo.name, + picture: newInfo.picture, + options: {locale: newInfo.locale}, + }); + assert.deepInclude(updatedUser.logins[0], { + email: newInfo.email.toLowerCase(), + displayEmail: newInfo.email, + }); + }); + }); + + describe('getUsers()', function () { + it('should return all users with their logins', async function () { + const localPart = 'getUsers-user'; + const existingUser = await createUniqueUser(localPart); + const users = await db.getUsers(); + assert.isAbove(users.length, 2); + const mapUsersById = new Map(users.map(user => [user.id, user])); + + // Check that we retrieve the existing user in the result with all their property + // except the personalOrg + const existingUserInResult = mapUsersById.get(existingUser.id); + assertExists(existingUserInResult); + assertExists(existingUserInResult.logins); + assert.lengthOf(existingUserInResult.logins, 1); + assert.deepEqual(existingUserInResult, omit(existingUser, 'personalOrg')); + + // Check that we retrieve special accounts among the result + assert.exists(mapUsersById.get(db.getSupportUserId())); + assert.exists(mapUsersById.get(db.getEveryoneUserId())); + assert.exists(mapUsersById.get(db.getAnonymousUserId())); + }); + }); }); describe('class method without db setup', function () { From ef86aa1cd3868b725a3a06f9a5cc13063d7d1215 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Sat, 7 Sep 2024 15:02:57 +0200 Subject: [PATCH 13/15] Document methods of the controller --- app/server/lib/scim/v2/ScimUserController.ts | 54 ++++++++++++++++++-- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/app/server/lib/scim/v2/ScimUserController.ts b/app/server/lib/scim/v2/ScimUserController.ts index fc0f5522..fe1ef02f 100644 --- a/app/server/lib/scim/v2/ScimUserController.ts +++ b/app/server/lib/scim/v2/ScimUserController.ts @@ -18,6 +18,12 @@ class ScimUserController { private _checkAccess: (context: RequestContext) => void ) {} + /** + * Gets a single user with the passed ID. + * + * @param resource The SCIMMY user resource performing the operation + * @param context The request context + */ public async getSingleUser(resource: any, context: RequestContext) { return this._runAndHandleErrors(context, async () => { const id = ScimUserController._getIdFromResource(resource); @@ -29,6 +35,12 @@ class ScimUserController { }); } + /** + * Gets all users or filters them based on the passed filter. + * + * @param resource The SCIMMY user resource performing the operation + * @param context The request context + */ public async getUsers(resource: any, context: RequestContext) { return this._runAndHandleErrors(context, async () => { const { filter } = resource; @@ -37,9 +49,15 @@ class ScimUserController { }); } + /** + * Creates a new user with the passed data. + * + * @param data The data to create the user with + * @param context The request context + */ public async createUser(data: any, context: RequestContext) { return this._runAndHandleErrors(context, async () => { - await this._checkEmailIsUnique(data.userName); + await this._checkEmailCanBeUsed(data.userName); const userProfile = toUserProfile(data); const newUser = await this._dbManager.getUserByLoginWithRetry(userProfile.email, { profile: userProfile @@ -48,15 +66,28 @@ class ScimUserController { }); } + /** + * Overrides a user with the passed data. + * + * @param resource The SCIMMY user resource performing the operation + * @param data The data to override the user with + * @param context The request context + */ public async overrideUser(resource: any, data: any, context: RequestContext) { return this._runAndHandleErrors(context, async () => { const id = ScimUserController._getIdFromResource(resource); - await this._checkEmailIsUnique(data.userName, id); + await this._checkEmailCanBeUsed(data.userName, id); const updatedUser = await this._dbManager.overrideUser(id, toUserProfile(data)); return toSCIMMYUser(updatedUser); }); } + /** + * Deletes a user with the passed ID. + * + * @param resource The SCIMMY user resource performing the operation + * @param context The request context + */ public async deleteUser(resource: any, context: RequestContext) { return this._runAndHandleErrors(context, async () => { const id = ScimUserController._getIdFromResource(resource); @@ -66,6 +97,14 @@ class ScimUserController { }); } + /** + * Runs the passed callback and handles any errors that might occur. + * Also checks if the user has access to the operation. + * Any public method of this class should be run through this method. + * + * @param context The request context to check access for the user + * @param cb The callback to run + */ private async _runAndHandleErrors<T>(context: RequestContext, cb: () => Promise<T>): Promise<T> { try { this._checkAccess(context); @@ -85,9 +124,16 @@ class ScimUserController { } } - private async _checkEmailIsUnique(email: string, id?: number) { + /** + * Checks if the passed email can be used for a new user or by the existing user. + * + * @param email The email to check + * @param userIdToUpdate The ID of the user to update. Pass this when updating a user, + * so it won't raise an error if the passed email is already used by this user. + */ + private async _checkEmailCanBeUsed(email: string, userIdToUpdate?: number) { const existingUser = await this._dbManager.getExistingUserByLogin(email); - if (existingUser !== undefined && existingUser.id !== id) { + if (existingUser !== undefined && existingUser.id !== userIdToUpdate) { throw new SCIMMY.Types.Error(409, 'uniqueness', 'An existing user with the passed email exist.'); } } From acb5539290fc54b43f3c14fc95e25f55465d7e92 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Thu, 12 Sep 2024 16:00:05 +0200 Subject: [PATCH 14/15] =?UTF-8?q?Rename=20ex=20=E2=86=92=20err=20in=20catc?= =?UTF-8?q?h=20block?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/server/lib/scim/v2/ScimUserController.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/server/lib/scim/v2/ScimUserController.ts b/app/server/lib/scim/v2/ScimUserController.ts index fe1ef02f..4aec86d6 100644 --- a/app/server/lib/scim/v2/ScimUserController.ts +++ b/app/server/lib/scim/v2/ScimUserController.ts @@ -109,18 +109,18 @@ class ScimUserController { 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); + } catch (err) { + if (err instanceof ApiError) { + if (err.status === 409) { + throw new SCIMMY.Types.Error(err.status, 'uniqueness', err.message); } - throw new SCIMMY.Types.Error(ex.status, null!, ex.message); + throw new SCIMMY.Types.Error(err.status, null!, err.message); } - if (ex instanceof SCIMMY.Types.Error) { - throw ex; + if (err instanceof SCIMMY.Types.Error) { + throw err; } // By default, return a 500 error - throw new SCIMMY.Types.Error(500, null!, ex.message); + throw new SCIMMY.Types.Error(500, null!, err.message); } } From 153fa5cdb8be9b24c43ceaa97e1b500efec05af6 Mon Sep 17 00:00:00 2001 From: fflorent <florent.git@zeteo.me> Date: Fri, 18 Oct 2024 14:42:19 +0200 Subject: [PATCH 15/15] Bump Scimmy and Scimmy-Routers --- package.json | 4 ++-- yarn.lock | 21 +++++++++------------ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/package.json b/package.json index 78aabe41..34e43840 100644 --- a/package.json +++ b/package.json @@ -189,8 +189,8 @@ "redis": "3.1.1", "redlock": "3.1.2", "saml2-js": "4.0.2", - "scimmy": "1.2.3", - "scimmy-routers": "1.2.1", + "scimmy": "1.2.4", + "scimmy-routers": "1.2.2", "short-uuid": "3.1.1", "slugify": "1.6.6", "swagger-ui-dist": "5.11.0", diff --git a/yarn.lock b/yarn.lock index 4a8d7a96..fc6fe3aa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3801,7 +3801,7 @@ express-rate-limit@7.2.0: resolved "https://registry.yarnpkg.com/express-rate-limit/-/express-rate-limit-7.2.0.tgz#06ce387dd5388f429cab8263c514fc07bf90a445" integrity sha512-T7nul1t4TNyfZMJ7pKRKkdeVJWa2CqB8NA1P8BwYaoDI5QSBZARv5oMS43J7b7I5P+4asjVXjb7ONuwDKucahg== -express@4.19.2, express@^4.19.2: +express@4.19.2: version "4.19.2" resolved "https://registry.yarnpkg.com/express/-/express-4.19.2.tgz#e25437827a3aa7f2a827bc8171bbbb664a356465" integrity sha512-5T6nhjsT+EOMzuck8JjBHARTHfMht0POzlA60WV2pMD3gyXw2LZnZ+ueGdNxG+0calOJcWKbpFcuzLZ91YWq9Q== @@ -7346,18 +7346,15 @@ schema-utils@^3.2.0: ajv "^6.12.5" ajv-keywords "^3.5.2" -scimmy-routers@1.2.1: - version "1.2.1" - resolved "https://registry.yarnpkg.com/scimmy-routers/-/scimmy-routers-1.2.1.tgz#a535ced83f051b2a0374e8df02da6e17424e8be0" - integrity sha512-PDX4/mwOScSd+TjUPX0k3gH6v50WeYVgK1bisZ8f3p3eyJ0Qy4qYebDo6gzHqYCBjXNQniQxGSQvtlvG22NLdA== - dependencies: - express "^4.19.2" - scimmy "^1.2.3" +scimmy-routers@1.2.2: + version "1.2.2" + resolved "https://registry.yarnpkg.com/scimmy-routers/-/scimmy-routers-1.2.2.tgz#e1fa506a8cdb0ba04a25e09a365bd726cd781585" + integrity sha512-qDB7DKb2cnujJzEgVdON8EnjZfs6oY+MJQkkCHbihNrQeRjSaEOAC9ohb6dGfMZdahYS0CZIJwGhvZlS6rkKsg== -scimmy@1.2.3, scimmy@^1.2.3: - version "1.2.3" - resolved "https://registry.yarnpkg.com/scimmy/-/scimmy-1.2.3.tgz#56ca9dbf11749b272e18090c923f81dbad4bc911" - integrity sha512-16oXCvnieVeKxTDQqi275bLuyOCwXci8Jywm2/M+4dWNNYoduUz0Crj1nFY0ETYMsuYvCdareWov6/Mebu92xA== +scimmy@1.2.4: + version "1.2.4" + resolved "https://registry.yarnpkg.com/scimmy/-/scimmy-1.2.4.tgz#3d708d9a5f3c7b3e00d848dcb8f0910d7c409509" + integrity sha512-5i+LwGL7ON61jH+KxL6flpy5h/ABhgx7tc9AdL3KMh9TfHidWl7KHrbD0cJN5bJ5Fb1nOTze8d+PbFl2bZYEJQ== selenium-webdriver@^4.20.0: version "4.20.0"