From ed62b2c8d1da1a3045169d4bce66439b8feefcd0 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Thu, 14 Nov 2024 12:55:59 -0600 Subject: [PATCH 1/4] chore: define `BLOCKED_ROLE` as a constant (#961) This change should have no impact on functionality, and is a minor change. In [an upcoming change][0], I plan to make several references to the blocked role. This defines the `BLOCKED_ROLE` constant for this purpose. [0]: https://github.com/digidem/comapeo-core/issues/188 --- src/roles.js | 53 ++++++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/src/roles.js b/src/roles.js index f3aa6566e..b9bb4811e 100644 --- a/src/roles.js +++ b/src/roles.js @@ -98,6 +98,33 @@ export const CREATOR_ROLE = { }, } +/** + * @type {Role} + */ +const BLOCKED_ROLE = { + roleId: BLOCKED_ROLE_ID, + name: 'Blocked', + docs: mapObject(currentSchemaVersions, (key) => { + return [ + key, + { + readOwn: false, + writeOwn: false, + readOthers: false, + writeOthers: false, + }, + ] + }), + roleAssignment: [], + sync: { + auth: 'blocked', + config: 'blocked', + data: 'blocked', + blobIndex: 'blocked', + blob: 'blocked', + }, +} + /** * This is the role assumed for a device when no role record can be found. This * can happen when an invited device did not manage to sync with the device that @@ -166,29 +193,7 @@ export const ROLES = { blob: 'allowed', }, }, - [BLOCKED_ROLE_ID]: { - roleId: BLOCKED_ROLE_ID, - name: 'Blocked', - docs: mapObject(currentSchemaVersions, (key) => { - return [ - key, - { - readOwn: false, - writeOwn: false, - readOthers: false, - writeOthers: false, - }, - ] - }), - roleAssignment: [], - sync: { - auth: 'blocked', - config: 'blocked', - data: 'blocked', - blobIndex: 'blocked', - blob: 'blocked', - }, - }, + [BLOCKED_ROLE_ID]: BLOCKED_ROLE, [LEFT_ROLE_ID]: { roleId: LEFT_ROLE_ID, name: 'Left', @@ -281,7 +286,7 @@ export class Roles extends TypedEmitter { } } if (!isRoleId(roleId)) { - return ROLES[BLOCKED_ROLE_ID] + return BLOCKED_ROLE } return ROLES[roleId] } From 6a4d0f41f2035c7716f4308378885619bef66217 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Thu, 14 Nov 2024 13:00:05 -0600 Subject: [PATCH 2/4] test: `DataType.prototype.getByVersionId` (#962) This method wasn't tested...now it is! --- test/data-type.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/data-type.js b/test/data-type.js index 297e3c4c0..cc709a2fb 100644 --- a/test/data-type.js +++ b/test/data-type.js @@ -114,6 +114,15 @@ test('private createWithDocId() method throws when doc exists', async () => { ) }) +test('getByVersionId fetches docs by their version ID', async () => { + const { dataType } = await testenv() + + const created = await dataType.create(obsFixture) + const fetched = await dataType.getByVersionId(created.versionId) + + assert.equal(created.docId, fetched.docId) +}) + test('`originalVersionId` field', async () => { const { dataType, dataStore } = await testenv() From 9cfdd0cf195c8d0203f5556dc98881ca8f76e5bc Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 18 Nov 2024 09:21:47 -0600 Subject: [PATCH 3/4] fix: only the project creator can change their role (#960) (This diff looks large, but it's just one line of source code and a bunch of tests.) We have some code like this: ``` if (isAssigningProjectCreatorRole && !this.#isProjectCreator()) { ``` The intent: only allow the project creator to change their own role. However, `this.#isProjectCreator` returned a `Promise`, which meant that the second part of the condition *always* evaluated to `false`, which meant that the whole condition always evaluated to false, which meant that non-creators could change the creator's role. This fixes that by making `#isProjectCreator` return a `boolean`, not `Promise`. Found this while working on [#188]. [#188]: https://github.com/digidem/comapeo-core/issues/188 --- src/roles.js | 2 +- test-e2e/members.js | 136 +++++++++++++++++++++++++++++++++++++++----- 2 files changed, 124 insertions(+), 14 deletions(-) diff --git a/src/roles.js b/src/roles.js index b9bb4811e..f037dc5e6 100644 --- a/src/roles.js +++ b/src/roles.js @@ -408,7 +408,7 @@ export class Roles extends TypedEmitter { } } - async #isProjectCreator() { + #isProjectCreator() { const ownAuthCoreId = this.#coreManager .getWriterCore('auth') .key.toString('hex') diff --git a/test-e2e/members.js b/test-e2e/members.js index b47f61f51..bdc0c9b81 100644 --- a/test-e2e/members.js +++ b/test-e2e/members.js @@ -6,6 +6,7 @@ import { once } from 'node:events' import { COORDINATOR_ROLE_ID, CREATOR_ROLE, + CREATOR_ROLE_ID, ROLES, MEMBER_ROLE_ID, NO_ROLE, @@ -18,6 +19,8 @@ import { waitForSync, } from './utils.js' import { kDataTypes } from '../src/mapeo-project.js' +/** @import { MapeoProject } from '../src/mapeo-project.js' */ +/** @import { RoleId } from '../src/roles.js' */ test('getting yourself after creating project', async (t) => { const [manager] = await createManagers(1, t, 'tablet') @@ -355,8 +358,8 @@ test('roles - getMany() on newly invited device before sync', async (t) => { }) test('roles - assignRole()', async (t) => { - const managers = await createManagers(2, t) - const [invitor, invitee] = managers + const managers = await createManagers(3, t) + const [invitor, invitee, invitee2] = managers const disconnectPeers = connectPeers(managers) t.after(disconnectPeers) @@ -365,7 +368,7 @@ test('roles - assignRole()', async (t) => { await invite({ invitor, projectId, - invitees: [invitee], + invitees: [invitee, invitee2], roleId: MEMBER_ROLE_ID, }) @@ -373,13 +376,40 @@ test('roles - assignRole()', async (t) => { managers.map((m) => m.getProject(projectId)) ) - const [invitorProject, inviteeProject] = projects + const [invitorProject, inviteeProject, invitee2Project] = projects + + /** + * @param {MapeoProject} project + * @param {string} otherDeviceId + * @param {RoleId} expectedRoleId + * @param {string} message + * @returns {Promise} + */ + const assertRole = async ( + project, + otherDeviceId, + expectedRoleId, + message + ) => { + assert.equal( + (await project.$member.getById(otherDeviceId)).role.roleId, + expectedRoleId, + message + ) + } - assert.deepEqual( - (await invitorProject.$member.getById(invitee.deviceId)).role, - ROLES[MEMBER_ROLE_ID], + await assertRole( + invitorProject, + invitee.deviceId, + MEMBER_ROLE_ID, 'invitee has member role from invitor perspective' ) + await assertRole( + invitorProject, + invitee2.deviceId, + MEMBER_ROLE_ID, + 'invitee 2 has member role from invitor perspective' + ) assert.deepEqual( await inviteeProject.$getOwnRole(), @@ -410,9 +440,10 @@ test('roles - assignRole()', async (t) => { await waitForSync(projects, 'initial') - assert.deepEqual( - (await invitorProject.$member.getById(invitee.deviceId)).role, - ROLES[COORDINATOR_ROLE_ID], + await assertRole( + invitorProject, + invitee.deviceId, + COORDINATOR_ROLE_ID, 'invitee now has coordinator role from invitor perspective' ) @@ -447,9 +478,10 @@ test('roles - assignRole()', async (t) => { await waitForSync(projects, 'initial') - assert.deepEqual( - (await invitorProject.$member.getById(invitee.deviceId)).role, - ROLES[MEMBER_ROLE_ID], + await assertRole( + invitorProject, + invitee.deviceId, + MEMBER_ROLE_ID, 'invitee now has member role from invitor perspective' ) @@ -459,6 +491,84 @@ test('roles - assignRole()', async (t) => { 'invitee now has member role from invitee perspective' ) }) + + await t.test( + 'regular members cannot assign roles to coordinator', + async () => { + await Promise.all( + [invitorProject, inviteeProject, invitee2Project].flatMap((project) => [ + assertRole( + project, + invitee.deviceId, + MEMBER_ROLE_ID, + 'test setup: everyone believes invitee 1 is a regular member' + ), + assertRole( + project, + invitee2.deviceId, + MEMBER_ROLE_ID, + 'test setup: everyone believes invitee 2 is a regular member' + ), + ]) + ) + + await assert.rejects(() => + inviteeProject.$member.assignRole(invitee.deviceId, COORDINATOR_ROLE_ID) + ) + await assert.rejects(() => + inviteeProject.$member.assignRole( + invitee2.deviceId, + COORDINATOR_ROLE_ID + ) + ) + + await waitForSync(projects, 'initial') + + await Promise.all( + [invitorProject, inviteeProject, invitee2Project].flatMap((project) => [ + assertRole( + project, + invitee.deviceId, + MEMBER_ROLE_ID, + 'everyone believes invitee 1 is a regular member, even after attempting to assign higher role' + ), + assertRole( + project, + invitee2.deviceId, + MEMBER_ROLE_ID, + 'everyone believes invitee 2 is a regular member, even after attempting to assign higher role' + ), + ]) + ) + } + ) + + await t.test( + 'non-creator members cannot change roles of creator', + async () => { + await invitorProject.$member.assignRole( + invitee.deviceId, + COORDINATOR_ROLE_ID + ) + await waitForSync(projects, 'initial') + + await assert.rejects(() => + inviteeProject.$member.assignRole(invitor.deviceId, COORDINATOR_ROLE_ID) + ) + + await waitForSync(projects, 'initial') + await Promise.all( + [invitorProject, inviteeProject, invitee2Project].map((project) => + assertRole( + project, + invitor.deviceId, + CREATOR_ROLE_ID, + 'everyone still believes creator to be a creator' + ) + ) + ) + } + ) }) test('roles - assignRole() with forked role', async (t) => { From 6d6e7d719a5e7f8ddd7ecc260fc807dea0257e8b Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 18 Nov 2024 10:46:58 -0600 Subject: [PATCH 4/4] test: assert correct number of connected devices (#964) In [#872], we observed a bug where we seemingly had the wrong number of remote sync states. This adds a test assertion that we have the right number. [#872]: https://github.com/digidem/comapeo-core/issues/872 --- test-e2e/sync.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test-e2e/sync.js b/test-e2e/sync.js index 95fb1a43b..72a0bf256 100644 --- a/test-e2e/sync.js +++ b/test-e2e/sync.js @@ -855,6 +855,12 @@ test('Correct sync state prior to data sync', async function (t) { managers.map((m) => m.getProject(projectId)) ) + for (const project of projects) { + const { remoteDeviceSyncState } = project.$sync.getState() + const otherDeviceCount = Object.keys(remoteDeviceSyncState).length + assert.equal(otherDeviceCount, COUNT - 1) + } + const generated = await seedDatabases(projects, { schemas: ['observation'] }) await waitForSync(projects, 'initial')