Integrated code lifecycle: Add email notifications for SSH keys
#9918
Conversation
github-actions
bot
added
server
WalkthroughThe changes in this pull request involve the introduction of new notification types related to SSH key management across various components of the application. Specifically, new enum values and constants have been added to represent notifications for SSH keys being added, expiring soon, and having expired. Additionally, methods and services have been updated or created to handle these notifications, including email templates for user notifications. Localization files have also been updated to include relevant messages in both English and German. Changes
Suggested labels
Suggested reviewers
Possibly related PRs
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: 7
🧹 Outside diff range and nitpick comments (15)
src/main/resources/i18n/messages.properties (1)
133-133: Consider adding the number of days until expiry.The warning message could be more specific by including the number of days until expiry.
-email.notification.sshKeyExpiry.sshKeyExpiresSoonWarning = One of your SSH keys will expire in a few days. +email.notification.sshKeyExpiry.sshKeyExpiresSoonWarning = One of your SSH keys will expire in {0} days.src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/NotificationSettingsService.java (1)
186-191: Update comment to reflect both data export and SSH notificationsThe comment above these settings only mentions "data export notification setting" but should be updated to include SSH notifications as well.
- // data export and SSH notification setting (cannot be overridden by user) + // data export and SSH notification settings (cannot be overridden by user)src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java (1)
Line range hint
39-65: Consider adding transaction boundary and error handling.While the notification integration is good, consider the following improvements:
- Add transaction boundary to ensure atomicity between key creation and notification
- Add explicit error handling for notification failures
Here's a suggested implementation:
+@Transactional public void createSshKeyForUser(User user, AuthorizedKeyEntry keyEntry, UserSshPublicKeyDTO sshPublicKey) throws GeneralSecurityException, IOException { PublicKey publicKey = keyEntry.resolvePublicKey(null, null, null); String keyHash = HashUtils.getSha512Fingerprint(publicKey); if (userSshPublicKeyRepository.findByKeyHash(keyHash).isPresent()) { throw new BadRequestAlertException("Key already exists", "SSH key", "keyAlreadyExists", true); } UserSshPublicKey newUserSshPublicKey = new UserSshPublicKey(); newUserSshPublicKey.setUserId(user.getId()); newUserSshPublicKey.setPublicKey(sshPublicKey.publicKey()); newUserSshPublicKey.setKeyHash(keyHash); setLabelForKey(newUserSshPublicKey, sshPublicKey.label()); newUserSshPublicKey.setCreationDate(ZonedDateTime.now()); newUserSshPublicKey.setExpiryDate(sshPublicKey.expiryDate()); - userSshPublicKeyRepository.save(newUserSshPublicKey); - singleUserNotificationService.notifyUserAboutNewlyAddedSshKey(user, newUserSshPublicKey); + try { + UserSshPublicKey savedKey = userSshPublicKeyRepository.save(newUserSshPublicKey); + singleUserNotificationService.notifyUserAboutNewlyAddedSshKey(user, savedKey); + } catch (Exception e) { + throw new BadRequestAlertException("Failed to create SSH key: " + e.getMessage(), "SSH key", "keyCreationFailed", true); + } }src/main/resources/templates/mail/notification/sshKeyHasExpiredEmail.html (1)
28-28: Replace empty paragraph with CSS-based spacingUsing an empty
<p></p>tag for spacing is not recommended. Consider using CSS margins instead.- <p></p> + <p class="spacing-bottom"> + <span th:text="#{email.notification.sshKeyExpiry.renew}">You can renew your SSH key here:</span> + </p>src/main/webapp/i18n/de/notification.json (1)
157-159: Fix inconsistent placeholder spacing in messages.The messages correctly use informal language ("du/dein") and are clear, but there's an inconsistency in the placeholder spacing.
Apply this diff to standardize the placeholder spacing:
- "sshKeyExpiresSoon": "Dein SSH-Schlüssel mit dem Label \"{{ placeholderValues.0}}\" läuft am {{ placeholderValues.1 }} ab.", - "sshKeyHasExpired": "Dein SSH-Schlüssel mit dem Label \"{{ placeholderValues.0}}\" ist am {{ placeholderValues.1 }} abgelaufen." + "sshKeyExpiresSoon": "Dein SSH-Schlüssel mit dem Label \"{{ placeholderValues.0 }}\" läuft am {{ placeholderValues.1 }} ab.", + "sshKeyHasExpired": "Dein SSH-Schlüssel mit dem Label \"{{ placeholderValues.0 }}\" ist am {{ placeholderValues.1 }} abgelaufen."src/main/resources/templates/mail/notification/sshKeyExpiresSoonEmail.html (1)
32-34: Improve link presentation in email.Instead of displaying the full URL as link text, consider using a more user-friendly text to avoid exposing internal URLs in emails.
Apply this diff:
- <a th:with="url=(@{|${baseUrl}/user-settings/ssh|})" th:href="${url}" th:text="${url}">Login - link</a> + <a th:with="url=(@{|${baseUrl}/user-settings/ssh|})" th:href="${url}" th:text="#{email.notification.sshKeyExpiry.renewLinkText}">Renew SSH Key</a>src/main/webapp/i18n/en/notification.json (2)
158-159: Fix placeholder formatting inconsistencyThe placeholder syntax has an inconsistent space before the closing brace.
Apply this diff to maintain consistent formatting:
- "sshKeyExpiresSoon": "Your SSH key with the label \"{{ placeholderValues.0}}\" will expire on {{ placeholderValues.1 }}.", - "sshKeyHasExpired": "Your SSH key with the label \"{{ placeholderValues.0}}\" has expired on {{ placeholderValues.1 }}." + "sshKeyExpiresSoon": "Your SSH key with the label \"{{ placeholderValues.0 }}\" will expire on {{ placeholderValues.1 }}.", + "sshKeyHasExpired": "Your SSH key with the label \"{{ placeholderValues.0 }}\" has expired on {{ placeholderValues.1 }}."
157-157: Consider adding more context to the SSH key added messageThe "sshKeyAdded" message could be more informative by including the key's label, similar to the other SSH key notifications.
Consider this enhancement:
- "sshKeyAdded": "You have successfully added a new SSH key", + "sshKeyAdded": "You have successfully added a new SSH key with the label \"{{ placeholderValues.0 }}\"",src/main/resources/templates/mail/notification/sshKeyAddedEmail.html (1)
26-28: Improve link text for better clarityThe hardcoded "Login link" text is misleading as this is a link to the SSH key settings page. Consider using a more descriptive text or adding it to the localization messages.
Apply this diff to improve clarity:
- <a th:with="url=(@{|${baseUrl}/user-settings/ssh|})" th:href="${url}" th:text="${url}">Login - link</a> + <a th:with="url=(@{|${baseUrl}/user-settings/ssh|})" th:href="${url}" th:text="#{email.notification.sshKeyAdded.settingsLink}">SSH Key Settings</a>src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (1)
285-285: Fix incorrect JavaDoc descriptionThe JavaDoc for
notifyUserAboutExpiredSshKeyincorrectly states "about an upcoming expiry" instead of "about an expired key".- * Notify user about an upcoming expiry of an SSH key + * Notify user about an expired SSH keysrc/main/resources/i18n/messages_de.properties (1)
129-138: Consider enhancing the SSH key notification messagesWhile the messages are generally well-structured, consider these improvements for better user experience:
- Add specific timeframe for "läuft in wenigen Tagen ab" to set clear expectations
- Consider adding security advice for the key addition message
Suggested improvements:
email.notification.sshKeyAdded.title = Der folgende SSH-Schlüssel wurde zu deinem Benutzerkonto hinzugefügt: +email.notification.sshKeyAdded.security = Stelle sicher, dass du diesen Schlüssel erkennst und er von dir hinzugefügt wurde. email.notification.sshKeyAdded.ifMistake = Wenn du glaubst, dass dieser Schlüssel irrtümlich hinzugefügt wurde, kannst du den Schlüssel entfernen und den Zugriff an folgender Stelle deaktivieren: -email.notification.sshKeyExpiry.sshKeyExpiresSoonWarning = Einer deiner SSH-Schlüssel läuft in wenigen Tagen ab. +email.notification.sshKeyExpiry.sshKeyExpiresSoonWarning = Einer deiner SSH-Schlüssel läuft in 7 Tagen ab.src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.java (2)
153-157: Consider grouping SSH key-related constants togetherWhile the constants follow the naming convention, consider grouping all SSH key-related constants (titles and texts) in a dedicated section for better maintainability. This would make it easier to manage related notifications.
323-324: Consider breaking down long builder chains for better readabilityThe builder chain is becoming quite long. Consider breaking it down into multiple lines for better readability:
- .put(PROGRAMMING_REPOSITORY_LOCKS, PROGRAMMING_REPOSITORY_LOCKS_TITLE).put(PROGRAMMING_BUILD_RUN_UPDATE, PROGRAMMING_BUILD_RUN_UPDATE_TITLE) - .put(SSH_KEY_ADDED, SSH_KEY_ADDED_TITLE).put(SSH_KEY_EXPIRES_SOON, SSH_KEY_EXPIRES_SOON_TITLE).put(SSH_KEY_HAS_EXPIRED, SSH_KEY_HAS_EXPIRED_TITLE).build(); + .put(PROGRAMMING_REPOSITORY_LOCKS, PROGRAMMING_REPOSITORY_LOCKS_TITLE) + .put(PROGRAMMING_BUILD_RUN_UPDATE, PROGRAMMING_BUILD_RUN_UPDATE_TITLE) + // SSH key notifications + .put(SSH_KEY_ADDED, SSH_KEY_ADDED_TITLE) + .put(SSH_KEY_EXPIRES_SOON, SSH_KEY_EXPIRES_SOON_TITLE) + .put(SSH_KEY_HAS_EXPIRED, SSH_KEY_HAS_EXPIRED_TITLE) + .build();src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java (1)
Line range hint
1-476: Consider splitting responsibilities to improve maintainability.The
MailServiceclass is handling multiple responsibilities:
- Email sending
- Content creation
- Template processing
- Notification type handling
Consider extracting some of these responsibilities into separate services to better adhere to the Single Responsibility Principle. For example:
NotificationContentServicefor content creationTemplateProcessingServicefor template handlingThis would make the codebase more maintainable and easier to test.
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java (1)
171-184: Consider adding HIGH priority for key expiry notificationsThe implementation looks good, but consider setting HIGH priority for SSH_KEY_EXPIRES_SOON and SSH_KEY_HAS_EXPIRED notifications, as these are time-sensitive security-related alerts that require prompt user attention.
Apply this diff to enhance the notifications:
case SSH_KEY_EXPIRES_SOON -> { - return new SingleUserNotification(recipient, SSH_KEY_EXPIRES_SOON_TITLE, SSH_KEY_EXPIRES_SOON_TEXT, true, createPlaceholdersSshKeyNotification(key)); + var notification = new SingleUserNotification(recipient, SSH_KEY_EXPIRES_SOON_TITLE, SSH_KEY_EXPIRES_SOON_TEXT, true, createPlaceholdersSshKeyNotification(key)); + notification.setPriority(HIGH); + return notification; } case SSH_KEY_HAS_EXPIRED -> { - return new SingleUserNotification(recipient, SSH_KEY_HAS_EXPIRED_TITLE, SSH_KEY_HAS_EXPIRED_TEXT, true, createPlaceholdersSshKeyNotification(key)); + var notification = new SingleUserNotification(recipient, SSH_KEY_HAS_EXPIRED_TITLE, SSH_KEY_HAS_EXPIRED_TEXT, true, createPlaceholdersSshKeyNotification(key)); + notification.setPriority(HIGH); + return notification; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (18)
src/main/java/de/tum/cit/aet/artemis/communication/domain/NotificationType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/NotificationSettingsService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/repository/UserSshPublicKeyRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/sshuserkeys/SshPublicKeysResource.java(2 hunks)src/main/resources/i18n/messages.properties(2 hunks)src/main/resources/i18n/messages_de.properties(3 hunks)src/main/resources/i18n/messages_en.properties(2 hunks)src/main/resources/templates/mail/notification/sshKeyAddedEmail.html(1 hunks)src/main/resources/templates/mail/notification/sshKeyExpiresSoonEmail.html(1 hunks)src/main/resources/templates/mail/notification/sshKeyHasExpiredEmail.html(1 hunks)src/main/webapp/i18n/de/notification.json(2 hunks)src/main/webapp/i18n/en/notification.json(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/web/sshuserkeys/SshPublicKeysResource.java
🧰 Additional context used
📓 Path-based instructions (10)
src/main/java/de/tum/cit/aet/artemis/programming/repository/UserSshPublicKeyRepository.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/domain/NotificationType.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/programming/service/sshuserkeys/UserSshPublicKeyService.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/webapp/i18n/de/notification.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
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
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.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/MailService.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/NotificationSettingsService.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/domain/notification/SingleUserNotificationFactory.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/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.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
📓 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9478
File: src/main/java/de/tum/cit/aet/artemis/programming/service/UserSshPublicKeyService.java:125-131
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In `UserSshPublicKeyService.java`, handling `EmptyResultDataAccessException` during SSH key deletion in the `deleteUserSshPublicKey` method is unnecessary, as it is redundant in this context.
🔇 Additional comments (38)
src/main/java/de/tum/cit/aet/artemis/programming/repository/UserSshPublicKeyRepository.java (2)
5-5: LGTM!
The import statement follows Java coding guidelines by importing the specific class rather than using star imports.
25-25: Consider optimizing the query performance
The method signature follows Spring Data JPA conventions and supports the SSH key expiry notification feature. However, consider these optimizations:
- Add an explicit query annotation for better control over the generated SQL
- Consider adding an index on the expiry_date column if not already present
+ @Query("SELECT k FROM UserSshPublicKey k WHERE k.expiryDate BETWEEN :from AND :to")
List<UserSshPublicKey> findByExpiryDateBetween(ZonedDateTime from, ZonedDateTime to);Let's verify if there's an index on the expiry_date column:
src/main/java/de/tum/cit/aet/artemis/communication/domain/NotificationType.java (1)
12-13: LGTM! Well-structured enum additions.
The new notification types for SSH key lifecycle events are appropriately named and placed at the end of the enum, following Java enum best practices.
src/main/resources/i18n/messages.properties (2)
129-139: LGTM! Security-conscious message templates.
The message templates are well-structured and follow security best practices by:
- Not exposing sensitive information
- Providing clear instructions for key removal
- Including essential metadata for key identification
166-168: LGTM! Consistent notification titles.
The notification titles follow the established pattern and maintain consistency with other notification types in the system.
src/main/resources/i18n/messages_en.properties (2)
129-139: LGTM! Proper i18n implementation.
The English translations are properly synchronized with the default message properties, following i18n best practices.
Also applies to: 165-167
129-139: Verify German translations.
Since this is a bilingual system (EN/DE), ensure that corresponding German translations are available.
✅ Verification successful
German translations are properly implemented
All SSH key related messages have corresponding German translations in messages_de.properties. The translations are complete and semantically appropriate for all the new SSH key notification messages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if German translations exist for the new SSH key messages
rg "^email\.(sshKey|notification\.sshKey)" src/main/resources/i18n/messages_de.properties
Length of output: 961
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/NotificationSettingsService.java (4)
35-37: LGTM!
The import statements for SSH key notification types are properly organized and follow the existing pattern.
140-145: LGTM!
The SSH notification constants are well-organized, follow the naming convention, and are properly grouped with clear comments.
226-228: LGTM!
The notification type mappings for SSH notifications are correctly implemented with proper one-to-one mapping between settings and types.
238-239: Verify notification settings configuration
The SSH notifications have been added to the instant notification support set. Let's verify that this aligns with the UI configuration mentioned in the comment.
src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java (2)
Line range hint 1-27: LGTM! Package restructuring improves organization.
The move to a dedicated sshuserkeys subpackage better reflects the single responsibility principle and improves code organization.
Line range hint 28-38: LGTM! Constructor follows dependency injection best practices.
The constructor properly implements constructor injection, and the dependencies are correctly marked as final. The addition of SingleUserNotificationService follows the single responsibility principle by delegating notification logic.
src/main/resources/templates/mail/notification/sshKeyHasExpiredEmail.html (4)
13-27: Verify message keys and object access
The template uses proper internationalization, but let's ensure all required message keys exist and the object properties are accessed safely.
✅ Verification successful
All message keys and object properties are properly defined and used consistently
The verification confirms:
- All required message keys (
email.notification.sshKeyExpiry.*) exist in all i18n files (messages.properties, messages_en.properties, messages_de.properties) - The SSH key property access patterns (
getLabel()andgetKeyHash()) are used consistently across all notification email templates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all required message keys exist in the i18n files
echo "Checking for required message keys..."
rg "email\.notification\.sshKeyExpiry\." "src/main/resources/i18n/"
# Check SSH key object property access patterns in other templates
echo "Checking SSH key property access patterns..."
rg "sshKey\.get(Label|KeyHash)" "src/main/resources/templates/"
Length of output: 3357
1-4: Verify the existence of the head fragment template
The template structure looks good, but let's ensure the referenced fragment exists.
✅ Verification successful
Let me verify the actual content of the fragments.html file to ensure it contains the head fragment definition.
Let me check for the actual head fragment definition since we only found a reference to it.
The head fragment exists and is properly defined
The head fragment is correctly defined in fragments.html and includes all necessary components:
- Title with dynamic notification title
- Meta data and icon link fragment
- CSS styles fragment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the head fragment exists in the expected location
rg -l "fragments :: head" "src/main/resources/templates/mail/notification/"
Length of output: 1429
Script:
#!/bin/bash
# Check the content of fragments.html for head fragment definition
rg -A 5 "fragments :: head" "src/main/resources/templates/mail/notification/fragments.html"
Length of output: 279
Script:
#!/bin/bash
# Search for the head fragment definition in fragments.html
cat "src/main/resources/templates/mail/notification/fragments.html"
Length of output: 9723
30-34: Verify URL construction and security measures
The link construction needs verification for:
- XSS prevention
- Proper URL escaping
- Consistency with application routing
✅ Verification successful
Let me verify the routing and URL construction further by checking the template engine's configuration and URL handling.
Based on the gathered information, I can now provide the final response:
URL construction is secure and consistent with application routing
The URL construction in the email template is secure and properly implemented:
- Uses Thymeleaf's
@{...}URL expression syntax which automatically handles URL escaping - The
/user-settings/sshpath is consistently used across the application:- Defined in the Angular routing module
- Used in multiple email templates
- Referenced in component navigation
- The
baseUrlvariable is properly handled in the template engine context and used consistently across all notification templates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the URL pattern usage and routing
echo "Checking for similar URL patterns in other templates..."
rg "baseUrl.*user-settings/ssh" "src/main/resources/templates/"
echo "Checking for route definition..."
rg "user-settings/ssh" --type ts
Length of output: 4328
Script:
#!/bin/bash
# Check for Thymeleaf configuration and security settings
echo "Checking for Thymeleaf configuration..."
rg -A 5 "ThymeleafProperties|ThymeleafAutoConfiguration" --type java
echo "Checking for URL handling in templates..."
rg "@\{.*baseUrl.*\}" "src/main/resources/templates/"
echo "Checking for baseUrl definition..."
rg "baseUrl.*=" --type java
Length of output: 3339
1-40: Verify integration with notification system and security measures
As this template handles sensitive information:
- Ensure proper sanitization of SSH key details
- Verify integration with the notification service
- Consider rate limiting for security notifications
✅ Verification successful
Template is properly integrated with security measures and notification system
The verification shows that:
- The template is properly integrated with the notification system through
MailServiceandSingleUserNotificationService - The SSH key details are consistently handled across all SSH-related email templates using Thymeleaf's built-in escaping with
th:utext - Rate limiting is implemented at the service level through
IrisRateLimitServicefor sensitive operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check notification system integration
echo "Checking notification service integration..."
rg "sshKeyHasExpiredEmail" "src/main/java/"
echo "Checking security measures in similar notification templates..."
rg "keyHash" "src/main/resources/templates/mail/"
Length of output: 531
Script:
#!/bin/bash
# Check for sanitization and security measures in MailService and related components
echo "Checking MailService implementation..."
rg -A 5 "sshKeyHasExpiredEmail|SSH_KEY_HAS_EXPIRED" "src/main/java/"
echo "Checking other SSH key related templates and components..."
fd -e html -e java . | grep -i "ssh" | xargs rg "getKeyHash|sanitize|escape"
echo "Checking rate limiting implementations..."
rg "RateLimit|rateLimit" "src/main/java/"
Length of output: 42580
src/main/webapp/i18n/de/notification.json (1)
127-129: LGTM! Notification titles are clear and consistent.
The titles effectively communicate the SSH key states (added, expiring soon, expired) while maintaining consistent terminology.
src/main/resources/templates/mail/notification/sshKeyExpiresSoonEmail.html (3)
1-5: LGTM! Document structure follows best practices.
The template properly declares DOCTYPE and Thymeleaf namespace, maintaining consistency with other email templates.
4-4: Verify template fragment integration.
The template uses several fragments. Let's ensure they contain the required components for SSH key notifications.
Also applies to: 8-8, 11-11, 36-36
✅ Verification successful
Template fragments are properly integrated
The fragments.html file contains all required components for SSH key notifications:
headfragment with proper styling and meta informationheaderfragment with logo and brandinggreetingsFullNamefragment for personalized greetingfarewellfragment for email conclusion
All referenced fragments (lines 4, 8, 11, and 36) in sshKeyExpiresSoonEmail.html are present and properly implemented in the fragments.html file, ensuring consistent email template structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required fragments exist
echo "Checking for required fragments..."
fd "fragments.html$" src/main/resources/templates/mail/notification/ -x grep -l "head\|header\|greetingsFullName\|farewell" {}
Length of output: 271
Script:
#!/bin/bash
# Check the content of fragments.html to verify all required components
echo "Content of fragments.html:"
cat src/main/resources/templates/mail/notification/fragments.html
Length of output: 9786
24-27: 🛠️ Refactor suggestion
Verify date formatting and translations.
A few concerns:
- The message key span is empty, suggesting a missing translation
- Date formatting should be standardized to ensure consistency
Consider applying this diff to improve date formatting:
- <span th:utext="#{email.notification.sshKeyExpiry.expiryDate}"></span>
- <span th:utext="${expiryDate}"></span>
+ <span th:text="#{email.notification.sshKeyExpiry.expiryDate}">Expiry date</span>
+ <span th:text="${#temporals.format(expiryDate, 'dd.MM.yyyy')}">01.01.2024</span>src/main/webapp/i18n/en/notification.json (1)
126-129: LGTM! Title entries are consistent and clear
The notification titles follow the established conventions and maintain consistency with existing entries.
src/main/resources/templates/mail/notification/sshKeyAddedEmail.html (3)
1-3: LGTM!
The document structure and Thymeleaf namespace declaration are correctly implemented.
14-14: Verify localization message keys exist
Please ensure the following message keys are defined in the localization files:
email.notification.sshKeyAdded.titleemail.notification.sshKeyAdded.ifMistake
Also applies to: 24-24
✅ Verification successful
Message keys are properly defined in localization files
Both required message keys are defined in the default messages.properties and translations (en, de):
email.notification.sshKeyAdded.titleemail.notification.sshKeyAdded.ifMistake
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of required message keys in localization files
# Check for message keys in i18n files
rg "email\.notification\.sshKeyAdded\.(title|ifMistake)" "src/main/resources/i18n/"
Length of output: 1152
4-8: Verify the referenced fragments exist and contain appropriate content.
The template uses several fragments. Please ensure they exist and contain appropriate content:
mail/notification/fragments :: headmail/notification/fragments :: headermail/notification/fragments :: greetingsFullNamemail/notification/fragments :: farewell
Also applies to: 30-30
✅ Verification successful
All referenced fragments are present and properly structured
The verification confirms that all required fragments exist in src/main/resources/templates/mail/notification/fragments.html:
head: Contains title and nested fragment references for metadata and CSSheader: Includes the logo table structuregreetingsFullName: Contains greeting text with user's full namefarewell: Has farewell message with localized text
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of referenced fragments
# Check if the fragments file exists and contains the required fragments
rg -A 5 'th:fragment="(head|header|greetingsFullName|farewell)"' "src/main/resources/templates/mail/notification/fragments.html"
Length of output: 1035
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java (3)
16-18: LGTM: SSH key notification types imported correctly
The imports are properly organized and specific to the new SSH key notification functionality.
152-152: LGTM: SSH key notification types handled in switch expression
The switch expression is properly extended to handle the new SSH key notification types, maintaining consistency with existing patterns.
264-292: Verify notification handling in the email service
Since these are new notification types, we should verify that the email service properly handles these notifications.
✅ Verification successful
Let me check the email templates to ensure they exist and are properly set up.
Email templates for SSH key notifications are properly configured
The email service is correctly set up to handle all three SSH key notification types:
- Added SSH key notification with template
sshKeyAddedEmail.html - Expiring SSH key notification with template
sshKeyExpiresSoonEmail.html - Expired SSH key notification with template
sshKeyHasExpiredEmail.html
All templates are properly structured with the required Thymeleaf variables and translations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if email templates and handlers exist for SSH key notifications
# Search for email template handling of SSH key notifications
rg -A 5 "SSH_KEY_(ADDED|EXPIRES_SOON|HAS_EXPIRED)" --type java
# Search for email template files
fd -e html -e ftl "ssh.*key"
Length of output: 28435
Script:
#!/bin/bash
# Check the content of email templates for SSH key notifications
cat src/main/resources/templates/mail/notification/sshKeyAddedEmail.html
cat src/main/resources/templates/mail/notification/sshKeyExpiresSoonEmail.html
cat src/main/resources/templates/mail/notification/sshKeyHasExpiredEmail.html
Length of output: 3851
src/main/resources/i18n/messages_de.properties (2)
96-96: LGTM: Fixed German umlaut character
The correction from "?bereinstimmung" to "Übereinstimmung" properly fixes the character encoding issue.
166-168: LGTM: Notification titles are consistent and clear
The notification titles:
- Follow the existing naming pattern
- Are concise and clear
- Match their corresponding message keys
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.java (1)
46-48: LGTM: Import statements follow conventions
The new imports maintain consistency with the existing codebase's import style.
src/main/java/de/tum/cit/aet/artemis/communication/service/notifications/MailService.java (3)
91-93: LGTM: Constants are well-defined and properly scoped.
The new constants follow Java naming conventions and are appropriately grouped with other notification-related constants.
408-411: Verify the existence of referenced email templates.
The implementation correctly adds new cases for SSH key notifications. However, we should verify that the referenced email templates exist and are properly implemented.
✅ Verification successful
All referenced email templates are present in the codebase
All three SSH key notification templates exist at the expected location under src/main/resources/templates/mail/notification/:
- sshKeyAddedEmail.html
- sshKeyExpiresSoonEmail.html
- sshKeyHasExpiredEmail.html
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the referenced email templates exist
# Expected: All three template files should exist
echo "Checking for SSH key notification templates..."
for template in sshKeyAddedEmail sshKeyExpiresSoonEmail sshKeyHasExpiredEmail; do
fd -t f "${template}.html" -E "node_modules/*"
done
Length of output: 727
272-277: 🛠️ Refactor suggestion
Consider improving date formatting and null safety.
The current implementation has several areas for improvement:
- The date format should respect the user's locale instead of using a hardcoded pattern
- Consider adding a null check for the SSH key itself
- The time component in the date format might not be necessary for expiry dates
Consider applying these improvements:
if (notificationSubject instanceof UserSshPublicKey userSshPublicKey) {
+ if (userSshPublicKey == null) {
+ log.warn("Received null SSH key in notification");
+ return;
+ }
context.setVariable(SSH_KEY, userSshPublicKey);
if (userSshPublicKey.getExpiryDate() != null) {
- context.setVariable(SSH_KEY_EXPIRY_DATE, userSshPublicKey.getExpiryDate().format(DateTimeFormatter.ofPattern("dd.MM.yyyy HH:mm")));
+ DateTimeFormatter formatter = DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM)
+ .withLocale(context.getLocale());
+ context.setVariable(SSH_KEY_EXPIRY_DATE, userSshPublicKey.getExpiryDate().format(formatter));
}
}Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java (1)
24-25: LGTM: Import statements are well-organized
The new imports follow the existing pattern and are properly utilized in the new notification functionality.
Also applies to: 54-59, 78-78
src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.java (3)
1-30: LGTM! Well-structured class following best practices.
The class follows good practices with:
- Constructor-based dependency injection
- Final fields
- Clear single responsibility
- Appropriate Spring annotations
37-38: Verify date calculations across time zones.
The date calculations using ZonedDateTime.now() might behave differently across time zones.
✅ Verification successful
Based on the search results, I can see that ZonedDateTime.now() is used extensively in test code, but there's no explicit timezone configuration or handling in the actual service class under review. The date calculations in UserSshPublicKeyExpiryNotificationService.java are relative (using plusDays()) and don't involve any timezone-sensitive operations.
The date calculations are safe across time zones.
The use of ZonedDateTime.now() with relative time operations (plusDays()) maintains the timezone information and correctly handles the calculations regardless of the timezone. When adding days to a ZonedDateTime, Java automatically handles daylight saving time transitions and timezone rules. The 6-day and 7-day offsets will remain constant in any timezone.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other ZonedDateTime usage patterns in the codebase
rg -A 2 "ZonedDateTime\.now\(\)" --type java
# Look for timezone configurations
rg "timezone|ZoneId" --type java
Length of output: 115036
1-57: Verify integration with dependent components.
Let's ensure the repository and notification service methods exist with correct signatures.
✅ Verification successful
Integration with dependent components is correctly implemented
The verification confirms that:
UserSshPublicKeyRepositoryinterface correctly defines thefindByExpiryDateBetweenmethod with expected parametersSingleUserNotificationServiceimplements both required notification methods:notifyUserAboutSoonExpiringSshKey(User recipient, UserSshPublicKey key)notifyUserAboutExpiredSshKey(User recipient, UserSshPublicKey key)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check repository method
ast-grep --pattern 'interface UserSshPublicKeyRepository {
$$$
findByExpiryDateBetween($_, $_) {
$$$
}
$$$
}'
# Check notification service methods
ast-grep --pattern 'class SingleUserNotificationService {
$$$
notifyUserAboutSoonExpiringSshKey($_, $_) {
$$$
}
notifyUserAboutExpiredSshKey($_, $_) {
$$$
}
$$$
}'
Length of output: 557
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find the repository interface and notification service
echo "=== Checking UserSshPublicKeyRepository ==="
rg "interface UserSshPublicKeyRepository" -A 5
echo "=== Checking findByExpiryDateBetween method ==="
rg "findByExpiryDateBetween"
echo "=== Checking notification methods ==="
rg "notifyUserAboutSoonExpiringSshKey|notifyUserAboutExpiredSshKey" -A 2
Length of output: 3858
src/main/resources/templates/mail/notification/sshKeyExpiresSoonEmail.html
Show resolved
Hide resolved
src/main/resources/templates/mail/notification/sshKeyAddedEmail.html
Outdated
Show resolved
Hide resolved
...e/tum/cit/aet/artemis/communication/service/notifications/SingleUserNotificationService.java
Show resolved
Hide resolved
...ain/java/de/tum/cit/aet/artemis/communication/domain/notification/NotificationConstants.java
Show resolved
Hide resolved
.../de/tum/cit/aet/artemis/communication/domain/notification/SingleUserNotificationFactory.java
Outdated
Show resolved
Hide resolved
...t/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.java
Outdated
Show resolved
Hide resolved
...t/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java(4 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.html(2 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.java
🧰 Additional context used
📓 Path-based instructions (3)
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.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
📓 Learnings (2)
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9478
File: src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts:104-111
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In `ssh-user-settings-key-details.component.ts`, changing the error handling code by adding optional chaining (`?.`) and replacing `.indexOf()` with `.includes()` may alter semantics and should be avoided.
src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java (1)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#9478
File: src/main/java/de/tum/cit/aet/artemis/programming/service/UserSshPublicKeyService.java:125-131
Timestamp: 2024-11-12T12:51:46.554Z
Learning: In `UserSshPublicKeyService.java`, handling `EmptyResultDataAccessException` during SSH key deletion in the `deleteUserSshPublicKey` method is unnecessary, as it is redundant in this context.
🔇 Additional comments (9)
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.ts (4)
12-12: LGTM: Clean implementation of expiration tracking
The addition of DateTimePickerType import and hasExpired property aligns well with the PR's objective of managing SSH key expiration states.
Also applies to: 48-48
85-85: LGTM: Proper implementation of expiration check
The expiration check is implemented correctly using dayjs for date comparison and includes proper null checking for the expiry date.
Line range hint 108-111: LGTM: Error handling follows established patterns
The error handling implementation correctly uses indexOf for error key checking, which aligns with previous learnings about maintaining consistent error handling patterns in the codebase.
144-145: LGTM: Proper declaration of DateTimePickerType
The DateTimePickerType is correctly declared as a protected readonly property for template usage.
src/main/webapp/app/shared/user-settings/ssh-settings/details/ssh-user-settings-key-details.component.html (2)
75-75: LGTM: Proper configuration of date picker
The date picker is correctly configured to use calendar mode through the DateTimePickerType.CALENDAR property.
106-106: LGTM: Dynamic expiration status display
The implementation correctly switches between 'hasExpiredOn' and 'expiresOn' translation keys based on the expiration status.
Let's verify that both translation keys exist:
✅ Verification successful
Translation keys verified successfully
Both hasExpiredOn and expiresOn translation keys are properly defined in the English translations file, confirming that the dynamic expiration status display will work as intended.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of SSH key expiration translation keys
# Check for the translation keys in the English translations
rg "hasExpiredOn|expiresOn" "src/main/webapp/i18n/en/userSettings.json"
Length of output: 160
src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java (3)
1-1: LGTM: Package restructuring improves organization
The move to a dedicated 'sshuserkeys' subpackage better aligns with the single responsibility principle, improving code organization and maintainability.
Also applies to: 18-18
33-37: LGTM: Constructor injection properly implemented
The addition of SingleUserNotificationService follows best practices:
- Constructor injection used (as per guidelines)
- Fields are properly marked as final
- Service is appropriately scoped
63-66: Verify timezone handling for expiry dates
The expiry date is set to 3 AM, but the timezone handling needs verification. Consider:
- Whether ZonedDateTime.now()'s zone is consistently used
- If 3 AM is appropriate across all timezones
- Impact on users in different timezones
✅ Verification successful
Let's search for timezone-related configurations and usages in the relevant files to verify the timezone handling for expiry dates.
Let's check how the expiry date is used in the notification service to understand the complete timezone handling flow.
Based on the analysis of the code, I can now provide a final response regarding the timezone handling:
Timezone handling for SSH key expiry dates is appropriate
The implementation is correct for the following reasons:
- The expiry date inherits the timezone from the input
ZonedDateTimeobject, preserving the user's timezone - Setting it to 3 AM is intentional as it aligns with the notification service which:
- Checks for soon-to-expire keys at 7 AM daily
- Checks for expired keys at 7 AM daily
- The 4-hour gap between expiry (3 AM) and notifications (7 AM) ensures consistent behavior across timezone transitions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for timezone-related configurations and usages
rg -l "ZoneId|ZonedDateTime.now\(\)|TimeZone" --type java
Length of output: 27649
Script:
#!/bin/bash
# Search for timezone configurations and usages in UserSshPublicKeyService and related files
rg -A 5 -B 5 "ZoneId|ZonedDateTime|TimeZone" src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java src/main/java/de/tum/cit/aet/artemis/core/service/TimeService.java
Length of output: 6124
Script:
#!/bin/bash
# Search for expiry date handling in the notification service
rg -A 10 "expiryDate|ZonedDateTime" src/main/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyExpiryNotificationService.java
Length of output: 1710
...ain/java/de/tum/cit/aet/artemis/programming/service/sshuserkeys/UserSshPublicKeyService.java
Show resolved
Hide resolved
SimonEntholzer
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCSshSettingsTest.java (3)
25-25: Consider making the mock configuration more specificWhile the current mock configuration works, it could be more specific about the expected parameters to better document the test's assumptions and catch potential issues earlier.
Consider replacing with:
-doNothing().when(singleUserNotificationService).notifyUserAboutNewlyAddedSshKey(any(), any()); +doNothing().when(singleUserNotificationService).notifyUserAboutNewlyAddedSshKey( + argThat(user -> user.getLogin().startsWith(TEST_PREFIX)), + any() +);
Line range hint
35-71: Add test coverage for SSH key notificationsWhile the existing tests cover the core SSH key operations, there's no explicit verification of the notification behavior. Consider adding tests to verify that notifications are triggered appropriately.
Add a new test method like:
@Test @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") void verifyNotificationOnSshKeyAdd() throws Exception { sshSettingsTestService.addUserSshPublicKey(); verify(singleUserNotificationService).notifyUserAboutNewlyAddedSshKey( argThat(user -> user.getLogin().equals(TEST_PREFIX + "student1")), any() ); }
Line range hint
1-71: Consider using AssertJ assertions for better test readabilityAccording to the coding guidelines, tests should use
assertThatstyle assertions. While the current tests delegate toSshSettingsTestService, consider enhancing the service to use AssertJ assertions for better readability and more specific failure messages.Example assertion style to use in
SshSettingsTestService:assertThat(sshKeys) .isNotNull() .hasSize(expectedSize) .allMatch(key -> key.getLabel() != null);src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java (6)
403-404: Declare constants as private static finalThe constant
RSA_KEYshould be declared asprivate static finalto follow Java conventions for constants.Apply this diff to adjust the constant declaration:
- String RSA_KEY = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIEbgjoSpKnry5yuMiWh/uwhMG2Jq5Sh8Uw9vz+39or2i"; + private static final String RSA_KEY = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIEbgjoSpKnry5yuMiWh/uwhMG2Jq5Sh8Uw9vz+39or2i";
405-406: Declare constants as private static finalThe constant
KEY_IDshould be declared asprivate static finalto follow Java conventions for constants.Apply this diff:
- long KEY_ID = 4L; + private static final long KEY_ID = 4L;
407-408: Declare constants as private static finalThe constant
KEY_LABELshould be declared asprivate static finalto follow Java conventions for constants.Apply this diff:
- String KEY_LABEL = "key "; + private static final String KEY_LABEL = "key ";
438-449: Ensure consistency in notification assertionsIn
notifyUserAboutUpcomingSshKeyExpiry(), consider using consistent methods to access list elements. MixinggetFirst()andget(1)can be confusing.If
sentNotificationsis aList, use indices for clarity:- assertThat(sentNotifications.getFirst()).isInstanceOf(SingleUserNotification.class); - assertThat((sentNotifications.get(1)).getText()).isEqualTo(SSH_KEY_EXPIRES_SOON_TEXT); + assertThat(sentNotifications.get(0)).isInstanceOf(SingleUserNotification.class); + assertThat(sentNotifications.get(1).getText()).isEqualTo(SSH_KEY_EXPIRES_SOON_TEXT);
451-462: Ensure consistency in notification assertionsIn
notifyUserAboutExpiredSshKey(), use consistent list access methods.Apply this diff for clarity:
- assertThat(((SingleUserNotification) sentNotifications.getFirst()).getRecipient()).isEqualTo(user); + assertThat(((SingleUserNotification) sentNotifications.get(0)).getRecipient()).isEqualTo(user);
475-484: Test method missing specific assertionsIn
notNotifyUserAboutUpcomingSshKeyExpiryWhenKeyDoesNotExpireSoon(), while you assert the size ofsentNotifications, consider adding assertions to verify the content of the notifications to ensure no unintended notifications are sent.Example:
sentNotifications = notificationRepository.findAll(); assertThat(sentNotifications).hasSize(1); assertThat(sentNotifications.get(0).getText()).isEqualTo(SSH_KEY_ADDED_TEXT);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java(7 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCSshSettingsTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/test/java/de/tum/cit/aet/artemis/programming/icl/LocalVCSshSettingsTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (6)
src/test/java/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java (6)
25-27: Imports for SSH key notification constants are appropriate
The new imports for SSH_KEY_ADDED_TEXT, SSH_KEY_EXPIRES_SOON_TEXT, and SSH_KEY_HAS_EXPIRED_TEXT are necessary for the SSH key notification tests.
55-55: Import of GeneralSecurityException is appropriate
The import of java.security.GeneralSecurityException is required for exception handling in the new test methods.
65-68: New imports for SSH key tests and test structure are appropriate
The imports for AuthorizedKeyEntry, @AfterEach, and @Nested are necessary for the SSH key notification tests and proper test organization.
111-113: Imports for SSH key DTO and services are appropriate
The imports for UserSshPublicKeyDTO, UserSshPublicKeyExpiryNotificationService, and UserSshPublicKeyService are required for the SSH key notification tests.
400-402: Good use of @nested class with descriptive naming
The nested test class UserSshPublicKeyExpiryNotificationServiceShould effectively uses JUnit 5's @Nested annotation and follows descriptive naming conventions.
684-684: Verify usage of helper methods
The method verifyEmail() seems to be a utility for verifying email sending. Ensure that this method is used in the relevant test cases where email notifications are expected.
Please confirm that verifyEmail() is called in test methods that involve email notifications.
...ava/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java
Outdated
Show resolved
Hide resolved
...ava/de/tum/cit/aet/artemis/communication/notification/SingleUserNotificationServiceTest.java
Show resolved
Hide resolved
SimonEntholzer
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
BBesrour
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
| public void notifyUserOnKeyExpiry() { | ||
| ZonedDateTime fromDate = ZonedDateTime.now().minusDays(1); | ||
| ZonedDateTime toDate = ZonedDateTime.now(); | ||
| userSshPublicKeyRepository.findByExpiryDateBetween(fromDate, toDate).forEach(userSshPublicKey -> { |
There was a problem hiding this comment.
can you load the users directly in one query? we should avoid loops with database requests if possible
There was a problem hiding this comment.
is there an index on expiryDate to speed up the query in case of many database entries?
| /** | ||
| * Notifies the user about the expiry of one of their SSH keys | ||
| */ | ||
| @Scheduled(cron = "0 0 7 * * *") // execute this every morning at 7:00:00 am |
There was a problem hiding this comment.
Does it make sense to start both at the exact same time? I suggest to have one cron that executes both methods sequentially
krusche
changed the title
Integrated code lifecycle: Add email notifications for SSH key creation and expiryIntegrated code lifecycle: Add email notifications for SSH keys
Checklist
General
Server
Client
Motivation and Context
Users currently might not notice when their SSH keys expire. This could cause problems, when they try to access repositories with the expired key.
Description
This PR adds three new notifications:
Steps for Testing
Note: this is only testable with either changed cron times, or by testing it at 7:00 in the morning.
(4.) if you deployed with manually adjusted cron tasks running every minute/hour/.. you can observe those mails even when it's not 7:00 in the morning (e.g. change @scheduled(cron = "0 0 7 * * *") to @scheduled(cron = "0 * * * * *") in UserSshPublicKeyExpiryNotificationService)
For testing locally you could use https://mailcatcher.me/ and manually change the database entrie's expiry dates. Extend your local.yml to be able to use mailcatcher:
artemis-local.yml
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
Client
Server
Screenshots
New SSH key notification:
Key will expire in one week notification:
Key has expired notification:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
SSH_KEY_ADDED,SSH_KEY_EXPIRES_SOON, andSSH_KEY_HAS_EXPIRED.Bug Fixes
Documentation
Refactor