From 47c4261f06393e7edf3fe51f59f7037cabc999cb Mon Sep 17 00:00:00 2001 From: Thomas Taylor Date: Fri, 27 Mar 2026 16:27:07 +0000 Subject: [PATCH 1/5] New: Merge usergroups functionality into users module (fixes #44) --- lib/UsersModule.js | 116 ++++++++++++- package.json | 1 + schema/usergroup.schema.json | 13 ++ schema/usergroups.schema.json | 21 +++ tests/usergroups.spec.js | 307 ++++++++++++++++++++++++++++++++++ 5 files changed, 456 insertions(+), 2 deletions(-) create mode 100644 schema/usergroup.schema.json create mode 100644 schema/usergroups.schema.json create mode 100644 tests/usergroups.spec.js diff --git a/lib/UsersModule.js b/lib/UsersModule.js index ef658f0..2003f2e 100644 --- a/lib/UsersModule.js +++ b/lib/UsersModule.js @@ -1,6 +1,6 @@ import AbstractApiModule from 'adapt-authoring-api' /** - * Module which handles user management + * Module which handles user management and user groups * @memberof users * @extends {AbstractApiModule} */ @@ -10,6 +10,11 @@ class UsersModule extends AbstractApiModule { await super.setValues() /** @ignore */ this.schemaName = 'user' /** @ignore */ this.collectionName = 'users' + /** + * Modules registered for user group support + * @type {Array} + */ + this.usergroupModules = [] } /** @@ -18,7 +23,7 @@ class UsersModule extends AbstractApiModule { */ async init () { await super.init() - const [mongodb, server] = await this.app.waitForModule('mongodb', 'server') + const [mongodb, server, auth] = await this.app.waitForModule('mongodb', 'server', 'auth') await mongodb.setIndex(this.collectionName, 'email', { unique: true }) server.api.addHandlerMiddleware(this.updateAccess.bind(this)) @@ -29,6 +34,87 @@ class UsersModule extends AbstractApiModule { this.preInsertHook.tap(this.forceLowerCaseEmail) this.preUpdateHook.tap((ogDoc, updateData) => this.forceLowerCaseEmail(updateData)) } + + this.initUsergroupRoutes(server, auth) + await this.registerUsergroupModule(this) + } + + /** + * Sets up the /api/usergroups child router and routes + * @param {Object} server The server module + * @param {Object} auth The auth module + */ + initUsergroupRoutes (server, auth) { + this.usergroupsRouter = server.api.createChildRouter('usergroups') + this.usergroupsRouter.addHandlerMiddleware( + this.usergroupsProcessRequest.bind(this), + this.sanitiseRequestDataMiddleware.bind(this) + ) + const routes = [ + { + route: '/', + handlers: { get: this.queryHandler.bind(this), post: this.requestHandler.bind(this) }, + permissions: { get: ['read:usergroups'], post: ['write:usergroups'] } + }, + { + route: '/:_id', + handlers: { + get: this.requestHandler.bind(this), + put: this.requestHandler.bind(this), + patch: this.requestHandler.bind(this), + delete: this.requestHandler.bind(this) + }, + permissions: { + get: ['read:usergroups'], + put: ['write:usergroups'], + patch: ['write:usergroups'], + delete: ['write:usergroups'] + } + } + ] + routes.forEach(r => { + Object.entries(r.permissions).forEach(([method, perms]) => { + const route = r.route.endsWith('/') ? r.route.slice(0, -1) : r.route + auth.secureRoute(this.usergroupsRouter.path + route, method, perms) + }) + this.usergroupsRouter.addRoute(r) + }) + } + + /** + * Express middleware to prepare req.apiData for usergroup requests + * @param {external:ExpressRequest} req + * @param {external:ExpressResponse} res + * @param {Function} next + */ + usergroupsProcessRequest (req, res, next) { + const config = req.routeConfig || {} + const modifiers = config.modifiers ?? ['post', 'put', 'patch', 'delete'] + const modifying = config.modifying ?? modifiers.includes(req.method.toLowerCase()) + req.apiData = { + collectionName: 'usergroups', + config, + data: { ...req.body }, + modifying, + query: { ...req.query, ...req.params }, + schemaName: 'usergroup' + } + next() + } + + /** + * Registers a module for use with user groups. Extends the module's schema + * with the userGroups field. + * @param {AbstractApiModule} mod + */ + async registerUsergroupModule (mod) { + if (!mod.schemaName) { + return this.log('warn', 'cannot register module, module doesn\'t define a schemaName') + } + const jsonschema = await this.app.waitForModule('jsonschema') + jsonschema.extendSchema(mod.schemaName, 'usergroups') + this.log('debug', `registered ${mod.name} for use with usergroups`) + this.usergroupModules.push(mod) } forceLowerCaseEmail (data) { @@ -89,6 +175,32 @@ class UsersModule extends AbstractApiModule { query.email = this.getConfig('forceLowerCaseEmail') ? query.email?.toLowerCase() : undefined return super.find(query, options, mongoOptions) } + + /** @override */ + async delete (query, options = {}, mongoOptions = {}) { + const doc = await super.delete(query, options, mongoOptions) + if (options.collectionName === 'usergroups') { + await this.removeUsergroupRefs(doc._id) + } + return doc + } + + /** + * Removes references to a deleted usergroup from all registered modules + * @param {String} groupId The _id of the deleted usergroup + */ + async removeUsergroupRefs (groupId) { + await Promise.all(this.usergroupModules.map(async m => { + const docs = await m.find({ userGroups: groupId }) + return Promise.all(docs.map(async d => { + try { + await m.update({ _id: d._id }, { $pull: { userGroups: groupId } }, { rawUpdate: true }) + } catch (e) { + this.log('warn', `Failed to remove usergroup, ${e}`) + } + })) + })) + } } export default UsersModule diff --git a/package.json b/package.json index 440af0c..95fffcc 100644 --- a/package.json +++ b/package.json @@ -12,6 +12,7 @@ }, "peerDependencies": { "adapt-authoring-core": "^2.0.0", + "adapt-authoring-jsonschema": "^1.2.0", "adapt-authoring-mongodb": "^3.0.0", "adapt-authoring-server": "^2.1.0" }, diff --git a/schema/usergroup.schema.json b/schema/usergroup.schema.json new file mode 100644 index 0000000..161e62f --- /dev/null +++ b/schema/usergroup.schema.json @@ -0,0 +1,13 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$anchor": "usergroup", + "description": "A group of system users", + "type": "object", + "properties": { + "displayName": { + "description": "Display name of the user group", + "type": "string" + } + }, + "required": ["displayName"] +} diff --git a/schema/usergroups.schema.json b/schema/usergroups.schema.json new file mode 100644 index 0000000..cd7753d --- /dev/null +++ b/schema/usergroups.schema.json @@ -0,0 +1,21 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "$anchor": "usergroups", + "description": "Properties related to user group sharing", + "type": "object", + "$patch": { + "source": { "$ref": "user" }, + "with": { + "properties": { + "userGroups": { + "description": "User groups the target object should be accessible to", + "type": "array", + "items": { + "type": "string", + "isObjectId": true + } + } + } + } + } +} diff --git a/tests/usergroups.spec.js b/tests/usergroups.spec.js new file mode 100644 index 0000000..1014c37 --- /dev/null +++ b/tests/usergroups.spec.js @@ -0,0 +1,307 @@ +import { describe, it, mock, beforeEach } from 'node:test' +import assert from 'node:assert/strict' + +describe('UsersModule usergroups', () => { + describe('#usergroupsProcessRequest()', () => { + function usergroupsProcessRequest (req, res, next) { + const config = req.routeConfig || {} + const modifiers = config.modifiers ?? ['post', 'put', 'patch', 'delete'] + const modifying = config.modifying ?? modifiers.includes(req.method.toLowerCase()) + req.apiData = { + collectionName: 'usergroups', + config, + data: { ...req.body }, + modifying, + query: { ...req.query, ...req.params }, + schemaName: 'usergroup' + } + next() + } + + it('should set collectionName to usergroups', () => { + const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: { route: '/' } } + const next = mock.fn() + usergroupsProcessRequest(req, {}, next) + assert.equal(req.apiData.collectionName, 'usergroups') + }) + + it('should set schemaName to usergroup', () => { + const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: { route: '/' } } + const next = mock.fn() + usergroupsProcessRequest(req, {}, next) + assert.equal(req.apiData.schemaName, 'usergroup') + }) + + it('should merge query and params', () => { + const req = { method: 'GET', body: {}, query: { search: 'test' }, params: { _id: '123' }, routeConfig: {} } + const next = mock.fn() + usergroupsProcessRequest(req, {}, next) + assert.deepEqual(req.apiData.query, { search: 'test', _id: '123' }) + }) + + it('should set modifying true for POST', () => { + const req = { method: 'POST', body: { displayName: 'Group' }, query: {}, params: {}, routeConfig: {} } + const next = mock.fn() + usergroupsProcessRequest(req, {}, next) + assert.equal(req.apiData.modifying, true) + }) + + it('should set modifying false for GET', () => { + const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: {} } + const next = mock.fn() + usergroupsProcessRequest(req, {}, next) + assert.equal(req.apiData.modifying, false) + }) + + it('should call next', () => { + const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: {} } + const next = mock.fn() + usergroupsProcessRequest(req, {}, next) + assert.equal(next.mock.callCount(), 1) + }) + }) + + describe('#registerUsergroupModule()', () => { + let instance, logCalls + + async function registerUsergroupModule (mod) { + if (!mod.schemaName) { + return this.log('warn', 'cannot register module, module doesn\'t define a schemaName') + } + const jsonschema = await this.app.waitForModule('jsonschema') + jsonschema.extendSchema(mod.schemaName, 'usergroups') + this.log('debug', `registered ${mod.name} for use with usergroups`) + this.usergroupModules.push(mod) + } + + beforeEach(() => { + logCalls = [] + instance = { + usergroupModules: [], + app: { + waitForModule: mock.fn(async () => ({ + extendSchema: mock.fn() + })) + }, + log: function (...args) { logCalls.push(args) } + } + }) + + it('should log a warning if mod has no schemaName', async () => { + await registerUsergroupModule.call(instance, {}) + assert.equal(logCalls.length, 1) + assert.equal(logCalls[0][0], 'warn') + assert.ok(logCalls[0][1].includes('schemaName')) + assert.equal(instance.usergroupModules.length, 0) + }) + + it('should log a warning if mod.schemaName is empty string', async () => { + await registerUsergroupModule.call(instance, { schemaName: '' }) + assert.equal(logCalls.length, 1) + assert.equal(logCalls[0][0], 'warn') + assert.equal(instance.usergroupModules.length, 0) + }) + + it('should register a module with a valid schemaName', async () => { + const extendSchemaCalls = [] + instance.app.waitForModule = mock.fn(async () => ({ + extendSchema: function (schemaName, extensionName) { + extendSchemaCalls.push({ schemaName, extensionName }) + } + })) + + const mod = { schemaName: 'user', name: 'users' } + await registerUsergroupModule.call(instance, mod) + + assert.equal(extendSchemaCalls.length, 1) + assert.equal(extendSchemaCalls[0].schemaName, 'user') + assert.equal(extendSchemaCalls[0].extensionName, 'usergroups') + assert.equal(instance.usergroupModules.length, 1) + assert.equal(instance.usergroupModules[0], mod) + }) + + it('should log debug message with module name after registering', async () => { + instance.app.waitForModule = mock.fn(async () => ({ extendSchema: mock.fn() })) + + const mod = { schemaName: 'user', name: 'users' } + await registerUsergroupModule.call(instance, mod) + + assert.equal(logCalls.length, 1) + assert.equal(logCalls[0][0], 'debug') + assert.ok(logCalls[0][1].includes('users')) + assert.ok(logCalls[0][1].includes('usergroups')) + }) + + it('should allow registering multiple modules', async () => { + instance.app.waitForModule = mock.fn(async () => ({ extendSchema: mock.fn() })) + + const mod1 = { schemaName: 'user', name: 'users' } + const mod2 = { schemaName: 'course', name: 'courses' } + + await registerUsergroupModule.call(instance, mod1) + await registerUsergroupModule.call(instance, mod2) + + assert.equal(instance.usergroupModules.length, 2) + assert.equal(instance.usergroupModules[0], mod1) + assert.equal(instance.usergroupModules[1], mod2) + }) + }) + + describe('#removeUsergroupRefs()', () => { + let instance, logCalls + + async function removeUsergroupRefs (groupId) { + await Promise.all(this.usergroupModules.map(async m => { + const docs = await m.find({ userGroups: groupId }) + return Promise.all(docs.map(async d => { + try { + await m.update({ _id: d._id }, { $pull: { userGroups: groupId } }, { rawUpdate: true }) + } catch (e) { + this.log('warn', `Failed to remove usergroup, ${e}`) + } + })) + })) + } + + beforeEach(() => { + logCalls = [] + instance = { + usergroupModules: [], + log: function (...args) { logCalls.push(args) } + } + }) + + it('should remove usergroup reference from all registered module documents', async () => { + const deletedId = 'group123' + const updateCalls = [] + const mockModule = { + find: mock.fn(async () => [ + { _id: 'doc1', userGroups: [deletedId, 'other'] }, + { _id: 'doc2', userGroups: [deletedId] } + ]), + update: mock.fn(async (query, data, opts) => { + updateCalls.push({ query, data, opts }) + }) + } + instance.usergroupModules = [mockModule] + + await removeUsergroupRefs.call(instance, deletedId) + + assert.equal(mockModule.find.mock.callCount(), 1) + assert.deepEqual(mockModule.find.mock.calls[0].arguments[0], { userGroups: deletedId }) + assert.equal(mockModule.update.mock.callCount(), 2) + + assert.deepEqual(updateCalls[0].query, { _id: 'doc1' }) + assert.deepEqual(updateCalls[0].data, { $pull: { userGroups: deletedId } }) + assert.deepEqual(updateCalls[0].opts, { rawUpdate: true }) + + assert.deepEqual(updateCalls[1].query, { _id: 'doc2' }) + assert.deepEqual(updateCalls[1].data, { $pull: { userGroups: deletedId } }) + assert.deepEqual(updateCalls[1].opts, { rawUpdate: true }) + }) + + it('should handle empty documents list gracefully', async () => { + const mockModule = { + find: mock.fn(async () => []), + update: mock.fn() + } + instance.usergroupModules = [mockModule] + + await removeUsergroupRefs.call(instance, 'group456') + + assert.equal(mockModule.find.mock.callCount(), 1) + assert.equal(mockModule.update.mock.callCount(), 0) + }) + + it('should handle no registered modules', async () => { + instance.usergroupModules = [] + await removeUsergroupRefs.call(instance, 'group789') + }) + + it('should log warning and continue when update fails', async () => { + const updateError = new Error('DB write failed') + const mockModule = { + find: mock.fn(async () => [{ _id: 'doc1' }, { _id: 'doc2' }]), + update: mock.fn(async (query) => { + if (query._id === 'doc1') throw updateError + }) + } + instance.usergroupModules = [mockModule] + + await removeUsergroupRefs.call(instance, 'group-err') + + assert.equal(logCalls.length, 1) + assert.equal(logCalls[0][0], 'warn') + assert.ok(logCalls[0][1].includes('Failed to remove usergroup')) + assert.ok(logCalls[0][1].includes('DB write failed')) + assert.equal(mockModule.update.mock.callCount(), 2) + }) + + it('should process multiple modules independently', async () => { + const updateCalls = [] + const mockModule1 = { + find: mock.fn(async () => [{ _id: 'mod1doc1' }]), + update: mock.fn(async (query, data, opts) => { + updateCalls.push({ module: 'mod1', query, data, opts }) + }) + } + const mockModule2 = { + find: mock.fn(async () => [{ _id: 'mod2doc1' }, { _id: 'mod2doc2' }]), + update: mock.fn(async (query, data, opts) => { + updateCalls.push({ module: 'mod2', query, data, opts }) + }) + } + instance.usergroupModules = [mockModule1, mockModule2] + + await removeUsergroupRefs.call(instance, 'groupMulti') + + assert.equal(mockModule1.find.mock.callCount(), 1) + assert.equal(mockModule2.find.mock.callCount(), 1) + assert.equal(updateCalls.length, 3) + + const mod1Updates = updateCalls.filter(c => c.module === 'mod1') + const mod2Updates = updateCalls.filter(c => c.module === 'mod2') + assert.equal(mod1Updates.length, 1) + assert.equal(mod2Updates.length, 2) + }) + }) + + describe('#delete() cascade', () => { + it('should call removeUsergroupRefs when deleting from usergroups collection', async () => { + const deletedId = 'groupReturn' + const removeCalls = [] + const superDeleteResult = { _id: deletedId } + + async function deleteMethod (query, options = {}, mongoOptions = {}) { + // simulate setDefaultOptions + super.delete + if (!options.collectionName) options.collectionName = 'users' + const doc = superDeleteResult + if (options.collectionName === 'usergroups') { + removeCalls.push(doc._id) + } + return doc + } + + const result = await deleteMethod({}, { collectionName: 'usergroups' }) + assert.equal(result._id, deletedId) + assert.equal(removeCalls.length, 1) + assert.equal(removeCalls[0], deletedId) + }) + + it('should not call removeUsergroupRefs when deleting from users collection', async () => { + const removeCalls = [] + + async function deleteMethod (query, options = {}, mongoOptions = {}) { + if (!options.collectionName) options.collectionName = 'users' + const doc = { _id: 'user123' } + if (options.collectionName === 'usergroups') { + removeCalls.push(doc._id) + } + return doc + } + + await deleteMethod({}, { collectionName: 'users' }) + assert.equal(removeCalls.length, 0) + }) + }) +}) From f6cf350fa0c1e889673090e3ac0bcf92791cc85c Mon Sep 17 00:00:00 2001 From: Thomas Taylor Date: Fri, 27 Mar 2026 16:32:57 +0000 Subject: [PATCH 2/5] Update: Load usergroup routes from routes.json via loadRouteConfig --- lib/UsersModule.js | 50 ++++++++++++++++++++---------------------- usergroups/routes.json | 3 +++ 2 files changed, 27 insertions(+), 26 deletions(-) create mode 100644 usergroups/routes.json diff --git a/lib/UsersModule.js b/lib/UsersModule.js index 2003f2e..0b37d1b 100644 --- a/lib/UsersModule.js +++ b/lib/UsersModule.js @@ -1,4 +1,6 @@ +import { fileURLToPath } from 'node:url' import AbstractApiModule from 'adapt-authoring-api' +import { loadRouteConfig } from 'adapt-authoring-server' /** * Module which handles user management and user groups * @memberof users @@ -35,7 +37,7 @@ class UsersModule extends AbstractApiModule { this.preUpdateHook.tap((ogDoc, updateData) => this.forceLowerCaseEmail(updateData)) } - this.initUsergroupRoutes(server, auth) + await this.initUsergroupRoutes(server, auth) await this.registerUsergroupModule(this) } @@ -44,36 +46,32 @@ class UsersModule extends AbstractApiModule { * @param {Object} server The server module * @param {Object} auth The auth module */ - initUsergroupRoutes (server, auth) { - this.usergroupsRouter = server.api.createChildRouter('usergroups') + async initUsergroupRoutes (server, auth) { + const rootDir = fileURLToPath(new URL('../usergroups', import.meta.url)) + const defaults = fileURLToPath(import.meta.resolve('adapt-authoring-api/default-routes.json')) + const config = await loadRouteConfig(rootDir, this, { + schema: 'apiroutes', + handlerAliases: { serveSchema: this.serveSchema.bind(this) }, + defaults + }) + if (!config) return + + /* eslint-disable no-template-curly-in-string */ + const replacements = { + '${scope}': config.permissionsScope || config.root, + '${schemaName}': 'usergroup', + '${collectionName}': 'usergroups' + } + /* eslint-enable no-template-curly-in-string */ + const routes = config.routes.map(r => this.replacePlaceholders(r, replacements)) + + this.usergroupsRouter = server.api.createChildRouter(config.root) this.usergroupsRouter.addHandlerMiddleware( this.usergroupsProcessRequest.bind(this), this.sanitiseRequestDataMiddleware.bind(this) ) - const routes = [ - { - route: '/', - handlers: { get: this.queryHandler.bind(this), post: this.requestHandler.bind(this) }, - permissions: { get: ['read:usergroups'], post: ['write:usergroups'] } - }, - { - route: '/:_id', - handlers: { - get: this.requestHandler.bind(this), - put: this.requestHandler.bind(this), - patch: this.requestHandler.bind(this), - delete: this.requestHandler.bind(this) - }, - permissions: { - get: ['read:usergroups'], - put: ['write:usergroups'], - patch: ['write:usergroups'], - delete: ['write:usergroups'] - } - } - ] routes.forEach(r => { - Object.entries(r.permissions).forEach(([method, perms]) => { + Object.entries(r.permissions || {}).forEach(([method, perms]) => { const route = r.route.endsWith('/') ? r.route.slice(0, -1) : r.route auth.secureRoute(this.usergroupsRouter.path + route, method, perms) }) diff --git a/usergroups/routes.json b/usergroups/routes.json new file mode 100644 index 0000000..7534a35 --- /dev/null +++ b/usergroups/routes.json @@ -0,0 +1,3 @@ +{ + "root": "usergroups" +} From e3793e0dd94b85ee2d5933e49902d5501a24ac57 Mon Sep 17 00:00:00 2001 From: Thomas Taylor Date: Fri, 27 Mar 2026 16:45:54 +0000 Subject: [PATCH 3/5] Update: Move usergroup routes into users routes.json as /groups sub-routes --- lib/UsersModule.js | 66 ++---------------------------------- routes.json | 30 +++++++++++++++++ tests/usergroups.spec.js | 73 +++++++++++++--------------------------- usergroups/routes.json | 3 -- 4 files changed, 55 insertions(+), 117 deletions(-) delete mode 100644 usergroups/routes.json diff --git a/lib/UsersModule.js b/lib/UsersModule.js index 0b37d1b..76d7261 100644 --- a/lib/UsersModule.js +++ b/lib/UsersModule.js @@ -1,6 +1,4 @@ -import { fileURLToPath } from 'node:url' import AbstractApiModule from 'adapt-authoring-api' -import { loadRouteConfig } from 'adapt-authoring-server' /** * Module which handles user management and user groups * @memberof users @@ -25,7 +23,7 @@ class UsersModule extends AbstractApiModule { */ async init () { await super.init() - const [mongodb, server, auth] = await this.app.waitForModule('mongodb', 'server', 'auth') + const [mongodb, server] = await this.app.waitForModule('mongodb', 'server') await mongodb.setIndex(this.collectionName, 'email', { unique: true }) server.api.addHandlerMiddleware(this.updateAccess.bind(this)) @@ -37,69 +35,9 @@ class UsersModule extends AbstractApiModule { this.preUpdateHook.tap((ogDoc, updateData) => this.forceLowerCaseEmail(updateData)) } - await this.initUsergroupRoutes(server, auth) await this.registerUsergroupModule(this) } - /** - * Sets up the /api/usergroups child router and routes - * @param {Object} server The server module - * @param {Object} auth The auth module - */ - async initUsergroupRoutes (server, auth) { - const rootDir = fileURLToPath(new URL('../usergroups', import.meta.url)) - const defaults = fileURLToPath(import.meta.resolve('adapt-authoring-api/default-routes.json')) - const config = await loadRouteConfig(rootDir, this, { - schema: 'apiroutes', - handlerAliases: { serveSchema: this.serveSchema.bind(this) }, - defaults - }) - if (!config) return - - /* eslint-disable no-template-curly-in-string */ - const replacements = { - '${scope}': config.permissionsScope || config.root, - '${schemaName}': 'usergroup', - '${collectionName}': 'usergroups' - } - /* eslint-enable no-template-curly-in-string */ - const routes = config.routes.map(r => this.replacePlaceholders(r, replacements)) - - this.usergroupsRouter = server.api.createChildRouter(config.root) - this.usergroupsRouter.addHandlerMiddleware( - this.usergroupsProcessRequest.bind(this), - this.sanitiseRequestDataMiddleware.bind(this) - ) - routes.forEach(r => { - Object.entries(r.permissions || {}).forEach(([method, perms]) => { - const route = r.route.endsWith('/') ? r.route.slice(0, -1) : r.route - auth.secureRoute(this.usergroupsRouter.path + route, method, perms) - }) - this.usergroupsRouter.addRoute(r) - }) - } - - /** - * Express middleware to prepare req.apiData for usergroup requests - * @param {external:ExpressRequest} req - * @param {external:ExpressResponse} res - * @param {Function} next - */ - usergroupsProcessRequest (req, res, next) { - const config = req.routeConfig || {} - const modifiers = config.modifiers ?? ['post', 'put', 'patch', 'delete'] - const modifying = config.modifying ?? modifiers.includes(req.method.toLowerCase()) - req.apiData = { - collectionName: 'usergroups', - config, - data: { ...req.body }, - modifying, - query: { ...req.query, ...req.params }, - schemaName: 'usergroup' - } - next() - } - /** * Registers a module for use with user groups. Extends the module's schema * with the userGroups field. @@ -122,7 +60,7 @@ class UsersModule extends AbstractApiModule { /** @override */ async processRequestMiddleware (req, res, next) { super.processRequestMiddleware(req, res, () => { - req.apiData.schemaName = req.auth.userSchemaName + req.apiData.schemaName = req.routeConfig.schemaName || req.auth.userSchemaName next() }) } diff --git a/routes.json b/routes.json index 996b988..e19b160 100644 --- a/routes.json +++ b/routes.json @@ -34,6 +34,36 @@ "modifying": false, "handlers": { "post": "queryHandler" }, "permissions": { "post": ["read:${scope}"] } + }, + { + "route": "/groups", + "collectionName": "usergroups", + "schemaName": "usergroup", + "handlers": { "get": "queryHandler", "post": "requestHandler" }, + "permissions": { "get": ["read:usergroups"], "post": ["write:usergroups"] } + }, + { + "route": "/groups/schema", + "collectionName": "usergroups", + "schemaName": "usergroup", + "handlers": { "get": "serveSchema" }, + "permissions": { "get": ["read:schema"] } + }, + { + "route": "/groups/:_id", + "collectionName": "usergroups", + "schemaName": "usergroup", + "handlers": { "get": "requestHandler", "put": "requestHandler", "patch": "requestHandler", "delete": "requestHandler" }, + "permissions": { "get": ["read:usergroups"], "put": ["write:usergroups"], "patch": ["write:usergroups"], "delete": ["write:usergroups"] } + }, + { + "route": "/groups/query", + "collectionName": "usergroups", + "schemaName": "usergroup", + "validate": false, + "modifying": false, + "handlers": { "post": "queryHandler" }, + "permissions": { "post": ["read:usergroups"] } } ] } diff --git a/tests/usergroups.spec.js b/tests/usergroups.spec.js index 1014c37..4e024fa 100644 --- a/tests/usergroups.spec.js +++ b/tests/usergroups.spec.js @@ -2,62 +2,35 @@ import { describe, it, mock, beforeEach } from 'node:test' import assert from 'node:assert/strict' describe('UsersModule usergroups', () => { - describe('#usergroupsProcessRequest()', () => { - function usergroupsProcessRequest (req, res, next) { - const config = req.routeConfig || {} - const modifiers = config.modifiers ?? ['post', 'put', 'patch', 'delete'] - const modifying = config.modifying ?? modifiers.includes(req.method.toLowerCase()) - req.apiData = { - collectionName: 'usergroups', - config, - data: { ...req.body }, - modifying, - query: { ...req.query, ...req.params }, - schemaName: 'usergroup' - } - next() + describe('#processRequestMiddleware() schema routing', () => { + function processRequestMiddleware (req, superCallback) { + // Simulates the override: use per-route schemaName if set, otherwise userSchemaName + req.apiData = { schemaName: 'user' } // super sets this + superCallback() } - it('should set collectionName to usergroups', () => { - const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: { route: '/' } } - const next = mock.fn() - usergroupsProcessRequest(req, {}, next) - assert.equal(req.apiData.collectionName, 'usergroups') - }) + function override (req) { + req.apiData.schemaName = req.routeConfig.schemaName || req.auth.userSchemaName + } - it('should set schemaName to usergroup', () => { - const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: { route: '/' } } - const next = mock.fn() - usergroupsProcessRequest(req, {}, next) + it('should use per-route schemaName for usergroup routes', () => { + const req = { + routeConfig: { schemaName: 'usergroup' }, + auth: { userSchemaName: 'superuser' }, + apiData: {} + } + processRequestMiddleware(req, () => override(req)) assert.equal(req.apiData.schemaName, 'usergroup') }) - it('should merge query and params', () => { - const req = { method: 'GET', body: {}, query: { search: 'test' }, params: { _id: '123' }, routeConfig: {} } - const next = mock.fn() - usergroupsProcessRequest(req, {}, next) - assert.deepEqual(req.apiData.query, { search: 'test', _id: '123' }) - }) - - it('should set modifying true for POST', () => { - const req = { method: 'POST', body: { displayName: 'Group' }, query: {}, params: {}, routeConfig: {} } - const next = mock.fn() - usergroupsProcessRequest(req, {}, next) - assert.equal(req.apiData.modifying, true) - }) - - it('should set modifying false for GET', () => { - const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: {} } - const next = mock.fn() - usergroupsProcessRequest(req, {}, next) - assert.equal(req.apiData.modifying, false) - }) - - it('should call next', () => { - const req = { method: 'GET', body: {}, query: {}, params: {}, routeConfig: {} } - const next = mock.fn() - usergroupsProcessRequest(req, {}, next) - assert.equal(next.mock.callCount(), 1) + it('should fall back to auth userSchemaName for user routes', () => { + const req = { + routeConfig: {}, + auth: { userSchemaName: 'superuser' }, + apiData: {} + } + processRequestMiddleware(req, () => override(req)) + assert.equal(req.apiData.schemaName, 'superuser') }) }) diff --git a/usergroups/routes.json b/usergroups/routes.json deleted file mode 100644 index 7534a35..0000000 --- a/usergroups/routes.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "root": "usergroups" -} From 6dbf0816753475333baa90b7898c668b4bf66d27 Mon Sep 17 00:00:00 2001 From: Thomas Taylor Date: Fri, 27 Mar 2026 16:56:09 +0000 Subject: [PATCH 4/5] Update: Rename internal usergroup references to group --- lib/UsersModule.js | 24 ++++++------- tests/usergroups.spec.js | 74 ++++++++++++++++++++-------------------- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/lib/UsersModule.js b/lib/UsersModule.js index 76d7261..ae160e4 100644 --- a/lib/UsersModule.js +++ b/lib/UsersModule.js @@ -14,7 +14,7 @@ class UsersModule extends AbstractApiModule { * Modules registered for user group support * @type {Array} */ - this.usergroupModules = [] + this.groupModules = [] } /** @@ -35,22 +35,22 @@ class UsersModule extends AbstractApiModule { this.preUpdateHook.tap((ogDoc, updateData) => this.forceLowerCaseEmail(updateData)) } - await this.registerUsergroupModule(this) + await this.registerGroupModule(this) } /** - * Registers a module for use with user groups. Extends the module's schema + * Registers a module for use with groups. Extends the module's schema * with the userGroups field. * @param {AbstractApiModule} mod */ - async registerUsergroupModule (mod) { + async registerGroupModule (mod) { if (!mod.schemaName) { return this.log('warn', 'cannot register module, module doesn\'t define a schemaName') } const jsonschema = await this.app.waitForModule('jsonschema') jsonschema.extendSchema(mod.schemaName, 'usergroups') - this.log('debug', `registered ${mod.name} for use with usergroups`) - this.usergroupModules.push(mod) + this.log('debug', `registered ${mod.name} for use with groups`) + this.groupModules.push(mod) } forceLowerCaseEmail (data) { @@ -116,23 +116,23 @@ class UsersModule extends AbstractApiModule { async delete (query, options = {}, mongoOptions = {}) { const doc = await super.delete(query, options, mongoOptions) if (options.collectionName === 'usergroups') { - await this.removeUsergroupRefs(doc._id) + await this.removeGroupRefs(doc._id) } return doc } /** - * Removes references to a deleted usergroup from all registered modules - * @param {String} groupId The _id of the deleted usergroup + * Removes references to a deleted group from all registered modules + * @param {String} groupId The _id of the deleted group */ - async removeUsergroupRefs (groupId) { - await Promise.all(this.usergroupModules.map(async m => { + async removeGroupRefs (groupId) { + await Promise.all(this.groupModules.map(async m => { const docs = await m.find({ userGroups: groupId }) return Promise.all(docs.map(async d => { try { await m.update({ _id: d._id }, { $pull: { userGroups: groupId } }, { rawUpdate: true }) } catch (e) { - this.log('warn', `Failed to remove usergroup, ${e}`) + this.log('warn', `Failed to remove group reference, ${e}`) } })) })) diff --git a/tests/usergroups.spec.js b/tests/usergroups.spec.js index 4e024fa..8305287 100644 --- a/tests/usergroups.spec.js +++ b/tests/usergroups.spec.js @@ -34,23 +34,23 @@ describe('UsersModule usergroups', () => { }) }) - describe('#registerUsergroupModule()', () => { + describe('#registerGroupModule()', () => { let instance, logCalls - async function registerUsergroupModule (mod) { + async function registerGroupModule (mod) { if (!mod.schemaName) { return this.log('warn', 'cannot register module, module doesn\'t define a schemaName') } const jsonschema = await this.app.waitForModule('jsonschema') jsonschema.extendSchema(mod.schemaName, 'usergroups') - this.log('debug', `registered ${mod.name} for use with usergroups`) - this.usergroupModules.push(mod) + this.log('debug', `registered ${mod.name} for use with groups`) + this.groupModules.push(mod) } beforeEach(() => { logCalls = [] instance = { - usergroupModules: [], + groupModules: [], app: { waitForModule: mock.fn(async () => ({ extendSchema: mock.fn() @@ -61,18 +61,18 @@ describe('UsersModule usergroups', () => { }) it('should log a warning if mod has no schemaName', async () => { - await registerUsergroupModule.call(instance, {}) + await registerGroupModule.call(instance, {}) assert.equal(logCalls.length, 1) assert.equal(logCalls[0][0], 'warn') assert.ok(logCalls[0][1].includes('schemaName')) - assert.equal(instance.usergroupModules.length, 0) + assert.equal(instance.groupModules.length, 0) }) it('should log a warning if mod.schemaName is empty string', async () => { - await registerUsergroupModule.call(instance, { schemaName: '' }) + await registerGroupModule.call(instance, { schemaName: '' }) assert.equal(logCalls.length, 1) assert.equal(logCalls[0][0], 'warn') - assert.equal(instance.usergroupModules.length, 0) + assert.equal(instance.groupModules.length, 0) }) it('should register a module with a valid schemaName', async () => { @@ -84,25 +84,25 @@ describe('UsersModule usergroups', () => { })) const mod = { schemaName: 'user', name: 'users' } - await registerUsergroupModule.call(instance, mod) + await registerGroupModule.call(instance, mod) assert.equal(extendSchemaCalls.length, 1) assert.equal(extendSchemaCalls[0].schemaName, 'user') assert.equal(extendSchemaCalls[0].extensionName, 'usergroups') - assert.equal(instance.usergroupModules.length, 1) - assert.equal(instance.usergroupModules[0], mod) + assert.equal(instance.groupModules.length, 1) + assert.equal(instance.groupModules[0], mod) }) it('should log debug message with module name after registering', async () => { instance.app.waitForModule = mock.fn(async () => ({ extendSchema: mock.fn() })) const mod = { schemaName: 'user', name: 'users' } - await registerUsergroupModule.call(instance, mod) + await registerGroupModule.call(instance, mod) assert.equal(logCalls.length, 1) assert.equal(logCalls[0][0], 'debug') assert.ok(logCalls[0][1].includes('users')) - assert.ok(logCalls[0][1].includes('usergroups')) + assert.ok(logCalls[0][1].includes('groups')) }) it('should allow registering multiple modules', async () => { @@ -111,26 +111,26 @@ describe('UsersModule usergroups', () => { const mod1 = { schemaName: 'user', name: 'users' } const mod2 = { schemaName: 'course', name: 'courses' } - await registerUsergroupModule.call(instance, mod1) - await registerUsergroupModule.call(instance, mod2) + await registerGroupModule.call(instance, mod1) + await registerGroupModule.call(instance, mod2) - assert.equal(instance.usergroupModules.length, 2) - assert.equal(instance.usergroupModules[0], mod1) - assert.equal(instance.usergroupModules[1], mod2) + assert.equal(instance.groupModules.length, 2) + assert.equal(instance.groupModules[0], mod1) + assert.equal(instance.groupModules[1], mod2) }) }) - describe('#removeUsergroupRefs()', () => { + describe('#removeGroupRefs()', () => { let instance, logCalls - async function removeUsergroupRefs (groupId) { - await Promise.all(this.usergroupModules.map(async m => { + async function removeGroupRefs (groupId) { + await Promise.all(this.groupModules.map(async m => { const docs = await m.find({ userGroups: groupId }) return Promise.all(docs.map(async d => { try { await m.update({ _id: d._id }, { $pull: { userGroups: groupId } }, { rawUpdate: true }) } catch (e) { - this.log('warn', `Failed to remove usergroup, ${e}`) + this.log('warn', `Failed to remove group reference, ${e}`) } })) })) @@ -139,7 +139,7 @@ describe('UsersModule usergroups', () => { beforeEach(() => { logCalls = [] instance = { - usergroupModules: [], + groupModules: [], log: function (...args) { logCalls.push(args) } } }) @@ -156,9 +156,9 @@ describe('UsersModule usergroups', () => { updateCalls.push({ query, data, opts }) }) } - instance.usergroupModules = [mockModule] + instance.groupModules = [mockModule] - await removeUsergroupRefs.call(instance, deletedId) + await removeGroupRefs.call(instance, deletedId) assert.equal(mockModule.find.mock.callCount(), 1) assert.deepEqual(mockModule.find.mock.calls[0].arguments[0], { userGroups: deletedId }) @@ -178,17 +178,17 @@ describe('UsersModule usergroups', () => { find: mock.fn(async () => []), update: mock.fn() } - instance.usergroupModules = [mockModule] + instance.groupModules = [mockModule] - await removeUsergroupRefs.call(instance, 'group456') + await removeGroupRefs.call(instance, 'group456') assert.equal(mockModule.find.mock.callCount(), 1) assert.equal(mockModule.update.mock.callCount(), 0) }) it('should handle no registered modules', async () => { - instance.usergroupModules = [] - await removeUsergroupRefs.call(instance, 'group789') + instance.groupModules = [] + await removeGroupRefs.call(instance, 'group789') }) it('should log warning and continue when update fails', async () => { @@ -199,13 +199,13 @@ describe('UsersModule usergroups', () => { if (query._id === 'doc1') throw updateError }) } - instance.usergroupModules = [mockModule] + instance.groupModules = [mockModule] - await removeUsergroupRefs.call(instance, 'group-err') + await removeGroupRefs.call(instance, 'group-err') assert.equal(logCalls.length, 1) assert.equal(logCalls[0][0], 'warn') - assert.ok(logCalls[0][1].includes('Failed to remove usergroup')) + assert.ok(logCalls[0][1].includes('Failed to remove group reference')) assert.ok(logCalls[0][1].includes('DB write failed')) assert.equal(mockModule.update.mock.callCount(), 2) }) @@ -224,9 +224,9 @@ describe('UsersModule usergroups', () => { updateCalls.push({ module: 'mod2', query, data, opts }) }) } - instance.usergroupModules = [mockModule1, mockModule2] + instance.groupModules = [mockModule1, mockModule2] - await removeUsergroupRefs.call(instance, 'groupMulti') + await removeGroupRefs.call(instance, 'groupMulti') assert.equal(mockModule1.find.mock.callCount(), 1) assert.equal(mockModule2.find.mock.callCount(), 1) @@ -240,7 +240,7 @@ describe('UsersModule usergroups', () => { }) describe('#delete() cascade', () => { - it('should call removeUsergroupRefs when deleting from usergroups collection', async () => { + it('should call removeGroupRefs when deleting from usergroups collection', async () => { const deletedId = 'groupReturn' const removeCalls = [] const superDeleteResult = { _id: deletedId } @@ -261,7 +261,7 @@ describe('UsersModule usergroups', () => { assert.equal(removeCalls[0], deletedId) }) - it('should not call removeUsergroupRefs when deleting from users collection', async () => { + it('should not call removeGroupRefs when deleting from users collection', async () => { const removeCalls = [] async function deleteMethod (query, options = {}, mongoOptions = {}) { From 24a1669e27c66dc058082b0fbacd5111322794f6 Mon Sep 17 00:00:00 2001 From: Thomas Taylor Date: Fri, 27 Mar 2026 17:19:58 +0000 Subject: [PATCH 5/5] New: Add group-based access check hook for registered modules --- lib/UsersModule.js | 4 +++ lib/utils.js | 1 + lib/utils/hasGroupAccess.js | 14 ++++++++ tests/utils-hasGroupAccess.spec.js | 55 ++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+) create mode 100644 lib/utils.js create mode 100644 lib/utils/hasGroupAccess.js create mode 100644 tests/utils-hasGroupAccess.spec.js diff --git a/lib/UsersModule.js b/lib/UsersModule.js index ae160e4..2a08742 100644 --- a/lib/UsersModule.js +++ b/lib/UsersModule.js @@ -1,4 +1,5 @@ import AbstractApiModule from 'adapt-authoring-api' +import { hasGroupAccess } from './utils.js' /** * Module which handles user management and user groups * @memberof users @@ -49,6 +50,9 @@ class UsersModule extends AbstractApiModule { } const jsonschema = await this.app.waitForModule('jsonschema') jsonschema.extendSchema(mod.schemaName, 'usergroups') + mod.accessCheckHook.tap((req, resource) => { + return hasGroupAccess(resource.userGroups, req.auth?.user?.userGroups) + }) this.log('debug', `registered ${mod.name} for use with groups`) this.groupModules.push(mod) } diff --git a/lib/utils.js b/lib/utils.js new file mode 100644 index 0000000..5261ed1 --- /dev/null +++ b/lib/utils.js @@ -0,0 +1 @@ +export { hasGroupAccess } from './utils/hasGroupAccess.js' diff --git a/lib/utils/hasGroupAccess.js b/lib/utils/hasGroupAccess.js new file mode 100644 index 0000000..27d0e2d --- /dev/null +++ b/lib/utils/hasGroupAccess.js @@ -0,0 +1,14 @@ +/** + * Checks whether a user shares at least one group with a document. + * Returns true if the document has no group restrictions. + * @param {Array} docGroups The userGroups array from the document + * @param {Array} userGroups The userGroups array from the requesting user + * @return {Boolean} + * @memberof users + */ +export function hasGroupAccess (docGroups, userGroups) { + if (!docGroups?.length) return true + if (!userGroups?.length) return false + const userSet = new Set(userGroups.map(g => g.toString())) + return docGroups.some(g => userSet.has(g.toString())) +} diff --git a/tests/utils-hasGroupAccess.spec.js b/tests/utils-hasGroupAccess.spec.js new file mode 100644 index 0000000..1cef5f8 --- /dev/null +++ b/tests/utils-hasGroupAccess.spec.js @@ -0,0 +1,55 @@ +import { describe, it } from 'node:test' +import assert from 'node:assert/strict' +import { hasGroupAccess } from '../lib/utils/hasGroupAccess.js' + +describe('hasGroupAccess()', () => { + it('should return true when document has no groups', () => { + assert.equal(hasGroupAccess([], ['group1']), true) + }) + + it('should return true when document groups is undefined', () => { + assert.equal(hasGroupAccess(undefined, ['group1']), true) + }) + + it('should return true when document groups is null', () => { + assert.equal(hasGroupAccess(null, ['group1']), true) + }) + + it('should return false when user has no groups but document does', () => { + assert.equal(hasGroupAccess(['group1'], []), false) + }) + + it('should return false when user groups is undefined', () => { + assert.equal(hasGroupAccess(['group1'], undefined), false) + }) + + it('should return false when user groups is null', () => { + assert.equal(hasGroupAccess(['group1'], null), false) + }) + + it('should return true when user shares a group with document', () => { + assert.equal(hasGroupAccess(['group1', 'group2'], ['group2', 'group3']), true) + }) + + it('should return false when user shares no groups with document', () => { + assert.equal(hasGroupAccess(['group1', 'group2'], ['group3', 'group4']), false) + }) + + it('should handle ObjectId-like objects by comparing via toString()', () => { + const docId = { toString: () => '507f1f77bcf86cd799439011' } + const userId = { toString: () => '507f1f77bcf86cd799439011' } + assert.equal(hasGroupAccess([docId], [userId]), true) + }) + + it('should handle mixed string and object IDs', () => { + const objectId = { toString: () => 'abc123' } + assert.equal(hasGroupAccess([objectId], ['abc123']), true) + assert.equal(hasGroupAccess(['abc123'], [objectId]), true) + }) + + it('should return false for different ObjectId-like objects', () => { + const docId = { toString: () => '507f1f77bcf86cd799439011' } + const userId = { toString: () => '507f1f77bcf86cd799439022' } + assert.equal(hasGroupAccess([docId], [userId]), false) + }) +})