Page MenuHomePhabricator
Differential D13017

[lib] Fix role permissions for channels
ClosedPublic
Actions

Authored by ashoat on Aug 7 2024, 1:26 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 5, 4:10 PM
Unknown Object (File)
Sat, Apr 5, 7:08 AM
Unknown Object (File)
Fri, Apr 4, 11:49 PM
Unknown Object (File)
Fri, Apr 4, 9:00 PM
Unknown Object (File)
Fri, Apr 4, 6:34 AM
Unknown Object (File)
Fri, Mar 21, 11:17 AM
Unknown Object (File)
Sat, Mar 15, 2:38 PM
Unknown Object (File)
Mar 12 2025, 3:03 AM
View All Files
Subscribers
None

Details

Reviewers
tomek
inka
Commits
rCOMM1d06aaf0135a: [lib] Fix role permissions for channels
Summary

This diff addresses ENG-8953. I went through each broken permission listed here.

  1. The changes in lib/types/thread-permission-types.js update the mapping from UserSurfacedPermission to role permission strings, the representation used in MariaDB. We add permission strings that guarantee each user-surfaced permission propagates to descendants in the right way.
  2. The changes in lib/permissions/thread-permissions.js removes these permissions from being assigned based on ThreadType. The user-surfaced permissions are responsible for these permissions, so we need to make sure they don't appear if the admin didn't grant them.

Depends on D13016

Test Plan

The whole stack was tested as follows:

  1. Unit tests from D9686, which toggle user-surfaced permissions on and off and make sure no difference is caught. This ensures that the original issue introduced in D9686 isn't reintroduced
  2. Careful review of each descendant permission removed in D9686
  3. Create a community as userA and add userB. Grant tagging permissions to all members. Make sure userB can tag inside non-root channels
  4. Do above, then create a channel without userB, and make sure userB can't tag there either (or do anything other than view). This is the repro described here
  5. Do above, but also create a thread inside the channel (as userA) and make sure userB can't do anything inside the thread other than view, until they join the parent channel

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat created this revision.Aug 7 2024, 1:26 PM
ashoat added a child revision: D13018: [lib] Remove descendantJoinThread from knowOfSecretChannelsPermissions.
ashoat updated this revision to Diff 43238.Aug 7 2024, 1:47 PM

Remove OPEN_TOP_LEVEL from descendantAddMembers

ashoat edited the test plan for this revision. (Show Details)Aug 7 2024, 1:48 PM
Harbormaster failed remote builds in B30989: Diff 43234!Aug 7 2024, 1:49 PM
ashoat added inline comments.Aug 7 2024, 1:54 PM
lib/types/thread-permission-types.js
225

TOP_LEVEL here means "non-sidebar". This disallows channels inside sidebars

229–232

We remove this because it's mostly a subset of the permission granted by the new descendantAddMembers permission, with the exception of allowing non-members of a thread to add members to an open channel whose parent is the root, which is a non-desirable behavior (since we want to restrict all of these permissions to only apply to members of a channel)

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 7 2024, 2:10 PM
Harbormaster failed remote builds in B30993: Diff 43238!
ashoat requested review of this revision.Aug 7 2024, 2:37 PM
tomek accepted this revision.Aug 9 2024, 2:17 AM
This revision is now accepted and ready to land.Aug 9 2024, 2:17 AM
This revision was landed with ongoing or failed builds.Aug 9 2024, 7:28 PM
Closed by commit rCOMM1d06aaf0135a: [lib] Fix role permissions for channels (authored by ashoat). · Explain Why
This revision was automatically updated to reflect the committed changes.
ashoat added a commit: rCOMM1d06aaf0135a: [lib] Fix role permissions for channels.
ashoat mentioned this in D13061: [lib] Make sure community admins can always react and edit messages.Aug 13 2024, 7:17 AM
ashoat mentioned this in D13068: [lib] Don't restrict permissions in descendant chats for announcement roots.Aug 13 2024, 8:50 AM
ashoat mentioned this in rCOMMf1ed5990ecef: [lib] Make sure community admins can always react and edit messages.Aug 14 2024, 9:50 AM
ashoat mentioned this in rCOMM5b88805d0331: [lib] Don't restrict permissions in descendant chats for announcement roots.

Revision Contents
Changeset List

PathSize
lib/
permissions/
37 lines
types/
97 lines

Diff 43238

View Options

lib/permissions/thread-permissions.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines
View Options

lib/types/thread-permission-types.js

Show First 20 Lines • Show All 147 Lines • ▼ Show 20 Lines `${permission} can be disabled by a block, but threadHasPermission can't ` +
'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!', 'check for a block on RawThreadInfo. Please pass in ThreadInfo instead!',
); );
if (threadInfo.minimallyEncoded) { if (threadInfo.minimallyEncoded) {
return hasPermission(threadInfo.currentUser.permissions, permission); return hasPermission(threadInfo.currentUser.permissions, permission);
} }
return permissionLookup(threadInfo.currentUser.permissions, permission); return permissionLookup(threadInfo.currentUser.permissions, permission);
} }
function useThreadHasPermission( function useThreadsWithPermission(
threadInfo: ?ThreadInfo, threadInfos: $ReadOnlyArray<ThreadInfo>,
permission: ThreadPermission, permission: ThreadPermission,
): boolean { ): $ReadOnlyArray<ThreadInfo> {
const loggedInUserInfo = useLoggedInUserInfo(); const loggedInUserInfo = useLoggedInUserInfo();
const userInfos = useSelector(state => state.userStore.userInfos); const userInfos = useSelector(state => state.userStore.userInfos);
return React.useMemo(() => { return React.useMemo(() => {
if (!threadInfo) { return threadInfos.filter((threadInfo: ThreadInfo) => {
return false;
}
const permissions = threadFrozenDueToBlock( const permissions = threadFrozenDueToBlock(
threadInfo, threadInfo,
loggedInUserInfo?.id, loggedInUserInfo?.id,
userInfos, userInfos,
) )
? filterOutDisabledPermissions(threadInfo.currentUser.permissions) ? filterOutDisabledPermissions(threadInfo.currentUser.permissions)
: threadInfo.currentUser.permissions; : threadInfo.currentUser.permissions;
return hasPermission(permissions, permission); return hasPermission(permissions, permission);
}, [loggedInUserInfo?.id, permission, threadInfo, userInfos]); });
}, [loggedInUserInfo?.id, permission, threadInfos, userInfos]);
}
function useThreadHasPermission(
threadInfo: ?ThreadInfo,
permission: ThreadPermission,
): boolean {
return (
useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
.length === 1
);
ashoatUnsubmitted
Done
Inline Actions
): boolean {
- return (
- useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission)
- .length === 1
- );
+ const threads = useThreadsWithPermission(threadInfo ? [threadInfo] : [], permission);
+ return threads.length === 1;
}
function viewerIsMember(

Can we extract this to its own line? Similar to how we want await to be visible, I think it's good for hooks to be as well

ashoat: Can we extract this to its own line? Similar to how we want `await` to be visible, I think it's…
} }
function viewerIsMember( function viewerIsMember(
threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo), threadInfo: ?(ThreadInfo | LegacyRawThreadInfo | RawThreadInfo),
): boolean { ): boolean {
return !!( return !!(
threadInfo && threadInfo &&
threadInfo.currentUser.role !== null && threadInfo.currentUser.role !== null &&
▲ Show 20 Lines • Show All 1,812 Lines • Show Last 20 Lines