-
Notifications
You must be signed in to change notification settings - Fork 32
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
SO-5566 Version API search restrictions #1226
base: 9.x
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 9.x #1226 +/- ##
============================================
+ Coverage 47.96% 48.00% +0.03%
- Complexity 13831 13861 +30
============================================
Files 1942 1942
Lines 94946 94971 +25
Branches 10955 10961 +6
============================================
+ Hits 45541 45588 +47
+ Misses 46378 46353 -25
- Partials 3027 3030 +3 ☔ View full report in Codecov by Sentry. |
Set<String> accessibleResources = authz.getAccessibleResources(context, context.service(User.class)); | ||
|
||
resourceIds.removeIf(resourceId -> !accessibleResources.contains(resourceId)); | ||
queryBuilder.filter(VersionDocument.Expressions.resourceIds(resourceIds)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If resourceIds
becomes empty after authorization check, the search request can return quickly by throwing a NoResultException.
if (containsKey(OptionKey.RESOURCE)) { | ||
Collection<String> resources = getCollection(OptionKey.RESOURCE, String.class); | ||
Collection<String> resourceIds = resources.stream() | ||
.map(resource -> resource.contains("/") ? new ResourceURI(resource).getResourceId() : resource) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.map(resource -> resource.contains("/") ? new ResourceURI(resource).getResourceId() : resource) | |
.map(resource -> resource.contains(Branch.SEPARATOR) ? new ResourceURI(resource).getResourceId() : resource) |
.map(resource -> resource.contains("/") ? new ResourceURI(resource).getResourceId() : resource) | ||
.toList(); | ||
|
||
if (user.isAdministrator()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not just the administrator user can access all resources, any user with the browse:*
permission can do that, please fix and create test cases for the various scenarios. Thanks!
@@ -172,6 +172,7 @@ public String getResourceId() { | |||
return getResource().getResourceId(); | |||
} | |||
|
|||
@JsonIgnore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a valid API response property. Please remove this JsonIgnore
annotation.
This reverts commit 89e46ebabf07609c9fe285f3b57ff5c2c34a7f6e.
resource id set is mutable
6bec14a
to
0fdc6f4
Compare
No description provided.