Backend/auth#92
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @szymongrabski, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on enhancing the backend's authorization mechanisms, particularly for group-related functionalities. It introduces a dedicated SecurityService to centralize access control logic, modifies existing endpoints and services to leverage this service by passing the authenticated user object, and adds specific checks for group membership and ownership before allowing operations like viewing chat messages, updating groups, or managing group members. A new exception and handler are included for clearer error reporting on authorization failures.
Highlights
- Centralized Authorization: Introduced a new
SecurityServiceto centralize authorization logic related to group membership and ownership checks. - Authenticated User Propagation: Modified controller and service methods across Activity, Group, and GroupMembership features to accept the authenticated
Userobject directly, enabling granular authorization checks within the service layer. - Group Chat Authorization: Added a check in the
GroupChatControllerto ensure the requesting user is a member of the group before allowing access to chat messages. - Custom Exception Handling: Added a new custom exception
UserIsNotGroupOwnerand an exception handler to return a 403 Forbidden status when a user attempts an action requiring group ownership without the necessary role. - Test Coverage: Updated existing tests and added new ones (including a new test class for GroupBalanceService) to cover the new authorization checks and ensure the security logic behaves as expected.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Code Coverage
|
There was a problem hiding this comment.
Code Review
This pull request significantly enhances backend authorization by introducing a SecurityService for centralized checks and consistently applying these checks across controllers and services. Key improvements include standardized use of @AuthenticationPrincipal User, delegation of authorization decisions, a new UserIsNotGroupOwner exception, and updated tests. The changes generally improve security and maintainability.
A few areas for discussion and minor improvements are noted in the specific comments. The inclusion of GroupBalanceServiceTest.java seems somewhat out of scope for an "auth" focused PR, but it has been reviewed as part of the changeset and appears to be well-tested.
| Group group = groupRepository.findById(groupId) | ||
| .orElseThrow(() -> new GroupNotFoundException(groupId)); | ||
|
|
||
| securityService.checkIfUserIsInGroup(user, group); |
There was a problem hiding this comment.
The retrieval of the Group entity followed by a security check securityService.checkIfUserIsInGroup(user, group) is a good practice for ensuring the user has access to the group chat. This explicit check enhances security before accessing sensitive chat data.
|
|
||
| @ExceptionHandler(UserIsNotGroupOwner.class) | ||
| public final ResponseEntity<ErrorDetails> userIsNotInGroupOwnerException(Exception ex, WebRequest request) { | ||
| ErrorDetails errorDetails = new ErrorDetails(LocalDateTime.now(), ex.getMessage(), request.getDescription(false), ex.getStackTrace().length); |
There was a problem hiding this comment.
The ErrorDetails constructor is called with ex.getStackTrace().length as the argument for what appears to be an error count field. The length of a stack trace does not typically represent a count of errors. This pattern is present in the newly added userIsNotInGroupOwnerException handler. It's advisable to use a more semantically correct value for this field or clarify its purpose in ErrorDetails. For validation errors, ex.getBindingResult().getErrorCount() is used, which is appropriate.
| Activity activity = activityRepository.findById(activityId) | ||
| .orElseThrow(() -> new ActivityNotFoundException("Activity with ID: " + activityId + " not found")); | ||
|
|
||
| securityService.checkIfUserIsInGroup(currentUser, activity.getGroup()); |
There was a problem hiding this comment.
Fetching the Activity entity before deletion to perform securityService.checkIfUserIsInGroup(currentUser, activity.getGroup()) is a good security practice. This ensures that the authorization check is performed against the correct group associated with the activity being deleted.
| Group group = groupRepository.findById(groupId) | ||
| .orElseThrow(() -> new GroupNotFoundException(groupId)); | ||
|
|
||
| securityService.checkIfUserIsInGroup(currentUser, group); |
There was a problem hiding this comment.
The authorization check securityService.checkIfUserIsInGroup(currentUser, group) ensures the currentUser is a member of the group before they can add another user. If the design intends for any group member to add new users, this is sufficient. However, if adding members should be restricted to specific roles (e.g., ADMIN, OWNER), this check might need to be augmented with a role-specific verification to ensure the currentUser has the necessary privileges.
| public void checkIfUserCanRemoveMember(User currentUser, User userToRemove, Group group) { | ||
| GroupMembership membership = findMembershipOrThrow(currentUser, group); | ||
| if (membership.getRole() != GroupRole.OWNER && !currentUser.getId().equals(userToRemove.getId())) { | ||
| throw new UserIsNotGroupOwner(currentUser.getId(), group.getId()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in checkIfUserCanRemoveMember correctly allows a group owner to remove members and also allows any member to remove themselves from the group. This is a common and sensible policy. The use of findMembershipOrThrow for the currentUser ensures they are part of the group before attempting role checks or self-removal logic.
| mockMvc.perform(delete(URI + "/{userId}", testGroup.getId(), newUser.getId()) | ||
| .with(user(testUser)) | ||
| .principal(() -> String.valueOf(testUser.getId()))) | ||
| .andExpect(status().isConflict()) | ||
| .andExpect(jsonPath("$.message").value("User is not a member of the group")); | ||
| .andExpect(status().isConflict()); |
There was a problem hiding this comment.
In removeNonMemberTest, the assertion for the error message jsonPath("$.message") was removed. When a user who is not a member of the group is attempted to be removed, GroupMembershipService throws UserIsNotInGroupException with the message "User is not a member of the group". It would make the test more robust to reinstate the check for this specific error message to ensure the correct exception and reason are being surfaced.
mockMvc.perform(delete(URI + "/{userId}", testGroup.getId(), newUser.getId())
.with(user(testUser))
.principal(() -> String.valueOf(testUser.getId())))
.andExpect(status().isConflict())
.andExpect(jsonPath("$.message").value("User is not a member of the group"));| doNothing().when(groupMembershipRepository).delete(testMembership); | ||
|
|
||
| groupMembershipService.removeMemberFromGroup(1L, 1L); | ||
| groupMembershipService.removeMemberFromGroup(1L, 2L, testUser); |
There was a problem hiding this comment.
In tests involving testUser2 (e.g., testRemoveMemberFromGroupSuccess, testRemoveMemberFromGroupOwnerCannotBeRemoved, testUpdateMemberRoleSuccess), the mock when(groupMembershipRepository.findByGroupAndUser(testGroup, testUser2)).thenReturn(Optional.ofNullable(testMembership)); uses testMembership. This testMembership instance is initialized in setUp() for testUser. While this might work if testMembership's state is manipulated correctly per test, it can be confusing. For better test clarity and maintainability, consider creating a distinct GroupMembership instance for testUser2 within the setup of these specific tests or ensure testMembership is clearly repurposed and documented for testUser2's context.
Code Coverage
|
3 participants
No description provided.