Add ipAddress and deviceId to vault key retrieve event and show last device access in user profile #320
Conversation
show the last device access in user profile Co-authored-by: iammajid <[email protected]>
Co-authored-by: SailReal <[email protected]>
…/hub into feature/enhance-user-profile
Co-authored-by: iammajid <[email protected]>
WalkthroughThis pull request introduces additional fields to capture extended contextual information within the application’s logging and device tracking functionalities. In the backend, the Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java (1)
48-58: Consider using window functions for better performance.While the current query is correct, for large datasets, using window functions (RANK/ROW_NUMBER) could be more performant than a correlated subquery. Here's an alternative implementation:
@NamedQuery(name = "AuditEvent.lastVaultKeyRetrieve", query = """ - SELECT e1 - FROM VaultKeyRetrievedEvent e1 - WHERE e1.deviceId IN (:deviceIds) - AND e1.timestamp = ( - SELECT MAX(e2.timestamp) - FROM VaultKeyRetrievedEvent e2 - WHERE e2.deviceId = e1.deviceId - ) + SELECT e FROM VaultKeyRetrievedEvent e + WHERE e.deviceId IN (:deviceIds) + AND ( + SELECT COUNT(e2.timestamp) + FROM VaultKeyRetrievedEvent e2 + WHERE e2.deviceId = e.deviceId + AND e2.timestamp > e.timestamp + ) = 0 """)backend/src/main/java/org/cryptomator/hub/entities/events/VaultKeyRetrievedEvent.java (2)
81-87: Improve null check in equals method.The equals method could be more robust by adding a self-reference check at the beginning.
@Override public boolean equals(Object o) { + if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; if (!super.equals(o)) return false; VaultKeyRetrievedEvent that = (VaultKeyRetrievedEvent) o; return Objects.equals(retrievedBy, that.retrievedBy) && Objects.equals(vaultId, that.vaultId) && result == that.result && Objects.equals(ipAddress, that.ipAddress) && Objects.equals(deviceId, that.deviceId); }
72-74: Fix parameter name inconsistency.The parameter name 'device' doesn't match the field name 'deviceId', which could lead to confusion.
- public void setDeviceId(String device) { - this.deviceId = device; + public void setDeviceId(String deviceId) { + this.deviceId = deviceId; }backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (1)
166-176: Consider optimizing the database query.The current implementation makes two database calls:
- Fetching device entities
- Fetching last vault key retrieval events
Consider joining these queries to reduce database round trips.
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
326-326: Consider validating the device ID header.The device ID is retrieved from the header without validation. Consider adding length and format checks.
+ var deviceId = Optional.ofNullable(request.getHeader("Hub-Device-ID")) + .filter(id -> id.matches("^[a-zA-Z0-9-_]{1,64}$")) + .orElse(null); - var deviceId = request.getHeader("Hub-Device-ID");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java(2 hunks)backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java(1 hunks)backend/src/main/java/org/cryptomator/hub/api/UsersResource.java(3 hunks)backend/src/main/java/org/cryptomator/hub/api/VaultResource.java(5 hunks)backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java(2 hunks)backend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/events/VaultKeyRetrievedEvent.java(2 hunks)backend/src/main/resources/org/cryptomator/hub/flyway/V19__Store_Ip_Address_of_Vault_Key_Retrieved.sql(1 hunks)frontend/src/common/auditlog.ts(1 hunks)frontend/src/common/backend.ts(3 hunks)frontend/src/common/userdata.ts(2 hunks)frontend/src/components/AuditLogDetailsVaultKeyRetrieve.vue(3 hunks)frontend/src/components/DeviceList.vue(4 hunks)frontend/src/components/UserkeyFingerprint.vue(1 hunks)frontend/src/components/VaultDetails.vue(1 hunks)frontend/src/i18n/en-US.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/components/UserkeyFingerprint.vue
🔇 Additional comments (18)
frontend/src/components/AuditLogDetailsVaultKeyRetrieve.vue (1)
25-41: LGTM! Well-structured UI implementation.The new UI elements for IP address and device information are well-implemented with:
- Proper conditional rendering
- Consistent styling with existing elements
- Clear separation of concerns
frontend/src/common/userdata.ts (1)
21-27: LGTM! Well-implemented caching strategy.The implementation of
meWithLastAccessis well-designed:
- Maintains consistent caching behavior
- Properly updates both
#meand#meWithLastAccess- Follows the existing Promise-based pattern
backend/src/main/java/org/cryptomator/hub/entities/events/EventLogger.java (1)
87-96: LGTM! Clean implementation of extended logging.The updated
logVaultKeyRetrievedmethod properly incorporates the new IP address and device ID fields while maintaining consistency with other logging methods in the class.frontend/src/common/auditlog.ts (1)
84-85: LGTM!The addition of optional
ipAddressanddeviceIdfields toAuditEventVaultKeyRetrieveDtoaligns with the PR objectives and follows TypeScript best practices.frontend/src/components/DeviceList.vue (3)
32-47: LGTM!The new columns are well-structured with:
- Clear tooltips for better user understanding
- Consistent styling with existing columns
- Proper accessibility through ARIA attributes
79-84: LGTM!The data display implementation:
- Correctly handles conditional rendering
- Uses consistent date formatting
- Maintains the existing table structure
106-106: LGTM!The script changes:
- Added necessary icon import
- Updated data fetching to include last access information
Also applies to: 127-127
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java (1)
177-179:❓ Verification inconclusive
Verify null handling in frontend.
The
lastIpAddressandlastAccessTimefields are initialized as null infromEntity. Please ensure that the frontend components handle these null values gracefully.Run the following script to verify null handling in frontend components:
Also applies to: 182-182
🏁 Script executed:
#!/bin/bash # Description: Check frontend components for null handling of lastIpAddress and lastAccessTime # Search for usage of lastIpAddress and lastAccessTime rg -A 5 'lastIpAddress|lastAccessTime' --type vue --type tsLength of output: 89
I've updated the verification script to explicitly search in Vue and TypeScript files. Please run the following revised script to check if there is any code in the frontend that handles the
lastIpAddressandlastAccessTimevalues (which may benull):#!/bin/bash # Revised script: Check frontend components for occurrences of lastIpAddress and lastAccessTime rg -A 5 'lastIpAddress|lastAccessTime' -g "*.ts" -g "*.vue"If this search returns results, please verify that the handling logic appropriately deals with
nullvalues. If no output is produced from this search, it’s advisable to manually confirm that the null checks are in place in the frontend code.Action Required: Run the revised script and manually inspect the results to ensure that the frontend components gracefully handle null values for these fields.
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (2)
63-64: LGTM!The AuditEvent repository is correctly injected.
177-179: LGTM!The code correctly handles both cases:
- With last access: includes IP and access time
- Without last access: maintains existing behavior
backend/src/main/java/org/cryptomator/hub/api/AuditLogResource.java (1)
133-133: LGTM! The audit log now includes IP address and device ID.The change correctly extends the
VaultKeyRetrievedEventDtoinstantiation to include the new tracking parameters.frontend/src/common/backend.ts (3)
58-59: LGTM! Device DTO now includes last access information.The addition of
lastIpAddressandlastAccessTimefields enables tracking of device access details.
207-213: LGTM! Access token retrieval now includes device identification.The method correctly handles the optional device ID by adding it to the request headers when present.
253-254: LGTM! User info retrieval now supports last access details.The method has been updated to optionally include last access information in the response.
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (2)
101-102: LGTM! Proper request context injection.The
HttpServerRequestis correctly injected using the@Contextannotation.
287-287: LGTM! Consistent event logging.The event logging is consistent across both success and unauthorized scenarios, properly including the IP address and device ID.
Also applies to: 292-292, 329-329, 336-336
frontend/src/components/VaultDetails.vue (1)
305-306: LGTM! Device identification added to vault access.The changes correctly retrieve the device ID from browser keys and pass it to the access token method.
backend/src/main/resources/org/cryptomator/hub/flyway/V19__Store_Ip_Address_of_Vault_Key_Retrieved.sql (1)
1-2: SQL Schema Update: Correctly Adding New ColumnsThe ALTER TABLE statement correctly adds the two new columns with appropriate data types. The
"ip_address"column uses a VARCHAR(46), which is sufficient for IPv6 addresses, and"device_id"is defined as VARCHAR(255) with aCOLLATE "C", which is in line with ensuring consistent collation.
backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
backend/src/test/java/org/cryptomator/hub/api/DeviceResourceIT.java (2)
73-79: 🛠️ Refactor suggestionUpdate GET endpoint test to verify new fields in response.
The GET endpoint test should be updated to verify the presence and format of the new
lastIpAddressandlastAccessTimefields in the response.Update the test assertion:
given().when().get("/devices/{deviceId}", "device1") .then().statusCode(200) .body("id", is("device1")) .body("name", is("Computer 1")) - .body("userPrivateKey", is("jwe.jwe.jwe.user1.device1")); + .body("userPrivateKey", is("jwe.jwe.jwe.user1.device1")) + .body("lastIpAddress", is(nullValue())) + .body("lastAccessTime", is(nullValue()));
42-49: 🛠️ Refactor suggestionAdd test coverage for different device entry types.
According to the PR objectives, the system should support three types of device entries:
- New devices with both IP address and deviceId
- Old devices with only IP address
- Old entries without either
Consider adding test cases to verify the behavior for each of these scenarios.
Example test structure:
@Test @DisplayName("GET /devices returns correct fields for different device types") public void testDeviceTypes() { // Test case 1: New device with both fields // Test case 2: Legacy device with only IP // Test case 3: Old device without either field }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/test/java/org/cryptomator/hub/api/DeviceResourceIT.java(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
Co-authored-by: iammajid <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (1)
325-326: Extract "Hub-Device-ID" as a constant.Consider extracting the hardcoded header name "Hub-Device-ID" as a class constant to improve maintainability and prevent typos.
+ private static final String HUB_DEVICE_ID_HEADER = "Hub-Device-ID"; + @Context HttpServerRequest request; // ... in unlock method: - var deviceId = request.getHeader("Hub-Device-ID"); + var deviceId = request.getHeader(HUB_DEVICE_ID_HEADER);Also applies to: 329-329, 336-336
frontend/src/components/DeviceList.vue (1)
32-47: Consider adding aria-label for tooltip icons.The new columns are well-structured with proper tooltips. Consider enhancing accessibility by adding aria-labels to the tooltip icons.
- <QuestionMarkCircleIcon class="h-4 w-4 text-gray-400 cursor-help"/> + <QuestionMarkCircleIcon class="h-4 w-4 text-gray-400 cursor-help" :aria-label="t('deviceList.lastAccess.toolTip')"/>- <QuestionMarkCircleIcon class="h-4 w-4 text-gray-400 cursor-help" /> + <QuestionMarkCircleIcon class="h-4 w-4 text-gray-400 cursor-help" :aria-label="t('deviceList.ipAddress.toolTip')" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java(5 hunks)frontend/src/components/DeviceList.vue(4 hunks)frontend/src/i18n/en-US.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/i18n/en-US.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (5)
backend/src/main/java/org/cryptomator/hub/api/VaultResource.java (2)
8-8: LGTM! Clean dependency injection for request context.The addition of
HttpServerRequestwith@Contextinjection is a clean way to access client information.Also applies to: 33-33, 101-102
284-284: Standardize IP address extraction between methods.The method uses
hostAddress()while a past implementation usedtoString(). This inconsistency was previously flagged.Also applies to: 287-287, 291-292
frontend/src/components/DeviceList.vue (3)
106-106: LGTM!The
QuestionMarkCircleIconimport is correctly added and properly grouped with other icon imports.
79-84: LGTM!The new table cells are well-implemented with:
- Consistent styling with other cells
- Proper date formatting for
lastAccessTime- Correct handling of null values
127-127:❓ Verification inconclusive
Verify the backend integration for last access data.
The change to use
meWithLastAccesslooks good. Let's verify that the backend endpoint is properly configured to return the last access information.
🏁 Script executed:
#!/bin/bash # Description: Verify the backend endpoint for last access data # Check if the backend endpoint exists and returns the expected fields curl -s http://localhost:8080/api/users/me?withLastAccess=true | jq '.devices[] | select(.lastAccessTime != null) | {lastAccessTime, lastIpAddress}'Length of output: 209
Action Required: Manually verify the backend endpoint's JSON output.
The change to use
meWithLastAccessin the frontend looks correct and aligned with backend updates. However, the shell script command indicates that the backend response might not be returning valid JSON (error: "parse error: Invalid numeric literal"). Please manually verify that:
- The endpoint
http://localhost:8080/api/users/me?withLastAccess=truereturns a properly formatted JSON object.- The JSON includes the expected fields (e.g.,
lastAccessTime,lastIpAddress) under thedevicesarray.Once confirmed, we can fully approve the change.
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (2)
175-176: Consider extracting the device mapping function.The device mapping function is quite long and could be extracted to improve readability and maintainability.
- Function<Device, DeviceResource.DeviceDto> mapDevices = d -> new DeviceResource.DeviceDto(d.getId(), d.getName(), d.getType(), d.getPublickey(), d.getUserPrivateKeys(), d.getOwner().getId(), d.getCreationTime().truncatedTo(ChronoUnit.MILLIS), null, null); - devices = withDevices ? user.devices.stream().map(mapDevices).collect(Collectors.toSet()) : Set.of(); + Function<Device, DeviceResource.DeviceDto> mapDevices = DeviceResource.DeviceDto::fromEntity; + devices = withDevices ? user.devices.stream().map(mapDevices).collect(Collectors.toSet()) : Set.of();
166-174: Consider adding pagination for device listing.For users with many devices, fetching all devices and their last access events in a single request could impact performance. Consider implementing pagination to handle large device sets efficiently.
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java (2)
172-180: Add validation for IP address field.Consider adding validation constraints for the
lastIpAddressfield to ensure it contains a valid IP address format. This helps maintain data integrity and security.Add a custom validator or use an existing one:
public record DeviceDto(@JsonProperty("id") @ValidId String id, @JsonProperty("name") @NoHtmlOrScriptChars @NotBlank String name, @JsonProperty("type") Device.Type type, @JsonProperty("publicKey") @NotNull @OnlyBase64Chars String publicKey, @JsonProperty("userPrivateKey") @NotNull @ValidJWE String userPrivateKeys, @JsonProperty("owner") @ValidId String ownerId, @JsonProperty("creationTime") Instant creationTime, + @Pattern(regexp = "^((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$", message = "Invalid IPv4 address format") @JsonProperty("lastIpAddress") String lastIpAddress, @JsonProperty("lastAccessTime") Instant lastAccessTime)
186-190: Consider reducing code duplication between fromEntity methods.While the implementation is correct, there's duplication in the field mapping logic between the two
fromEntitymethods.Consider refactoring to reduce duplication:
- public static DeviceDto fromEntity(Device entity) { - return new DeviceDto(entity.getId(), entity.getName(), entity.getType(), entity.getPublickey(), entity.getUserPrivateKeys(), entity.getOwner().getId(), entity.getCreationTime().truncatedTo(ChronoUnit.MILLIS), null, null); - } - - public static DeviceDto fromEntity(Device d, VaultKeyRetrievedEvent event) { - var lastIpAddress = (event != null) ? event.getIpAddress() : null; - var lastAccessTime = (event != null) ? event.getTimestamp() : null; - return new DeviceResource.DeviceDto(d.getId(), d.getName(), d.getType(), d.getPublickey(), d.getUserPrivateKeys(), d.getOwner().getId(), d.getCreationTime().truncatedTo(ChronoUnit.MILLIS), lastIpAddress, lastAccessTime); + public static DeviceDto fromEntity(Device d) { + return fromEntity(d, null); + } + + public static DeviceDto fromEntity(Device d, VaultKeyRetrievedEvent event) { + return new DeviceDto( + d.getId(), + d.getName(), + d.getType(), + d.getPublickey(), + d.getUserPrivateKeys(), + d.getOwner().getId(), + d.getCreationTime().truncatedTo(ChronoUnit.MILLIS), + event != null ? event.getIpAddress() : null, + event != null ? event.getTimestamp() : null + ); }backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (1)
160-175: Enhance test coverage for audit log verification.The test verifies the basic functionality but could be improved with additional assertions:
Apply this diff to enhance the test coverage:
@Test @DisplayName("GET /vaults/7E57C0DE-0000-4000-8000-000100001111/access-token with remote IP and device ID stores it in audit log") public void testUnlock4() throws SQLException { given().header("HUB-DEVICE-ID", "123456789123456789") .header("X-Forwarded-For", "1.2.3.4") .when().get("/vaults/{vaultId}/access-token", "7E57C0DE-0000-4000-8000-000100001111") .then().statusCode(200) .body(is("jwe.jwe.jwe.vault1.user1")); try (var c = dataSource.getConnection(); var s = c.createStatement()) { var rs = s.executeQuery(""" SELECT * FROM "audit_event_vault_key_retrieve" WHERE "device_id" = '123456789123456789' AND "ip_address" = '1.2.3.4'; """); Assertions.assertTrue(rs.next()); + // Verify additional fields in the audit log + Assertions.assertEquals("7E57C0DE-0000-4000-8000-000100001111", rs.getString("vault_id")); + Assertions.assertEquals("user1", rs.getString("user_id")); + // Verify there's only one matching record + Assertions.assertFalse(rs.next(), "Expected only one audit log entry"); } }backend/src/main/resources/application.properties (1)
92-92:❓ Verification inconclusive
Consider enabling proxy address forwarding in other profiles.
The property is correctly added for the test profile, but consider if it's also needed in other profiles (dev, prod) to ensure consistent behavior across environments.
Run the following script to check if the property is defined in other profiles:
🏁 Script executed:
#!/bin/bash # Description: Check if proxy address forwarding is enabled in other profiles # Search for proxy address forwarding configuration in all property files rg -l "quarkus.http.proxy.proxy-address-forwarding"Length of output: 101
Review Comment Revision – Verify Proxy Configuration Across Environments
Our investigation shows that the only instance of the proxy forwarding configuration is in the test profile (i.e.
%test.quarkus.http.proxy.proxy-address-forwarding=trueinbackend/src/main/resources/application.properties). If uniform behavior is desired across environments, please consider enabling this property for development and production profiles as well.
- Location:
backend/src/main/resources/application.properties(line 92)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java(2 hunks)backend/src/main/java/org/cryptomator/hub/api/UsersResource.java(3 hunks)backend/src/main/resources/application.properties(1 hunks)backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (6)
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (2)
27-29: LGTM! Clean integration of audit event tracking.The addition of audit event imports and repository injection is well-structured and follows the existing patterns.
Also applies to: 63-64
163-164: LGTM! Clean API extension.The addition of the
withLastAccessparameter is backward compatible and aligns with the PR objectives to show last device access in user profiles.backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java (2)
28-28: LGTM!The import is correctly added and follows the existing import organization pattern.
182-184: LGTM!The method correctly handles the new fields while maintaining backward compatibility by providing null defaults.
backend/src/test/java/org/cryptomator/hub/api/VaultResourceIT.java (1)
160-175: LGTM! Well-structured test for the new audit logging functionality.The test effectively verifies that:
- Device ID and IP address are correctly captured from request headers
- The information is properly stored in the audit log database
backend/src/main/resources/application.properties (1)
92-92: LGTM! Appropriate configuration for proxy address forwarding in tests.The property enables the forwarding of proxy headers like X-Forwarded-For, which is essential for capturing client IP addresses in the test environment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
20-20: User Profile Device Information Detail
The changelog now mentions, "Show last IP address and last vault access timestamp of devices in user profile (#320)". Please verify that the phrasing accurately reflects the intended functionality. The PR title suggests "last device access" rather than "vault access timestamp". If this is a naming discrepancy, consider updating the changelog for consistency.
37-37: Enhanced VaultKeyRetrievedEvent Audit Log Entry
This entry now indicates that the VaultKeyRetrievedEvent audit log contains the additional fields (IP address and device ID) as required (#320). To improve clarity and grammatical flow, consider rephrasing "contains now" to "now includes". For example:-Enhanced audit log VaultKeyRetrievedEvent, contains now IP address and device ID (#320) +Enhanced audit log VaultKeyRetrievedEvent now includes IP address and device ID (#320)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(2 hunks)
🔇 Additional comments (2)
CHANGELOG.md (2)
18-18: New Log Event Entries Added
The addition of new log event entries—UserAccountReset, UserKeysChange, and UserSetupCodeChange (#310)—is clearly documented here and aligns with the expanded logging functionality outlined in the PR objectives.
19-19: Audit Log Filter by Event Type
The new audit log filter entry by event type (#312) is clearly specified and will improve the granularity of log queries.
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
Outdated
Show resolved
Hide resolved
SailReal
force-pushed
the
feature/enhance-user-profile
branch
from
baced62 to
8436980
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java (1)
49-59: Consider optimizing the query performance.The current query uses a correlated subquery which may not perform optimally with large datasets. Consider using a window function for better performance.
@NamedQuery(name = "AuditEvent.lastVaultKeyRetrieve", query = """ - SELECT e1 - FROM VaultKeyRetrievedEvent e1 - WHERE e1.deviceId IN (:deviceIds) - AND e1.timestamp = ( - SELECT MAX(e2.timestamp) - FROM VaultKeyRetrievedEvent e2 - WHERE e2.deviceId = e1.deviceId - ) + SELECT e + FROM VaultKeyRetrievedEvent e + WHERE e.deviceId IN (:deviceIds) + AND (e.deviceId, e.timestamp) IN ( + SELECT v.deviceId, MAX(v.timestamp) + FROM VaultKeyRetrievedEvent v + WHERE v.deviceId IN (:deviceIds) + GROUP BY v.deviceId + ) """)backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (2)
163-163: Add documentation for the new parameter.Consider adding Javadoc for the
withLastAccessparameter to clarify its purpose and impact on the response.@Operation(summary = "get the logged-in user") @APIResponse(responseCode = "200", description = "returns the current user") @APIResponse(responseCode = "404", description = "no user matching the subject of the JWT passed as Bearer Token") +/** + * Retrieves information about the currently logged-in user. + * + * @param withDevices whether to include the user's devices in the response + * @param withLastAccess whether to include last access information (IP address and timestamp) for each device + * @return the user information + * @throws NotFoundException if no user matches the JWT subject + */
165-175: Consider optimizing stream operations.The current implementation creates multiple intermediate collections. Consider optimizing by:
- Using a single stream pipeline
- Avoiding the intermediate collections with
toMap- Set<DeviceResource.DeviceDto> deviceDtos; - if (withLastAccess) { - var devices = user.devices.stream().collect(Collectors.toMap(Device::getId, Function.identity())); - var events = auditEventRepo.findLastVaultKeyRetrieve(devices.keySet()).collect(Collectors.toMap(VaultKeyRetrievedEvent::getDeviceId, Function.identity())); - deviceDtos = devices.values().stream().map(d -> { - var event = events.get(d.getId()); - return DeviceResource.DeviceDto.fromEntity(d, event); - }).collect(Collectors.toSet()); - } else { - deviceDtos = withDevices ? user.devices.stream().map(DeviceResource.DeviceDto::fromEntity).collect(Collectors.toSet()) : Set.of(); - } + Set<DeviceResource.DeviceDto> deviceDtos = !withDevices ? Set.of() : ( + withLastAccess + ? user.devices.stream() + .map(device -> { + var event = auditEventRepo.findLastVaultKeyRetrieve(Set.of(device.getId())) + .findFirst() + .orElse(null); + return DeviceResource.DeviceDto.fromEntity(device, event); + }) + .collect(Collectors.toSet()) + : user.devices.stream() + .map(DeviceResource.DeviceDto::fromEntity) + .collect(Collectors.toSet()) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java(3 hunks)backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (2)
backend/src/main/java/org/cryptomator/hub/entities/events/AuditEvent.java (1)
143-145: Add documentation and parameter validation.The method should include JavaDoc and parameter validation as previously suggested.
+ /** + * Retrieves the most recent VaultKeyRetrievedEvent for each device ID in the provided set. + * + * @param deviceIds set of device IDs to query, must not be null or empty + * @return stream of VaultKeyRetrievedEvent objects, one per device ID + * @throws IllegalArgumentException if deviceIds is null or empty + */ public Stream<VaultKeyRetrievedEvent> findLastVaultKeyRetrieve(Set<String> deviceIds) { + if (deviceIds == null || deviceIds.isEmpty()) { + throw new IllegalArgumentException("Device IDs set must not be null or empty"); + } return find("#AuditEvent.lastVaultKeyRetrieve", Parameters.with("deviceIds", deviceIds)).stream(); }backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (1)
27-29: LGTM!The addition of audit event imports and repository injection follows the established patterns in the codebase and aligns with the PR objectives.
Also applies to: 63-64
So that Hub knows the client IP address, we need to add
QUARKUS_HTTP_PROXY_PROXY_ADDRESS_FORWARDING: trueto the deployment to pass the headers from the TLS terminating proxy to Hub, read more in https://quarkus.io/guides/http-reference#reverse-proxy and https://quarkus.io/guides/all-config#quarkus-vertx-http_quarkus-http-proxy-proxy-address-forwarding.
(Note:
localhostdoes ignore this, so do not expect to see the client ip address in the dev context)We support the following type of entries:
ToDo
Optimize db-subquery (https://github.com/cryptomator/hub/pull/320/files#diff-aa71c42ab43fc516f56c450c9919cb6748bc8368c2c4019679bf8323586d5652R46-R56)as agreed, we stay with the sub-query for nowQUARKUS_HTTP_PROXY_PROXY_ADDRESS_FORWARDING) / newsletter