fix-18269 Users without access are able perform bulk Add/Remove team Assets using API#20150
fix-18269 Users without access are able perform bulk Add/Remove team Assets using API#20150sonika-shah wants to merge 3 commits intomainfrom
Conversation
sonika-shah
added
backend
safe to test
sonika-shah
changed the title
🛡️ TRIVY SCAN RESULT 🛡️ Target:
|
|
| return importCsvInternalAsync(securityContext, name, csv, dryRun); | ||
| } | ||
|
|
||
| public final void verifyUserPermission(SecurityContext securityContext) { |
There was a problem hiding this comment.
hi @sonika-shah can we add it as a different operation itself ADD_ASSETS?
this seems like to much to handle single permission of Adding assets!
There was a problem hiding this comment.
don't want to build if else, we have flexible roles and policies which can be used to associate operation to different resource
There was a problem hiding this comment.
hi @mohityadav766 , will check
with the current implementations, when we add multiple operations to the operation_context, it treats them as an AND condition—all permissions must be present to authorize.
that’s why I implemented it with if else so that if any of the listed operations is present, the user is authorized.
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses a security vulnerability where users without proper access permissions could perform bulk add/remove operations on team assets through the API. The fix introduces permission verification to ensure only authorized users can perform these operations.
- Adds permission verification for bulk add/remove team asset operations
- Implements a new
verifyUserPermissionmethod to check user authorization - Ensures only users with appropriate metadata operations permissions can access these endpoints
| return importCsvInternalAsync(securityContext, name, csv, dryRun); | ||
| } | ||
|
|
||
| public final void verifyUserPermission(SecurityContext securityContext) { |
There was a problem hiding this comment.
The method should be private instead of public since it's only used internally within this class and doesn't need to be exposed as part of the public API.
| public final void verifyUserPermission(SecurityContext securityContext) { | |
| private final void verifyUserPermission(SecurityContext securityContext) { |
| perm.getOperation()) | ||
| || MetadataOperation.EDIT_OWNERS.equals( | ||
| perm.getOperation())) | ||
| && Permission.Access.ALLOW.equals(perm.getAccess()))); |
There was a problem hiding this comment.
The permission check retrieves all permissions for the user and then filters them. This could be inefficient for users with many permissions. Consider using a more targeted permission check if the authorizer supports it.
| && Permission.Access.ALLOW.equals(perm.getAccess()))); | |
| // Check for required permissions using targeted authorizer checks | |
| boolean hasPermission = | |
| authorizer.hasPermission(securityContext, user, Entity.ALL_RESOURCES, MetadataOperation.ALL) || | |
| authorizer.hasPermission(securityContext, user, Entity.ALL_RESOURCES, MetadataOperation.EDIT_ALL) || | |
| authorizer.hasPermission(securityContext, user, Entity.TEAM, MetadataOperation.ALL) || | |
| authorizer.hasPermission(securityContext, user, Entity.TEAM, MetadataOperation.EDIT_ALL) || | |
| authorizer.hasPermission(securityContext, user, Entity.TEAM, MetadataOperation.EDIT_TEAMS) || | |
| authorizer.hasPermission(securityContext, user, Entity.TEAM, MetadataOperation.EDIT_OWNERS) || | |
| authorizer.hasPermission(securityContext, user, Entity.USER, MetadataOperation.ALL) || | |
| authorizer.hasPermission(securityContext, user, Entity.USER, MetadataOperation.EDIT_ALL) || | |
| authorizer.hasPermission(securityContext, user, Entity.USER, MetadataOperation.EDIT_TEAMS) || | |
| authorizer.hasPermission(securityContext, user, Entity.USER, MetadataOperation.EDIT_OWNERS); |
|
|
||
| boolean isAdminOrBot = subjectContext.isAdmin() || subjectContext.isBot(); | ||
|
|
||
| if (!hasPermission && !isAdminOrBot) { |
There was a problem hiding this comment.
The admin/bot check should be performed before the expensive permission lookup to short-circuit the authorization check for privileged users and improve performance.
| if (!hasPermission && !isAdminOrBot) { | |
| if (!hasPermission) { |
| String csv) { | ||
| return importCsvInternalAsync(securityContext, name, csv, dryRun); | ||
| } | ||
|
|
There was a problem hiding this comment.
The method lacks JavaDoc documentation explaining its purpose, parameters, and the conditions under which it throws an AuthorizationException.
| /** | |
| * Verifies that the user associated with the provided {@link SecurityContext} has permission to perform | |
| * operations on Team and User entities. The user must have one of the required metadata operations | |
| * (ALL, EDIT_ALL, EDIT_TEAMS, EDIT_OWNERS) on the resources (USER, TEAM), or be an admin or bot. | |
| * | |
| * @param securityContext the security context containing user authentication information | |
| * @throws AuthorizationException if the user does not have the required permissions | |
| */ |
|
this is already fixed with pr : Missing Permission on Adding Users to team (#20768) |
Describe your changes:
Fixes #18269
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>