Membership, roles, and removal

[gmaclennan]

Our historical naming of roles creates confusion for discussing this, so let's start with some clear definitions:

• A membership record is what defines a device's membership in a project from a specific point-in-time, and it defines what capabilities they have in the project. We do not store the capabilities in the membership record, but instead have some pre-defined "roles" with pre-defined capabilities. Currently these membership records are called a Role record in @comapeo/schema, which is confusing - this should be called a Membership record.

• A role record is a predefined group of capabilities determining:

  • which core namespaces may be synced
  • what datatypes can be written or read
  • capability to read or write data from others
  • which roles can be assigned to others

  In the core code we correctly call these roles, but we confusingly call the class Roles also, but this class actually manages membership records, but returns them with the role records attached.

When we are talking about devices being removed from a project or leaving, I think things have also got confusing because we have tried to encode that somehow into the role, having a BLOCKED_ROLE and a LEFT_ROLE, however I don't think those two roles should be any different to each other because I don't think BLOCKED_ROLE should block auth core sync, and would probably be best renamed to NO_ACCESS_ROLE (NB. we could consider NO_ROLE could also be the same as NO_ACCESS_ROLE, however currently NO_ROLE does have permission to read and write their own data, with the idea that a user could start collecting data once they have access to a project but have not managed to sync their membership (role) record. However, this could be a loophole in the security/capability model (e.g. theoretically a NO_ROLE device currently could create presets), so we may want to actually reconsider it and also make it the same NO_ACCESS_ROLE.

I think actually we need a new property on membership records to define whether a device has left / been removed separately to the capabilities. When a user is removed or left, we want UX effects, like no longer being able to see the project on their device. Deciding this based on hard-coded role IDs is fragile, and could break if we add custom roles later, and deciding this based on existing capabilities is also fragile, because we could theoretically in the future want a device to continue as a read-only member of a project, or a non-syncing member of the project, and it adds a implicit assumption "this specific capabilities combination results in removal", which is confusing to maintain because it's not explicit. We could add a property on the role records, rather than membership records, but that might result in needing to create multiple role records with the same capabilities for specific cases in the future - let's keep role records to just be about capabilities, and store project membership status on the membership records.

We could add a left boolean property on the membership records, which would default to false if we define it as a non-optional protobuf field. I think it might be better as an enum though so that we can distinguish between devices that were removed and devices that left themselves, e.g. it could be status: 'member' | 'removed' | 'left'. If we set 'member' to be enum 0 on a non-optional protobuf enum, this will be the default value that is decoded when this field is not specified on the existing record, so that would work well for backwards compatibility.

We can also add the optional leftReason field to membership records.

I think the backend should be responsible for deleting encryption keys when a device learns that it has been removed, and will need to also update the project persisted state to hasLeftProject: true, so it should listen on the membership (role) update event to perform that action. This probably means updating the current leaveProject() method to only update the role, and then rely on the event / side-effect for removing encryption keys and updating the persisted state.

TL;DR I recommend the following implementation:

• Remove BLOCKED_ROLE altogether, I don't think we have yet used it anywhere.
• Rename LEFT_ROLE to NO_ACCESS_ROLE (for clarity)
  • Remove this checklist
• Add new properties in schema for Role (potentially rename to Membership at the same time)
  • status: enum MEMBER | REMOVED | LEFT - define MEMBER as enum 0 so that is what is decoded on existing data which did not include this data
  • leftReason as an optional protobuf field, this will ensure that existing data will be decoded with this as undefined rather than an empty string ('').
• When a user leaves, they update their membership record to set status: 'left' and they change their role ID to NO_ACCESS_ROLE_ID.
• When a user is removed, their membership is set (by another device) to status: 'removed' and change their role ID to NO_ACCESS_ROLE_ID.
• We should probably add a check when reading membership records to always return NO_ACCESS_ROLE whenever membership.status !== 'member'.
• Project constructor adds a listener on this.#roles.on('update', (docIds) => if (docIds.has(this.deviceId)) this.emit('own-role-update', this.$getOwnRole())) (should this be own-membership-update and $getOwnMembership()?)
• Project manager adds a listener on any new project project.on('own-role-update', membership => if (membership !== 'member') this.#removeEncryptionKeysAndSetHasLeftProject())
• manager.leaveProject() just becomes await project[kProjectLeave](), and the rest of the code is moved to the side-effect this.#removeEncryptionKeysAndSetHasLeftProject(), or we call that anyway in leaveProject() (it should be ok to call twice).
• Don't delete from activeProjects on leave or remove.

Then from the front-end perspective they can know:

• manager.listProjects() will not return projects that a user has either left or been removed from, unless they pass includeLeft: true.
• They can listen on project.on('own-role-udpate') (or own-membership-update) and check whether the returned membership record has status='member', and take action accordingly - this will also have leftReason which they can use in the notification to the user. In the future we can use this to notify users when their membership has changed to be a coordinator or participant also.

We should probably add a check in manager.getProject() to do if (await project.$getOwnRole().status !== 'member') this.#removeEncryptionKeysAndSetHasLeftProject(), in case somehow we miss the event (e.g. unexpected app shutdown)

[RangerMauve]

Some thoughts:

1. we already use the blocked role to removed archive servers so I don't think removal is an option
2. Adding a status field and defaulting to 0 won't cover all the existing left users and blocked servers.
3. I worry it'd require additional validation, e.g. do we need to anticipate status member but role no access, or status coordinator but role left? I like how the existing roles give us a view of all the membership info at once.
4. If we really want a status I think we should add it to the roles.js definition
5. Instead of left reason I think it should just be reason to leave things open for adding reasons for addition or promotion to coordinator (or demotion to member?)

I think given the urgency of this release it'd be good to aim smaller so we can get it done in time for the front end team to wire things up.

I agree with the points outside of these objections 👍

[RangerMauve]

Another thought re status: 'member' | 'removed' | 'left', is there much utility in the distinction between removed and left? I feel like for the frontend we only really need to know is a member vs no longer a member.

@ErikSin @cimigree Thoughts re: above?

[cimigree]

@RangerMauve Would this impact our ability to know if someone needs to be shown the message that they have been removed from a project? If they leave (left), we don't need to show them anything...

[ErikSin]

I can imagine a case where we want to prevent someone from re-joining if they have been removed, but allowing some to re-join if they left the project themselves.