From 1a0af6daf1562c3c40c054d5807c37f9fa234dab Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 18 Nov 2024 22:13:15 +0000 Subject: [PATCH 1/9] revert: remove `getByDocId` optional parameter This reverts commit 56f7e18ae618e6d36dcfdf478ffe42f879aecd2e and replaces it with an internal-only function, `getByDocIdIfExists`. --- src/datatype/get-if-exists.js | 22 +++++ src/datatype/index.d.ts | 8 +- src/datatype/index.js | 31 ++---- src/mapeo-project.js | 9 +- src/roles.js | 13 +-- src/translation-api.js | 3 +- test/data-type/get-if-exists.js | 20 ++++ test/{data-type.js => data-type/index.js} | 110 +++------------------- test/data-type/test-helpers.js | 93 ++++++++++++++++++ 9 files changed, 169 insertions(+), 140 deletions(-) create mode 100644 src/datatype/get-if-exists.js create mode 100644 test/data-type/get-if-exists.js rename test/{data-type.js => data-type/index.js} (73%) create mode 100644 test/data-type/test-helpers.js diff --git a/src/datatype/get-if-exists.js b/src/datatype/get-if-exists.js new file mode 100644 index 000000000..b8cd7130d --- /dev/null +++ b/src/datatype/get-if-exists.js @@ -0,0 +1,22 @@ +import { NotFoundError } from '../errors.js' +/** @import { DataStore } from '../datastore/index.js' */ +/** @import { MapeoDocMap, MapeoValueMap } from '../types.js' */ +/** @import { DataType, MapeoDocTables } from './index.js' */ + +/** + * @template {MapeoDocTables} TTable + * @template {TTable['_']['name']} TSchemaName + * @template {MapeoDocMap[TSchemaName]} TDoc + * @template {MapeoValueMap[TSchemaName]} TValue + * @param {DataType} dataType + * @param {string} docId + * @returns {Promise} + */ +export async function getByDocIdIfExists(dataType, docId) { + try { + return await dataType.getByDocId(docId) + } catch (err) { + if (err instanceof NotFoundError) return null + throw err + } +} diff --git a/src/datatype/index.d.ts b/src/datatype/index.d.ts index 39922b265..f7f36a8a9 100644 --- a/src/datatype/index.d.ts +++ b/src/datatype/index.d.ts @@ -18,7 +18,7 @@ import TranslationApi from '../translation-api.js' type MapeoDocTableName = `${MapeoDoc['schemaName']}Table` type GetMapeoDocTables = T[keyof T & MapeoDocTableName] /** Union of Drizzle schema tables that correspond to MapeoDoc types (e.g. excluding backlink tables and other utility tables) */ -type MapeoDocTables = +export type MapeoDocTables = | GetMapeoDocTables | GetMapeoDocTables type MapeoDocTablesMap = { @@ -87,12 +87,8 @@ export class DataType< getByDocId( docId: string, - opts?: { mustBeFound?: true; lang?: string } + opts?: { lang?: string } ): Promise - getByDocId( - docId: string, - opts?: { mustBeFound?: boolean; lang?: string } - ): Promise getByVersionId(versionId: string, opts?: { lang?: string }): Promise diff --git a/src/datatype/index.js b/src/datatype/index.js index 1784987cd..82459ca2c 100644 --- a/src/datatype/index.js +++ b/src/datatype/index.js @@ -164,30 +164,16 @@ export class DataType extends TypedEmitter { } /** - * @overload * @param {string} docId - * @param {object} [options] - * @param {true} [options.mustBeFound] - * @param {string} [options.lang] - * @returns {Promise} - */ - /** - * @param {string} docId - * @param {object} [options] - * @param {boolean} [options.mustBeFound] - * @param {string} [options.lang] - * @returns {Promise} + * @param {{ lang?: string }} [opts] */ - async getByDocId(docId, { mustBeFound = true, lang } = {}) { + async getByDocId(docId, { lang } = {}) { await this.#dataStore.indexer.idle() - const result = this.#sql.getByDocId.get({ docId }) - if (result) { - return this.#translate(deNullify(result), { lang }) - } else if (mustBeFound) { - throw new NotFoundError() - } else { - return null - } + const result = /** @type {undefined | MapeoDoc} */ ( + this.#sql.getByDocId.get({ docId }) + ) + if (!result) throw new NotFoundError() + return this.#translate(deNullify(result), { lang }) } /** @@ -200,7 +186,7 @@ export class DataType extends TypedEmitter { } /** - * @param {any} doc + * @param {MapeoDoc} doc * @param {{ lang?: string }} [opts] */ async #translate(doc, { lang } = {}) { @@ -292,6 +278,7 @@ export class DataType extends TypedEmitter { const doc = { ...existingDoc, updatedAt: new Date().toISOString(), + // @ts-expect-error - TS just doesn't work in this class links: [existingDoc.versionId, ...existingDoc.forks], deleted: true, } diff --git a/src/mapeo-project.js b/src/mapeo-project.js index dcb719373..68e5b92fe 100644 --- a/src/mapeo-project.js +++ b/src/mapeo-project.js @@ -57,6 +57,7 @@ import { IconApi } from './icon-api.js' import { readConfig } from './config-import.js' import TranslationApi from './translation-api.js' import { NotFoundError } from './errors.js' +import { getByDocIdIfExists } from './datatype/get-if-exists.js' /** @import { ProjectSettingsValue } from '@comapeo/schema' */ /** @import { CoreStorage, KeyPair, Namespace, ReplicationStream } from './types.js' */ @@ -600,9 +601,7 @@ export class MapeoProject extends TypedEmitter { async $setProjectSettings(settings) { const { projectSettings } = this.#dataTypes - const existing = await projectSettings.getByDocId(this.#projectId, { - mustBeFound: false, - }) + const existing = await getByDocIdIfExists(projectSettings, this.#projectId) if (existing) { return extractEditableProjectSettings( @@ -710,9 +709,7 @@ export class MapeoProject extends TypedEmitter { schemaName: /** @type {const} */ ('deviceInfo'), } - const existingDoc = await deviceInfo.getByDocId(configCoreId, { - mustBeFound: false, - }) + const existingDoc = await getByDocIdIfExists(deviceInfo, configCoreId) if (existingDoc) { return await deviceInfo.update(existingDoc.versionId, doc) } else { diff --git a/src/roles.js b/src/roles.js index a7fc8dfb3..261eaf5d5 100644 --- a/src/roles.js +++ b/src/roles.js @@ -2,6 +2,7 @@ import { currentSchemaVersions } from '@comapeo/schema' import mapObject from 'map-obj' import { kCreateWithDocId, kDataStore } from './datatype/index.js' import { assert, setHas } from './utils.js' +import { getByDocIdIfExists } from './datatype/get-if-exists.js' import { TypedEmitter } from 'tiny-typed-emitter' /** @import { Namespace } from './types.js' */ @@ -269,10 +270,8 @@ export class Roles extends TypedEmitter { * @returns {Promise} */ async getRole(deviceId) { - const roleAssignment = await this.#dataType.getByDocId(deviceId, { - mustBeFound: false, - }) - if (!roleAssignment) { + const roleRecord = await getByDocIdIfExists(this.#dataType, deviceId) + if (!roleRecord) { // The project creator will have the creator role const authCoreId = await this.#coreOwnership.getCoreId(deviceId, 'auth') if (authCoreId === this.#projectCreatorAuthCoreId) { @@ -284,7 +283,7 @@ export class Roles extends TypedEmitter { } } - const { roleId } = roleAssignment + const { roleId } = roleRecord if (!isRoleId(roleId)) { return BLOCKED_ROLE } @@ -386,9 +385,7 @@ export class Roles extends TypedEmitter { } } - const existingRoleDoc = await this.#dataType.getByDocId(deviceId, { - mustBeFound: false, - }) + const existingRoleDoc = await getByDocIdIfExists(this.#dataType, deviceId) if (existingRoleDoc) { await this.#dataType.update( diff --git a/src/translation-api.js b/src/translation-api.js index 636a8a20f..a57ec835a 100644 --- a/src/translation-api.js +++ b/src/translation-api.js @@ -1,5 +1,6 @@ import { and, sql } from 'drizzle-orm' import { kCreateWithDocId, kSelect } from './datatype/index.js' +import { getByDocIdIfExists } from './datatype/get-if-exists.js' import { hashObject } from './utils.js' import { omit } from './lib/omit.js' /** @import { Translation, TranslationValue } from '@comapeo/schema' */ @@ -49,7 +50,7 @@ export default class TranslationApi { async put(value) { const identifiers = omit(value, ['message']) const docId = hashObject(identifiers) - const doc = await this.#dataType.getByDocId(docId, { mustBeFound: false }) + const doc = await getByDocIdIfExists(this.#dataType, docId) if (doc) { return await this.#dataType.update(doc.versionId, value) } else { diff --git a/test/data-type/get-if-exists.js b/test/data-type/get-if-exists.js new file mode 100644 index 000000000..f34021e06 --- /dev/null +++ b/test/data-type/get-if-exists.js @@ -0,0 +1,20 @@ +import { testenv } from './test-helpers.js' +import test, { describe } from 'node:test' +import assert from 'node:assert/strict' +import { getByDocIdIfExists } from '../../src/datatype/get-if-exists.js' +import { valueOf } from '@comapeo/schema' +import { generate } from '@mapeo/mock-data' + +describe('getByDocIdIfExists', () => { + test('resolves with null if no document exists with that ID', async () => { + const { dataType } = await testenv() + assert.equal(await getByDocIdIfExists(dataType, 'foo bar'), null) + }) + + test('resolves with the document if it exists', async () => { + const { dataType } = await testenv() + const fixture = valueOf(generate('observation')[0]) + const observation = await dataType.create(fixture) + assert(await getByDocIdIfExists(dataType, observation.docId)) + }) +}) diff --git a/test/data-type.js b/test/data-type/index.js similarity index 73% rename from test/data-type.js rename to test/data-type/index.js index 1559f7028..00746b449 100644 --- a/test/data-type.js +++ b/test/data-type/index.js @@ -1,25 +1,26 @@ import test from 'node:test' import assert from 'node:assert/strict' -import { DataStore } from '../src/datastore/index.js' +import { DataStore } from '../../src/datastore/index.js' import { createCoreManager, waitForCores, replicate, -} from './helpers/core-manager.js' +} from '../helpers/core-manager.js' import RAM from 'random-access-memory' import crypto from 'hypercore-crypto' -import { observationTable, translationTable } from '../src/schema/project.js' -import { DataType, kCreateWithDocId } from '../src/datatype/index.js' -import { IndexWriter } from '../src/index-writer/index.js' -import { NotFoundError } from '../src/errors.js' +import { observationTable } from '../../src/schema/project.js' +import { DataType, kCreateWithDocId } from '../../src/datatype/index.js' +import { IndexWriter } from '../../src/index-writer/index.js' +import { NotFoundError } from '../../src/errors.js' import Database from 'better-sqlite3' import { drizzle } from 'drizzle-orm/better-sqlite3' import { migrate } from 'drizzle-orm/better-sqlite3/migrator' import { randomBytes } from 'crypto' -import TranslationApi from '../src/translation-api.js' import { getProperty } from 'dot-prop' -import { decode, decodeBlockPrefix, parseVersionId } from '@comapeo/schema' +import { parseVersionId } from '@comapeo/schema' + +import { testenv } from './test-helpers.js' /** @type {import('@comapeo/schema').ObservationValue} */ const obsFixture = { @@ -45,7 +46,8 @@ test('private createWithDocId() method', async () => { const sqlite = new Database(':memory:') const db = drizzle(sqlite) migrate(db, { - migrationsFolder: new URL('../drizzle/project', import.meta.url).pathname, + migrationsFolder: new URL('../../drizzle/project', import.meta.url) + .pathname, }) const coreManager = createCoreManager() @@ -81,7 +83,8 @@ test('private createWithDocId() method throws when doc exists', async () => { const sqlite = new Database(':memory:') const db = drizzle(sqlite) migrate(db, { - migrationsFolder: new URL('../drizzle/project', import.meta.url).pathname, + migrationsFolder: new URL('../../drizzle/project', import.meta.url) + .pathname, }) const coreManager = createCoreManager() @@ -209,14 +212,6 @@ test('getByDocId() throws if no document exists with that ID', async () => { await assert.rejects(() => dataType.getByDocId('foo bar'), NotFoundError) }) -test('getByDocId() can return null if no document exists with that ID', async () => { - const { dataType } = await testenv({ projectKey: randomBytes(32) }) - assert.equal( - await dataType.getByDocId('foo bar', { mustBeFound: false }), - null - ) -}) - test('delete()', async () => { const projectKey = randomBytes(32) const { dataType } = await testenv({ projectKey }) @@ -311,82 +306,3 @@ test('translation', async () => { `not passing a a language code returns the untranslated message` ) }) - -/** - * @param {object} [opts={}] - * @param {Buffer} [opts.projectKey] - */ -async function testenv(opts = {}) { - const sqlite = new Database(':memory:') - const db = drizzle(sqlite) - migrate(db, { - migrationsFolder: new URL('../drizzle/project', import.meta.url).pathname, - }) - - const coreManager = createCoreManager({ ...opts, db }) - - const indexWriter = new IndexWriter({ - tables: [observationTable, translationTable], - sqlite, - }) - - const dataStore = new DataStore({ - coreManager, - namespace: 'data', - batch: async (entries) => indexWriter.batch(entries), - storage: () => new RAM(), - reindex: false, - }) - - const configDataStore = new DataStore({ - coreManager, - namespace: 'config', - batch: async (entries) => { - /** @type {import('multi-core-indexer').Entry[]} */ - const entriesToIndex = [] - for (const entry of entries) { - const { schemaName } = decodeBlockPrefix(entry.block) - try { - if (schemaName === 'translation') { - const doc = decode(entry.block, { - coreDiscoveryKey: entry.key, - index: entry.index, - }) - assert( - doc.schemaName === 'translation', - 'expected a translation doc' - ) - translationApi.index(doc) - } - entriesToIndex.push(entry) - } catch { - // Ignore errors thrown by values that can't be decoded for now - } - } - const indexed = await indexWriter.batch(entriesToIndex) - return indexed - }, - storage: () => new RAM(), - reindex: false, - }) - - const translationDataType = new DataType({ - dataStore: configDataStore, - table: translationTable, - db, - getTranslations: () => { - throw new Error('Cannot get translations for translations') - }, - }) - - const translationApi = new TranslationApi({ dataType: translationDataType }) - - const dataType = new DataType({ - dataStore, - table: observationTable, - db, - getTranslations: translationApi.get.bind(translationApi), - }) - - return { coreManager, dataType, dataStore, translationApi } -} diff --git a/test/data-type/test-helpers.js b/test/data-type/test-helpers.js new file mode 100644 index 000000000..ea361c1d0 --- /dev/null +++ b/test/data-type/test-helpers.js @@ -0,0 +1,93 @@ +import assert from 'node:assert/strict' +import RAM from 'random-access-memory' +import { DataStore } from '../../src/datastore/index.js' +import { DataType } from '../../src/datatype/index.js' +import { IndexWriter } from '../../src/index-writer/index.js' +import { observationTable, translationTable } from '../../src/schema/project.js' +import { createCoreManager } from '../helpers/core-manager.js' + +import { decode, decodeBlockPrefix } from '@comapeo/schema' +import Database from 'better-sqlite3' +import { drizzle } from 'drizzle-orm/better-sqlite3' +import { migrate } from 'drizzle-orm/better-sqlite3/migrator' +import TranslationApi from '../../src/translation-api.js' + +/** + * @param {object} [opts={}] + * @param {Buffer} [opts.projectKey] + */ +export async function testenv(opts = {}) { + const sqlite = new Database(':memory:') + const db = drizzle(sqlite) + migrate(db, { + migrationsFolder: new URL('../../drizzle/project', import.meta.url) + .pathname, + }) + + const coreManager = createCoreManager({ ...opts, db }) + + const indexWriter = new IndexWriter({ + tables: [observationTable, translationTable], + sqlite, + }) + + const dataStore = new DataStore({ + coreManager, + namespace: 'data', + batch: async (entries) => indexWriter.batch(entries), + storage: () => new RAM(), + reindex: false, + }) + + const configDataStore = new DataStore({ + coreManager, + namespace: 'config', + batch: async (entries) => { + /** @type {import('multi-core-indexer').Entry[]} */ + const entriesToIndex = [] + for (const entry of entries) { + const { schemaName } = decodeBlockPrefix(entry.block) + try { + if (schemaName === 'translation') { + const doc = decode(entry.block, { + coreDiscoveryKey: entry.key, + index: entry.index, + }) + assert( + doc.schemaName === 'translation', + 'expected a translation doc' + ) + translationApi.index(doc) + } + entriesToIndex.push(entry) + } catch { + // Ignore errors thrown by values that can't be decoded for now + } + } + const indexed = await indexWriter.batch(entriesToIndex) + return indexed + }, + storage: () => new RAM(), + reindex: false, + }) + + const translationDataType = new DataType({ + dataStore: configDataStore, + table: translationTable, + db, + getTranslations: () => { + throw new Error('Cannot get translations for translations') + }, + }) + + const translationApi = new TranslationApi({ dataType: translationDataType }) + + const dataType = new DataType({ + dataStore, + table: observationTable, + db, + getTranslations: translationApi.get.bind(translationApi), + }) + + return { coreManager, dataType, dataStore, translationApi } +} From 335f04844774a9783c1f8da420c07be57ee5340e Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Tue, 12 Nov 2024 15:15:29 -0600 Subject: [PATCH 2/9] feat: validate role records When fetching a role, we now validate that the role is valid, returning a blocked role if it's not. See [#188] for more details. [0]: https://github.com/digidem/comapeo-core/issues/188 --- src/datatype/get-if-exists.js | 36 ++++-- src/member-api.js | 6 +- src/roles.js | 196 +++++++++++++++++++++++++++++--- test-e2e/members.js | 54 +++++++++ test/data-type/get-if-exists.js | 29 ++++- 5 files changed, 291 insertions(+), 30 deletions(-) diff --git a/src/datatype/get-if-exists.js b/src/datatype/get-if-exists.js index b8cd7130d..4f5f4f98d 100644 --- a/src/datatype/get-if-exists.js +++ b/src/datatype/get-if-exists.js @@ -3,6 +3,20 @@ import { NotFoundError } from '../errors.js' /** @import { MapeoDocMap, MapeoValueMap } from '../types.js' */ /** @import { DataType, MapeoDocTables } from './index.js' */ +/** + * @template T + * @param {() => PromiseLike} fn + * @returns {Promise} + */ +async function nullIfNotFound(fn) { + try { + return await fn() + } catch (err) { + if (err instanceof NotFoundError) return null + throw err + } +} + /** * @template {MapeoDocTables} TTable * @template {TTable['_']['name']} TSchemaName @@ -12,11 +26,17 @@ import { NotFoundError } from '../errors.js' * @param {string} docId * @returns {Promise} */ -export async function getByDocIdIfExists(dataType, docId) { - try { - return await dataType.getByDocId(docId) - } catch (err) { - if (err instanceof NotFoundError) return null - throw err - } -} +export const getByDocIdIfExists = (dataType, docId) => + nullIfNotFound(() => dataType.getByDocId(docId)) + +/** + * @template {MapeoDocTables} TTable + * @template {TTable['_']['name']} TSchemaName + * @template {MapeoDocMap[TSchemaName]} TDoc + * @template {MapeoValueMap[TSchemaName]} TValue + * @param {DataType} dataType + * @param {string} versionId + * @returns {Promise} + */ +export const getByVersionIdIfExists = (dataType, versionId) => + nullIfNotFound(() => dataType.getByVersionId(versionId)) diff --git a/src/member-api.js b/src/member-api.js index 08b64a387..3bcd3da1f 100644 --- a/src/member-api.js +++ b/src/member-api.js @@ -496,10 +496,12 @@ export class MemberApi extends TypedEmitter { /** * @param {string} deviceId * @param {import('./roles.js').RoleIdAssignableToOthers} roleId + * @param {object} [options] + * @param {boolean} [options.__testOnlyAllowAnyRoleToBeAssigned] * @returns {Promise} */ - async assignRole(deviceId, roleId) { - return this.#roles.assignRole(deviceId, roleId) + async assignRole(deviceId, roleId, options) { + return this.#roles.assignRole(deviceId, roleId, options) } } diff --git a/src/roles.js b/src/roles.js index 261eaf5d5..cdc396b73 100644 --- a/src/roles.js +++ b/src/roles.js @@ -1,9 +1,14 @@ -import { currentSchemaVersions } from '@comapeo/schema' +import { currentSchemaVersions, parseVersionId } from '@comapeo/schema' import mapObject from 'map-obj' import { kCreateWithDocId, kDataStore } from './datatype/index.js' import { assert, setHas } from './utils.js' -import { getByDocIdIfExists } from './datatype/get-if-exists.js' +import { + getByDocIdIfExists, + getByVersionIdIfExists, +} from './datatype/get-if-exists.js' import { TypedEmitter } from 'tiny-typed-emitter' +/** @import { Role as RoleRecord } from '@comapeo/schema' */ +/** @import { ReadonlyDeep } from 'type-fest' */ /** @import { Namespace } from './types.js' */ // Randomly generated 8-byte encoded as hex @@ -14,6 +19,8 @@ export const BLOCKED_ROLE_ID = '9e6d29263cba36c9' export const LEFT_ROLE_ID = '8ced989b1904606b' export const NO_ROLE_ID = '08e4251e36f6e7ed' +const CREATOR_ROLE_RECORD = Symbol('creator role assignment') + /** * @typedef {T extends Iterable ? U : never} ElementOf * @template T @@ -270,24 +277,143 @@ export class Roles extends TypedEmitter { * @returns {Promise} */ async getRole(deviceId) { - const roleRecord = await getByDocIdIfExists(this.#dataType, deviceId) - if (!roleRecord) { - // The project creator will have the creator role - const authCoreId = await this.#coreOwnership.getCoreId(deviceId, 'auth') - if (authCoreId === this.#projectCreatorAuthCoreId) { - return CREATOR_ROLE - } else { - // When no role assignment exists, e.g. a newly added device which has - // not yet synced role records. + const roleRecord = await this.#getRoleRecord(deviceId) + + switch (roleRecord) { + case null: return NO_ROLE + case CREATOR_ROLE_RECORD: + return CREATOR_ROLE + default: { + const { roleId } = roleRecord + if (isRoleId(roleId) && (await this.#isRoleChainValid(roleRecord))) { + return ROLES[roleId] + } else { + return BLOCKED_ROLE + } } } + } + + /** + * @param {string} deviceId + * @returns {Promise} + */ + async #getRoleRecord(deviceId) { + const result = await getByDocIdIfExists(this.#dataType, deviceId) + if (result) return result - const { roleId } = roleRecord - if (!isRoleId(roleId)) { - return BLOCKED_ROLE + // The project creator will have the creator role + const authCoreId = await this.#coreOwnership.getCoreId(deviceId, 'auth') + if (authCoreId === this.#projectCreatorAuthCoreId) { + return CREATOR_ROLE_RECORD } - return ROLES[roleId] + + // When no role assignment exists, e.g. a newly added device which has + // not yet synced role records. + return null + } + + /** + * @param {ReadonlyDeep} roleRecord + * @returns {Promise} + */ + async #isRoleChainValid(roleRecord) { + if (roleRecord.roleId === LEFT_ROLE_ID) return true + + /** @type {null | ReadonlyDeep} */ + let currentRoleRecord = roleRecord + + while (currentRoleRecord) { + if (this.#isAssignedByProjectCreator(currentRoleRecord)) { + return true + } + + const parentRoleRecord = await this.#getParentRoleRecord( + currentRoleRecord + ) + switch (parentRoleRecord) { + case null: + break + case CREATOR_ROLE_RECORD: + return true + default: + if ( + !canAssign({ + assigner: parentRoleRecord, + assignee: currentRoleRecord, + }) + ) { + return false + } + break + } + + currentRoleRecord = parentRoleRecord + } + + return false + } + + /** + * @param {ReadonlyDeep>} roleRecord + * @returns {boolean} + */ + #isAssignedByProjectCreator({ versionId }) { + const { coreDiscoveryKey } = parseVersionId(versionId) + const coreDiscoveryKeyString = coreDiscoveryKey.toString('hex') + return coreDiscoveryKeyString === this.#projectCreatorAuthCoreId + } + + /** + * @param {ReadonlyDeep} roleRecord + * @returns {Promise} + */ + async #getParentRoleRecord(roleRecord) { + const { + coreDiscoveryKey: assignerCoreDiscoveryKey, + index: assignerIndexAtAssignmentTime, + } = parseVersionId(roleRecord.versionId) + + const assignerCore = this.#coreManager.getCoreByDiscoveryKey( + assignerCoreDiscoveryKey + ) + if (assignerCore?.namespace !== 'auth') return null + + const assignerCoreId = assignerCore.key.toString('hex') + const assignerDeviceId = await this.#coreOwnership + .getOwner(assignerCoreId) + .catch(() => null) + if (!assignerDeviceId) return null + + const latestRoleRecord = await this.#getRoleRecord(assignerDeviceId) + + let roleRecordToCheck = latestRoleRecord + /** @type {RoleRecord[]} */ const roleRecordsToCheck = [] + while (roleRecordToCheck) { + if ( + roleRecordToCheck === CREATOR_ROLE_RECORD || + (roleRecordToCheck.fromIndex <= assignerIndexAtAssignmentTime && + roleRecordToCheck.versionId !== roleRecord.versionId) + ) { + return roleRecordToCheck + } + + const linkedRoleRecords = await Promise.all( + roleRecordToCheck.links.map((linkedVersionId) => + getByVersionIdIfExists(this.#dataType, linkedVersionId) + ) + ) + for (const linkedRoleRecord of linkedRoleRecords) { + if (linkedRoleRecord) { + roleRecordsToCheck.push(linkedRoleRecord) + } + } + + roleRecordToCheck = roleRecordsToCheck.shift() || null + } + + return null } /** @@ -344,8 +470,14 @@ export class Roles extends TypedEmitter { * * @param {string} deviceId * @param {RoleIdAssignableToAnyone} roleId + * @param {object} [options] + * @param {boolean} [options.__testOnlyAllowAnyRoleToBeAssigned] */ - async assignRole(deviceId, roleId) { + async assignRole( + deviceId, + roleId, + { __testOnlyAllowAnyRoleToBeAssigned = false } = {} + ) { assert( isRoleIdAssignableToAnyone(roleId), `Role ID should be assignable to anyone but got ${roleId}` @@ -368,7 +500,11 @@ export class Roles extends TypedEmitter { } const isAssigningProjectCreatorRole = authCoreId === this.#projectCreatorAuthCoreId - if (isAssigningProjectCreatorRole && !this.#isProjectCreator()) { + if ( + isAssigningProjectCreatorRole && + !this.#isProjectCreator() && + !__testOnlyAllowAnyRoleToBeAssigned + ) { throw new Error( "Only the project creator can assign the project creator's role" ) @@ -380,7 +516,10 @@ export class Roles extends TypedEmitter { } } else { const ownRole = await this.getRole(this.#ownDeviceId) - if (!ownRole.roleAssignment.includes(roleId)) { + if ( + !ownRole.roleAssignment.includes(roleId) && + !__testOnlyAllowAnyRoleToBeAssigned + ) { throw new Error('Lacks permission to assign role ' + roleId) } } @@ -412,3 +551,24 @@ export class Roles extends TypedEmitter { return ownAuthCoreId === this.#projectCreatorAuthCoreId } } + +/** + * @param {object} options + * @param {ReadonlyDeep>} options.assigner + * @param {ReadonlyDeep>} options.assignee + * @returns {boolean} + */ +function canAssign({ assigner, assignee }) { + return ( + isRoleIdAssignableToOthers(assignee.roleId) && + roleRecordToRole(assigner).roleAssignment.includes(assignee.roleId) + ) +} + +/** + * @param {ReadonlyDeep>} roleRecord + * @returns {Role} + */ +function roleRecordToRole({ roleId }) { + return isRoleId(roleId) ? ROLES[roleId] : BLOCKED_ROLE +} diff --git a/test-e2e/members.js b/test-e2e/members.js index bdc0c9b81..9618e4b73 100644 --- a/test-e2e/members.js +++ b/test-e2e/members.js @@ -4,6 +4,7 @@ import { randomBytes } from 'crypto' import { once } from 'node:events' import { + BLOCKED_ROLE_ID, COORDINATOR_ROLE_ID, CREATOR_ROLE, CREATOR_ROLE_ID, @@ -261,6 +262,59 @@ test('roles - creator role and role assignment', async (t) => { ) }) +test('role validation', async (t) => { + const managers = await createManagers(2, t) + const [creator, member] = managers + + const disconnectPeers = connectPeers(managers) + t.after(disconnectPeers) + + const projectId = await creator.createProject({ name: 'role test' }) + await invite({ + projectId, + invitor: creator, + invitees: [member], + roleId: MEMBER_ROLE_ID, + }) + + const projects = await Promise.all( + managers.map((manager) => manager.getProject(projectId)) + ) + const [creatorProject, memberProject] = projects + await waitForSync(projects, 'initial') + + assert.equal( + (await creatorProject.$member.getById(member.deviceId)).role.roleId, + MEMBER_ROLE_ID, + 'test setup: creator sees correct role for member' + ) + + await memberProject.$member.assignRole(member.deviceId, COORDINATOR_ROLE_ID, { + __testOnlyAllowAnyRoleToBeAssigned: true, + }) + await waitForSync(projects, 'initial') + + assert.equal( + (await creatorProject.$member.getById(member.deviceId)).role.roleId, + BLOCKED_ROLE_ID, + "creator sees member's bogus role assignment, and blocks them" + ) + + await creatorProject.$member.assignRole(member.deviceId, COORDINATOR_ROLE_ID) + assert.equal( + (await creatorProject.$member.getById(member.deviceId)).role.roleId, + COORDINATOR_ROLE_ID, + "creator can update the member's role" + ) + + await creatorProject.$member.assignRole(member.deviceId, MEMBER_ROLE_ID) + assert.equal( + (await creatorProject.$member.getById(member.deviceId)).role.roleId, + MEMBER_ROLE_ID, + "creator can update the member's role again" + ) +}) + test('roles - new device without role', async (t) => { const [manager] = await createManagers(1, t) diff --git a/test/data-type/get-if-exists.js b/test/data-type/get-if-exists.js index f34021e06..2a70a751a 100644 --- a/test/data-type/get-if-exists.js +++ b/test/data-type/get-if-exists.js @@ -1,8 +1,11 @@ import { testenv } from './test-helpers.js' import test, { describe } from 'node:test' import assert from 'node:assert/strict' -import { getByDocIdIfExists } from '../../src/datatype/get-if-exists.js' -import { valueOf } from '@comapeo/schema' +import { + getByDocIdIfExists, + getByVersionIdIfExists, +} from '../../src/datatype/get-if-exists.js' +import { getVersionId, parseVersionId, valueOf } from '@comapeo/schema' import { generate } from '@mapeo/mock-data' describe('getByDocIdIfExists', () => { @@ -18,3 +21,25 @@ describe('getByDocIdIfExists', () => { assert(await getByDocIdIfExists(dataType, observation.docId)) }) }) + +describe('getByVersionIdIfExists', () => { + test('resolves with null if no document exists with that ID', async () => { + const { dataType } = await testenv() + + const fixture = valueOf(generate('observation')[0]) + const observation = await dataType.create(fixture) + const bogusVersionId = getVersionId({ + ...parseVersionId(observation.versionId), + index: 9999999, + }) + + assert.equal(await getByVersionIdIfExists(dataType, bogusVersionId), null) + }) + + test('resolves with the document if it exists', async () => { + const { dataType } = await testenv() + const fixture = valueOf(generate('observation')[0]) + const observation = await dataType.create(fixture) + assert(await getByVersionIdIfExists(dataType, observation.versionId)) + }) +}) From a652b0570e7d9efc6d5fd1d11f0d60867ca09b23 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Wed, 20 Nov 2024 16:19:25 +0000 Subject: [PATCH 3/9] nullIfNotFound instead --- src/datatype/get-if-exists.js | 22 ----- src/errors.js | 9 ++ src/mapeo-project.js | 11 ++- src/roles.js | 10 ++- src/translation-api.js | 4 +- test/{data-type/index.js => data-type.js} | 102 +++++++++++++++++++--- test/data-type/get-if-exists.js | 20 ----- test/data-type/test-helpers.js | 93 -------------------- test/errors.js | 12 ++- 9 files changed, 125 insertions(+), 158 deletions(-) delete mode 100644 src/datatype/get-if-exists.js rename test/{data-type/index.js => data-type.js} (75%) delete mode 100644 test/data-type/get-if-exists.js delete mode 100644 test/data-type/test-helpers.js diff --git a/src/datatype/get-if-exists.js b/src/datatype/get-if-exists.js deleted file mode 100644 index b8cd7130d..000000000 --- a/src/datatype/get-if-exists.js +++ /dev/null @@ -1,22 +0,0 @@ -import { NotFoundError } from '../errors.js' -/** @import { DataStore } from '../datastore/index.js' */ -/** @import { MapeoDocMap, MapeoValueMap } from '../types.js' */ -/** @import { DataType, MapeoDocTables } from './index.js' */ - -/** - * @template {MapeoDocTables} TTable - * @template {TTable['_']['name']} TSchemaName - * @template {MapeoDocMap[TSchemaName]} TDoc - * @template {MapeoValueMap[TSchemaName]} TValue - * @param {DataType} dataType - * @param {string} docId - * @returns {Promise} - */ -export async function getByDocIdIfExists(dataType, docId) { - try { - return await dataType.getByDocId(docId) - } catch (err) { - if (err instanceof NotFoundError) return null - throw err - } -} diff --git a/src/errors.js b/src/errors.js index 65bd6c968..38bc87640 100644 --- a/src/errors.js +++ b/src/errors.js @@ -3,3 +3,12 @@ export class NotFoundError extends Error { super(message) } } + +/** + * @param {unknown} err + * @returns {null} + */ +export function nullIfNotFound(err) { + if (err instanceof NotFoundError) return null + throw err +} diff --git a/src/mapeo-project.js b/src/mapeo-project.js index 68e5b92fe..da236e91a 100644 --- a/src/mapeo-project.js +++ b/src/mapeo-project.js @@ -56,8 +56,7 @@ import { Logger } from './logger.js' import { IconApi } from './icon-api.js' import { readConfig } from './config-import.js' import TranslationApi from './translation-api.js' -import { NotFoundError } from './errors.js' -import { getByDocIdIfExists } from './datatype/get-if-exists.js' +import { NotFoundError, nullIfNotFound } from './errors.js' /** @import { ProjectSettingsValue } from '@comapeo/schema' */ /** @import { CoreStorage, KeyPair, Namespace, ReplicationStream } from './types.js' */ @@ -601,7 +600,9 @@ export class MapeoProject extends TypedEmitter { async $setProjectSettings(settings) { const { projectSettings } = this.#dataTypes - const existing = await getByDocIdIfExists(projectSettings, this.#projectId) + const existing = await projectSettings + .getByDocId(this.#projectId) + .catch(nullIfNotFound) if (existing) { return extractEditableProjectSettings( @@ -709,7 +710,9 @@ export class MapeoProject extends TypedEmitter { schemaName: /** @type {const} */ ('deviceInfo'), } - const existingDoc = await getByDocIdIfExists(deviceInfo, configCoreId) + const existingDoc = await deviceInfo + .getByDocId(configCoreId) + .catch(nullIfNotFound) if (existingDoc) { return await deviceInfo.update(existingDoc.versionId, doc) } else { diff --git a/src/roles.js b/src/roles.js index 261eaf5d5..596f8a456 100644 --- a/src/roles.js +++ b/src/roles.js @@ -2,7 +2,7 @@ import { currentSchemaVersions } from '@comapeo/schema' import mapObject from 'map-obj' import { kCreateWithDocId, kDataStore } from './datatype/index.js' import { assert, setHas } from './utils.js' -import { getByDocIdIfExists } from './datatype/get-if-exists.js' +import { nullIfNotFound } from './errors.js' import { TypedEmitter } from 'tiny-typed-emitter' /** @import { Namespace } from './types.js' */ @@ -270,7 +270,9 @@ export class Roles extends TypedEmitter { * @returns {Promise} */ async getRole(deviceId) { - const roleRecord = await getByDocIdIfExists(this.#dataType, deviceId) + const roleRecord = await this.#dataType + .getByDocId(deviceId) + .catch(nullIfNotFound) if (!roleRecord) { // The project creator will have the creator role const authCoreId = await this.#coreOwnership.getCoreId(deviceId, 'auth') @@ -385,7 +387,9 @@ export class Roles extends TypedEmitter { } } - const existingRoleDoc = await getByDocIdIfExists(this.#dataType, deviceId) + const existingRoleDoc = await this.#dataType + .getByDocId(deviceId) + .catch(nullIfNotFound) if (existingRoleDoc) { await this.#dataType.update( diff --git a/src/translation-api.js b/src/translation-api.js index a57ec835a..e32e6fe09 100644 --- a/src/translation-api.js +++ b/src/translation-api.js @@ -1,7 +1,7 @@ import { and, sql } from 'drizzle-orm' import { kCreateWithDocId, kSelect } from './datatype/index.js' -import { getByDocIdIfExists } from './datatype/get-if-exists.js' import { hashObject } from './utils.js' +import { nullIfNotFound } from './errors.js' import { omit } from './lib/omit.js' /** @import { Translation, TranslationValue } from '@comapeo/schema' */ /** @import { SetOptional } from 'type-fest' */ @@ -50,7 +50,7 @@ export default class TranslationApi { async put(value) { const identifiers = omit(value, ['message']) const docId = hashObject(identifiers) - const doc = await getByDocIdIfExists(this.#dataType, docId) + const doc = await this.#dataType.getByDocId(docId).catch(nullIfNotFound) if (doc) { return await this.#dataType.update(doc.versionId, value) } else { diff --git a/test/data-type/index.js b/test/data-type.js similarity index 75% rename from test/data-type/index.js rename to test/data-type.js index 00746b449..cc709a2fb 100644 --- a/test/data-type/index.js +++ b/test/data-type.js @@ -1,26 +1,25 @@ import test from 'node:test' import assert from 'node:assert/strict' -import { DataStore } from '../../src/datastore/index.js' +import { DataStore } from '../src/datastore/index.js' import { createCoreManager, waitForCores, replicate, -} from '../helpers/core-manager.js' +} from './helpers/core-manager.js' import RAM from 'random-access-memory' import crypto from 'hypercore-crypto' -import { observationTable } from '../../src/schema/project.js' -import { DataType, kCreateWithDocId } from '../../src/datatype/index.js' -import { IndexWriter } from '../../src/index-writer/index.js' -import { NotFoundError } from '../../src/errors.js' +import { observationTable, translationTable } from '../src/schema/project.js' +import { DataType, kCreateWithDocId } from '../src/datatype/index.js' +import { IndexWriter } from '../src/index-writer/index.js' +import { NotFoundError } from '../src/errors.js' import Database from 'better-sqlite3' import { drizzle } from 'drizzle-orm/better-sqlite3' import { migrate } from 'drizzle-orm/better-sqlite3/migrator' import { randomBytes } from 'crypto' +import TranslationApi from '../src/translation-api.js' import { getProperty } from 'dot-prop' -import { parseVersionId } from '@comapeo/schema' - -import { testenv } from './test-helpers.js' +import { decode, decodeBlockPrefix, parseVersionId } from '@comapeo/schema' /** @type {import('@comapeo/schema').ObservationValue} */ const obsFixture = { @@ -46,8 +45,7 @@ test('private createWithDocId() method', async () => { const sqlite = new Database(':memory:') const db = drizzle(sqlite) migrate(db, { - migrationsFolder: new URL('../../drizzle/project', import.meta.url) - .pathname, + migrationsFolder: new URL('../drizzle/project', import.meta.url).pathname, }) const coreManager = createCoreManager() @@ -83,8 +81,7 @@ test('private createWithDocId() method throws when doc exists', async () => { const sqlite = new Database(':memory:') const db = drizzle(sqlite) migrate(db, { - migrationsFolder: new URL('../../drizzle/project', import.meta.url) - .pathname, + migrationsFolder: new URL('../drizzle/project', import.meta.url).pathname, }) const coreManager = createCoreManager() @@ -306,3 +303,82 @@ test('translation', async () => { `not passing a a language code returns the untranslated message` ) }) + +/** + * @param {object} [opts={}] + * @param {Buffer} [opts.projectKey] + */ +async function testenv(opts = {}) { + const sqlite = new Database(':memory:') + const db = drizzle(sqlite) + migrate(db, { + migrationsFolder: new URL('../drizzle/project', import.meta.url).pathname, + }) + + const coreManager = createCoreManager({ ...opts, db }) + + const indexWriter = new IndexWriter({ + tables: [observationTable, translationTable], + sqlite, + }) + + const dataStore = new DataStore({ + coreManager, + namespace: 'data', + batch: async (entries) => indexWriter.batch(entries), + storage: () => new RAM(), + reindex: false, + }) + + const configDataStore = new DataStore({ + coreManager, + namespace: 'config', + batch: async (entries) => { + /** @type {import('multi-core-indexer').Entry[]} */ + const entriesToIndex = [] + for (const entry of entries) { + const { schemaName } = decodeBlockPrefix(entry.block) + try { + if (schemaName === 'translation') { + const doc = decode(entry.block, { + coreDiscoveryKey: entry.key, + index: entry.index, + }) + assert( + doc.schemaName === 'translation', + 'expected a translation doc' + ) + translationApi.index(doc) + } + entriesToIndex.push(entry) + } catch { + // Ignore errors thrown by values that can't be decoded for now + } + } + const indexed = await indexWriter.batch(entriesToIndex) + return indexed + }, + storage: () => new RAM(), + reindex: false, + }) + + const translationDataType = new DataType({ + dataStore: configDataStore, + table: translationTable, + db, + getTranslations: () => { + throw new Error('Cannot get translations for translations') + }, + }) + + const translationApi = new TranslationApi({ dataType: translationDataType }) + + const dataType = new DataType({ + dataStore, + table: observationTable, + db, + getTranslations: translationApi.get.bind(translationApi), + }) + + return { coreManager, dataType, dataStore, translationApi } +} diff --git a/test/data-type/get-if-exists.js b/test/data-type/get-if-exists.js deleted file mode 100644 index f34021e06..000000000 --- a/test/data-type/get-if-exists.js +++ /dev/null @@ -1,20 +0,0 @@ -import { testenv } from './test-helpers.js' -import test, { describe } from 'node:test' -import assert from 'node:assert/strict' -import { getByDocIdIfExists } from '../../src/datatype/get-if-exists.js' -import { valueOf } from '@comapeo/schema' -import { generate } from '@mapeo/mock-data' - -describe('getByDocIdIfExists', () => { - test('resolves with null if no document exists with that ID', async () => { - const { dataType } = await testenv() - assert.equal(await getByDocIdIfExists(dataType, 'foo bar'), null) - }) - - test('resolves with the document if it exists', async () => { - const { dataType } = await testenv() - const fixture = valueOf(generate('observation')[0]) - const observation = await dataType.create(fixture) - assert(await getByDocIdIfExists(dataType, observation.docId)) - }) -}) diff --git a/test/data-type/test-helpers.js b/test/data-type/test-helpers.js deleted file mode 100644 index ea361c1d0..000000000 --- a/test/data-type/test-helpers.js +++ /dev/null @@ -1,93 +0,0 @@ -import assert from 'node:assert/strict' -import RAM from 'random-access-memory' -import { DataStore } from '../../src/datastore/index.js' -import { DataType } from '../../src/datatype/index.js' -import { IndexWriter } from '../../src/index-writer/index.js' -import { observationTable, translationTable } from '../../src/schema/project.js' -import { createCoreManager } from '../helpers/core-manager.js' - -import { decode, decodeBlockPrefix } from '@comapeo/schema' -import Database from 'better-sqlite3' -import { drizzle } from 'drizzle-orm/better-sqlite3' -import { migrate } from 'drizzle-orm/better-sqlite3/migrator' -import TranslationApi from '../../src/translation-api.js' - -/** - * @param {object} [opts={}] - * @param {Buffer} [opts.projectKey] - */ -export async function testenv(opts = {}) { - const sqlite = new Database(':memory:') - const db = drizzle(sqlite) - migrate(db, { - migrationsFolder: new URL('../../drizzle/project', import.meta.url) - .pathname, - }) - - const coreManager = createCoreManager({ ...opts, db }) - - const indexWriter = new IndexWriter({ - tables: [observationTable, translationTable], - sqlite, - }) - - const dataStore = new DataStore({ - coreManager, - namespace: 'data', - batch: async (entries) => indexWriter.batch(entries), - storage: () => new RAM(), - reindex: false, - }) - - const configDataStore = new DataStore({ - coreManager, - namespace: 'config', - batch: async (entries) => { - /** @type {import('multi-core-indexer').Entry[]} */ - const entriesToIndex = [] - for (const entry of entries) { - const { schemaName } = decodeBlockPrefix(entry.block) - try { - if (schemaName === 'translation') { - const doc = decode(entry.block, { - coreDiscoveryKey: entry.key, - index: entry.index, - }) - assert( - doc.schemaName === 'translation', - 'expected a translation doc' - ) - translationApi.index(doc) - } - entriesToIndex.push(entry) - } catch { - // Ignore errors thrown by values that can't be decoded for now - } - } - const indexed = await indexWriter.batch(entriesToIndex) - return indexed - }, - storage: () => new RAM(), - reindex: false, - }) - - const translationDataType = new DataType({ - dataStore: configDataStore, - table: translationTable, - db, - getTranslations: () => { - throw new Error('Cannot get translations for translations') - }, - }) - - const translationApi = new TranslationApi({ dataType: translationDataType }) - - const dataType = new DataType({ - dataStore, - table: observationTable, - db, - getTranslations: translationApi.get.bind(translationApi), - }) - - return { coreManager, dataType, dataStore, translationApi } -} diff --git a/test/errors.js b/test/errors.js index 2e68dee80..5bfbd003f 100644 --- a/test/errors.js +++ b/test/errors.js @@ -1,6 +1,6 @@ import test, { describe } from 'node:test' import assert from 'node:assert/strict' -import { NotFoundError } from '../src/errors.js' +import { NotFoundError, nullIfNotFound } from '../src/errors.js' describe('NotFoundError', () => { test('subclasses Error', () => { @@ -15,3 +15,13 @@ describe('NotFoundError', () => { assert.equal(new NotFoundError('foo').message, 'foo') }) }) + +describe('nullIfNotFound', () => { + test('returns null if passed a NotFoundError', () => { + assert.equal(nullIfNotFound(new NotFoundError()), null) + }) + + test('throws if passed something other than a NotFoundError', () => { + assert.throws(() => nullIfNotFound(new Error('foo')), { message: 'foo' }) + }) +}) From 7cd7e7087d5ce83901d1fee96c8173f0df7417b7 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Wed, 20 Nov 2024 16:21:49 +0000 Subject: [PATCH 4/9] More reversions --- src/datatype/index.d.ts | 8 ++++++-- src/datatype/index.js | 31 ++++++++++++++++++++++--------- 2 files changed, 28 insertions(+), 11 deletions(-) diff --git a/src/datatype/index.d.ts b/src/datatype/index.d.ts index f7f36a8a9..39922b265 100644 --- a/src/datatype/index.d.ts +++ b/src/datatype/index.d.ts @@ -18,7 +18,7 @@ import TranslationApi from '../translation-api.js' type MapeoDocTableName = `${MapeoDoc['schemaName']}Table` type GetMapeoDocTables = T[keyof T & MapeoDocTableName] /** Union of Drizzle schema tables that correspond to MapeoDoc types (e.g. excluding backlink tables and other utility tables) */ -export type MapeoDocTables = +type MapeoDocTables = | GetMapeoDocTables | GetMapeoDocTables type MapeoDocTablesMap = { @@ -87,8 +87,12 @@ export class DataType< getByDocId( docId: string, - opts?: { lang?: string } + opts?: { mustBeFound?: true; lang?: string } ): Promise + getByDocId( + docId: string, + opts?: { mustBeFound?: boolean; lang?: string } + ): Promise getByVersionId(versionId: string, opts?: { lang?: string }): Promise diff --git a/src/datatype/index.js b/src/datatype/index.js index 82459ca2c..1784987cd 100644 --- a/src/datatype/index.js +++ b/src/datatype/index.js @@ -164,16 +164,30 @@ export class DataType extends TypedEmitter { } /** + * @overload * @param {string} docId - * @param {{ lang?: string }} [opts] + * @param {object} [options] + * @param {true} [options.mustBeFound] + * @param {string} [options.lang] + * @returns {Promise} + */ + /** + * @param {string} docId + * @param {object} [options] + * @param {boolean} [options.mustBeFound] + * @param {string} [options.lang] + * @returns {Promise} */ - async getByDocId(docId, { lang } = {}) { + async getByDocId(docId, { mustBeFound = true, lang } = {}) { await this.#dataStore.indexer.idle() - const result = /** @type {undefined | MapeoDoc} */ ( - this.#sql.getByDocId.get({ docId }) - ) - if (!result) throw new NotFoundError() - return this.#translate(deNullify(result), { lang }) + const result = this.#sql.getByDocId.get({ docId }) + if (result) { + return this.#translate(deNullify(result), { lang }) + } else if (mustBeFound) { + throw new NotFoundError() + } else { + return null + } } /** @@ -186,7 +200,7 @@ export class DataType extends TypedEmitter { } /** - * @param {MapeoDoc} doc + * @param {any} doc * @param {{ lang?: string }} [opts] */ async #translate(doc, { lang } = {}) { @@ -278,7 +292,6 @@ export class DataType extends TypedEmitter { const doc = { ...existingDoc, updatedAt: new Date().toISOString(), - // @ts-expect-error - TS just doesn't work in this class links: [existingDoc.versionId, ...existingDoc.forks], deleted: true, } From e7b7c7d6816967fc84f9d00375dcd2a0394a897d Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Wed, 20 Nov 2024 16:32:42 +0000 Subject: [PATCH 5/9] Rename "role record" to "membership record" --- src/roles.js | 113 +++++++++++++++++++++++++++------------------------ 1 file changed, 60 insertions(+), 53 deletions(-) diff --git a/src/roles.js b/src/roles.js index 6bcf2d056..2e7dda4c9 100644 --- a/src/roles.js +++ b/src/roles.js @@ -4,7 +4,7 @@ import { kCreateWithDocId, kDataStore } from './datatype/index.js' import { assert, setHas } from './utils.js' import { nullIfNotFound } from './errors.js' import { TypedEmitter } from 'tiny-typed-emitter' -/** @import { Role as RoleRecord } from '@comapeo/schema' */ +/** @import { Role as MembershipRecord } from '@comapeo/schema' */ /** @import { ReadonlyDeep } from 'type-fest' */ /** @import { Namespace } from './types.js' */ @@ -16,7 +16,7 @@ export const BLOCKED_ROLE_ID = '9e6d29263cba36c9' export const LEFT_ROLE_ID = '8ced989b1904606b' export const NO_ROLE_ID = '08e4251e36f6e7ed' -const CREATOR_ROLE_RECORD = Symbol('creator role assignment') +const CREATOR_MEMBERSHIP_RECORD = Symbol('creator role assignment') /** * @typedef {T extends Iterable ? U : never} ElementOf @@ -131,11 +131,11 @@ const BLOCKED_ROLE = { } /** - * This is the role assumed for a device when no role record can be found. This + * This is the role assumed for a device when no membership record can be found. This * can happen when an invited device did not manage to sync with the device that * invited them, and they then try to sync with someone else. We want them to be * able to sync the auth and config store, because that way they may be able to - * receive their role record, and they can get the project config so that they + * receive their membership record, and they can get the project config so that they * can start collecting data. * * @type {Role} @@ -227,7 +227,7 @@ export const ROLES = { /** * @typedef {object} RolesEvents - * @property {(docIds: Set) => void} update Emitted when new role records are indexed + * @property {(docIds: Set) => void} update Emitted when new membership records are indexed */ /** @@ -274,16 +274,19 @@ export class Roles extends TypedEmitter { * @returns {Promise} */ async getRole(deviceId) { - const roleRecord = await this.#getRoleRecord(deviceId) + const membershipRecord = await this.#getMembershipRecord(deviceId) - switch (roleRecord) { + switch (membershipRecord) { case null: return NO_ROLE - case CREATOR_ROLE_RECORD: + case CREATOR_MEMBERSHIP_RECORD: return CREATOR_ROLE default: { - const { roleId } = roleRecord - if (isRoleId(roleId) && (await this.#isRoleChainValid(roleRecord))) { + const { roleId } = membershipRecord + if ( + isRoleId(roleId) && + (await this.#isRoleChainValid(membershipRecord)) + ) { return ROLES[roleId] } else { return BLOCKED_ROLE @@ -294,9 +297,9 @@ export class Roles extends TypedEmitter { /** * @param {string} deviceId - * @returns {Promise} + * @returns {Promise} */ - async #getRoleRecord(deviceId) { + async #getMembershipRecord(deviceId) { const result = await this.#dataType .getByDocId(deviceId) .catch(nullIfNotFound) @@ -305,42 +308,43 @@ export class Roles extends TypedEmitter { // The project creator will have the creator role const authCoreId = await this.#coreOwnership.getCoreId(deviceId, 'auth') if (authCoreId === this.#projectCreatorAuthCoreId) { - return CREATOR_ROLE_RECORD + return CREATOR_MEMBERSHIP_RECORD } // When no role assignment exists, e.g. a newly added device which has - // not yet synced role records. + // not yet synced membership records. return null } /** - * @param {ReadonlyDeep} roleRecord + * @param {ReadonlyDeep} membershipRecord * @returns {Promise} */ - async #isRoleChainValid(roleRecord) { - if (roleRecord.roleId === LEFT_ROLE_ID) return true + async #isRoleChainValid(membershipRecord) { + if (membershipRecord.roleId === LEFT_ROLE_ID) return true - /** @type {null | ReadonlyDeep} */ - let currentRoleRecord = roleRecord + /** @type {null | ReadonlyDeep} */ + let currentMembershipRecord = membershipRecord - while (currentRoleRecord) { - if (this.#isAssignedByProjectCreator(currentRoleRecord)) { + while (currentMembershipRecord) { + // TODO(evanhahn): This may be unnecessary? + if (this.#isAssignedByProjectCreator(currentMembershipRecord)) { return true } - const parentRoleRecord = await this.#getParentRoleRecord( - currentRoleRecord + const parentMembershipRecord = await this.#getParentMembershipRecord( + currentMembershipRecord ) - switch (parentRoleRecord) { + switch (parentMembershipRecord) { case null: break - case CREATOR_ROLE_RECORD: + case CREATOR_MEMBERSHIP_RECORD: return true default: if ( !canAssign({ - assigner: parentRoleRecord, - assignee: currentRoleRecord, + assigner: parentMembershipRecord, + assignee: currentMembershipRecord, }) ) { return false @@ -348,14 +352,14 @@ export class Roles extends TypedEmitter { break } - currentRoleRecord = parentRoleRecord + currentMembershipRecord = parentMembershipRecord } return false } /** - * @param {ReadonlyDeep>} roleRecord + * @param {ReadonlyDeep>} membershipRecord * @returns {boolean} */ #isAssignedByProjectCreator({ versionId }) { @@ -365,14 +369,14 @@ export class Roles extends TypedEmitter { } /** - * @param {ReadonlyDeep} roleRecord - * @returns {Promise} + * @param {ReadonlyDeep} membershipRecord + * @returns {Promise} */ - async #getParentRoleRecord(roleRecord) { + async #getParentMembershipRecord(membershipRecord) { const { coreDiscoveryKey: assignerCoreDiscoveryKey, index: assignerIndexAtAssignmentTime, - } = parseVersionId(roleRecord.versionId) + } = parseVersionId(membershipRecord.versionId) const assignerCore = this.#coreManager.getCoreByDiscoveryKey( assignerCoreDiscoveryKey @@ -385,31 +389,34 @@ export class Roles extends TypedEmitter { .catch(() => null) if (!assignerDeviceId) return null - const latestRoleRecord = await this.#getRoleRecord(assignerDeviceId) + const latestMembershipRecord = await this.#getMembershipRecord( + assignerDeviceId + ) - let roleRecordToCheck = latestRoleRecord - /** @type {RoleRecord[]} */ const roleRecordsToCheck = [] - while (roleRecordToCheck) { + let membershipRecordToCheck = latestMembershipRecord + /** @type {MembershipRecord[]} */ const membershipRecordsToCheck = [] + while (membershipRecordToCheck) { if ( - roleRecordToCheck === CREATOR_ROLE_RECORD || - (roleRecordToCheck.fromIndex <= assignerIndexAtAssignmentTime && - roleRecordToCheck.versionId !== roleRecord.versionId) + membershipRecordToCheck === CREATOR_MEMBERSHIP_RECORD || + (membershipRecordToCheck.fromIndex <= assignerIndexAtAssignmentTime && + membershipRecordToCheck.versionId !== membershipRecord.versionId) ) { - return roleRecordToCheck + return membershipRecordToCheck } - const linkedRoleRecords = await Promise.all( - roleRecordToCheck.links.map((linkedVersionId) => + const linkedMembershipRecords = await Promise.all( + membershipRecordToCheck.links.map((linkedVersionId) => this.#dataType.getByVersionId(linkedVersionId).catch(nullIfNotFound) ) ) - for (const linkedRoleRecord of linkedRoleRecords) { - if (linkedRoleRecord) { - roleRecordsToCheck.push(linkedRoleRecord) + // TODO(evanhahn): rather than this, just find the least permissive one (if possible; then updatedAt, then versionId) + for (const linkedMembershipRecord of linkedMembershipRecords) { + if (linkedMembershipRecord) { + membershipRecordsToCheck.push(linkedMembershipRecord) } } - roleRecordToCheck = roleRecordsToCheck.shift() || null + membershipRecordToCheck = membershipRecordsToCheck.shift() || null } return null @@ -417,7 +424,7 @@ export class Roles extends TypedEmitter { /** * Get roles of all devices in the project. For your own device, if you have - * not yet synced your own role record, the "no role" capabilties is + * not yet synced your own membership record, the "no role" capabilties is * returned. The project creator will have the creator role unless a * different one has been assigned. * @@ -555,21 +562,21 @@ export class Roles extends TypedEmitter { /** * @param {object} options - * @param {ReadonlyDeep>} options.assigner - * @param {ReadonlyDeep>} options.assignee + * @param {ReadonlyDeep>} options.assigner + * @param {ReadonlyDeep>} options.assignee * @returns {boolean} */ function canAssign({ assigner, assignee }) { return ( isRoleIdAssignableToOthers(assignee.roleId) && - roleRecordToRole(assigner).roleAssignment.includes(assignee.roleId) + membershipRecordToRole(assigner).roleAssignment.includes(assignee.roleId) ) } /** - * @param {ReadonlyDeep>} roleRecord + * @param {ReadonlyDeep>} membershipRecord * @returns {Role} */ -function roleRecordToRole({ roleId }) { +function membershipRecordToRole({ roleId }) { return isRoleId(roleId) ? ROLES[roleId] : BLOCKED_ROLE } From 293586769e9ca1af1712b89d567502038feb9cbc Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Wed, 20 Nov 2024 18:04:37 +0000 Subject: [PATCH 6/9] Use least permissive role when finding parent --- package-lock.json | 13 ++++++ package.json | 1 + src/lib/ponyfills.js | 16 +++++++ src/roles.js | 63 ++++++++++++++++++++++------ test/lib/ponyfills.js | 98 ++++++++++++++++++++++++++++--------------- 5 files changed, 145 insertions(+), 46 deletions(-) diff --git a/package-lock.json b/package-lock.json index 7ec004e9e..8ac8d6452 100644 --- a/package-lock.json +++ b/package-lock.json @@ -42,6 +42,7 @@ "multi-core-indexer": "^1.0.0", "p-defer": "^4.0.0", "p-event": "^6.0.1", + "p-reduce": "^3.0.0", "p-timeout": "^6.1.2", "protobufjs": "^7.2.3", "protomux": "^3.4.1", @@ -7337,6 +7338,18 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/p-reduce": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/p-reduce/-/p-reduce-3.0.0.tgz", + "integrity": "sha512-xsrIUgI0Kn6iyDYm9StOpOeK29XM1aboGji26+QEortiFST1hGZaUQOLhtEbqHErPpGW/aSz6allwK2qcptp0Q==", + "license": "MIT", + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, "node_modules/p-timeout": { "version": "6.1.2", "resolved": "https://registry.npmjs.org/p-timeout/-/p-timeout-6.1.2.tgz", diff --git a/package.json b/package.json index dced69c0a..ec0ff9098 100644 --- a/package.json +++ b/package.json @@ -190,6 +190,7 @@ "multi-core-indexer": "^1.0.0", "p-defer": "^4.0.0", "p-event": "^6.0.1", + "p-reduce": "^3.0.0", "p-timeout": "^6.1.2", "protobufjs": "^7.2.3", "protomux": "^3.4.1", diff --git a/src/lib/ponyfills.js b/src/lib/ponyfills.js index daf885577..471e3bf1d 100644 --- a/src/lib/ponyfills.js +++ b/src/lib/ponyfills.js @@ -23,3 +23,19 @@ export function abortSignalAny(iterable) { return controller.signal } + +/** + * Ponyfill of `Set.prototype.isSubsetOf()`. + * + * [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Set/isSubsetOf + * + * @param {ReadonlySet} me + * @param {ReadonlySet} other + * @returns {boolean} + */ +export function setIsSubsetOf(me, other) { + for (const value of me) { + if (!other.has(value)) return false + } + return true +} diff --git a/src/roles.js b/src/roles.js index 2e7dda4c9..72de81a65 100644 --- a/src/roles.js +++ b/src/roles.js @@ -1,9 +1,11 @@ import { currentSchemaVersions, parseVersionId } from '@comapeo/schema' import mapObject from 'map-obj' +import pReduce from 'p-reduce' import { kCreateWithDocId, kDataStore } from './datatype/index.js' import { assert, setHas } from './utils.js' import { nullIfNotFound } from './errors.js' import { TypedEmitter } from 'tiny-typed-emitter' +import { setIsSubsetOf } from './lib/ponyfills.js' /** @import { Role as MembershipRecord } from '@comapeo/schema' */ /** @import { ReadonlyDeep } from 'type-fest' */ /** @import { Namespace } from './types.js' */ @@ -393,8 +395,8 @@ export class Roles extends TypedEmitter { assignerDeviceId ) + /** @type {null | typeof CREATOR_MEMBERSHIP_RECORD | MembershipRecord} */ let membershipRecordToCheck = latestMembershipRecord - /** @type {MembershipRecord[]} */ const membershipRecordsToCheck = [] while (membershipRecordToCheck) { if ( membershipRecordToCheck === CREATOR_MEMBERSHIP_RECORD || @@ -404,19 +406,28 @@ export class Roles extends TypedEmitter { return membershipRecordToCheck } - const linkedMembershipRecords = await Promise.all( - membershipRecordToCheck.links.map((linkedVersionId) => - this.#dataType.getByVersionId(linkedVersionId).catch(nullIfNotFound) - ) + membershipRecordToCheck = await pReduce( + membershipRecord.links, + /** + * @param {null | Readonly} result + * @param {string} linkedVersionId + * @returns {Promise} + */ + async (result, linkedVersionId) => { + const linkedMembershipRecord = await this.#dataType + .getByVersionId(linkedVersionId) + .catch(nullIfNotFound) + if (linkedMembershipRecord && result) { + return chooseLeastPermissiveMembershipRecord( + result, + linkedMembershipRecord + ) + } else { + return linkedMembershipRecord || result + } + }, + null ) - // TODO(evanhahn): rather than this, just find the least permissive one (if possible; then updatedAt, then versionId) - for (const linkedMembershipRecord of linkedMembershipRecords) { - if (linkedMembershipRecord) { - membershipRecordsToCheck.push(linkedMembershipRecord) - } - } - - membershipRecordToCheck = membershipRecordsToCheck.shift() || null } return null @@ -580,3 +591,29 @@ function canAssign({ assigner, assignee }) { function membershipRecordToRole({ roleId }) { return isRoleId(roleId) ? ROLES[roleId] : BLOCKED_ROLE } + +/** + * @param {MembershipRecord} a + * @param {MembershipRecord} b + * @returns {MembershipRecord} + */ +function chooseLeastPermissiveMembershipRecord(a, b) { + const aRoleAssignments = new Set(membershipRecordToRole(a).roleAssignment) + const bRoleAssignments = new Set(membershipRecordToRole(b).roleAssignment) + if (setIsSmallerSubsetOf(aRoleAssignments, bRoleAssignments)) return a + if (setIsSmallerSubsetOf(bRoleAssignments, aRoleAssignments)) return b + + if (a.updatedAt > b.updatedAt) return a + if (b.updatedAt > a.updatedAt) return b + + return a.versionId > b.versionId ? a : b +} + +/** + * @param {ReadonlySet} me + * @param {ReadonlySet} other + * @returns {boolean} + */ +function setIsSmallerSubsetOf(me, other) { + return setIsSubsetOf(me, other) && me.size < other.size +} diff --git a/test/lib/ponyfills.js b/test/lib/ponyfills.js index 34dd2ca99..93741275b 100644 --- a/test/lib/ponyfills.js +++ b/test/lib/ponyfills.js @@ -1,48 +1,80 @@ -import test from 'node:test' +import test, { describe } from 'node:test' import assert from 'node:assert/strict' -import { abortSignalAny } from '../../src/lib/ponyfills.js' +import { abortSignalAny, setIsSubsetOf } from '../../src/lib/ponyfills.js' -test('abortSignalAny() handles empty iterables', () => { - assert.notEqual(abortSignalAny([]).aborted, 'not immediately aborted') - assert.notEqual(abortSignalAny(new Set()).aborted, 'not immediately aborted') -}) +describe('abortSignalAny', () => { + test('handles empty iterables', () => { + assert.notEqual(abortSignalAny([]).aborted, 'not immediately aborted') + assert.notEqual( + abortSignalAny(new Set()).aborted, + 'not immediately aborted' + ) + }) -test('abortSignalAny() aborts immediately if one of the arguments was aborted', () => { - const result = abortSignalAny([ - new AbortController().signal, - AbortSignal.abort('foo'), - AbortSignal.abort('ignored'), - new AbortController().signal, - ]) + test('aborts immediately if one of the arguments was aborted', () => { + const result = abortSignalAny([ + new AbortController().signal, + AbortSignal.abort('foo'), + AbortSignal.abort('ignored'), + new AbortController().signal, + ]) - assert(result.aborted, 'immediately aborted') - assert.equal(result.reason, 'foo', 'gets first abort reason') -}) + assert(result.aborted, 'immediately aborted') + assert.equal(result.reason, 'foo', 'gets first abort reason') + }) + + test('aborts as soon as one of its arguments aborts', () => { + const a = new AbortController() + const b = new AbortController() + const c = new AbortController() + + const result = abortSignalAny([a.signal, b.signal, c.signal]) + + assert.notEqual(result.aborted, 'not immediately aborted') + + b.abort('foo') + c.abort('ignored') -test('abortSignalAny() aborts as soon as one of its arguments aborts', () => { - const a = new AbortController() - const b = new AbortController() - const c = new AbortController() + assert(result.aborted, 'aborted') + assert.equal(result.reason, 'foo', 'gets first abort reason') + }) - const result = abortSignalAny([a.signal, b.signal, c.signal]) + test('handles non-array iterables', () => { + const a = new AbortController() + const b = new AbortController() + const c = new AbortController() - assert.notEqual(result.aborted, 'not immediately aborted') + const result = abortSignalAny(new Set([a.signal, b.signal, c.signal])) - b.abort('foo') - c.abort('ignored') + b.abort('foo') - assert(result.aborted, 'aborted') - assert.equal(result.reason, 'foo', 'gets first abort reason') + assert(result.aborted, 'aborted') + }) }) -test('abortSignalAny() handles non-array iterables', () => { - const a = new AbortController() - const b = new AbortController() - const c = new AbortController() +describe('setIsSubsetOf', () => { + const empty = new Set() + const justTwo = new Set([2]) + const evens = new Set([2, 4, 6]) + const odds = new Set([1, 3, 5]) + const firstSix = new Set([1, 2, 3, 4, 5, 6]) - const result = abortSignalAny(new Set([a.signal, b.signal, c.signal])) + test('sets are subsets of themselves', () => { + for (const set of [empty, justTwo, evens, odds, firstSix]) { + assert(setIsSubsetOf(set, set)) + } + }) - b.abort('foo') + test('returns true for subsets', () => { + assert(setIsSubsetOf(empty, justTwo)) + assert(setIsSubsetOf(justTwo, evens)) + assert(setIsSubsetOf(evens, firstSix)) + assert(setIsSubsetOf(odds, firstSix)) + }) - assert(result.aborted, 'aborted') + test('returns false for non-subsets', () => { + assert(!setIsSubsetOf(justTwo, empty)) + assert(!setIsSubsetOf(firstSix, evens)) + assert(!setIsSubsetOf(evens, odds)) + }) }) From 1bc2aa6a8176c7eacdedbe64b9aa8ac9b60b076f Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Wed, 20 Nov 2024 18:07:03 +0000 Subject: [PATCH 7/9] Simplify "is assigned by project creator" check --- src/roles.js | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-) diff --git a/src/roles.js b/src/roles.js index 72de81a65..29fab799b 100644 --- a/src/roles.js +++ b/src/roles.js @@ -329,11 +329,6 @@ export class Roles extends TypedEmitter { let currentMembershipRecord = membershipRecord while (currentMembershipRecord) { - // TODO(evanhahn): This may be unnecessary? - if (this.#isAssignedByProjectCreator(currentMembershipRecord)) { - return true - } - const parentMembershipRecord = await this.#getParentMembershipRecord( currentMembershipRecord ) @@ -360,16 +355,6 @@ export class Roles extends TypedEmitter { return false } - /** - * @param {ReadonlyDeep>} membershipRecord - * @returns {boolean} - */ - #isAssignedByProjectCreator({ versionId }) { - const { coreDiscoveryKey } = parseVersionId(versionId) - const coreDiscoveryKeyString = coreDiscoveryKey.toString('hex') - return coreDiscoveryKeyString === this.#projectCreatorAuthCoreId - } - /** * @param {ReadonlyDeep} membershipRecord * @returns {Promise} @@ -380,6 +365,11 @@ export class Roles extends TypedEmitter { index: assignerIndexAtAssignmentTime, } = parseVersionId(membershipRecord.versionId) + const isAssignedByProjectCreator = + assignerCoreDiscoveryKey.toString('hex') === + this.#projectCreatorAuthCoreId + if (isAssignedByProjectCreator) return CREATOR_MEMBERSHIP_RECORD + const assignerCore = this.#coreManager.getCoreByDiscoveryKey( assignerCoreDiscoveryKey ) From 136cbba4d604f9642b01f16ada0c9b5528b5ff74 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Thu, 21 Nov 2024 21:18:40 +0000 Subject: [PATCH 8/9] Add iteration limit --- src/roles.js | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/roles.js b/src/roles.js index 29fab799b..157d6131a 100644 --- a/src/roles.js +++ b/src/roles.js @@ -19,6 +19,7 @@ export const LEFT_ROLE_ID = '8ced989b1904606b' export const NO_ROLE_ID = '08e4251e36f6e7ed' const CREATOR_MEMBERSHIP_RECORD = Symbol('creator role assignment') +const ROLE_CHAIN_ITERATION_LIMIT = 1000 /** * @typedef {T extends Iterable ? U : never} ElementOf @@ -328,7 +329,11 @@ export class Roles extends TypedEmitter { /** @type {null | ReadonlyDeep} */ let currentMembershipRecord = membershipRecord - while (currentMembershipRecord) { + for ( + let i = 0; + currentMembershipRecord && i < ROLE_CHAIN_ITERATION_LIMIT; + i++ + ) { const parentMembershipRecord = await this.#getParentMembershipRecord( currentMembershipRecord ) @@ -387,7 +392,11 @@ export class Roles extends TypedEmitter { /** @type {null | typeof CREATOR_MEMBERSHIP_RECORD | MembershipRecord} */ let membershipRecordToCheck = latestMembershipRecord - while (membershipRecordToCheck) { + for ( + let i = 0; + membershipRecordToCheck && i < ROLE_CHAIN_ITERATION_LIMIT; + i++ + ) { if ( membershipRecordToCheck === CREATOR_MEMBERSHIP_RECORD || (membershipRecordToCheck.fromIndex <= assignerIndexAtAssignmentTime && From 07497f35b87c8da87e40c3b2e8a70c31ef77279e Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Thu, 21 Nov 2024 21:20:46 +0000 Subject: [PATCH 9/9] Use nullIfNotFound --- src/roles.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/roles.js b/src/roles.js index 157d6131a..ebab3325b 100644 --- a/src/roles.js +++ b/src/roles.js @@ -383,7 +383,7 @@ export class Roles extends TypedEmitter { const assignerCoreId = assignerCore.key.toString('hex') const assignerDeviceId = await this.#coreOwnership .getOwner(assignerCoreId) - .catch(() => null) + .catch(nullIfNotFound) if (!assignerDeviceId) return null const latestMembershipRecord = await this.#getMembershipRecord(