feat: add project.leave() #410

achou11 · 2023-12-07T21:05:28Z

Closes #241

TODO:

Delete indexed data from the project-specific database (will be follow-up)
Update sync mode? (may need a separate PR to introduce said sync mode) (will be follow-up)

achou11 · 2023-12-11T20:58:34Z

@gmaclennan do you mind taking a look at what's currently implemented here and let me know what glaring omissions there are? mostly have uncertainties around the steps that need to happen within the $leave() method (e.g. data deletion)

gmaclennan

This approach is looking correct to me. However we don't currently have a way to assign a role, so it's not exactly working as expected - see #413. Initial fix in #414 but that just causes project.$leave() to throw right now.

This also raises questions for me about the implementation of Capabilities. The idea is that DEFAULT_CAPABILITIES will eventually move into the database, and can be edited by users. I think there are two separate types of roles:

"invitable roles" - e.g. roles that someone with the right capability can assign to a new device in a project. These should also be the only roles that can be assigned with $member.assignRole().
"special roles" used to implement internal details of the capability system - "creator", "blocked device" and "left of own choice". I think those assignments should only be used internally, e.g. we should implement a $member.block() method, and we have this project.$leave() method.

I think we should implement a capabilities.getRoles() function or similar which currently returns the corodinator and member roles, but in the future will read from the database. We can then use this as an assertion in assignRole().

I think maybe we give the "leave" role special treatment - any role can assign the leave role to themselves, apart from a device with the blocked role - hence blocked should be treated as a "special role" with slightly different rules.

I think this PR is blocked for now by #413 which we should resolve first.

src/mapeo-project.js

- need to update leave tests to check data access after leaving (read+write) - need to update leave tests to confirm that syncing auth still works after leaving - need to add test for Capabilities.assignRole() change

achou11 · 2024-01-08T15:59:00Z

@gmaclennan do you mind taking another look at the changes here and providing input on what else is needed? Not sure what from #410 (review) should be done separately, and not sure if the TODOs I have in the changes diff reflect what's needed

achou11 · 2024-01-08T20:45:26Z

src/capabilities.js

+  // TODO: Does this make sense?
+  roleAssignment: [LEFT_ROLE_ID],


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

achou11 · 2024-01-08T20:45:51Z

src/capabilities.js

+  [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',
+    },
+  },


is this an accurate representation for this role?

achou11 · 2024-01-08T20:47:58Z

src/mapeo-project.js

+    this.#sharedDb
+      .delete(projectSettingsTable)
+      .where(eq(projectSettingsTable.docId, this.#projectId))
+      .run()


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?

achou11 · 2024-01-08T20:51:18Z

src/mapeo-project.js

+      )
+    )
+
+    // 3. Clear data from project database


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 😅

src/mapeo-project.js

gmaclennan

This looks good to me, I think the behaviour is all correct, although there are a couple of changes that could be made to make it a bit less fragile and also a possible structure change:

Comparing roles based on the capability.name property is fragile if we start storing these records in the database (instead of hard-coding them) then we need to remember to never allow the name to change. Listing role records I think is a more robust way of doing what you want (I think that would work, but haven't tried it myself).
Possible change to whether we keep LEFT_ROLE_ID in the roleAssignment array, or treat it as a special case. I think it's fine as you have it, but we should document somewhere to make sure it is always included in roleAssignment if we use this approach.
Looking at all the parameters that now need to be passed to a project instance, I wonder whether it makes sense to split project.$leave into two methods? A private project[kleave]() method and manager.leaveProject(projectId). I think it's fine as it is, I think just that adding all these other parameters that a project instance must know about feels not quite right to me, but not sure if that's just aesthetics or there is a concrete reason for that.

I think the only thing missing is deleting the indexed data in the sqliteDb, but maybe that can be in a follow-up PR so that we can get this completed and merged?

gmaclennan · 2024-01-08T20:57:30Z

src/mapeo-project.js

+    }
+
+    if (
+      deviceIdToCaps[deviceId].name ===


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.

ah yes, makes sense

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 🤔

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

gmaclennan · 2024-01-08T21:05:46Z

src/capabilities.js

@@ -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) {


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)) {).

addressed via ca03ef3

Good to merge and address anything else in follow up

achou11 force-pushed the 241/leave-project branch from bb0743d to 92967a7 Compare December 11, 2023 18:52

achou11 requested a review from gmaclennan December 11, 2023 20:57

gmaclennan mentioned this pull request Dec 12, 2023

fix: waitForSync('full') should resolve even if the device being synced doesn't have sync capability for all namespaces #415

Open

gmaclennan previously requested changes Dec 12, 2023

View reviewed changes

src/mapeo-project.js Outdated Show resolved Hide resolved

src/mapeo-project.js Outdated Show resolved Hide resolved

achou11 force-pushed the 241/leave-project branch from 92967a7 to 3adf2a7 Compare December 12, 2023 22:48

achou11 mentioned this pull request Dec 13, 2023

feat: add method for deleting core data from storage #419

Merged

achou11 force-pushed the 241/leave-project branch from 3adf2a7 to 6b6ae3f Compare December 13, 2023 19:08

achou11 added 2 commits December 14, 2023 11:16

WIP

a876fdf

- need to update leave tests to check data access after leaving (read+write) - need to update leave tests to confirm that syncing auth still works after leaving - need to add test for Capabilities.assignRole() change

fix namespaces iteration

184320e

achou11 force-pushed the 241/leave-project branch from 6b6ae3f to 184320e Compare December 14, 2023 16:16

achou11 added 5 commits December 20, 2023 09:58

Merge branch 'main' into 241/leave-project

f0df02b

use CoreManager.deleteData()

ecc5e71

Merge branch 'main' into 241/leave-project

e0aae10

Merge branch 'main' into 241/leave-project

ea766b4

move import statement

5980999

achou11 requested a review from gmaclennan January 8, 2024 15:57

achou11 added 7 commits January 8, 2024 13:34

fix and improve checking if device can leave project

4c622b5

add test for blocked member attempting to leave project

794fc45

remove comment in test

3b02652

remove accidental solo

5747f17

delete relevant entry in project settings table

734e375

add test for data access after leaving

75fe9b2

remove accidental solo, again

0684919

achou11 changed the title ~~WIP: feat: add project.leave()~~ feat: add project.leave() Jan 8, 2024

achou11 marked this pull request as ready for review January 8, 2024 20:34

minor test improvements

1bd5c64

achou11 commented Jan 8, 2024

View reviewed changes

gmaclennan reviewed Jan 8, 2024

View reviewed changes

achou11 added 2 commits January 10, 2024 16:09

update saveToProjectKeysTable

ec0c1d4

Merge branch 'main' into 241/leave-project

e47809a

gmaclennan approved these changes Jan 11, 2024

View reviewed changes

achou11 merged commit 42c90e4 into main Jan 11, 2024
7 checks passed

achou11 deleted the 241/leave-project branch January 11, 2024 15:05

optic-release-automation bot mentioned this pull request Mar 14, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.5 #519

Merged

This was referenced Apr 11, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.6 #558

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.7 #573

Merged

optic-release-automation bot mentioned this pull request Apr 30, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.8 #592

Merged

optic-release-automation bot mentioned this pull request May 9, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.9 #632

Merged

This was referenced Jun 20, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.10 #709

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.11 #711

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.12 #714

Merged

This was referenced Jul 25, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.13 #726

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.14 #740

Merged

optic-release-automation bot mentioned this pull request Aug 13, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.15 #748

Merged

This was referenced Aug 21, 2024

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.16 #758

Merged

[OPTIC-RELEASE-AUTOMATION] release/v9.0.0-alpha.17 #787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add project.leave() #410

feat: add project.leave() #410

achou11 commented Dec 7, 2023 •

edited

Loading

achou11 commented Dec 11, 2023 •

edited

Loading

gmaclennan left a comment

achou11 commented Jan 8, 2024

achou11 Jan 8, 2024

achou11 Jan 8, 2024

achou11 Jan 8, 2024

achou11 Jan 8, 2024 •

edited

Loading

gmaclennan left a comment

gmaclennan Jan 8, 2024

achou11 Jan 8, 2024

achou11 Jan 9, 2024 •

edited

Loading

achou11 Jan 9, 2024

gmaclennan Jan 8, 2024

achou11 Jan 9, 2024

		// TODO: Does this make sense?
		roleAssignment: [LEFT_ROLE_ID],

feat: add project.leave() #410

feat: add project.leave() #410

Conversation

achou11 commented Dec 7, 2023 • edited Loading

achou11 commented Dec 11, 2023 • edited Loading

gmaclennan left a comment

Choose a reason for hiding this comment

achou11 commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

gmaclennan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achou11 commented Dec 7, 2023 •

edited

Loading

achou11 commented Dec 11, 2023 •

edited

Loading

achou11 Jan 8, 2024 •

edited

Loading

achou11 Jan 9, 2024 •

edited

Loading