feat: add stage/pubtype restrictions to pub read api #988
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…/platform into tfk/pub-read-restrictions
tefkah
requested review from
kalilsn,
allisonking and
3mcd
and removed request for
allisonking
tefkah
commented
Feb 26, 2025
Comment on lines
+136
to
+218
| [ApiAccessType.read]: ({ value, onChange }) => { | ||
| const context = useContext(CreateTokenFormContext); | ||
|
|
||
| return ( | ||
| <div className="flex flex-col gap-2"> | ||
| <h3 className="font-semibold">Stages</h3> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Select the stages this token can read Pubs from | ||
| </span> | ||
| <MultiSelect | ||
| variant="inverted" | ||
| options={context.stages.allOptions} | ||
| defaultValue={ | ||
| value === true || !value ? context.stages.allValues : value.stages | ||
| } | ||
| onValueChange={(val) => { | ||
| const allStagesSelected = | ||
| val.length === context.stages.allValues.length; | ||
|
|
||
| const allPubTypesSelected = | ||
| typeof value === "object" && | ||
| value.pubTypes?.length === context.pubTypes.allValues.length; | ||
|
|
||
| if ((allStagesSelected || val.length === 0) && allPubTypesSelected) { | ||
| onChange(true); | ||
| return; | ||
| } | ||
|
|
||
| onChange({ | ||
| stages: | ||
| val.length === 0 | ||
| ? context.stages.allValues | ||
| : (val as StagesId[]), | ||
| pubTypes: | ||
| typeof value === "object" | ||
| ? value.pubTypes | ||
| : context.pubTypes.allValues, | ||
| }); | ||
| }} | ||
| animation={0} | ||
| data-testid={`pub-${ApiAccessType.read}-stages-select`} | ||
| /> | ||
|
|
||
| <h3 className="font-semibold">Types</h3> | ||
| <span className="text-xs text-muted-foreground"> | ||
| Select the types of Pubs this token can read | ||
| </span> | ||
| <MultiSelect | ||
| variant="inverted" | ||
| options={context.pubTypes.allOptions} | ||
| defaultValue={ | ||
| value === true || !value ? context.pubTypes.allValues : value.pubTypes | ||
| } | ||
| onValueChange={(val) => { | ||
| const allPubTypesSelected = | ||
| val.length === context.pubTypes.allValues.length; | ||
|
|
||
| const allStagesSelected = | ||
| typeof value === "object" && | ||
| value.stages?.length === context.stages.allValues.length; | ||
|
|
||
| if ((allStagesSelected || val.length === 0) && allPubTypesSelected) { | ||
| onChange(true); | ||
| return; | ||
| } | ||
|
|
||
| onChange({ | ||
| pubTypes: | ||
| val.length === 0 | ||
| ? context.pubTypes.allValues | ||
| : (val as PubTypesId[]), | ||
| stages: | ||
| typeof value == "object" | ||
| ? value.stages | ||
| : context.stages.allValues, | ||
| }); | ||
| }} | ||
| animation={0} | ||
| data-testid={`pub-${ApiAccessType.read}-pubTypes-select`} | ||
| /> | ||
| </div> | ||
| ); | ||
| }, |
There was a problem hiding this comment.
if we add another restriction at some point, we should make these components a bit DRYer
tefkah
commented
Feb 26, 2025
| props: ExtraParams | ||
| ) => React.ReactNode; | ||
|
|
||
| return CorrectlyTypedExtraContraints; |
There was a problem hiding this comment.
i really hate this typescript quirk.
here is a somewhat clearer demo of what happens
tefkah
commented
Feb 26, 2025
| @@ -328,7 +414,7 @@ export const ConstraintFormFieldRender = ({ | |||
| return ( | |||
| <FormItemWrapper | |||
| dataTestId={`${scope}-${type}-checkbox`} | |||
| checked={Boolean(field.value)} | |||
| checked={typeof field.value === "object" ? "indeterminate" : Boolean(field.value)} | |||
There was a problem hiding this comment.
this makes the checkbox have a - in it instead of also looking like a checkmark when a restriction is selected
tefkah
commented
Feb 26, 2025
Comment on lines
-1368
to
+1375
| pubTypeId?: PubTypesId; | ||
| stageId?: StagesId; | ||
| /** | ||
| * Multiple pubIds to filter by | ||
| */ | ||
| pubIds?: PubsId[]; | ||
| pubTypeId?: PubTypesId[]; | ||
| stageId?: StageConstraint[]; |
There was a problem hiding this comment.
not reeeaally happy with having pubTypeId and stageId be singulars that accept arrays. mostly i did not want to introduce a bunch of noise to this PR, but i guess i already did
should i
a. change pubTypeId->pubTypeIds, same for stages
b. have both pubTypeId?: PubTypesId, pubTypeIds?: PubTypesIds[]
c. keep it like this
i think having pubIds separate from pubId makes sense, as they indicate a difference in the returned type (pubId returns a singular pub, while pubTypeIds returns an array)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue(s) Resolved
(not a ticket for this i believe)
Allow community admins to restrict Api access tokens to only read from an allowed list of stages/pub-types.
High-level Explanation of PR
Adds a new restriction for Pub.read api access tokens, allowing admins to specify "oh you can only read from these pub types/stages".
Some quirks:
Since we wanted to allow restricting both pub-types and stages, I was a bit stuck on how to do the selection UX.
Does only selecting one stage but no pub types allow you to read all pub types from that stage, or none, making the token effectively useless?
To solve this, I changed how the restriction selectors work: instead of having you select the things you want, you need to unselect the stages/pubtypes you do not want the token to have access to.
In addition, I added a "no stage" option to all the existing and new stage restrictions. While you could set a Pub create token to only allow creating pubs in certain stages, there was no option to allow pubs to be created without a stage. You can now explicitly allow this.
Test Plan
arcadia./api/v0/c/arcadia-research/pubs. Observe you only get back pubs from the pubtypes you specified./api/v0/c/arcadia-research/pubs?pubTypeId=.... Observe you only get back pubs from the pubtype in the URL./api/v0/c/arcadia-research/pubs?pubTypeId=<some other pubtypeid>. Observe you only get nothing.Screenshots (if applicable)
Notes