Page MenuHomePhabricator

[lib] Add several permissions to threadPermissionsDisabledByBlock
ClosedPublic

Authored by ashoat on Aug 13 2024, 8:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Sep 25, 4:12 PM
Unknown Object (File)
Wed, Sep 25, 4:12 PM
Unknown Object (File)
Sat, Sep 14, 11:47 PM
Unknown Object (File)
Sat, Sep 14, 11:46 PM
Unknown Object (File)
Sat, Sep 14, 11:44 PM
Unknown Object (File)
Sat, Sep 7, 1:56 AM
Unknown Object (File)
Wed, Sep 4, 9:00 PM
Unknown Object (File)
Sun, Sep 1, 4:23 AM
Subscribers
None

Details

Summary

The other EDIT_THREAD_* permissions are there. I think it makes sense to include EDIT_THREAD_AVATAR as well.

This means that if user A blocks user B, then neither user will be able to change the thread avatar in chats that contain only those two users.

(EDIT_MESSAGE and REACT_TO_MESSAGE were also added after review.)

Depends on D13061

Test Plan

I made sure this change is safe by looking at where EDIT_THREAD_AVATAR is checked on the client. It happens in two places: once on native, and once on web. In both cases we use useThreadHasPermission, which is able to check the blocked status.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added inline comments.
lib/types/thread-permission-types.js
30–32 ↗(On Diff #43362)

Shouldn't we do the same for these permissions? At least editing messages sounds like something that should be affected by the blocked status.

This revision is now accepted and ready to land.Aug 14 2024, 6:55 AM
lib/types/thread-permission-types.js
30–32 ↗(On Diff #43362)

It looks like both REACT_TO_MESSAGE and EDIT_MESSAGE should be safe to move, since their client checks go through useThreadHasPermission, and the keyserver checks go through checkThreadPermission / checkThread.

However, the check for MANAGE_PINS on the keyserver side is not safe. It goes through canToggleMessagePin, which calls threadHasPermission directly. Since it doesn't currently consider block status, we would need to refactor the check to make it work correctly for threadPermissionsNotAffectedByBlock. (The Flow types would catch this if we tried to move it.)

I'll make the change for the first two. I can create a follow-up for MANAGE_PINS if you'd like, but I suspect this isn't a very important case (by default, only admins have this permission anyways, and the block does not affect admins).

lib/types/thread-permission-types.js
30–32 ↗(On Diff #43362)

I suspect this isn't a very important case (by default, only admins have this permission anyways, and the block does not affect admins).

Makes sense! In that case, we don't need the follow-up.

Add EDIT_MESSAGE and REACT_TO_MESSAGE to threadPermissionsDisabledByBlock as well

ashoat retitled this revision from [lib] Add EDIT_THREAD_AVATAR to threadPermissionsDisabledByBlock to [lib] Add several permissions to threadPermissionsDisabledByBlock.Aug 14 2024, 9:43 AM
ashoat edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Aug 14 2024, 9:50 AM
This revision was automatically updated to reflect the committed changes.