From c564788582092717002bb3fc78aad0e92cec92d6 Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Mon, 29 Apr 2024 12:01:54 +0800 Subject: [PATCH 1/7] RBAC refactor --- services/api/README.md | 2 +- services/api/src/models/definitions/user.json | 6 +- services/api/src/models/user.js | 4 +- services/api/src/permissions.json | 9 - services/api/src/roles.json | 32 +-- services/api/src/routes/applications.js | 4 +- services/api/src/routes/audit-entries.js | 2 +- services/api/src/routes/invites.js | 4 +- services/api/src/routes/organizations.js | 4 +- services/api/src/routes/uploads.js | 3 +- services/api/src/routes/users.js | 62 +++--- services/api/src/utils/__tests__/document.js | 83 ++++++++ .../api/src/utils/__tests__/permissions.js | 98 +++++++-- services/api/src/utils/document.js | 25 +++ .../utils/middleware/__tests__/permissions.js | 188 ++++++++++++++++-- .../api/src/utils/middleware/permissions.js | 62 ++++-- services/api/src/utils/permissions.js | 127 ++++-------- 17 files changed, 499 insertions(+), 216 deletions(-) delete mode 100644 services/api/src/permissions.json create mode 100644 services/api/src/utils/__tests__/document.js create mode 100644 services/api/src/utils/document.js diff --git a/services/api/README.md b/services/api/README.md index 9ea817aba..8f2f91ee8 100644 --- a/services/api/README.md +++ b/services/api/README.md @@ -207,7 +207,7 @@ const { requirePermissions } = require('../utils/middleware/permissions'); router .use(authenticate()) // Only allow access to users that have write permissions for this organization - .use(requirePermissions({ endpoint: 'shops', level: 'write', context: 'organization' })) + .use(requirePermissions('shops.write', 'organization')) .post( '/', validate({ diff --git a/services/api/src/models/definitions/user.json b/services/api/src/models/definitions/user.json index 2e5694e4c..a4a965646 100644 --- a/services/api/src/models/definitions/user.json +++ b/services/api/src/models/definitions/user.json @@ -44,7 +44,11 @@ }, "scopeRef": { "type": "ObjectId", - "ref": "Organization" + "ref": "Organization", + "enum": [ + "global", + "organization" + ] } } ], diff --git a/services/api/src/models/user.js b/services/api/src/models/user.js index 72b2a824d..1ccba2dd3 100644 --- a/services/api/src/models/user.js +++ b/services/api/src/models/user.js @@ -1,11 +1,9 @@ const mongoose = require('mongoose'); const { createSchema } = require('@bedrockio/model'); - -const { validScopes } = require('../utils/permissions'); const { setPassword } = require('../utils/auth/password'); + const definition = require('./definitions/user.json'); -definition.attributes.roles[0].scope.enum = validScopes; const schema = createSchema(definition); schema.virtual('name').get(function () { diff --git a/services/api/src/permissions.json b/services/api/src/permissions.json deleted file mode 100644 index c26ff58ee..000000000 --- a/services/api/src/permissions.json +++ /dev/null @@ -1,9 +0,0 @@ -{ - "users": { - "name": "Users", - "maximums": { - "global": "read-write", - "organization": "none" - } - } -} diff --git a/services/api/src/roles.json b/services/api/src/roles.json index 32f93e490..d392b1460 100644 --- a/services/api/src/roles.json +++ b/services/api/src/roles.json @@ -5,13 +5,14 @@ "global" ], "permissions": { - "auditEntries": "read", - "users": "read-write", - "shops": "read-write", - "products": "read-write", - "organizations": "read-write", - "applications": "read-write", - "uploads": "read-write" + "applications": "all", + "auditEntries": "all", + "organizations": "all", + "products": "all", + "roles": "all", + "shops": "all", + "users": "all", + "uploads": "all" }, "allowAuthenticationOnRoles": [ "admin", @@ -25,11 +26,12 @@ ], "permissions": { "auditEntries": "read", - "users": "read-write", - "shops": "read-write", - "products": "read-write", - "applications": "read-write", - "uploads": "read-write" + "applications": "all", + "products": "all", + "roles": "all", + "shops": "all", + "users": "all", + "uploads": "all" } }, "viewer": { @@ -38,11 +40,11 @@ "organization" ], "permissions": { + "applications": "read", "auditEntries": "read", - "users": "read", - "shops": "read", "products": "read", - "applications": "read", + "shops": "read", + "users": "read", "uploads": "read" } } diff --git a/services/api/src/routes/applications.js b/services/api/src/routes/applications.js index b10ddb2cd..ebfea9c0b 100644 --- a/services/api/src/routes/applications.js +++ b/services/api/src/routes/applications.js @@ -11,7 +11,7 @@ const router = new Router(); router .use(authenticate()) - .use(requirePermissions({ endpoint: 'applications', permission: 'read', scope: 'global' })) + .use(requirePermissions('applications.read')) .param('id', fetchByParam(Application)) .post('/mine/search', validateBody(Application.getSearchValidation()), async (ctx) => { const { body } = ctx.request; @@ -54,7 +54,7 @@ router data: ctx.state.application, }; }) - .use(requirePermissions({ endpoint: 'applications', permission: 'write', scope: 'global' })) + .use(requirePermissions('applications.write')) .post('/', validateBody(Application.getCreateValidation()), async (ctx) => { const { body } = ctx.request; const apiKey = kebabCase(body.name); diff --git a/services/api/src/routes/audit-entries.js b/services/api/src/routes/audit-entries.js index ec03f1569..ef444e0c2 100644 --- a/services/api/src/routes/audit-entries.js +++ b/services/api/src/routes/audit-entries.js @@ -10,7 +10,7 @@ const router = new Router(); router .use(authenticate()) - .use(requirePermissions({ endpoint: 'auditEntries', permission: 'read', scope: 'global' })) + .use(requirePermissions('auditEntries.read')) .post( '/search', validateBody( diff --git a/services/api/src/routes/invites.js b/services/api/src/routes/invites.js index ae889d344..1caed951b 100644 --- a/services/api/src/routes/invites.js +++ b/services/api/src/routes/invites.js @@ -85,7 +85,7 @@ router } ) .use(authenticate()) - .use(requirePermissions({ endpoint: 'users', permission: 'read', scope: 'global' })) + .use(requirePermissions('users.read')) .param('id', fetchByParam(Invite)) .post('/search', validateBody(Invite.getSearchValidation()), async (ctx) => { const { data, meta } = await Invite.search(ctx.request.body); @@ -94,7 +94,7 @@ router meta, }; }) - .use(requirePermissions({ endpoint: 'users', permission: 'write', scope: 'global' })) + .use(requirePermissions('users.invite')) .post( '/', validateBody({ diff --git a/services/api/src/routes/organizations.js b/services/api/src/routes/organizations.js index 04005965b..39d4f4a31 100644 --- a/services/api/src/routes/organizations.js +++ b/services/api/src/routes/organizations.js @@ -38,7 +38,7 @@ router data: organization, }; }) - .use(requirePermissions({ endpoint: 'organizations', permission: 'read', scope: 'global' })) + .use(requirePermissions('organizations.read')) .post('/search', validateBody(Organization.getSearchValidation()), async (ctx) => { const { data, meta } = await Organization.search(ctx.request.body); ctx.body = { @@ -46,7 +46,7 @@ router meta, }; }) - .use(requirePermissions({ endpoint: 'organizations', permission: 'write', scope: 'global' })) + .use(requirePermissions('organizations.write')) .post('/', validateBody(Organization.getCreateValidation()), async (ctx) => { const organization = await Organization.create(ctx.request.body); ctx.body = { diff --git a/services/api/src/routes/uploads.js b/services/api/src/routes/uploads.js index 47fa1cc4e..0ffa83858 100644 --- a/services/api/src/routes/uploads.js +++ b/services/api/src/routes/uploads.js @@ -4,6 +4,7 @@ const { authenticate } = require('../utils/middleware/authenticate'); const { validateFiles } = require('../utils/middleware/validate'); const { createUploads, getUploadUrl, createUrlStream } = require('../utils/uploads'); const { userHasAccess } = require('./../utils/permissions'); +const { isEqual } = require('../utils/document'); const { Upload } = require('../models'); const router = new Router(); @@ -41,7 +42,7 @@ router if (ctx.method === 'GET') { return true; } else { - return upload.owner?.equals(ctx.state.authUser.id); + return isEqual(upload.owner, ctx.state.authUser); } }, }) diff --git a/services/api/src/routes/users.js b/services/api/src/routes/users.js index 6396d51d1..ff7c3c363 100644 --- a/services/api/src/routes/users.js +++ b/services/api/src/routes/users.js @@ -11,7 +11,6 @@ const { expandRoles } = require('./../utils/permissions'); const { User } = require('../models'); const roles = require('./../roles.json'); -const permissions = require('./../permissions.json'); const { AuditEntry } = require('../models'); @@ -45,41 +44,37 @@ router }; } ) - .post( - '/:id/authenticate', - requirePermissions({ endpoint: 'users', permission: 'write', scope: 'global' }), - async (ctx) => { - const { user } = ctx.state; - const authUser = ctx.state.authUser; + .post('/:id/authenticate', requirePermissions('users.impersonate'), async (ctx) => { + const { user } = ctx.state; + const authUser = ctx.state.authUser; - // Don't allow an superAdmin to imitate another superAdmin - const allowedRoles = expandRoles(authUser, ctx).roles.reduce( - (result, { roleDefinition }) => result.concat(roleDefinition.allowAuthenticationOnRoles || []), - [] - ); + // Don't allow an superAdmin to imitate another superAdmin + const allowedRoles = expandRoles(authUser, ctx).roles.reduce( + (result, { roleDefinition }) => result.concat(roleDefinition.allowAuthenticationOnRoles || []), + [] + ); - const isAllowed = [...user.roles].every(({ role }) => allowedRoles.includes(role)); - if (!isAllowed) { - ctx.throw(403, 'You are not allowed to authenticate as this user'); - } + const isAllowed = [...user.roles].every(({ role }) => allowedRoles.includes(role)); + if (!isAllowed) { + ctx.throw(403, 'You are not allowed to authenticate as this user'); + } - const token = createImpersonateAuthToken(user, authUser, ctx); - await authUser.save(); + const token = createImpersonateAuthToken(user, authUser, ctx); + await authUser.save(); - await AuditEntry.append('Authenticated as user', { - ctx, - object: user, - actor: authUser, - }); + await AuditEntry.append('Authenticated as user', { + ctx, + object: user, + actor: authUser, + }); - ctx.body = { - data: { - token, - }, - }; - } - ) - .use(requirePermissions({ endpoint: 'users', permission: 'read', scope: 'global' })) + ctx.body = { + data: { + token, + }, + }; + }) + .use(requirePermissions('roles.list')) .get('/roles', (ctx) => { ctx.body = { data: roles, @@ -87,7 +82,8 @@ router }) .get('/permissions', (ctx) => { ctx.body = { - data: permissions, + // TODO: what is needed here? + // data: permissions, }; }) .post( @@ -114,7 +110,7 @@ router data: expandRoles(ctx.state.user, ctx), }; }) - .use(requirePermissions({ endpoint: 'users', permission: 'write', scope: 'global' })) + .use(requirePermissions('users.create')) .post( '/', validateBody( diff --git a/services/api/src/utils/__tests__/document.js b/services/api/src/utils/__tests__/document.js new file mode 100644 index 000000000..b76045f8b --- /dev/null +++ b/services/api/src/utils/__tests__/document.js @@ -0,0 +1,83 @@ +const mongoose = require('mongoose'); +const { createTestModel } = require('@bedrockio/model'); +const { isEqual } = require('../document'); + +describe('isEqual', () => { + describe('ids', () => { + it('should compare two object ids', async () => { + expect( + isEqual( + new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), + new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f') + ) + ).toBe(true); + + expect( + isEqual( + new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d'), + new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f') + ) + ).toBe(false); + + expect( + isEqual( + new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), + new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d') + ) + ).toBe(false); + }); + + it('should compare an object id and a string', async () => { + expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), '662f11c8af6870637eab9f0f')).toBe(true); + expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d'), '662f11c8af6870637eab9f0f')).toBe(false); + expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), '662f11c8af6870637eab9f0d')).toBe(false); + + expect(isEqual('662f11c8af6870637eab9f0f', new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'))).toBe(true); + expect(isEqual('662f11c8af6870637eab9f0d', new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'))).toBe(false); + expect(isEqual('662f11c8af6870637eab9f0f', new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d'))).toBe(false); + }); + }); + + describe('documents', () => { + const Shop = createTestModel({ + name: 'String', + }); + + it('should compare two documents', async () => { + const shop1 = await Shop.create({ + name: 'Shop', + }); + + const shop2 = await Shop.create({ + name: 'Shop 2', + }); + expect(isEqual(shop1, shop1)).toBe(true); + expect(isEqual(shop1, shop2)).toBe(false); + expect(isEqual(shop2, shop1)).toBe(false); + }); + + it('should compare a document and an object id', async () => { + const shop = await Shop.create({ + name: 'Shop', + }); + + expect(isEqual(shop, new mongoose.Types.ObjectId(String(shop._id)))).toBe(true); + expect(isEqual(shop, new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'))).toBe(false); + + expect(isEqual(new mongoose.Types.ObjectId(String(shop._id)), shop)).toBe(true); + expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), shop)).toBe(false); + }); + + it('should compare a document and a string', async () => { + const shop = await Shop.create({ + name: 'Shop', + }); + + expect(isEqual(shop, shop.id)).toBe(true); + expect(isEqual(shop, '662f11c8af6870637eab9f0f')).toBe(false); + + expect(isEqual(shop.id, shop)).toBe(true); + expect(isEqual('662f11c8af6870637eab9f0f', shop)).toBe(false); + }); + }); +}); diff --git a/services/api/src/utils/__tests__/permissions.js b/services/api/src/utils/__tests__/permissions.js index d7e425bd3..b90973b42 100644 --- a/services/api/src/utils/__tests__/permissions.js +++ b/services/api/src/utils/__tests__/permissions.js @@ -1,23 +1,8 @@ const mongoose = require('mongoose'); const { User } = require('./../../models'); -const { validatePermissions, userHasAccess } = require('../permissions'); - -describe('permissions', () => { - it('validatePermissions', () => { - const validation1 = () => { - return validatePermissions('global', { users: 'read-write' }); - }; - expect(validation1()).toBe(true); - const validation2 = () => { - return validatePermissions('global', { users: 'none' }); - }; - expect(validation2()).toBe(true); - const validation3 = () => { - return validatePermissions('global', { invoices: 'read-write' }); - }; - expect(validation3).toThrow(Error); - }); +const { userHasAccess } = require('../permissions'); +describe('userHasAccess', () => { it('should validate correctly for super admin', async () => { const superAdmin = await User.create({ email: 'admin@permissions.com', @@ -30,13 +15,47 @@ describe('permissions', () => { }, ], }); - expect(userHasAccess(superAdmin, { scope: 'global', permission: 'read', endpoint: 'users' })).toBe(true); + + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'read', + endpoint: 'users', + }) + ).toBe(true); + expect( - userHasAccess(superAdmin, { scope: 'organization', permission: 'read', endpoint: 'users', scopeRef: '123' }) + userHasAccess(superAdmin, { + scope: 'organization', + permission: 'read', + endpoint: 'users', + scopeRef: '123', + }) + ).toBe(true); + + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'read', + endpoint: 'unknown', + }) + ).toBe(false); + + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'write', + endpoint: 'users', + }) + ).toBe(true); + + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'write', + endpoint: 'users', + }) ).toBe(true); - expect(userHasAccess(superAdmin, { scope: 'global', permission: 'read', endpoint: 'unknown' })).toBe(false); - expect(userHasAccess(superAdmin, { scope: 'global', permission: 'write', endpoint: 'users' })).toBe(true); - expect(userHasAccess(superAdmin, { scope: 'global', permission: 'write', endpoint: 'users' })).toBe(true); }); it('should validate correctly for organization admin', async () => { @@ -54,6 +73,7 @@ describe('permissions', () => { }, ], }); + expect( userHasAccess(admin, { scope: 'organization', @@ -62,6 +82,7 @@ describe('permissions', () => { scopeRef: organization1Id, }) ).toBe(true); + expect( userHasAccess(admin, { scope: 'global', @@ -70,6 +91,7 @@ describe('permissions', () => { scopeRef: organization1Id, }) ).toBe(false); + expect( userHasAccess(admin, { scope: 'organization', @@ -78,6 +100,7 @@ describe('permissions', () => { scopeRef: organization2Id, }) ).toBe(false); + expect( userHasAccess(admin, { scope: 'organization', @@ -86,6 +109,7 @@ describe('permissions', () => { scopeRef: organization1Id, }) ).toBe(false); + expect( userHasAccess(admin, { scope: 'organization', @@ -94,6 +118,7 @@ describe('permissions', () => { scopeRef: organization1Id, }) ).toBe(true); + expect( userHasAccess(admin, { scope: 'organization', @@ -102,6 +127,7 @@ describe('permissions', () => { scopeRef: organization2Id, }) ).toBe(false); + expect( userHasAccess(admin, { scope: 'organization', @@ -171,4 +197,32 @@ describe('permissions', () => { }) ).toBe(false); }); + + it('should assume global access', async () => { + const superAdmin = await User.create({ + email: 'admin@permissions.com', + firstName: 'John', + lastName: 'Doe', + roles: [ + { + scope: 'global', + role: 'superAdmin', + }, + ], + }); + + expect( + userHasAccess(superAdmin, { + permission: 'read', + endpoint: 'users', + }) + ).toBe(true); + + expect( + userHasAccess(superAdmin, { + permission: 'read', + endpoint: 'unknown', + }) + ).toBe(false); + }); }); diff --git a/services/api/src/utils/document.js b/services/api/src/utils/document.js new file mode 100644 index 000000000..c098c9a2b --- /dev/null +++ b/services/api/src/utils/document.js @@ -0,0 +1,25 @@ +const mongoose = require('mongoose'); + +// Mongoose provides an "equals" method on both documents and +// ObjectIds, however it does not provide a static method to +// compare two unknown values that may be either, so provide +// it here. +function isEqual(a, b) { + if (a === b) { + return true; + } else if (a instanceof mongoose.Document) { + return a.equals(b); + } else if (b instanceof mongoose.Document) { + return b.equals(a); + } else if (a instanceof mongoose.Types.ObjectId) { + return a.equals(b); + } else if (b instanceof mongoose.Types.ObjectId) { + return b.equals(a); + } else { + return false; + } +} + +module.exports = { + isEqual, +}; diff --git a/services/api/src/utils/middleware/__tests__/permissions.js b/services/api/src/utils/middleware/__tests__/permissions.js index e8bca5de7..07e5abcf0 100644 --- a/services/api/src/utils/middleware/__tests__/permissions.js +++ b/services/api/src/utils/middleware/__tests__/permissions.js @@ -1,7 +1,8 @@ const { requirePermissions } = require('../permissions'); const { createUser, context } = require('../../testing'); +const { Organization } = require('../../../models'); -describe('permissions', () => { +describe('requirePermissions', () => { it('should trigger an error if no auth user is set', async () => { const authUser = await createUser({ email: 'test@test.com', @@ -9,16 +10,19 @@ describe('permissions', () => { const middleware = requirePermissions({ scope: 'global', endpoint: 'users', permission: 'read' }); let ctx; ctx = context({}); - await expect(middleware(ctx)).rejects.toHaveProperty('message', 'This endpoint requires authentication'); + await expect(middleware(ctx)).rejects.toHaveProperty('message', 'This endpoint requires authentication.'); ctx = context({}); ctx.state = { authUser }; await expect(middleware(ctx)).rejects.toHaveProperty( 'message', - "You don't have the right permission for endpoint users (required permission: global/read)" + "You don't have the right permissions (required permission: users.read)." ); }); - it('should allow global role based access control to certain endpoints', async () => { - const superAdminUser = await createUser({ + + it('should allow global access to specific endpoints', async () => { + let ctx; + + const superAdmin = await createUser({ email: 'admin@permissions.com', roles: [ { @@ -27,21 +31,181 @@ describe('permissions', () => { }, ], }); - const plainUser = await createUser({ + const user = await createUser({ email: 'plain@permissions.com', }); - const middleware = requirePermissions({ scope: 'global', endpoint: 'users', permission: 'read' }); + const middleware = requirePermissions({ + scope: 'global', + endpoint: 'users', + permission: 'read', + }); + + ctx = context({}); + await expect(middleware(ctx)).rejects.toHaveProperty('message', 'This endpoint requires authentication.'); + + ctx = context({}); + ctx.state = { authUser: user }; + await expect(middleware(ctx)).rejects.toHaveProperty( + 'message', + "You don't have the right permissions (required permission: users.read)." + ); + + ctx = context({}); + ctx.state = { authUser: superAdmin }; + await expect(middleware(ctx, () => {})).resolves.not.toThrow(); + }); + + it('should allow global access to organization endpoints', async () => { let ctx; + + const organization = await Organization.create({ + name: 'Default Organization', + }); + + const superAdmin = await createUser({ + email: 'admin@permissions.com', + roles: [ + { + scope: 'global', + role: 'superAdmin', + }, + ], + }); + + const user = await createUser({ + email: 'plain@permissions.com', + }); + + const middleware = requirePermissions({ + scope: 'organization', + endpoint: 'users', + permission: 'write', + }); + ctx = context({}); - await expect(middleware(ctx)).rejects.toHaveProperty('message', 'This endpoint requires authentication'); + ctx.state = { + organization, + authUser: superAdmin, + }; + await expect(middleware(ctx, () => {})).resolves.not.toThrow(); + ctx = context({}); - ctx.state = { authUser: plainUser }; + ctx.state = { + organization, + authUser: user, + }; await expect(middleware(ctx)).rejects.toHaveProperty( 'message', - "You don't have the right permission for endpoint users (required permission: global/read)" + "You don't have the right permissions (required permission: users.write)." ); + }); + + it('should test organization access', async () => { + let ctx; + + const organization1 = await Organization.create({ + name: 'Default Organization', + }); + + const organization2 = await Organization.create({ + name: 'Other Organization', + }); + + const admin = await createUser({ + email: 'admin@permissions.com', + roles: [ + { + role: 'admin', + scope: 'organization', + scopeRef: organization1.id, + }, + ], + }); + + const middleware = requirePermissions({ + scope: 'organization', + endpoint: 'users', + permission: 'read', + }); + + ctx = context({}); + ctx.state = { + authUser: admin, + organization: organization1, + }; + await expect(middleware(ctx, () => {})).resolves.not.toThrow(); + ctx = context({}); - ctx.state = { authUser: superAdminUser }; - await expect(middleware(ctx, () => {})).resolves.toBe(undefined); + ctx.state = { + authUser: admin, + organization: organization2, + }; + await expect(middleware(ctx)).rejects.toHaveProperty( + 'message', + "You don't have the right permissions (required permission: users.read)." + ); + }); + + describe('shortcuts', () => { + it('should allow a dot shortcut', async () => { + let ctx; + + const superAdmin = await createUser({ + email: 'admin@permissions.com', + roles: [ + { + scope: 'global', + role: 'superAdmin', + }, + ], + }); + + const middleware = requirePermissions('users.read'); + ctx = context({}); + ctx.state = { authUser: superAdmin }; + await expect(middleware(ctx, () => {})).resolves.not.toThrow(); + }); + + it('should be able to pass a scope', async () => { + let ctx; + + const organization1 = await Organization.create({ + name: 'Default Organization', + }); + + const organization2 = await Organization.create({ + name: 'Other Organization', + }); + + const admin = await createUser({ + email: 'admin@permissions.com', + roles: [ + { + role: 'admin', + scope: 'organization', + scopeRef: organization1.id, + }, + ], + }); + + const middleware = requirePermissions('users.read', 'organization'); + + ctx = context({}); + ctx.state = { + authUser: admin, + organization: organization1, + }; + await expect(middleware(ctx, () => {})).resolves.not.toThrow(); + + ctx = context({}); + ctx.state = { + authUser: admin, + organization: organization2, + }; + await expect(middleware(ctx)).rejects.toHaveProperty( + 'message', + "You don't have the right permissions (required permission: users.read)." + ); + }); }); }); diff --git a/services/api/src/utils/middleware/permissions.js b/services/api/src/utils/middleware/permissions.js index 97a321d28..31a87a2d7 100644 --- a/services/api/src/utils/middleware/permissions.js +++ b/services/api/src/utils/middleware/permissions.js @@ -1,32 +1,43 @@ const { userHasAccess } = require('./../permissions'); -function requirePermissions({ endpoint, permission, scope = 'global' }) { - if (!endpoint || !permission) { - throw new Error('Need endpoint and permission for requirePermissions'); +const DEFAULT_SCOPE = 'global'; + +function requirePermissions(...args) { + const options = resolveOptions(args); + const { endpoint, permission, scope = DEFAULT_SCOPE } = options; + + if (!endpoint) { + throw new Error('Endpoint required.'); + } else if (!permission) { + throw new Error('Permission required.'); } + const fn = async (ctx, next) => { - if (!ctx.state.authUser) { - return ctx.throw(401, `This endpoint requires authentication`); + const { authUser, organization } = ctx.state; + + if (!authUser) { + return ctx.throw(401, 'This endpoint requires authentication.'); + } else if (scope === 'organization' && !organization) { + return ctx.throw(500, 'Organization is required for permissions check.'); } - const hasGlobalAccess = userHasAccess(ctx.state.authUser, { scope: 'global', endpoint, permission }); + + let allowed; if (scope === 'organization') { - if (hasGlobalAccess) return next(); - const organization = ctx.state.organization; - const hasOrganizationAccess = userHasAccess(ctx.state.authUser, { - scope: 'organization', - endpoint, - permission, + allowed = userHasAccess(authUser, { + ...options, scopeRef: organization._id, }); - if (hasOrganizationAccess) return next(); - } else if (scope === 'global') { - if (hasGlobalAccess) return next(); + } else { + allowed = userHasAccess(authUser, options); + } + + if (allowed) { + return next(); + } else { + return ctx.throw(403, `You don't have the right permissions (required permission: ${endpoint}.${permission}).`); } - return ctx.throw( - 403, - `You don't have the right permission for endpoint ${endpoint} (required permission: ${scope}/${permission})` - ); }; + // Allows docs to see the permissions on the middleware // layer to generate an OpenApi entry for it. fn.permissions = { @@ -37,6 +48,19 @@ function requirePermissions({ endpoint, permission, scope = 'global' }) { return fn; } +function resolveOptions(args) { + if (typeof args[0] === 'string') { + const [endpoint, permission] = args[0].split('.'); + return { + endpoint, + permission, + scope: args[1], + }; + } else { + return args[0]; + } +} + module.exports = { requirePermissions, }; diff --git a/services/api/src/utils/permissions.js b/services/api/src/utils/permissions.js index 0e7ff849b..30003114a 100644 --- a/services/api/src/utils/permissions.js +++ b/services/api/src/utils/permissions.js @@ -1,95 +1,45 @@ -const { serializeDocument } = require('./serialize'); const roleDefinitions = require('../roles.json'); -const endpointDefinitions = require('../permissions.json'); -const endpoints = Object.keys(endpointDefinitions); -const validScopes = ['global', 'organization']; -const permissionValues = ['none', 'read', 'read-write']; -const permissionDefaultValue = 'none'; - -function permissionWithinBounds(maximumValue, value) { - if (maximumValue === 'none' && value === 'none') return true; - if (maximumValue === 'read' && value === 'none') return true; - if (maximumValue === 'read' && value === 'read') return true; - if (maximumValue === 'read-write' && value === 'none') return true; - if (maximumValue === 'read-write' && value === 'read') return true; - if (maximumValue === 'read-write' && value === 'read-write') return true; - return false; -} - -function validatePermissions(scope, permissions) { - const endpoints = Object.keys(permissions); - for (const endpoint of endpoints) { - if (endpoint[0] === '$') continue; - const permissionValue = permissions[endpoint] || 'none'; - const maximumPermissionValue = endpointDefinitions[endpoint].maximums[scope] || 'none'; - if (!permissionWithinBounds(maximumPermissionValue, permissionValue)) { - throw new Error( - `Permission ${permissionValue} for endpoint ${endpoint} exceeds maximum permission ${maximumPermissionValue} in scope ${scope}` - ); - } +const { serializeDocument } = require('./serialize'); +const { isEqual } = require('./document'); + +const VALID_SCOPES = ['global', 'organization']; + +function userHasAccess(user, options) { + const { endpoint, permission, scope = 'global', scopeRef } = options; + if (!endpoint) { + throw new Error('Expected endpoint (e.g. users)'); + } else if (!permission) { + throw new Error('Expected permission (e.g. read)'); + } else if (!scope) { + throw new Error('Expected scope (e.g. organization)'); + } else if (!VALID_SCOPES.includes(scope)) { + throw new Error('Invalid scope'); } - return true; -} -function createDefaultPermissions() { - const defaultPermissions = {}; - for (const endpoint of endpoints) { - defaultPermissions[endpoint] = { type: String, enum: permissionValues, default: permissionDefaultValue }; - } - return defaultPermissions; -} + return user.roles.some((r) => { + if (scope === 'global' && r.scope !== 'global') { + return false; + } else if (scope === 'organization' && r.scope === 'organization') { + if (!isEqual(r.scopeRef, scopeRef)) { + return false; + } + } -function createMaxPermissions(scope) { - const defaultPermissions = {}; - for (const endpoint of endpoints) { - defaultPermissions[endpoint] = endpointDefinitions[endpoint].maximums[scope] || 'none'; - } - return defaultPermissions; -} + const definition = roleDefinitions[r.role]; + const allowed = definition?.permissions?.[endpoint]; -function meetsLevel(permissionValue, requiredPermission) { - if (permissionValue === 'none') return false; - if (permissionValue === 'read-write' && requiredPermission === 'write') return true; - if (permissionValue === 'read-write' && requiredPermission === 'read') return true; - if (permissionValue === 'read' && requiredPermission === 'read') return true; - return false; -} + if (!definition) { + throw new Error(`Unknown role "${r.role}".`); + } -function userHasAccess(user, { endpoint, permission, scope, scopeRef }) { - if (!endpoint) throw new Error('Expected endpoint (e.g. users)'); - if (!permission) throw new Error('Expected permission (e.g. read)'); - if (!scope) throw new Error('Expected scope (e.g. organization)'); - if (!validScopes.includes(scope)) throw new Error('Invalid scope'); - const roles = []; - // Gather all relevant roles - for (const roleRef of user.roles) { - const roleId = roleRef.role.toString(); - if (roleRef.scope === 'global') { - const role = roleDefinitions[roleId]; - if (!role) continue; - roles.push(role); + if (Array.isArray(allowed)) { + return allowed.includes(permission); + } else if (allowed === permission || allowed === 'all') { + return true; } else { - if (roleRef.scope !== scope) continue; - // Only include scopeRef roles (e.g. matching organization ID) when not global scope - if (scope !== 'global') { - if (!scopeRef) continue; - if (!roleRef.scopeRef) continue; - const roleTargetId = roleRef.scopeRef.toString(); - if (scopeRef.toString() !== roleTargetId) continue; - } - const role = roleDefinitions[roleId]; - if (!role) continue; - roles.push(role); - } - } - let hasAccess = false; - for (const role of roles) { - const permissionValue = role.permissions[endpoint] || 'none'; - if (meetsLevel(permissionValue, permission)) { - hasAccess = true; + return false; } - } - return hasAccess; + }); } function expandRoles(user, ctx) { @@ -106,15 +56,6 @@ function expandRoles(user, ctx) { } module.exports = { - validScopes, - endpointDefinitions, - endpoints, - permissionValues, - createDefaultPermissions, - createMaxPermissions, - meetsLevel, - permissionDefaultValue, - validatePermissions, userHasAccess, expandRoles, }; From 9736184e9e7ec8439fb8f54abaed150b19c692ab Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Mon, 29 Apr 2024 13:06:45 +0800 Subject: [PATCH 2/7] few further tweaks and making multi-tenancy opt-in --- services/api/.env | 3 +++ services/api/src/app.js | 2 ++ .../api/src/utils/middleware/organization.js | 26 +++++++++++++++++++ .../api/src/utils/middleware/organizations.js | 25 ------------------ .../api/src/utils/middleware/permissions.js | 15 ++++++++--- 5 files changed, 43 insertions(+), 28 deletions(-) create mode 100644 services/api/src/utils/middleware/organization.js delete mode 100644 services/api/src/utils/middleware/organizations.js diff --git a/services/api/.env b/services/api/.env index dadf94a73..702c2f00b 100644 --- a/services/api/.env +++ b/services/api/.env @@ -31,6 +31,9 @@ API_URL=http://localhost:2300 UPLOADS_STORE=local UPLOADS_GCS_BUCKET=bedrock-staging-uploads +# Multi-Tenancy +DEFAULT_ORGANIZATION_NAME= + # Sentry Error Tracking SENTRY_DSN= diff --git a/services/api/src/app.js b/services/api/src/app.js index 2eea86952..74a6cd408 100644 --- a/services/api/src/app.js +++ b/services/api/src/app.js @@ -6,6 +6,7 @@ const corsMiddleware = require('./utils/middleware/cors'); const bodyMiddleware = require('./utils/middleware/body'); const recordMiddleware = require('./utils/middleware/record'); const serializeMiddleware = require('./utils/middleware/serialize'); +const organizationMiddleware = require('./utils/middleware/organization'); const { applicationMiddleware } = require('./utils/middleware/application'); const { loadDefinition } = require('./utils/openapi'); const Sentry = require('@sentry/node'); @@ -38,6 +39,7 @@ if (['staging', 'development'].includes(ENV_NAME)) { } app.use(serializeMiddleware); +app.use(organizationMiddleware); // Record middleware must occur before serialization to // derive model names but after errorHandler to capture diff --git a/services/api/src/utils/middleware/organization.js b/services/api/src/utils/middleware/organization.js new file mode 100644 index 000000000..8137c2f2a --- /dev/null +++ b/services/api/src/utils/middleware/organization.js @@ -0,0 +1,26 @@ +const { Organization } = require('../../models'); +const config = require('@bedrockio/config'); + +const DEFAULT_ORGANIZATION_NAME = config.get('DEFAULT_ORGANIZATION_NAME'); + +async function organization(ctx, next) { + const identifier = ctx.request.get('organization') || ''; + + if (identifier) { + const organization = await Organization.findById(identifier); + if (!organization) { + return ctx.throw(404, `Can't find organization with ID or slug ${identifier}`); + } + ctx.state.organization = organization; + } else if (DEFAULT_ORGANIZATION_NAME) { + const organization = await Organization.findOneAndUpdate( + { name: DEFAULT_ORGANIZATION_NAME }, + { name: DEFAULT_ORGANIZATION_NAME }, + { new: true, upsert: true } + ); + ctx.state.organization = organization; + } + return next(); +} + +module.exports = organization; diff --git a/services/api/src/utils/middleware/organizations.js b/services/api/src/utils/middleware/organizations.js deleted file mode 100644 index 45d13ebd1..000000000 --- a/services/api/src/utils/middleware/organizations.js +++ /dev/null @@ -1,25 +0,0 @@ -const { Organization } = require('../../models'); -const config = require('@bedrockio/config'); - -const APP_NAME = config.get('APP_NAME'); - -function fetchOrganization() { - return async (ctx, next) => { - const identifier = ctx.request.get('organization') || ''; - if (identifier) { - const organization = await Organization.findById(identifier); - if (!organization) { - return ctx.throw(404, `Can't find organization with ID or slug ${identifier}`); - } - ctx.state.organization = organization; - } else { - ctx.state.organization = - (await Organization.findOne({ name: APP_NAME })) || (await Organization.create({ name: APP_NAME })); - } - return next(); - }; -} - -module.exports = { - fetchOrganization, -}; diff --git a/services/api/src/utils/middleware/permissions.js b/services/api/src/utils/middleware/permissions.js index 31a87a2d7..e3c52ef9d 100644 --- a/services/api/src/utils/middleware/permissions.js +++ b/services/api/src/utils/middleware/permissions.js @@ -1,10 +1,13 @@ const { userHasAccess } = require('./../permissions'); +// This can be changed to "organization" to quickly enable +// multi-tenancy. Be sure when doing this to lock down global +// permissions such as searching all users, impersonation, etc. const DEFAULT_SCOPE = 'global'; function requirePermissions(...args) { const options = resolveOptions(args); - const { endpoint, permission, scope = DEFAULT_SCOPE } = options; + const { endpoint, permission, scope } = options; if (!endpoint) { throw new Error('Endpoint required.'); @@ -49,16 +52,22 @@ function requirePermissions(...args) { } function resolveOptions(args) { + let options; if (typeof args[0] === 'string') { const [endpoint, permission] = args[0].split('.'); - return { + options = { endpoint, permission, scope: args[1], }; } else { - return args[0]; + options = args[0]; } + + return { + scope: DEFAULT_SCOPE, + ...options, + }; } module.exports = { From a5440a9e958fea91d2c816ea561ad48b8168c16e Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Mon, 29 Apr 2024 13:17:35 +0800 Subject: [PATCH 3/7] further tweaks and refactor web side permissions --- .../api/src/utils/__tests__/permissions.js | 10 + services/api/src/utils/permissions.js | 4 + services/web/src/components/Protected.js | 22 +- services/web/src/layouts/Dashboard.js | 43 ++- .../web/src/modals/OrganizationSelector.js | 18 +- services/web/src/stores/session.js | 28 +- .../src/utils/__tests__/permissions.test.js | 279 ++++++++++++++++++ services/web/src/utils/api/request.js | 6 + services/web/src/utils/organization.js | 20 ++ services/web/src/utils/permissions.js | 92 +++--- 10 files changed, 405 insertions(+), 117 deletions(-) create mode 100644 services/web/src/utils/__tests__/permissions.test.js create mode 100644 services/web/src/utils/organization.js diff --git a/services/api/src/utils/__tests__/permissions.js b/services/api/src/utils/__tests__/permissions.js index b90973b42..372b77e1c 100644 --- a/services/api/src/utils/__tests__/permissions.js +++ b/services/api/src/utils/__tests__/permissions.js @@ -225,4 +225,14 @@ describe('userHasAccess', () => { }) ).toBe(false); }); + + it('should not error when no user passed', async () => { + expect( + userHasAccess(null, { + scope: 'global', + permission: 'read', + endpoint: 'users', + }) + ).toBe(false); + }); }); diff --git a/services/api/src/utils/permissions.js b/services/api/src/utils/permissions.js index 30003114a..055a36625 100644 --- a/services/api/src/utils/permissions.js +++ b/services/api/src/utils/permissions.js @@ -5,6 +5,10 @@ const { isEqual } = require('./document'); const VALID_SCOPES = ['global', 'organization']; function userHasAccess(user, options) { + if (!user) { + return false; + } + const { endpoint, permission, scope = 'global', scopeRef } = options; if (!endpoint) { throw new Error('Expected endpoint (e.g. users)'); diff --git a/services/web/src/components/Protected.js b/services/web/src/components/Protected.js index 7c6524d9e..2d727ae8e 100644 --- a/services/web/src/components/Protected.js +++ b/services/web/src/components/Protected.js @@ -8,9 +8,23 @@ import { userHasAccess } from 'utils/permissions'; export default class Protected extends React.Component { render() { const { user } = this.context; - const { endpoint, permission = 'read', scope = 'global' } = this.props; - const hasAccess = - user && userHasAccess(user, { endpoint, permission, scope }); - return {hasAccess && this.props.children}; + const { + children, + endpoint, + permission = 'read', + scope = 'global', + } = this.props; + + const hasAccess = userHasAccess(user, { + endpoint, + permission, + scope, + }); + + if (hasAccess) { + return children; + } else { + return null; + } } } diff --git a/services/web/src/layouts/Dashboard.js b/services/web/src/layouts/Dashboard.js index 88ade7ccb..234ebc050 100644 --- a/services/web/src/layouts/Dashboard.js +++ b/services/web/src/layouts/Dashboard.js @@ -10,20 +10,19 @@ import Protected from 'components/Protected'; import ThemedImage from 'components/ThemedImage'; import Organization from 'modals/OrganizationSelector'; - import ConnectionError from 'components/ConnectionError'; import logo from 'assets/logo.svg'; import darkLogo from 'assets/logo-inverted.svg'; import favicon from 'assets/favicon.svg'; -import { userCanSwitchOrganizations, userHasAccess } from 'utils/permissions'; +import { userCanSwitchOrganizations } from 'utils/permissions'; import Sidebar from './Sidebar'; @withSession export default class DashboardLayout extends React.Component { render() { - const { user, getOrganization } = this.context; + const { user, organization } = this.context; return ( @@ -39,7 +38,7 @@ export default class DashboardLayout extends React.Component { trigger={
- {getOrganization()?.name || 'Select Organization'} + {organization?.name || 'Select Organization'}
} @@ -84,28 +83,22 @@ export default class DashboardLayout extends React.Component { Settings - {userHasAccess(this.context.user, { - endpoint: 'applications', - permission: 'read', - scope: 'global', - }) && ( - - - - Audit Trail - - - - Applications + + + + Audit Trail + + + + Applications + + + + + API Docs - - - - API Docs - - - - )} + + Log Out diff --git a/services/web/src/modals/OrganizationSelector.js b/services/web/src/modals/OrganizationSelector.js index 02d30ed3d..4195f2bfc 100644 --- a/services/web/src/modals/OrganizationSelector.js +++ b/services/web/src/modals/OrganizationSelector.js @@ -9,19 +9,19 @@ import SearchDropdown from 'components/SearchDropdown'; import { request } from 'utils/api'; import { userHasAccess } from 'utils/permissions'; +import { getOrganization, setOrganization } from 'utils/organization'; @modal @withSession export default class OrganizationSelector extends React.Component { fetchOrganizations = async (body) => { const { user } = this.context; - if ( - userHasAccess(user, { - endpoint: 'organizations', - permission: 'read', - scope: 'global', - }) - ) { + const hasGlobal = userHasAccess(user, { + endpoint: 'organizations', + permission: 'read', + scope: 'global', + }); + if (hasGlobal) { const { data } = await request({ method: 'POST', path: '/1/organizations/search', @@ -39,8 +39,8 @@ export default class OrganizationSelector extends React.Component { }; onChange = (evt, { value }) => { - this.context.setOrganization(value); this.props.close(); + setOrganization(value.id); }; render() { @@ -52,7 +52,7 @@ export default class OrganizationSelector extends React.Component { fluid clearable placeholder="Viewing all organizations" - value={this.context.getOrganization()} + value={getOrganization()} onDataNeeded={this.fetchOrganizations} onChange={this.onChange} /> diff --git a/services/web/src/stores/session.js b/services/web/src/stores/session.js index d341e4b64..9bf72cb90 100644 --- a/services/web/src/stores/session.js +++ b/services/web/src/stores/session.js @@ -3,6 +3,7 @@ import { merge, omit } from 'lodash'; import { withRouter } from 'react-router-dom'; import { request, hasToken, setToken } from 'utils/api'; +import { getOrganization, setOrganization } from 'utils/organization'; import { trackSession } from 'utils/analytics'; import { captureError } from 'utils/sentry'; import { wrapContext } from 'utils/hoc'; @@ -22,8 +23,8 @@ export class SessionProvider extends React.PureComponent { error: null, ready: false, loading: true, - stored: this.loadStored(), organization: null, + stored: this.loadStored(), }; } @@ -191,37 +192,22 @@ export class SessionProvider extends React.PureComponent { // Organizations loadOrganization = async () => { - const organizationId = this.state.stored['organizationId']; - if (organizationId) { + const organization = getOrganization(); + if (organization) { try { const { data } = await request({ method: 'GET', - path: `/1/organizations/${organizationId}`, + path: `/1/organizations/${organization}`, }); return data; } catch (err) { if (err.status < 500) { - this.removeStored('organizationId'); + setOrganization(null); } } } }; - setOrganization = (organization) => { - if (organization) { - this.setStored('organizationId', organization.id); - } else { - this.removeStored('organizationId'); - } - // Organizations may affect the context of all pages as well as - // persistent header/footer so need to do a hard-reload of the app. - window.location.reload(); - }; - - getOrganization = () => { - return this.state.organization; - }; - // Session storage setStored = (key, data) => { @@ -305,8 +291,6 @@ export class SessionProvider extends React.PureComponent { hasRole: this.hasRole, isAdmin: this.isAdmin, pushRedirect: this.pushRedirect, - setOrganization: this.setOrganization, - getOrganization: this.getOrganization, }}> {this.props.children} diff --git a/services/web/src/utils/__tests__/permissions.test.js b/services/web/src/utils/__tests__/permissions.test.js new file mode 100644 index 000000000..a2cc4d1c3 --- /dev/null +++ b/services/web/src/utils/__tests__/permissions.test.js @@ -0,0 +1,279 @@ +const { userHasAccess } = require('../permissions'); + +const organization1Id = '662f11c8af6870637eab9f0f'; +const organization2Id = '662f11c8af6870637eab9f0d'; + +const superAdmin = { + email: 'admin@permissions.com', + firstName: 'John', + lastName: 'Doe', + roles: [ + { + scope: 'global', + role: 'superAdmin', + roleDefinition: { + name: 'Super Admin', + allowScopes: ['global'], + permissions: { + applications: 'all', + auditEntries: 'all', + organizations: 'all', + products: 'all', + roles: 'all', + shops: 'all', + users: 'all', + }, + allowAuthenticationOnRoles: ['admin', 'viewer'], + }, + }, + ], +}; + +const admin = { + email: 'admin@permissions.com', + firstName: 'John', + lastName: 'Doe', + roles: [ + { + scope: 'organization', + role: 'admin', + scopeRef: organization1Id, + roleDefinition: { + name: 'Admin', + allowScopes: ['organization'], + permissions: { + auditEntries: 'read', + applications: 'all', + products: 'all', + roles: 'all', + shops: 'all', + users: 'all', + }, + }, + }, + ], +}; + +const viewer = { + email: 'viewer@permissions.com', + firstName: 'John', + lastName: 'Doe', + roles: [ + { + scope: 'organization', + role: 'viewer', + scopeRef: organization1Id, + roleDefinition: { + name: 'Viewer', + allowScopes: ['organization'], + permissions: { + applications: 'read', + auditEntries: 'read', + products: 'read', + shops: 'read', + users: 'read', + }, + }, + }, + ], +}; + +describe('userHasAccess', () => { + it('should validate correctly for super admin', async () => { + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'read', + endpoint: 'users', + }) + ).toBe(true); + + expect( + userHasAccess(superAdmin, { + scope: 'organization', + permission: 'read', + endpoint: 'users', + scopeRef: '123', + }) + ).toBe(true); + + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'read', + endpoint: 'unknown', + }) + ).toBe(false); + + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'write', + endpoint: 'users', + }) + ).toBe(true); + + expect( + userHasAccess(superAdmin, { + scope: 'global', + permission: 'write', + endpoint: 'users', + }) + ).toBe(true); + }); + + it('should validate correctly for organization admin', async () => { + expect( + userHasAccess(admin, { + scope: 'organization', + permission: 'read', + endpoint: 'users', + scopeRef: organization1Id, + }) + ).toBe(true); + + expect( + userHasAccess(admin, { + scope: 'global', + permission: 'read', + endpoint: 'users', + scopeRef: organization1Id, + }) + ).toBe(false); + + expect( + userHasAccess(admin, { + scope: 'organization', + permission: 'read', + endpoint: 'users', + scopeRef: organization2Id, + }) + ).toBe(false); + + expect( + userHasAccess(admin, { + scope: 'organization', + permission: 'read', + endpoint: 'unknown', + scopeRef: organization1Id, + }) + ).toBe(false); + + expect( + userHasAccess(admin, { + scope: 'organization', + permission: 'write', + endpoint: 'users', + scopeRef: organization1Id, + }) + ).toBe(true); + + expect( + userHasAccess(admin, { + scope: 'organization', + permission: 'write', + endpoint: 'users', + scopeRef: organization2Id, + }) + ).toBe(false); + + expect( + userHasAccess(admin, { + scope: 'organization', + permission: 'read', + endpoint: 'users', + scopeRef: organization2Id, + }) + ).toBe(false); + }); + + it('should validate correctly for organization viewer', async () => { + expect( + userHasAccess(viewer, { + scope: 'global', + permission: 'read', + endpoint: 'users', + }) + ).toBe(false); + expect( + userHasAccess(viewer, { + scope: 'global', + permission: 'read', + endpoint: 'unknown', + }) + ).toBe(false); + expect( + userHasAccess(viewer, { + scope: 'global', + permission: 'write', + endpoint: 'users', + }) + ).toBe(false); + expect( + userHasAccess(viewer, { + scope: 'organization', + permission: 'read', + endpoint: 'users', + scopeRef: organization1Id, + }) + ).toBe(true); + expect( + userHasAccess(viewer, { + scope: 'organization', + permission: 'read', + endpoint: 'users', + scopeRef: organization2Id, + }) + ).toBe(false); + expect( + userHasAccess(viewer, { + scope: 'organization', + permission: 'read', + endpoint: 'unknown', + scopeRef: organization1Id, + }) + ).toBe(false); + expect( + userHasAccess(viewer, { + scope: 'organization', + permission: 'write', + endpoint: 'users', + scopeRef: organization1Id, + }) + ).toBe(false); + expect( + userHasAccess(viewer, { + scope: 'organization', + permission: 'write', + endpoint: 'users', + scopeRef: organization1Id, + }) + ).toBe(false); + }); + + it('should assume global access', async () => { + expect( + userHasAccess(superAdmin, { + permission: 'read', + endpoint: 'users', + }) + ).toBe(true); + + expect( + userHasAccess(superAdmin, { + permission: 'read', + endpoint: 'unknown', + }) + ).toBe(false); + }); + + it('should not error when no user passed', async () => { + expect( + userHasAccess(null, { + scope: 'global', + permission: 'read', + endpoint: 'users', + }) + ).toBe(false); + }); +}); diff --git a/services/web/src/utils/api/request.js b/services/web/src/utils/api/request.js index 07e3aa782..b414e4777 100644 --- a/services/web/src/utils/api/request.js +++ b/services/web/src/utils/api/request.js @@ -1,5 +1,7 @@ import { API_KEY, API_URL } from 'utils/env'; +import { getOrganization } from 'utils/organization'; + import { trackRequest } from '../analytics'; import { fetchWithTimeout } from '../fetch'; import { ApiError, ApiParseError } from './errors'; @@ -12,6 +14,7 @@ export default async function request(options) { let { body } = options; const token = options.token || getToken(); + const organization = getOrganization(); const headers = Object.assign( { @@ -23,6 +26,9 @@ export default async function request(options) { 'Api-Record': 'on', }), 'API-Key': API_KEY, + ...(organization && { + Organization: organization, + }), }, options.headers ); diff --git a/services/web/src/utils/organization.js b/services/web/src/utils/organization.js new file mode 100644 index 000000000..ac68e6838 --- /dev/null +++ b/services/web/src/utils/organization.js @@ -0,0 +1,20 @@ +import { localStorage } from './storage'; + +const KEY = 'organizationId'; + +export function getOrganization() { + return localStorage.getItem(KEY); +} + +export function setOrganization(id) { + console.info('SETTTTTTTTTING', id); + if (id) { + localStorage.setItem(KEY, id); + } else { + localStorage.removeItem(KEY); + } + + // Organizations may affect the context of all pages as well as + // persistent header/footer so need to do a hard-reload of the app. + window.location.reload(); +} diff --git a/services/web/src/utils/permissions.js b/services/web/src/utils/permissions.js index e76d54c91..70398b28e 100644 --- a/services/web/src/utils/permissions.js +++ b/services/web/src/utils/permissions.js @@ -1,70 +1,48 @@ -function meetsLevel(permissionValue, permission) { - if (permissionValue === 'none') { +const VALID_SCOPES = ['global', 'organization']; + +// Note: this function is derived from the API and meant +// to be kept in sync. It is slightly modified to not use +// mongoose utility methods. +export function userHasAccess(user, options) { + if (!user) { return false; } - if (permissionValue === 'read-write' && permission === 'write') { - return true; - } - if (permissionValue === 'read-write' && permission === 'read') { - return true; - } - if (permissionValue === 'read' && permission === 'read') { - return true; - } - return false; -} -export function userHasAccess(user, { endpoint, permission, scope, scopeRef }) { + const { endpoint, permission, scope = 'global', scopeRef } = options; if (!endpoint) { throw new Error('Expected endpoint (e.g. users)'); - } - if (!permission) { + } else if (!permission) { throw new Error('Expected permission (e.g. read)'); + } else if (!scope) { + throw new Error('Expected scope (e.g. organization)'); + } else if (!VALID_SCOPES.includes(scope)) { + throw new Error('Invalid scope'); } - if (!scope) { - throw new Error('Expected scope (e.g. account)'); - } - const roles = []; - // Gather all relevant roles - for (const roleRef of user.roles) { - if (roleRef.scope === 'global') { - const role = roleRef.roleDefinition; - if (!role) { - continue; - } - roles.push(role); - } else { - if (roleRef.scope !== scope) { - continue; - } - // Only include scopeRef roles (e.g. matching organization ID) when not global scope - if (scope !== 'global') { - if (!scopeRef) { - continue; - } - if (!roleRef.scopeRef) { - continue; - } - const roleTargetId = roleRef.scopeRef.toString(); - if (scopeRef.toString() !== roleTargetId) { - continue; - } - } - const role = roleRef.roleDefinition; - if (!role) { - continue; + + return user.roles.some((r) => { + if (scope === 'global' && r.scope !== 'global') { + return false; + } else if (scope === 'organization' && r.scope === 'organization') { + if (r.scopeRef !== scopeRef) { + return false; } - roles.push(role); } - } - let hasAccess = false; - for (const role of roles) { - const permissionValue = role.permissions[endpoint] || 'none'; - if (meetsLevel(permissionValue, permission)) { - hasAccess = true; + + const definition = r.roleDefinition; + const allowed = definition?.permissions?.[endpoint]; + + if (!definition) { + throw new Error(`Unknown role "${r.role}".`); } - } - return hasAccess; + + if (Array.isArray(allowed)) { + return allowed.includes(permission); + } else if (allowed === permission || allowed === 'all') { + return true; + } else { + return false; + } + }); } export function userCanSwitchOrganizations(user) { From ef227b2386555789a526d81a29760de71dd7a55f Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Mon, 29 Apr 2024 13:47:58 +0800 Subject: [PATCH 4/7] validate user roles on create/patch --- services/api/src/models/definitions/user.json | 10 ++-- services/api/src/roles.json | 4 +- services/api/src/routes/__tests__/users.js | 47 +++++++++++++++++-- services/api/src/routes/users.js | 14 +++--- services/api/src/utils/permissions.js | 16 ++++++- 5 files changed, 74 insertions(+), 17 deletions(-) diff --git a/services/api/src/models/definitions/user.json b/services/api/src/models/definitions/user.json index a4a965646..1aead21ce 100644 --- a/services/api/src/models/definitions/user.json +++ b/services/api/src/models/definitions/user.json @@ -40,15 +40,15 @@ }, "scope": { "type": "String", - "required": true - }, - "scopeRef": { - "type": "ObjectId", - "ref": "Organization", + "required": true, "enum": [ "global", "organization" ] + }, + "scopeRef": { + "type": "ObjectId", + "ref": "Organization" } } ], diff --git a/services/api/src/roles.json b/services/api/src/roles.json index d392b1460..aa328a926 100644 --- a/services/api/src/roles.json +++ b/services/api/src/roles.json @@ -22,7 +22,7 @@ "admin": { "name": "Admin", "allowScopes": [ - "organization" + "global" ], "permissions": { "auditEntries": "read", @@ -37,7 +37,7 @@ "viewer": { "name": "Viewer", "allowScopes": [ - "organization" + "global" ], "permissions": { "applications": "read", diff --git a/services/api/src/routes/__tests__/users.js b/services/api/src/routes/__tests__/users.js index 257a6b65c..1ec5e6196 100644 --- a/services/api/src/routes/__tests__/users.js +++ b/services/api/src/routes/__tests__/users.js @@ -99,6 +99,28 @@ describe('/1/users', () => { ); expect(response.status).toBe(403); }); + + it('should error on invalid roles', async () => { + const admin = await createAdminUser(); + const response = await request( + 'POST', + '/1/users', + { + email: 'hello@dominiek.com', + password: 'verysecurepassword', + firstName: 'Mellow', + lastName: 'Yellow', + roles: [ + { + role: 'superAdmin', + scope: 'organization', + }, + ], + }, + { user: admin } + ); + expect(response.status).toBe(400); + }); }); describe('GET /:user', () => { @@ -300,7 +322,7 @@ describe('/1/users', () => { { roles: [ { - role: 'limitedAdmin', + role: 'admin', scope: 'global', }, ], @@ -309,14 +331,33 @@ describe('/1/users', () => { ); expect(response.status).toBe(200); expect(response.body.data.roles.length).toBe(1); - expect(response.body.data.roles[0].role).toBe('limitedAdmin'); + expect(response.body.data.roles[0].role).toBe('admin'); expect(response.body.data.roles[0].scope).toBe('global'); const dbUser = await User.findById(user1.id); expect(dbUser.roles.length).toBe(1); - expect(dbUser.roles[0].role).toBe('limitedAdmin'); + expect(dbUser.roles[0].role).toBe('admin'); expect(dbUser.roles[0].scope).toBe('global'); }); + it('should error on invalid roles', async () => { + const admin = await createAdminUser(); + const user = await createUser({ firstName: 'New', lastName: 'Name' }); + const response = await request( + 'PATCH', + `/1/users/${user.id}`, + { + roles: [ + { + role: 'superAdmin', + scope: 'organization', + }, + ], + }, + { user: admin } + ); + expect(response.status).toBe(400); + }); + it('should strip out reserved fields', async () => { const admin = await createAdmin(); const user1 = await createUser({ firstName: 'New', lastName: 'Name' }); diff --git a/services/api/src/routes/users.js b/services/api/src/routes/users.js index ff7c3c363..bddb8748a 100644 --- a/services/api/src/routes/users.js +++ b/services/api/src/routes/users.js @@ -7,7 +7,7 @@ const { requirePermissions } = require('../utils/middleware/permissions'); const { exportValidation, csvExport } = require('../utils/csv'); const { createImpersonateAuthToken } = require('../utils/auth/tokens'); -const { expandRoles } = require('./../utils/permissions'); +const { expandRoles, validateUserRoles } = require('./../utils/permissions'); const { User } = require('../models'); const roles = require('./../roles.json'); @@ -116,11 +116,13 @@ router validateBody( User.getCreateValidation({ password: yd.string().password(), - }).custom((val) => { - if (!val.email && !val.phone) { - throw new Error('email or phone number is required'); - } }) + .custom((val) => { + if (!val.email && !val.phone) { + throw new Error('email or phone number is required'); + } + }) + .custom(validateUserRoles) ), async (ctx) => { const { email, phone } = ctx.request.body; @@ -142,7 +144,7 @@ router }; } ) - .patch('/:id', validateBody(User.getUpdateValidation()), async (ctx) => { + .patch('/:id', validateBody(User.getUpdateValidation().custom(validateUserRoles)), async (ctx) => { const { user } = ctx.state; const snapshot = new User(user); user.assign(ctx.request.body); diff --git a/services/api/src/utils/permissions.js b/services/api/src/utils/permissions.js index 055a36625..02d05c5ba 100644 --- a/services/api/src/utils/permissions.js +++ b/services/api/src/utils/permissions.js @@ -46,6 +46,19 @@ function userHasAccess(user, options) { }); } +function validateUserRoles(user) { + const { roles = [] } = user; + for (let r of roles) { + const { role, scope } = r; + const definition = roleDefinitions[role]; + if (!definition) { + throw new Error(`Unknown role "${role}".`); + } else if (!definition.allowScopes.includes(scope)) { + throw new Error(`Scope "${scope}" is not allowed on ${role}.`); + } + } +} + function expandRoles(user, ctx) { const { roles = [], ...rest } = serializeDocument(user, ctx); return { @@ -60,6 +73,7 @@ function expandRoles(user, ctx) { } module.exports = { - userHasAccess, expandRoles, + userHasAccess, + validateUserRoles, }; From af5753021e8b79854474e9a525245c3983633005 Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Mon, 29 Apr 2024 14:24:38 +0800 Subject: [PATCH 5/7] removed console log --- services/web/src/utils/organization.js | 1 - 1 file changed, 1 deletion(-) diff --git a/services/web/src/utils/organization.js b/services/web/src/utils/organization.js index ac68e6838..c7086c61b 100644 --- a/services/web/src/utils/organization.js +++ b/services/web/src/utils/organization.js @@ -7,7 +7,6 @@ export function getOrganization() { } export function setOrganization(id) { - console.info('SETTTTTTTTTING', id); if (id) { localStorage.setItem(KEY, id); } else { From 11c71cc0a5bba673d35a3aae5223920d11b47376 Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Fri, 3 May 2024 20:53:51 +0800 Subject: [PATCH 6/7] moved isEqual to @bedrockio/model --- services/api/src/routes/__tests__/users.js | 4 +- services/api/src/routes/uploads.js | 2 +- services/api/src/utils/__tests__/document.js | 83 -------------------- services/api/src/utils/document.js | 25 ------ services/api/src/utils/permissions.js | 2 +- 5 files changed, 4 insertions(+), 112 deletions(-) delete mode 100644 services/api/src/utils/__tests__/document.js delete mode 100644 services/api/src/utils/document.js diff --git a/services/api/src/routes/__tests__/users.js b/services/api/src/routes/__tests__/users.js index 1ec5e6196..2683087a2 100644 --- a/services/api/src/routes/__tests__/users.js +++ b/services/api/src/routes/__tests__/users.js @@ -101,7 +101,7 @@ describe('/1/users', () => { }); it('should error on invalid roles', async () => { - const admin = await createAdminUser(); + const admin = await createAdmin(); const response = await request( 'POST', '/1/users', @@ -340,7 +340,7 @@ describe('/1/users', () => { }); it('should error on invalid roles', async () => { - const admin = await createAdminUser(); + const admin = await createAdmin(); const user = await createUser({ firstName: 'New', lastName: 'Name' }); const response = await request( 'PATCH', diff --git a/services/api/src/routes/uploads.js b/services/api/src/routes/uploads.js index 0ffa83858..a1d543f34 100644 --- a/services/api/src/routes/uploads.js +++ b/services/api/src/routes/uploads.js @@ -4,7 +4,7 @@ const { authenticate } = require('../utils/middleware/authenticate'); const { validateFiles } = require('../utils/middleware/validate'); const { createUploads, getUploadUrl, createUrlStream } = require('../utils/uploads'); const { userHasAccess } = require('./../utils/permissions'); -const { isEqual } = require('../utils/document'); +const { isEqual } = require('@bedrockio/model'); const { Upload } = require('../models'); const router = new Router(); diff --git a/services/api/src/utils/__tests__/document.js b/services/api/src/utils/__tests__/document.js deleted file mode 100644 index b76045f8b..000000000 --- a/services/api/src/utils/__tests__/document.js +++ /dev/null @@ -1,83 +0,0 @@ -const mongoose = require('mongoose'); -const { createTestModel } = require('@bedrockio/model'); -const { isEqual } = require('../document'); - -describe('isEqual', () => { - describe('ids', () => { - it('should compare two object ids', async () => { - expect( - isEqual( - new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), - new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f') - ) - ).toBe(true); - - expect( - isEqual( - new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d'), - new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f') - ) - ).toBe(false); - - expect( - isEqual( - new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), - new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d') - ) - ).toBe(false); - }); - - it('should compare an object id and a string', async () => { - expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), '662f11c8af6870637eab9f0f')).toBe(true); - expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d'), '662f11c8af6870637eab9f0f')).toBe(false); - expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), '662f11c8af6870637eab9f0d')).toBe(false); - - expect(isEqual('662f11c8af6870637eab9f0f', new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'))).toBe(true); - expect(isEqual('662f11c8af6870637eab9f0d', new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'))).toBe(false); - expect(isEqual('662f11c8af6870637eab9f0f', new mongoose.Types.ObjectId('662f11c8af6870637eab9f0d'))).toBe(false); - }); - }); - - describe('documents', () => { - const Shop = createTestModel({ - name: 'String', - }); - - it('should compare two documents', async () => { - const shop1 = await Shop.create({ - name: 'Shop', - }); - - const shop2 = await Shop.create({ - name: 'Shop 2', - }); - expect(isEqual(shop1, shop1)).toBe(true); - expect(isEqual(shop1, shop2)).toBe(false); - expect(isEqual(shop2, shop1)).toBe(false); - }); - - it('should compare a document and an object id', async () => { - const shop = await Shop.create({ - name: 'Shop', - }); - - expect(isEqual(shop, new mongoose.Types.ObjectId(String(shop._id)))).toBe(true); - expect(isEqual(shop, new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'))).toBe(false); - - expect(isEqual(new mongoose.Types.ObjectId(String(shop._id)), shop)).toBe(true); - expect(isEqual(new mongoose.Types.ObjectId('662f11c8af6870637eab9f0f'), shop)).toBe(false); - }); - - it('should compare a document and a string', async () => { - const shop = await Shop.create({ - name: 'Shop', - }); - - expect(isEqual(shop, shop.id)).toBe(true); - expect(isEqual(shop, '662f11c8af6870637eab9f0f')).toBe(false); - - expect(isEqual(shop.id, shop)).toBe(true); - expect(isEqual('662f11c8af6870637eab9f0f', shop)).toBe(false); - }); - }); -}); diff --git a/services/api/src/utils/document.js b/services/api/src/utils/document.js deleted file mode 100644 index c098c9a2b..000000000 --- a/services/api/src/utils/document.js +++ /dev/null @@ -1,25 +0,0 @@ -const mongoose = require('mongoose'); - -// Mongoose provides an "equals" method on both documents and -// ObjectIds, however it does not provide a static method to -// compare two unknown values that may be either, so provide -// it here. -function isEqual(a, b) { - if (a === b) { - return true; - } else if (a instanceof mongoose.Document) { - return a.equals(b); - } else if (b instanceof mongoose.Document) { - return b.equals(a); - } else if (a instanceof mongoose.Types.ObjectId) { - return a.equals(b); - } else if (b instanceof mongoose.Types.ObjectId) { - return b.equals(a); - } else { - return false; - } -} - -module.exports = { - isEqual, -}; diff --git a/services/api/src/utils/permissions.js b/services/api/src/utils/permissions.js index 02d05c5ba..19097390c 100644 --- a/services/api/src/utils/permissions.js +++ b/services/api/src/utils/permissions.js @@ -1,6 +1,6 @@ +const { isEqual } = require('@bedrockio/model'); const roleDefinitions = require('../roles.json'); const { serializeDocument } = require('./serialize'); -const { isEqual } = require('./document'); const VALID_SCOPES = ['global', 'organization']; From f837c7a7f218d32ab98e7b5e98419bf3942ebb3d Mon Sep 17 00:00:00 2001 From: Andrew Plummer Date: Fri, 24 May 2024 12:30:53 +0800 Subject: [PATCH 7/7] tweaked test --- services/api/src/routes/__tests__/uploads.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/services/api/src/routes/__tests__/uploads.js b/services/api/src/routes/__tests__/uploads.js index 8c72349b5..9a6c36c50 100644 --- a/services/api/src/routes/__tests__/uploads.js +++ b/services/api/src/routes/__tests__/uploads.js @@ -41,7 +41,7 @@ describe('/1/uploads', () => { expect(response.body.data.filename).toBe('test.png'); }); - it('should not be able to access private upload as viewer', async () => { + it('should be able to access private upload as viewer', async () => { const manager = await importFixtures('users/jack'); const upload = await createUpload({ private: true, @@ -54,7 +54,8 @@ describe('/1/uploads', () => { user: manager, } ); - expect(response.status).toBe(401); + expect(response.status).toBe(200); + expect(response.body.data.filename).toBe('test.png'); }); it('should not allow access private upload when unauthenticated', async () => {