Details

Reviewers

atul
ginsu
ashoat

Commits

rCOMM5a1516fd8cef: [keyserver] Validate community ID corresponds to a community root in role…

Summary

This diff adds a validation check to make sure the community ID passed in to the server corresponds to a community root (where community roles are valid)

This was some feedback from @ashoat to include this check, and I should have originally done it when writing this code

Depends on D9897

Test Plan

Made sure that it works for valid thread types, and that an error was thrown if a non-community ID was passed in

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Wed, Nov 15, 12:01 PM

Herald added a subscriber: tomek. · View Herald TranscriptWed, Nov 15, 12:01 PM

rohan added a child revision: D9901: [lib] Rename getRolePermissionBlobsForCommunity to getRolePermissionBlobsForCommunityRoot.Wed, Nov 15, 12:04 PM

rohan published this revision for review.Wed, Nov 15, 12:26 PM

ashoat added inline comments.Wed, Nov 15, 12:45 PM

keyserver/src/creators/role-creator.js
92–96 ↗	(On Diff #33284)	Can `checkThreadPermission` be rewritten to avoid fetching from the `threads` table twice? (We already need to fetch once in `fetchThreadInfos` below.) I think I asked you to do something similar in another diff recently, but in that case we had `fetchServerThreadInfos` instead of `fetchThreadInfos`. It should be a bit easier in this case, but let me know if you need pointers!
92–110 ↗	(On Diff #33284)	Can these two checks be performed simultaneously rather than sequentially?

Harbormaster completed remote builds in B24167: Diff 33284.Wed, Nov 15, 1:06 PM

Use threadHasPermission. Tested this by:

Making sure users who should be able to create roles still can
Allowed role creation for all users client-side, but kept this server check. When a regular member tried to create a role, hasPermission was false and an error was thrown

ashoat accepted this revision.Thu, Nov 16, 10:01 AM

This revision is now accepted and ready to land.Thu, Nov 16, 10:01 AM

Harbormaster failed remote builds in B24215: Diff 33348!Thu, Nov 16, 10:37 AM

Rebase

Harbormaster completed remote builds in B24245: Diff 33383.Fri, Nov 17, 8:14 AM

rohan edited the summary of this revision. (Show Details)Mon, Nov 20, 9:18 AM

rohan added a parent revision: D9897: [lib] Introduce getUniversalCommunityRootPermissionsBlob.

rohan removed a parent revision: D9899: [keyserver] Use getUniversalCommunityRootPermissionsBlob in role-creator.

Rebase

Harbormaster completed remote builds in B24310: Diff 33462.Tue, Nov 21, 9:06 AM

Rebase

Harbormaster completed remote builds in B24338: Diff 33495.Wed, Nov 22, 8:14 AM