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

fix(rbac): reduce requests to credentials API #2955

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AndrienkoAleksandr
Copy link
Contributor

Hey, I just made a Pull Request!

Reduce requests to credentials API, simplify code a bit.

✔️ 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)

@backstage-goalie
Copy link
Contributor

Missing Changesets

The following package(s) are changed by this PR but do not have a changeset:

  • @backstage-community/plugin-rbac-backend

See CONTRIBUTING.md for more information about how to add changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
@backstage-community/plugin-rbac-backend workspaces/rbac/plugins/rbac-backend none v5.4.0

Signed-off-by: Oleksandr Andriienko <[email protected]>

if (decision.result === AuthorizeResult.DENY) {
throw new NotAllowedError(); // 403
if (credentials.principal.type !== 'user') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the code get here (this)?
Maybe we can update the message there Only creadential principal with type 'user' permitted to modify permissions to:
Only credential principal of type 'user' is permitted to create or modify permissions


if (decision.result === AuthorizeResult.DENY) {
throw new NotAllowedError(); // 403
if (credentials.principal.type !== 'user') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.


if (decision.result === AuthorizeResult.DENY) {
throw new NotAllowedError(); // 403
if (credentials.principal.type !== 'user') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@dzemanov
Copy link
Contributor

Tested and works fine. Only had some small comments. Thank you @AndrienkoAleksandr, this greatly cleans up the code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants