feat!: rename "Capability" to "Role", attach ID #462
Conversation
This change makes several improvements to the concept formerly known as "capabilities": - `Capability` is now called `Role` across the project, resulting in many simple renames. - `Role`s have an attached `roleId` property, which they didn't before. - The creator role and "no role" role now have role IDs.[^1] - The `RoleId` type now includes all possible role types, now including the creator role and the "no role" role. I created narrower types such as `RoleIdAssignableToOthers` and `RoleIdAssignableToAnyone`, which don't include those. This should help the front-end know what role someone is (for example, <digidem/comapeo-mobile#79 (comment)>). [^1]: I generated these with `crypto.randomBytes(8).toString('hex')`.
src/roles.js
Outdated
| */ | ||
| function isKnownRoleId(roleId) { | ||
| return roleId in DEFAULT_CAPABILITIES | ||
| return roleId in ROLES |
There was a problem hiding this comment.
Highlighting this: DEFAULT_CAPABILITIES didn't have the creator or "no role" capabilities before. ROLES does have those. I think this is fine based on usage but want to make sure.
There was a problem hiding this comment.
I think we should avoid allowing the creator role id, maybe not in this function but instead where it is used on line 288? We will later validate role assignments, but I think it's safer that we don't allow a role record to define a role to be the project creator, since we're not allowing the project creator role to be assigned, just implicit.
src/roles.js
Outdated
| */ | ||
| function isKnownRoleId(roleId) { | ||
| return roleId in DEFAULT_CAPABILITIES | ||
| return roleId in ROLES |
There was a problem hiding this comment.
I think we should avoid allowing the creator role id, maybe not in this function but instead where it is used on line 288? We will later validate role assignments, but I think it's safer that we don't allow a role record to define a role to be the project creator, since we're not allowing the project creator role to be assigned, just implicit.
src/member-api.js
Outdated
| * @param {string} [opts.roleName] | ||
| * @param {string} [opts.roleDescription] | ||
| * @param {number} [opts.timeout] | ||
| * | ||
| * @returns {Promise<import('./generated/rpc.js').InviteResponse_Decision>} | ||
| */ | ||
| async invite(deviceId, { roleId, roleName, roleDescription, timeout }) { | ||
| if (!DEFAULT_CAPABILITIES[roleId]) { | ||
| if (!ROLES[roleId]) { |
There was a problem hiding this comment.
I think this needs to be stricter now, since we do not want to allow inviting someone as a project creator. This should be the "assignable to others" role list, although we should probably exclude the blocked role from that, but maybe as a to-do because I think some tests invite as blocked, just because I was lazy.
src/roles.js
Outdated
| /** | ||
| * @typedef {Extract<RoleId, | ||
| * typeof COORDINATOR_ROLE_ID | | ||
| * typeof MEMBER_ROLE_ID | | ||
| * typeof BLOCKED_ROLE_ID | | ||
| * typeof LEFT_ROLE_ID | ||
| * >} RoleIdAssignableToAnyone | ||
| */ |
There was a problem hiding this comment.
I think better to have both of these as runtime checks to e.g.
const ROLE_ID_ASSIGNABLE_TO_ANYONE = /** @type {const} */ ([
COORDINATOR_ROLE_ID,
MEMBER_ROLE_ID,
BLOCKED_ROLE_ID,
LEFT_ROLE_ID,
])
/** @typedef {typeof ROLE_ID_ASSIGNABLE_TO_ANYONE} RoleIdAssignableToAnyone */and when used as a type we should also add an assertion.
| @@ -163,10 +163,10 @@ export class MemberApi extends TypedEmitter { | |||
|
|
|||
| /** | |||
| * @param {string} deviceId | |||
| * @param {import('./capabilities.js').RoleId} roleId | |||
| * @param {import('./roles.js').RoleIdAssignableToOthers} roleId | |||
| * @returns {Promise<void>} | |||
| */ | |||
| async assignRole(deviceId, roleId) { | |||
There was a problem hiding this comment.
Can we add a TODO note here or open an issue to have stricter runtime checks here.
I also wonder if "assigning a blocked role" is confusing for the API consumer, and we should expose that separately as $member.block(deviceId) and limit assignRole() to just assignable roles within the project (currently participant and coordinator).
There was a problem hiding this comment.
Also in the future we might want to add options to how we block users (which internally we might implement as different roles, or additional fields on membership records - maybe the latter better for many scenarios), for example:
- Removed with "self-destruct", e.g. when a device discovers it is removed it deletes all data including its own. Would be useful for compromised phones.
- Removed with removed encryption keys and keeping own data (but deleting others) - useful for when removing a device that we know has important data on it, that we want to sync off it in the future, without allowing that device to access it. Device would need to self-delete its encryption keys when it finds out it is blocked this way.
- Removed with ignore all data from device - would re-index and ignore all edits or created data from the removed device.
Having a separate API to remove devices would be helpful for implementing these.
There was a problem hiding this comment.
Added a stricter runtime check in f24c85e. Let me know if you still want me to add a TODO/issue.
|
I believe I've addressed all comments. I've requested another review but will plan to merge tomorrow barring objections. |
This change makes several improvements to the concept formerly known as "capabilities":
Capabilityis now calledRoleacross the project, resulting in many simple renames.Roles have an attachedroleIdproperty, which they didn't before.The creator role and "no role" role now have role IDs.1
The
RoleIdtype now includes all possible role types, now including the creator role and the "no role" role. I created narrower types such asRoleIdAssignableToOthersandRoleIdAssignableToAnyone, which don't include those.This should help the front-end know what role someone is (for example, digidem/comapeo-mobile#79 (comment)).
Footnotes
I generated these with
crypto.randomBytes(8).toString('hex'). ↩