Single auth check for authorized resource filtering#4818
Single auth check for authorized resource filtering#4818nishantmonu51 merged 4 commits intoapache:masterfrom
Conversation
nishantmonu51
left a comment
nishantmonu51
left a comment
There was a problem hiding this comment.
changes seem good.
If possible, would be nice to add test for this
|
Hm, I don't know how to setup tests like SupervisorResourceTest with the servlet filters that would be used in a real environment (like PreResponseAuthorizationCheckFilter), any ideas there? |
| final AuthorizerMapper authorizerMapper | ||
| final Function<? super ResType, Iterable<ResourceAction>> resourceActionGenerator, | ||
| final AuthorizerMapper authorizerMapper, | ||
| final Collection<? super ResType> filteredResources |
There was a problem hiding this comment.
What's the reason for having an out-parameter instead of returning a Collection<ResType> or Iterable<ResType>? I'm wondering since out-params are harder to comprehend than return values.
I'm thinking that many callers would want to directly use a returned filteredResources, and for the callers that don't, they could do:
Iterables.addAll(filteredResources, filterAuthorizedResources(...))`There was a problem hiding this comment.
I have the out-parameter so that the caller can control the type of the Collection that gets the filtered values.
I implemented that way after seeing that SupervisorResource uses Sets for storing supervisor ids, maybe a future caller really cares about using a Set to avoid duplicates, and I thought it'd be better to have the caller pass in the output collection to avoid having to create extra lists and copy them over into other structures.
There was a problem hiding this comment.
In that case, I think it's best for filterAuthorizedResources to return an Iterable< ResType> (and maybe accept one as well, instead of the Collection it accepts now). There won't be any need to create useless collections since the caller can iterate the iterable and directly insert it into the target collection, using Iterables.addAll.
There was a problem hiding this comment.
alright, changed filtereAuthorizedResources to return Iterable
| final Set<String> authorizedSupervisorIds = Sets.newHashSet(); | ||
|
|
||
| // If there were no supervisorIds, go ahead and authorize the request. | ||
| if (supervisorIds.size() == 0) { |
There was a problem hiding this comment.
Is it necessary to have a separate branch for this case? Shouldn't filterAuthorizedResources on an empty list do the same thing?
There was a problem hiding this comment.
It's not, I had it there just to avoid creating the resource generating function if there were on supervisorIds, removed this
| supervisorHistory.keySet() | ||
| ); | ||
|
|
||
| final Map<String, List<VersionedSupervisorSpec>> authorizedSupervisorHistory = Maps.filterKeys( |
There was a problem hiding this comment.
This object is unused. Something is wrong with this code.
There was a problem hiding this comment.
thanks for the catching that, opened a fix at #5460
4 participants
Fixes #4813
Changes the AuthorizationUtils.filterAuthorizedResources() method and SupervisorResource to prevent multiple authorization checks being performed on requests (See #4816 (review) for context).