Communication: Fix mentioned users recieving notifications in conversations they are not part of
#9814
Conversation
…on/fix-notification-in-private-messages
PaRangger
added
bugfix
communication
github-actions
bot
added
the
server
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe changes involve modifications to the Changes
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java (2)
90-90: Update JavaDoc to reflect implementation detailsWhile the code change is correct, consider updating the method's JavaDoc to document that the notification creation now includes recipient filtering based on conversation access.
103-106: Consider refactoring for better readability and separation of concernsThe current implementation combines filtering and notification creation in a complex stream operation. Consider breaking this down for better readability and maintainability:
private void save(ConversationNotification notification, Set<User> mentionedUsers, String[] placeHolders, Post createdMessage) { conversationNotificationRepository.save(notification); - Set<SingleUserNotification> mentionedUserNotifications = singleUserNotificationService - .filterAllowedRecipientsInMentionedUsers(mentionedUsers, createdMessage.getConversation()).map(mentionedUser -> SingleUserNotificationFactory - .createNotification(notification.getMessage(), NotificationType.CONVERSATION_USER_MENTIONED, notification.getText(), placeHolders, mentionedUser)) - .collect(Collectors.toSet()); + // First filter allowed recipients + Set<User> allowedRecipients = singleUserNotificationService + .filterAllowedRecipientsInMentionedUsers(mentionedUsers, createdMessage.getConversation()) + .collect(Collectors.toSet()); + + // Then create notifications for allowed recipients + Set<SingleUserNotification> mentionedUserNotifications = allowedRecipients.stream() + .map(mentionedUser -> SingleUserNotificationFactory.createNotification( + notification.getMessage(), + NotificationType.CONVERSATION_USER_MENTIONED, + notification.getText(), + placeHolders, + mentionedUser)) + .collect(Collectors.toSet()); singleUserNotificationRepository.saveAll(mentionedUserNotifications); }src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (3)
461-472: Consider extracting complex conditions into well-named methods for better readability.The filtering logic combines multiple conditions that could be more readable if extracted into separate methods.
Consider this refactoring:
public Stream<User> filterAllowedRecipientsInMentionedUsers(Set<User> mentionedUsers, Conversation conversation) { return mentionedUsers.stream().filter(user -> { - boolean isChannelAndCourseWide = conversation instanceof Channel channel && channel.getIsCourseWide(); - boolean isChannelVisibleToStudents = !(conversation instanceof Channel channel) || conversationService.isChannelVisibleToStudents(channel); - boolean isChannelVisibleToMentionedUser = isChannelVisibleToStudents || authorizationCheckService.isAtLeastTeachingAssistantInCourse(conversation.getCourse(), user); - - // Only send a notification to the mentioned user if... - // (for course-wide channels) ...the course-wide channel is visible - // (for all other cases) ...the user is a member of the conversation - return (isChannelAndCourseWide && isChannelVisibleToMentionedUser) || conversationService.isMember(conversation.getId(), user.getId()); + return canReceiveNotification(user, conversation); }); } +private boolean canReceiveNotification(User user, Conversation conversation) { + if (conversation instanceof Channel channel) { + if (channel.getIsCourseWide()) { + return isChannelVisibleToUser(channel, user); + } + } + return conversationService.isMember(conversation.getId(), user.getId()); +} + +private boolean isChannelVisibleToUser(Channel channel, User user) { + return conversationService.isChannelVisibleToStudents(channel) || + authorizationCheckService.isAtLeastTeachingAssistantInCourse(channel.getCourse(), user); +}
446-447: Add defensive programming checks for robustness.The method should handle potential null values and service exceptions gracefully.
Consider adding these safety checks:
+ if (mentionedUsers == null || mentionedUsers.isEmpty()) { + return; + } + try { filterAllowedRecipientsInMentionedUsers(mentionedUsers, post.getConversation()) .forEach(mentionedUser -> notifyUserAboutNewMessageReply(savedAnswerMessage, notification, mentionedUser, author, CONVERSATION_USER_MENTIONED)); + } catch (Exception e) { + // Log error but don't block the notification flow for other users + log.error("Failed to process mentioned users notifications", e); + }
454-473: Implementation effectively addresses the security concern.The new filtering logic successfully prevents unauthorized access to conversation content by:
- Properly checking conversation membership
- Respecting channel visibility settings
- Handling both course-wide channels and private conversations
Consider adding metrics to monitor filtered notifications for security auditing purposes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/ConversationNotificationService.java (2)
44-52: LGTM: Clean dependency injection implementation
The addition of SingleUserNotificationService follows best practices with constructor injection and immutable field declaration.
100-100: Security fix implemented correctly
The method now properly filters recipients based on conversation access, addressing the security issue where users could receive notifications for conversations they're not part of.
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
cremertim
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
asliayk
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
Cathy0123456789
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
deleted the
bugfix/communication/fix-notification-in-private-messages
branch
Checklist
General
Server
Motivation and Context
Currently, when a user who is not part of a conversation (e.g., private channels) is mentioned in that conversation, the server stores a notification for them in the database. This behavior grants the user access to view messages from the conversation despite not being a participant, resulting in potential information leakage.
Addresses #9588
Description
This was not the case for answer posts. Thus I used the filtering method of the answer posts function to check if a mentioned user is permitted to recieve a notification. This should not lead to additional calls to the database, since the method used is the .getConversation method of a post, which is used beforehand.
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Server
Summary by CodeRabbit
New Features
Bug Fixes
Documentation