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.

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

Integrate permissions into entityAncestry endpoint in catalog-backend #8853

Merged

Conversation

Joonpark13
Copy link
Member

@Joonpark13 Joonpark13 commented Jan 10, 2022

Hey, I just made a Pull Request!

We call the underlying EntityCatalog's entityAncestry method from within the authorized service, and then batch authorize the resulting entities and filter out any unauthorized entities and their parents. Note: entityRefs to unauthorized parents may still appear within the returned results if the root child is authorized, but this was previously discussed as an acceptable tradeoff.

✔️ 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
  • All your commits have a Signed-off-by line in the message. (more info)

@github-actions github-actions bot added the area:catalog Related to the Catalog Project Area label Jan 10, 2022
@changeset-bot
Copy link

changeset-bot bot commented Jan 10, 2022

🦋 Changeset detected

Latest commit: 48248e2

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

This PR includes changesets to release 9 packages
Name Type
@backstage/plugin-catalog-backend Patch
example-backend Patch
@backstage/create-app Patch
@backstage/plugin-catalog-backend-module-ldap Patch
@backstage/plugin-catalog-backend-module-msgraph Patch
@backstage/plugin-scaffolder-backend Patch
@backstage/plugin-scaffolder-backend-module-cookiecutter Patch
@backstage/plugin-scaffolder-backend-module-rails Patch
@backstage/plugin-scaffolder-backend-module-yeoman 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

Copy link
Member

@freben freben left a comment

Choose a reason for hiding this comment

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

👍

plugins/catalog-backend/src/catalog/types.ts Outdated Show resolved Hide resolved
@Joonpark13 Joonpark13 marked this pull request as ready for review January 12, 2022 17:02
@Joonpark13 Joonpark13 requested a review from a team as a code owner January 12, 2022 17:02
Base automatically changed from catalog-backend-permission-integration/entities to master January 13, 2022 10:59
@Joonpark13 Joonpark13 force-pushed the catalog-backend-permission-integration/entity-ancestry branch from 53ed9b6 to cf60309 Compare January 13, 2022 11:02
this.findUnauthorizedParents(
rootEntityRef,
ancestryResult.items,
new Set(rootUnauthorizedEntityRefs),
Copy link
Member

Choose a reason for hiding this comment

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

Could have created this set upfront once on line 100.

Should be possible to batch form a single set upfront I think?

// as a matter of fact i'm not entirely sure that there can ever exist a
// parent entity ref that is NOT already among the entities themselves
// so we may not even have to look at the parentEntityRefs here
const allRefs = lodash.uniq(
  ancestryResult.items.flatMap(item => [
    stringifyEntityRef(item.entity),
    ...item.parentEntityRefs,
  ])
);

// Batch check them all
const auth = await this.permissionApi.authorize(
  allRefs.map(item => ({
    permission: catalogEntityReadPermission,
    resourceRef: item
  })),
  { token: options?.authorizationToken },
);

const forbiddenRefs = new Set(
  allRefs.filter((_, i) => auth[i].result === AuthorizeResult.DENY)
);

Feels like after this, we'd have to first check if rootEntityRef was forbidden and then throw immediately, right?

And then filter the results by the forbidden filter, as well as their respective parent refs, in s single .filter.map chain

Copy link
Member Author

Choose a reason for hiding this comment

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

The thing about this is that if one entity is denied but its parent is allowed, you'd get the parent back but not the entity itself. I'm recursing up the map in order to make sure any parents of a denied entity are not returned, regardless of whether that parent itself is allowed or not.

@Joonpark13 Joonpark13 force-pushed the catalog-backend-permission-integration/entity-ancestry branch from cf60309 to fdc1482 Compare January 17, 2022 13:13
@Joonpark13 Joonpark13 force-pushed the catalog-backend-permission-integration/entity-ancestry branch from fdc1482 to 48248e2 Compare January 17, 2022 14:13
Copy link
Member

@freben freben left a comment

Choose a reason for hiding this comment

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

Let's go!

@freben freben merged commit b4e6325 into master Jan 18, 2022
@freben freben deleted the catalog-backend-permission-integration/entity-ancestry branch January 18, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:catalog Related to the Catalog Project Area
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants