Details

Reviewers

kamil
ashoat

Commits

rCOMM1d2bb3b72dfb: [lib] Set roles and permissions in thick threads based on parent permissions

Summary

When we create a thick thread, we should check if the viewer is a member and set the role and permissions accordingly. This case can happen when we receive a create sidebar operation.

When a user leaves a sidebar, we should also update their permissions.

https://linear.app/comm/issue/ENG-9318/sidebar-joining-issues

Test Plan

Create a thick sidebar without the viewer and check if the join button is visible. Join and leave the sidebar - the button should reappear.
Created a new sidebar in a thread with three users and added one of them. Checked if the added user can send messages and if the second user can join the sidebar.

Diff Detail

Repository

rCOMM Comm

Branch

improvements2

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Fri, Sep 20, 7:54 AM

Squash

Harbormaster completed remote builds in B31754: Diff 44393.Fri, Sep 20, 8:11 AM

Harbormaster completed remote builds in B31755: Diff 44395.Fri, Sep 20, 8:13 AM

tomek requested review of this revision.Fri, Sep 20, 8:13 AM

I think adding these hacks to the permissions system is a really bad idea. The permissions system is already incredibly complex, and a huge cause of confusion / bugs / problems. If there's one place we should be thoughtful about technical debt, it's the permissions system

lib/permissions/thread-permissions.js
353	This seems potentially concerning for the callsite from `deprecatedUpdateRolesAndPermissions` / `legacyUpdateRolesAndPermissions`. Atul renamed `deprecatedUpdateRolesAndPermissions` to mark it as deprecated, but I'm not sure that's accurate long-term. He never finished migrating web to not have permissions in the stores, so I think that means if we change permissions, we'd have to run that migration. The migration goes through this function. It seems that the result of this is that we would assume the user is a member of all thick threads they can see, but that's not accurate, right?
469	This feels like a messy, inconsistent way of handling this. The functions in this file return permissions blobs that should only be applied when the user is a member. If the user is not a member, no permissions should be applied. I think the permissions you want to return when `isMember: false` can be applied from the parent.
475–477	Typically we handle this with permissions that come from the parent. Can you walk me through the decision-making here?
lib/shared/dm-ops/create-thread-spec.js
95–98	This spec isn't used for sidebars, right? If the user isn't a member of a non-sidebar thick thread, I think they should have no role permissions

This revision now requires changes to proceed.Fri, Sep 20, 8:43 AM

I think adding these hacks to the permissions system is a really bad idea. The permissions system is already incredibly complex, and a huge cause of confusion / bugs / problems. If there's one place we should be thoughtful about technical debt, it's the permissions system

The permissions system for thick threads is completely isolated from keyserver threads. So I don't think introducing hacks here makes our permissions system introduce a tech debt that has significant interest rates. The thick thread system doesn't have any mechanisms like permissions inheritance, prefixes, etc. that caused bugs in the past. Introducing these would make the system a lot more complicated. So I think the current approach is better for now, and can be easily replaced in the future, but I'm going to scope the right solution to make sure how expensive it is - https://linear.app/comm/issue/ENG-9361/scope-the-right-solution-to-permissions-of-thick-sidebars.

lib/permissions/thread-permissions.js
353	The migration goes through this function. It seems that the result of this is that we would assume the user is a member of all thick threads they can see, but that's not accurate, right? When we pass `true` as an argument to `getThickThreadRolePermissionsBlob` we get exactly the same behavior we had before this diff. But to make it correct, from the migration's point of view, we would need to extend `getRolePermissionBlobs` to also take the flag. (assuming we want to keep the approach from this diff, which is unlikely). And yes, we would assume that the user is a member, exactly as before this diff. About the `deprecatedUpdateRolesAndPermissions` function, it seems it was used in migrations on both native and web, in migration 78.
469	When a user isn't a member of a sidebar, they should be able to join it, so they should have permission to do so. But I have to check how this mechanism works on keyserver and how to migrate it to the client.
475–477	We don't have a mechanism for permission inheritance for thick threads on clients. Migrating this whole mechanism to a client sounds like a project on its own, and should be scoped. With the limited time to launch, this sounds like something too ambitious, but we will know better after the scoping.
lib/shared/dm-ops/create-thread-spec.js
95–98	`createThickRawThreadInfo` is used for both sidebars and threads. I think they should have no role permissions Yes, that's correct

Inherit permissions from the parent

tomek added inline comments.Tue, Sep 24, 8:57 AM

lib/permissions/thread-permissions.js
469–471 ↗	(On Diff #44488)	For thick threads, we have at most two levels of threads, so child and descendant should be equal, but I wanted to keep this consistent with thin threads.
lib/shared/dm-ops/create-thread-spec.js
64–93 ↗	(On Diff #44488)	I was wondering whether we should take a similar approach also when 1. the viewer is a member or when 2. creating membership permissions for non-viewer members. It would be more correct, but wouldn't change anything now, because the resulting permissions would be equal.

Harbormaster failed remote builds in B31820: Diff 44488!Tue, Sep 24, 8:58 AM

Harbormaster completed remote builds in B31820: Diff 44488.Tue, Sep 24, 9:20 AM

ashoat accepted this revision.Tue, Sep 24, 9:51 AM

ashoat added inline comments.

lib/shared/dm-ops/create-thread-spec.js
64–93 ↗	(On Diff #44488)	Yeah, I think that's a good idea. Besides it being "more correct", it would also unify the codepaths here, which I think helps with code clarity/readability
132–134 ↗	(On Diff #44488)	Can we add a log if `parentThreadID` exists but is missing from the store?
lib/shared/dm-ops/leave-thread-spec.js
130–132 ↗	(On Diff #44488)	Can we add a log if `parentThreadID` exists but is missing from the store?
142–146 ↗	(On Diff #44488)	I'd consider using `minimallyEncodeThreadCurrentUserInfo` instead of `permissionsToBitmaskHex` here... the latter is sort of "low level"

This revision is now accepted and ready to land.Tue, Sep 24, 9:51 AM

Set permissions based on parent

tomek retitled this revision from [lib] Fix roles and permissions in thick threads when viewer isn't a member to [lib] Set roles and permissions in thick threads based on parent permissions.Wed, Sep 25, 5:17 AM

tomek edited the test plan for this revision. (Show Details)

tomek marked an inline comment as done.

tomek added inline comments.

lib/shared/dm-ops/leave-thread-spec.js
142–146 ↗	(On Diff #44488)	Makes sense!

There are a lot of changes so requesting a review:

Get permissions from the parent even when the viewer is a member
Use permissions from the parent when setting the permissions for non-viewer members

Harbormaster completed remote builds in B31840: Diff 44528.Wed, Sep 25, 5:33 AM

ashoat accepted this revision.Wed, Sep 25, 5:47 AM

ashoat added inline comments.

lib/shared/dm-ops/add-members-spec.js
80 ↗	(On Diff #44528)	Keep all lines to 80 chars Can we differentiate each log, so we know where it's coming from when we see it?
lib/shared/dm-ops/create-thread-spec.js
68 ↗	(On Diff #44528)	I would move the definition of this above `createRoleAndPermissionForThickThreads`, now that it's called by that one
138 ↗	(On Diff #44528)	Keep all lines to 80 chars Can we differentiate each log, so we know where it's coming from when we see it?
lib/shared/dm-ops/join-thread-spec.js
144–146 ↗	(On Diff #44528)	Keep all lines to 80 chars Can we differentiate each log, so we know where it's coming from when we see it?
lib/shared/dm-ops/leave-thread-spec.js
133–137 ↗	(On Diff #44528)	Keep all lines to 80 chars Can we differentiate each log, so we know where it's coming from when we see it?

This revision is now accepted and ready to land.Wed, Sep 25, 5:47 AM

Update comments and reorder functions

Harbormaster completed remote builds in B31842: Diff 44534.Wed, Sep 25, 6:16 AM

Closed by commit rCOMM1d2bb3b72dfb: [lib] Set roles and permissions in thick threads based on parent permissions (authored by tomek). · Explain WhyWed, Sep 25, 6:46 AM

This revision was automatically updated to reflect the committed changes.

tomek added a commit: rCOMM1d2bb3b72dfb: [lib] Set roles and permissions in thick threads based on parent permissions.

[lib] Set roles and permissions in thick threads based on parent permissions
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 44395

lib/permissions/thread-permissions.js

lib/shared/dm-ops/create-thread-spec.js

lib/shared/dm-ops/leave-thread-spec.js

[lib] Set roles and permissions in thick threads based on parent permissionsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 44395

lib/permissions/thread-permissions.js

lib/shared/dm-ops/create-thread-spec.js

lib/shared/dm-ops/leave-thread-spec.js

[lib] Set roles and permissions in thick threads based on parent permissions
ClosedPublic
Actions

Revision Contents
Changeset List