[PM-18747] Add more protections to endpoints on backend so regular members can't hit endpoints and see data#7985
Conversation
…n management - Replaced GroupAuthorizationHandler with OrganizationCollectionManagementAccessHandler to streamline collection management access control. - Introduced CollectionPermissions class for reusable permission checks related to collection creation. - Updated BulkCollectionAuthorizationHandler to utilize the new permission checks. - Modified GroupsController and OrganizationUsersController to use the new authorization requirement. - Added integration tests for new authorization logic and permissions. - Removed obsolete GroupAuthorizationHandler and related operations. This refactor enhances the clarity and maintainability of the authorization logic for collection management within organizations.
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR tightens authorization on the Code Review DetailsNo findings. The implementation follows existing authorization patterns (mirrors |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Updated the OrganizationCollectionManagementAccessRequirement to include custom permissions (ManageUsers, ManageGroups, AccessReports) for accessing basic organization member information. - Modified the OrganizationUsersController to allow all organization members to retrieve basic member details, reflecting the broader need for member lookups across various app features. - Added integration tests to validate the new authorization logic for custom users with specific permissions. - Removed obsolete tests related to collection access that are no longer applicable. These changes improve the flexibility and clarity of access control for organization members while ensuring necessary permissions are respected.
…member-and-group-details
BTreston
left a comment
BTreston
left a comment
There was a problem hiding this comment.
LGTM aside from the comment verbosity. I'd recommend shortening up the comments. Adding a backend dev for review, as they'll probably have better opinions/feedback
JimmyVo16
left a comment
JimmyVo16
left a comment
There was a problem hiding this comment.
Just one confirmation question, but it's pretty much approved.
| /// of the organization, since a broad range of features across the app depend on basic member lookups | ||
| /// (collection management, group management, event logs, sponsorship, etc.) that are not specific to any one | ||
| /// permission. For this reason, it contains as little information as possible and no cryptographic keys or | ||
| /// other sensitive data. |
There was a problem hiding this comment.
Just want to confirm that this is the intended behavior. Are we keeping the permission check the same and only changing the comments?
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-18747
📔 Objective
Adds more permissions checks to backend so members can just hit the endpoint and see details not meant for them. Not too much of a security threat because this is not sensitive, but still something we figured we should tighten up