Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add project.leave() #410

Merged
merged 22 commits into from
Jan 11, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions src/capabilities.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { kCreateWithDocId } from './datatype/index.js'
export const COORDINATOR_ROLE_ID = 'f7c150f5a3a9a855'
export const MEMBER_ROLE_ID = '012fd2d431c0bf60'
export const BLOCKED_ROLE_ID = '9e6d29263cba36c9'
export const LEFT_ROLE_ID = '8ced989b1904606b'

/**
* @typedef {object} DocCapability
Expand All @@ -24,7 +25,7 @@ export const BLOCKED_ROLE_ID = '9e6d29263cba36c9'
*/

/**
* @typedef {typeof COORDINATOR_ROLE_ID | typeof MEMBER_ROLE_ID | typeof BLOCKED_ROLE_ID} RoleId
* @typedef {typeof COORDINATOR_ROLE_ID | typeof MEMBER_ROLE_ID | typeof BLOCKED_ROLE_ID | typeof LEFT_ROLE_ID} RoleId
*/

/**
Expand All @@ -42,7 +43,12 @@ export const CREATOR_CAPABILITIES = {
{ readOwn: true, writeOwn: true, readOthers: true, writeOthers: true },
]
}),
roleAssignment: [COORDINATOR_ROLE_ID, MEMBER_ROLE_ID, BLOCKED_ROLE_ID],
roleAssignment: [
COORDINATOR_ROLE_ID,
MEMBER_ROLE_ID,
BLOCKED_ROLE_ID,
LEFT_ROLE_ID,
],
sync: {
auth: 'allowed',
config: 'allowed',
Expand Down Expand Up @@ -70,7 +76,8 @@ export const NO_ROLE_CAPABILITIES = {
{ readOwn: true, writeOwn: true, readOthers: false, writeOthers: false },
]
}),
roleAssignment: [],
// TODO: Does this make sense?
roleAssignment: [LEFT_ROLE_ID],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I'm looking at this, the more it seems that this change isn't necessary (i.e. NO_ROLE being able to assign LEFT), but would like confirmation before undoing

sync: {
auth: 'allowed',
config: 'allowed',
Expand All @@ -90,7 +97,7 @@ export const DEFAULT_CAPABILITIES = {
{ readOwn: true, writeOwn: true, readOthers: true, writeOthers: false },
]
}),
roleAssignment: [],
roleAssignment: [LEFT_ROLE_ID],
sync: {
auth: 'allowed',
config: 'allowed',
Expand All @@ -107,7 +114,12 @@ export const DEFAULT_CAPABILITIES = {
{ readOwn: true, writeOwn: true, readOthers: true, writeOthers: true },
]
}),
roleAssignment: [COORDINATOR_ROLE_ID, MEMBER_ROLE_ID, BLOCKED_ROLE_ID],
roleAssignment: [
COORDINATOR_ROLE_ID,
MEMBER_ROLE_ID,
BLOCKED_ROLE_ID,
LEFT_ROLE_ID,
],
sync: {
auth: 'allowed',
config: 'allowed',
Expand Down Expand Up @@ -138,6 +150,28 @@ export const DEFAULT_CAPABILITIES = {
blob: 'blocked',
},
},
[LEFT_ROLE_ID]: {
name: 'Left',
docs: mapObject(currentSchemaVersions, (key) => {
return [
key,
{
readOwn: false,
writeOwn: false,
readOthers: false,
writeOthers: false,
},
]
}),
roleAssignment: [],
sync: {
auth: 'allowed',
config: 'blocked',
data: 'blocked',
blobIndex: 'blocked',
blob: 'blocked',
},
},
Comment on lines +142 to +163
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this an accurate representation for this role?

}

export class Capabilities {
Expand Down Expand Up @@ -254,6 +288,10 @@ export class Capabilities {
* @param {keyof typeof DEFAULT_CAPABILITIES} roleId
*/
async assignRole(deviceId, roleId) {
if (deviceId !== this.#ownDeviceId && roleId === LEFT_ROLE_ID) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to do it this way, although it does mean that we need to make it a hard requirement that all capability records can assign this (although there is the open question of whether a blocked user can also leave - they wouldn't be able to leave for other reasons, because they would not be able to write to the project any more).

An alternative approach would be to not include LEFT_ROLE_ID in the roleAssignments property and instead add this check to line 318 (if (!ownCapabilities.roleAssignment.includes(roleId)) {).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed via ca03ef3

throw new Error('Can only assign LEFT role to your own device')
}

let fromIndex = 0
let authCoreId
try {
Expand Down
1 change: 0 additions & 1 deletion src/core-manager/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,6 @@ export class CoreManager extends TypedEmitter {
* Get an array of all cores in the given namespace
*
* @param {Namespace} namespace
* @returns
*/
getCores(namespace) {
return this.#coreIndex.getByNamespace(namespace)
Expand Down
12 changes: 12 additions & 0 deletions src/mapeo-manager.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { Logger } from './logger.js'
import { kSyncState } from './sync/sync-api.js'

/** @typedef {import("@mapeo/schema").ProjectSettingsValue} ProjectValue */
/** @typedef {import('type-fest').SetNonNullable<ProjectKeys, 'encryptionKeys'>} ValidatedProjectKeys */

const CLIENT_SQLITE_FILE_NAME = 'client.db'

Expand Down Expand Up @@ -386,6 +387,7 @@ export class MapeoManager extends TypedEmitter {

/** @param {ProjectKeys} projectKeys */
#createProjectInstance(projectKeys) {
validateProjectKeys(projectKeys)
const projectId = keyToId(projectKeys.projectKey)
return new MapeoProject({
...this.#projectStorage(projectId),
Expand Down Expand Up @@ -699,3 +701,13 @@ function omitPeerProtomux(peers) {
}
)
}

/**
* @param {ProjectKeys} projectKeys
* @returns {asserts projectKeys is ValidatedProjectKeys}
*/
function validateProjectKeys(projectKeys) {
if (!projectKeys.encryptionKeys) {
throw new Error('encryptionKeys should not be undefined')
}
}
118 changes: 114 additions & 4 deletions src/mapeo-project.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@ import { drizzle } from 'drizzle-orm/better-sqlite3'
import { migrate } from 'drizzle-orm/better-sqlite3/migrator'
import { discoveryKey } from 'hypercore-crypto'
import { TypedEmitter } from 'tiny-typed-emitter'
import { eq } from 'drizzle-orm'

import { CoreManager, NAMESPACES } from './core-manager/index.js'
import { DataStore } from './datastore/index.js'
import { DataType, kCreateWithDocId } from './datatype/index.js'
import { BlobStore } from './blob-store/index.js'
import { BlobApi } from './blob-api.js'
import { IndexWriter } from './index-writer/index.js'
import { projectSettingsTable } from './schema/client.js'
import { projectKeysTable, projectSettingsTable } from './schema/client.js'
import {
coreOwnershipTable,
deviceInfoTable,
Expand All @@ -28,9 +29,17 @@ import {
getWinner,
mapAndValidateCoreOwnership,
} from './core-ownership.js'
import { Capabilities } from './capabilities.js'
import {
BLOCKED_ROLE_ID,
COORDINATOR_ROLE_ID,
CREATOR_CAPABILITIES,
Capabilities,
DEFAULT_CAPABILITIES,
LEFT_ROLE_ID,
} from './capabilities.js'
import {
getDeviceId,
projectIdToNonce,
projectKeyToId,
projectKeyToPublicId,
valueOf,
Expand All @@ -39,6 +48,7 @@ import { MemberApi } from './member-api.js'
import { SyncApi, kHandleDiscoveryKey } from './sync/sync-api.js'
import { Logger } from './logger.js'
import { IconApi } from './icon-api.js'
import { ProjectKeys } from './generated/keys.js'

/** @typedef {Omit<import('@mapeo/schema').ProjectSettingsValue, 'schemaName'>} EditableProjectSettings */

Expand Down Expand Up @@ -71,6 +81,10 @@ export class MapeoProject extends TypedEmitter {
#iconApi
#syncApi
#l
#sharedDb
#keyManager
#projectSecretKey
#encryptionKeys

static EMPTY_PROJECT_SETTINGS = EMPTY_PROJECT_SETTINGS

Expand All @@ -81,7 +95,7 @@ export class MapeoProject extends TypedEmitter {
* @param {import('@mapeo/crypto').KeyManager} opts.keyManager mapeo/crypto KeyManager instance
* @param {Buffer} opts.projectKey 32-byte public key of the project creator core
* @param {Buffer} [opts.projectSecretKey] 32-byte secret key of the project creator core
* @param {Partial<Record<import('./core-manager/index.js').Namespace, Buffer>>} [opts.encryptionKeys] Encryption keys for each namespace
* @param {import('./generated/keys.js').EncryptionKeys} opts.encryptionKeys Encryption keys for each namespace
* @param {import('drizzle-orm/better-sqlite3').BetterSQLite3Database} opts.sharedDb
* @param {IndexWriter} opts.sharedIndexWriter
* @param {import('./types.js').CoreStorage} opts.coreStorage Folder to store all hypercore data
Expand All @@ -108,7 +122,11 @@ export class MapeoProject extends TypedEmitter {

this.#l = Logger.create('project', logger)
this.#deviceId = getDeviceId(keyManager)
this.#keyManager = keyManager
this.#projectId = projectKeyToId(projectKey)
this.#projectSecretKey = projectSecretKey
this.#encryptionKeys = encryptionKeys
this.#sharedDb = sharedDb

///////// 1. Setup database
this.#sqlite = new Database(dbPath)
Expand Down Expand Up @@ -250,7 +268,6 @@ export class MapeoProject extends TypedEmitter {
deviceId: this.#deviceId,
capabilities: this.#capabilities,
coreOwnership: this.#coreOwnership,
// @ts-expect-error
encryptionKeys,
projectKey,
rpc: localPeers,
Expand Down Expand Up @@ -548,6 +565,59 @@ export class MapeoProject extends TypedEmitter {
get $icons() {
return this.#iconApi
}

async $leave() {
await assertDeviceCanLeaveProject(this.#deviceId, this.#capabilities)

// update client database (delete all encryption keys except auth)
const encoded = ProjectKeys.encode({
projectKey: Buffer.from(this.#projectId, 'hex'),
projectSecretKey: this.#projectSecretKey,
encryptionKeys: { auth: this.#encryptionKeys.auth },
}).finish()

const nonce = projectIdToNonce(this.#projectId)

// 1. Update relevant shared database tables
this.#sharedDb
.update(projectKeysTable)
.set({
keysCipher: this.#keyManager.encryptLocalMessage(
Buffer.from(encoded.buffer, encoded.byteOffset, encoded.byteLength),
nonce
),
})
.where(eq(projectKeysTable.projectId, this.#projectId))
.run()

this.#sharedDb
.delete(projectSettingsTable)
.where(eq(projectSettingsTable.docId, this.#projectId))
.run()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense but not entirely sure. My assumption is that once you leave a project, you shouldn't be able to access this kind of information, but maybe that's too strict in this case?


// 2. Clear data from cores
// TODO: only clear synced data
const namespacesWithoutAuth =
/** @satisfies {Exclude<import('./core-manager/index.js').Namespace, 'auth'>[]} */ ([
'config',
'data',
'blob',
'blobIndex',
])

await Promise.all(
namespacesWithoutAuth.map((namespace) =>
this.#coreManager.deleteData(namespace, { deleteOwn: true })
)
)

// 3. Clear data from project database
Copy link
Member Author

@achou11 achou11 Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't implemented yet because I have questions about how this should be done. Is it just a naive db.delete(...)? (where db = new Database(this.#sqlite))

Wondering if the deletion of data in sqlite should be handled by something like CoreManager.deleteData()? A little bit tangled with the different boundaries and what should be responsible for this 😅


// TODO: update sync mode to "unsynced-data-background-sync"?

// 4. Assign LEFT role for device
await this.#capabilities.assignRole(this.#deviceId, LEFT_ROLE_ID)
}
}

/**
Expand Down Expand Up @@ -603,3 +673,43 @@ function mapAndValidateDeviceInfo(doc, { coreDiscoveryKey }) {
}
return doc
}

/**
* @param {string} deviceId
* @param {import('./capabilities.js').Capabilities} capabilities
*/
async function assertDeviceCanLeaveProject(deviceId, capabilities) {
const deviceIdToCaps = await capabilities.getAll()

const deviceIdToCapsList = Object.entries(deviceIdToCaps)

if (deviceId in deviceIdToCaps) {
if (deviceIdToCapsList.length === 1) {
throw new Error('Cannot leave a project as the only device')
}

if (
deviceIdToCaps[deviceId].name ===
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this comparison on .name seems fragile - we would need to ensure that we never change .name in the codebase, or things would break.

I think what we need here is to do dataTypes.role.getAll(), which will return an array of records where docId === deviceId and you can read the roleId.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes, makes sense

Copy link
Member Author

@achou11 achou11 Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is getting trickier than expected. feels like I'm doing some logic that's done by Capabilities.getAll() in order to account for edge cases 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

never mind, I figured it out 😄 addressed via 5fab0af

DEFAULT_CAPABILITIES[BLOCKED_ROLE_ID].name
achou11 marked this conversation as resolved.
Show resolved Hide resolved
) {
throw new Error('Cannot leave a project as a blocked device')
}
}

for (const [id, cap] of deviceIdToCapsList) {
// Skip check for device of interest, which is done prior to this loop
if (id === deviceId) continue

// Device is only allowed to leave a project
// if some other known creator or coordinator exists
const isCoordinator =
cap.name === DEFAULT_CAPABILITIES[COORDINATOR_ROLE_ID].name
const isCreator = cap.name === CREATOR_CAPABILITIES.name

if (isCoordinator || isCreator) return
}

throw new Error(
'Cannot leave a project that does not have an external creator or another coordinator'
)
}
Loading