Skip to content

Remove check for multiple authorization#4816

Closed
jon-wei wants to merge 1 commit intoapache:masterfrom
jon-wei:authfix
Closed

Remove check for multiple authorization#4816
jon-wei wants to merge 1 commit intoapache:masterfrom
jon-wei:authfix

Conversation

@jon-wei
Copy link
Copy Markdown
Contributor

@jon-wei jon-wei commented Sep 17, 2017

Fixes #4813

PR #4271 had a check where an exception is throw if a request had multiple authorization checks performed, this is too aggressive for now and this PR removes that check.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Sep 17, 2017

#4817 should fix the TC inspections. Not sure why they weren't flagged before.

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

I'm not sure removing the check is the right fix. It looks like a better fix would be to modify SupervisorResource to use a "filter" helper where appropriate. It would at least be in specGetAllHistory and specGetAll, which are doing filtering without using the filter helper.

I guess the reason they're not using filter helpers now is that the filter helpers take a resourceActionGenerator with signature Function<? super ResType, ResourceAction>, and that doesn't fit supervisors well, since they could be supervising multiple datasources.

So I would suggest:

  • Keep the check.
  • Either modify AuthorizationUtils.filterAuthorizedResources to use a function like Function<? super ResType, Iterable<ResourceAction>> or instead add a new method with that ability.
  • Have the SupervisorResource methods use that new filtering capability where appropriate.

@jon-wei
Copy link
Copy Markdown
Contributor Author

jon-wei commented Sep 17, 2017

@gianm I'll make a patch that changes the way the supervisors authorize requests, closing this one

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants