Skip to content

Commit

Permalink
[lib] Fix threadPermissionsFromBitmaskHex invariant
Browse files Browse the repository at this point in the history
Summary:
Really shouldn't have made this mistake...

After moving things around to resolve circular dependency issue, I accidentally started using the `tHexEncodedRolePermission` check in `threadPermissionsFromBitmaskHex` instead of the `tHexEncodedPermissionsBitmask` check...

This required that we have >=3 hex characters, when there's no such minimum for thread permissions.

Caught this during `native` testing thanks to thread where permissions for some user were "3", which should have been valid.

I read through `minimally-encoded-thread-permissions` again very carefully to see if there were any other mistakes I could find. I think part of the reason for the error may have been the similar-ish naming of all the utilities/etc. I'll see if I can update those to be more clear.

Also went ahead and included unit test that would've caught the issue earlier.

---

Depends on D10295

Test Plan: Unit test, careful re-reading of `threadPermissionsFromBitmaskHex`.

Reviewers: ashoat, ginsu, tomek, rohan

Reviewed By: ashoat

Differential Revision: https://phab.comm.dev/D10296
  • Loading branch information
atulsmadhugiri committed Dec 13, 2023
1 parent 3477403 commit e40290b
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 3 deletions.
6 changes: 3 additions & 3 deletions lib/permissions/minimally-encoded-thread-permissions.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ const permissionsToBitmaskHex = (
return bitmask.toString(16);
};

const tHexEncodedRolePermission: TRegex = tRegex(/^[0-9a-fA-F]{3,}$/);
const tHexEncodedPermissionsBitmask: TRegex = tRegex(/^[0-9a-fA-F]+$/);
const threadPermissionsFromBitmaskHex = (
permissionsBitmaskHex: string,
): ThreadPermissionsInfo => {
invariant(
tHexEncodedRolePermission.is(permissionsBitmaskHex),
tHexEncodedPermissionsBitmask.is(permissionsBitmaskHex),
'permissionsBitmaskHex must be valid hex string.',
);

Expand Down Expand Up @@ -166,7 +166,7 @@ const inversePropagationPrefixes: Map<bigint, string> =
const inverseFilterPrefixes: Map<bigint, string> =
invertObjectToMap(filterPrefixes);

const tHexEncodedPermissionsBitmask: TRegex = tRegex(/^[0-9a-fA-F]+$/);
const tHexEncodedRolePermission: TRegex = tRegex(/^[0-9a-fA-F]{3,}$/);
const decodeRolePermissionBitmask = (bitmask: string): string => {
const bitmaskInt = BigInt(`0x${bitmask}`);
const basePermission = (bitmaskInt >> BigInt(4)) & BigInt(63);
Expand Down
32 changes: 32 additions & 0 deletions lib/permissions/minimally-encoded-thread-permissions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,38 @@ describe('threadPermissionsFromBitmaskHex', () => {
expectedDecodedThreadPermissions,
);
});

it('should decode bitmask strings under 3 characters', () => {
// We know that '3' in hex is 0b0011. Given that permissions are encoded
// from least significant bit (LSB) to most significant bit (MSB), we would
// except this to mean that only the first two permissions listed in
// `baseRolePermissionEncoding` are `true`. Which is the case.
const decodedThreadPermissions = threadPermissionsFromBitmaskHex('3');
expect(decodedThreadPermissions).toStrictEqual({
know_of: { value: true, source: 'null' },
visible: { value: true, source: 'null' },
voiced: { value: false, source: null },
edit_entries: { value: false, source: null },
edit_thread: { value: false, source: null },
edit_thread_description: { value: false, source: null },
edit_thread_color: { value: false, source: null },
delete_thread: { value: false, source: null },
create_subthreads: { value: false, source: null },
create_sidebars: { value: false, source: null },
join_thread: { value: false, source: null },
edit_permissions: { value: false, source: null },
add_members: { value: false, source: null },
remove_members: { value: false, source: null },
change_role: { value: false, source: null },
leave_thread: { value: false, source: null },
react_to_message: { value: false, source: null },
edit_message: { value: false, source: null },
edit_thread_avatar: { value: false, source: null },
manage_pins: { value: false, source: null },
manage_invite_links: { value: false, source: null },
voiced_in_announcement_channels: { value: false, source: null },
});
});
});

describe('rolePermissionToBitmaskHex', () => {
Expand Down

0 comments on commit e40290b

Please sign in to comment.