Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Permissions] Require at least one item in allOf/anyOf criteria #9348

Merged
merged 6 commits into from
Feb 11, 2022
Merged

[Permissions] Require at least one item in allOf/anyOf criteria #9348

merged 6 commits into from
Feb 11, 2022

Conversation

joeporpeglia
Copy link
Contributor

@joeporpeglia joeporpeglia commented Feb 4, 2022 •
edited

Signed-off-by: Joe Porpeglia josephp@spotify.com

Hey, I just made a Pull Request!

Closes #9280

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2022 •
edited

🦋 Changeset detected

Latest commit: bbf2150

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@backstage/plugin-permission-common Minor
@backstage/plugin-permission-node Minor
example-backend Patch
@backstage/search-common Patch
@backstage/test-utils Patch
@backstage/plugin-catalog-backend Patch
@backstage/plugin-catalog-common Patch
@backstage/plugin-catalog-react Patch
@backstage/plugin-permission-backend Patch
@backstage/plugin-permission-react Patch
@backstage/plugin-search-backend Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vinzscam vinzscam self-requested a review February 4, 2022 10:17
@joeporpeglia
Require at least one item in allOf/anyOf criteria
7f7d386 
Signed-off-by: Joe Porpeglia <josephp@spotify.com>
@joeporpeglia
Use strict() validation for permission criteria zod schemas
4448ba4 
Signed-off-by: Joe Porpeglia <josephp@spotify.com>
@joeporpeglia joeporpeglia force-pushed the permissions-criteria-validation branch from 28a559e to 4448ba4 Compare February 4, 2022 20:28
@joeporpeglia joeporpeglia marked this pull request as ready for review February 4, 2022 21:49
@joeporpeglia joeporpeglia requested a review from a team as a code owner February 4, 2022 21:49
@joeporpeglia
Fix api reports
0ccd3fe 
Signed-off-by: Joe Porpeglia <josephp@spotify.com>
@joeporpeglia joeporpeglia force-pushed the permissions-criteria-validation branch from 8e5514a to 0ccd3fe Compare February 7, 2022 17:53
@joeporpeglia
Add changesets
01c29fe 
Signed-off-by: Joe Porpeglia <josephp@spotify.com>
mtlewis
mtlewis approved these changes Feb 9, 2022
View reviewed changes
Copy link
Collaborator

@mtlewis mtlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! One question but 👍 either way.

plugins/permission-node/src/integration/createConditionTransformer.ts Outdated Show resolved Hide resolved
@joeporpeglia
Cast type instead of throwing
6fee938 
Signed-off-by: Joe Porpeglia <josephp@spotify.com>
Rugvip
Rugvip approved these changes Feb 11, 2022
View reviewed changes
Copy link
Member

@Rugvip Rugvip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 , just a minor nit if you agree with it

plugins/permission-common/api-report.md Outdated
@@ -65,6 +75,14 @@ export function isReadPermission(permission: Permission): boolean;
// @public
export function isUpdatePermission(permission: Permission): boolean;

// @public
export type NonEmptyArray<T> = [T, ...T[]];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly unfortunate to export this. If you mark it with /** @ignore */ you can avoid exporting it without warnings in the API report. The DX of using the criteria should stay the same afaik, as in you'll see the same editor hints

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I tried using @internal but that broke the api report. I'll give @ignore a try!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah @internal is for when you want to explicitly say that something should not be part of the external API, and API Extractor will then help you enforce that. @ignore is for when you wanna tell API Extractor "i know what I'm doing", it's our own invention though.

Update looks 👌

@joeporpeglia
Make NonEmptyArray type private/ignored
bbf2150 
Signed-off-by: Joe Porpeglia <josephp@spotify.com>
@joeporpeglia joeporpeglia merged commit 17c9f84 into master Feb 11, 2022
@joeporpeglia joeporpeglia deleted the permissions-criteria-validation branch February 11, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtlewis mtlewis mtlewis approved these changes

@Rugvip Rugvip Rugvip approved these changes

@vinzscam vinzscam Awaiting requested review from vinzscam

Assignees
No one assigned
Labels
auth
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Permissions] Ambiguous conditional decision behavior with empty anyOf criteria
4 participants
@joeporpeglia @mtlewis @Rugvip @vinzscam