Page MenuHomePhabricator
Differential D12657

[lib] Permissions for thick threads
AcceptedPublic
Actions

Authored by ashoat on Wed, Jul 3, 7:56 PM.
Tags
None
Referenced Files
F2209321: D12657.diff
Sun, Jul 7, 4:48 PM
Unknown Object (File)
Sat, Jul 6, 5:26 AM
Unknown Object (File)
Sat, Jul 6, 5:26 AM
Unknown Object (File)
Sat, Jul 6, 5:26 AM
Unknown Object (File)
Sat, Jul 6, 5:25 AM
Unknown Object (File)
Sat, Jul 6, 5:24 AM
Subscribers
None

Details

Reviewers
tomek
marcin
inka
Summary
  1. Updates getRolePermissionBlobs, which is called from the migration-related function legacyUpdateRolesAndPermissions. It's important that we support thick threads there so migrations don't break on them
  2. Introduces getThickThreadRolePermissionsBlob, which is called by the above, and will be called in a later diff to get the permissions for thick threads

Depends on D12656

Test Plan

I haven't tested this stack outside of Flow. I'd like to propose that we land this so the team can iterate. I'll make sure to test all of the functionality once it's integrated into the rest of the codebase.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Wed, Jul 3, 7:56 PM
ashoat added a child revision: D12658: [lib] DMOperationSpec for CREATE_THREAD operation.
Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Jul 3, 8:10 PM
Harbormaster failed remote builds in B30107: Diff 41978!
ashoat updated this revision to Diff 41984.Wed, Jul 3, 8:12 PM

Rebase

Harbormaster returned this revision to the author for changes because remote builds failed.Wed, Jul 3, 8:30 PM
Harbormaster failed remote builds in B30113: Diff 41984!
ashoat updated this revision to Diff 41988.Wed, Jul 3, 8:43 PM

Rebase

ashoat edited parent revisions, added: D12660: [keyserver] Update ServerThreadInfo to only cover ThinThreadType; removed: D12656: [lib] Make it possible to skip some thread/message properties.Wed, Jul 3, 8:44 PM
Harbormaster completed remote builds in B30117: Diff 41988.Wed, Jul 3, 9:02 PM
ashoat requested review of this revision.Wed, Jul 3, 9:02 PM
inka added a comment.Thu, Jul 4, 6:56 AM

I see that there is no CREATE_AND_EDIT_CHANNELS permissions, which lead me to finding a bug in our current app ENG-8710. Should we be able to create a subchannel of a thick thread?

lib/permissions/thread-permissions.js
528

Will we still require that the users are friends before they can add each other to thick threads?

ashoat added a comment.Thu, Jul 4, 10:44 AM
In D12657#358709, @inka wrote:

Should we be able to create a subchannel of a thick thread?

No, I intentionally excluded it. I think the permissions / visibility logic would get too complicated... this is something we'll limit to keyserver-hosted communities, probably indefinitely

lib/permissions/thread-permissions.js
528

So that check doesn't happen through threadPermissions currently. But to answer your question, I think there are 3 perspectives to consider: the adder, the addee, and the peer.

We can (and should) definitely check the friendship on the adder's side.

From the perspective of a peer that receives just a "add members" operation, there is no way to validate the friendship of the adder and the addee. They would have to get a second message from the addee to confirm.

In order to check on the addee's side, we could consider splitting this into an "invite member" (adder) and "accept invite" (addee) flow.

If we do that, we would have to answer some questions:

  1. Does the pending member get to see messages while they're pending?
  2. If so, does the invite expire in some way?
  3. Do the other peers see the pending member in the member list? Are they represented in some other way?

Given the complexity, I think for now our best bet is to only do the check on the adder's side.

inka accepted this revision.Fri, Jul 5, 1:49 AM
This revision is now accepted and ready to land.Fri, Jul 5, 1:49 AM

Revision Contents
Changeset List

PathSize
lib/
permissions/
68 lines

Diff 41988

View Options

lib/permissions/thread-permissions.js

Loading...