Show legacy devices in user profile and legacy device name in audit log #331
Conversation
SailReal
changed the title
WalkthroughThe changes introduce comprehensive legacy device support throughout the application. In the backend, new methods are added to the Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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: 1
🧹 Nitpick comments (8)
backend/src/main/java/org/cryptomator/hub/entities/User.java (1)
150-152: Field declaration follows existing patterns but lacks deprecation reasonThe
legacyDevicesfield declaration follows the same pattern as thedevicesfield above it, which is good for consistency. However, consider adding a deprecation reason to both this field and its getter to provide context on why they're deprecated and what developers should use instead.-@Deprecated +@Deprecated(since = "1.0.0", forRemoval = false) // Consider adding a reason like "Use unified device management instead" public Set<LegacyDevice> getLegacyDevices() { return legacyDevices; } -@Deprecated +@Deprecated(since = "1.0.0", forRemoval = false) // Consider adding a reason like "Use unified device management instead" @OneToMany(mappedBy = "owner", orphanRemoval = true, fetch = FetchType.LAZY) public Set<LegacyDevice> legacyDevices = new HashSet<>();frontend/src/components/LegacyDeviceList.vue (1)
81-86: TODO comment present in production codeThere's a TODO comment about styling in the production code. This should be addressed before finalizing the PR.
Consider implementing better styling for error messages, perhaps using a dedicated error component or a toast notification system rather than inline error messages that disrupt the table layout.
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (1)
185-204: Consider a better name for the deprecated method.As noted in the PR description, the function name
getMeWithLegacyDevicesAndAccessis acknowledged as being unattractive. While the implementation itself is sound, following the PR's own suggestion to rename this function would improve code readability before the PR is merged.The function name could be more concise while maintaining clarity, such as:
-@GET -@Path("/me-with-legacy-devices-and-access") -public UserDto getMeWithLegacyDevicesAndAccess() { +@GET +@Path("/me/legacy-devices") +public UserDto getMeWithLegacyDevices() {backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java (5)
82-92: Consider consolidating or clarifying the legacy vs. non-legacy retrieval endpoints.While introducing
getSomeLegacymaintains backward compatibility, unifying it withgetSomeor offering a single endpoint that can filter by device type (regular vs. legacy) may be more intuitive in the long run. Also confirm that the@RolesAllowed("admin")scope is the correct access level, as this endpoint returns all matching legacy devices for the given IDs.
173-174: Slight naming nitpick for the boolean parameter.
remove(deviceId, false)might be more readable if the parameter were namedisLegacyDeviceor replaced with an enum. Currently, it's not immediately evident whatfalserepresents without tracing the method signature.
176-187: Recommend merging or clearly documenting the separate legacy-device removal.
removeLegacyDeviceis nearly identical toremoveexcept for passingtrueinstead offalse. If the plan is to eventually remove these methods, that's fine, but for maintainability, consider merging them or balancing them behind a single endpoint with a parameter.
189-189: Good introduction of a single removal endpoint with a legacy flag.This private method centralizes the deletion logic, avoiding duplication. Consider renaming the boolean parameter to
isLegacyDevicefor greater clarity.
194-197: Ternary approach is clear but slightly verbose.Using a local variable to assign the repository/lookups based on
legacyDevicebefore the removal check might simplify the code, but this is a minor preference. The current approach works well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java(4 hunks)backend/src/main/java/org/cryptomator/hub/api/UsersResource.java(4 hunks)backend/src/main/java/org/cryptomator/hub/entities/LegacyAccessToken.java(1 hunks)backend/src/main/java/org/cryptomator/hub/entities/LegacyDevice.java(3 hunks)backend/src/main/java/org/cryptomator/hub/entities/User.java(1 hunks)backend/src/test/java/org/cryptomator/hub/api/DeviceResourceIT.java(9 hunks)backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java(2 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/AuditLog.vue(0 hunks)frontend/src/components/DeviceList.vue(1 hunks)frontend/src/components/LegacyDeviceList.vue(1 hunks)frontend/src/components/UserProfile.vue(2 hunks)frontend/src/i18n/en-US.json(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/src/components/AuditLog.vue
🔇 Additional comments (52)
frontend/src/components/DeviceList.vue (1)
98-98: Changes to import statement ordering look good.The reordering of imports doesn't affect functionality since all necessary components are still imported.
frontend/src/components/UserProfile.vue (2)
57-57: Successfully implemented legacy device list in user profile.The LegacyDeviceList component is appropriately placed in the UI layout, aligning with the PR's objective to show legacy devices in the user profile.
76-76: Import statement correctly added.The required import for the LegacyDeviceList component is properly included.
frontend/src/common/userdata.ts (2)
11-12: New property marked as deprecated with correct alternative.The new private property is appropriately marked with a deprecation notice directing users to the preferred alternative.
32-38: Implementation maintains existing patterns while signaling future direction.The getter method properly initializes the property if needed and returns its value. The deprecation notice correctly guides developers toward using the preferred
meWithLastAccessmethod.This addresses the PR objective about the unattractive function name, indicating that a transition to a better approach is in progress.
backend/src/main/java/org/cryptomator/hub/entities/LegacyAccessToken.java (2)
24-25: Updated entity relationship reference in query.The query now correctly references
device.owner.idinstead ofdevice.ownerId, reflecting a structural change in how legacy devices are associated with users. This change uses a more relational approach which improves data integrity.
31-32: Updated entity relationship reference in second query.Similar to the previous change, this query now uses the proper relationship path
device.owner.idinstead of directly accessingdevice.ownerId. This maintains consistency with the data model changes.backend/src/main/java/org/cryptomator/hub/entities/User.java (1)
139-142: Deprecated method implementation looks goodThe implementation of the deprecated
getLegacyDevices()method provides access to the newlegacyDevicesfield, maintaining consistency with other getter methods in the class.frontend/src/common/auditlog.ts (1)
158-170: Enhanced device resolution logic improves audit loggingThis implementation properly retrieves both standard and legacy devices in the audit log, addressing the PR objective. The error handling is robust, with fallbacks to empty arrays if either fetch fails, ensuring the application remains functional even if one endpoint is unavailable.
However, consider adding a log message when a legacy device fetch fails to aid in troubleshooting:
private debouncedResolvePendingDevices = debounce(async () => { await this.resolvePendingEntities<DeviceDto>( this.devices, (deviceIds: string[]) => Promise.all([ - backend.devices.listSome(deviceIds).catch(() => []), - backend.devices.listSomeLegacyDevices(deviceIds).catch(() => []) + backend.devices.listSome(deviceIds).catch((error) => { + console.warn('Failed to fetch standard devices:', error); + return []; + }), + backend.devices.listSomeLegacyDevices(deviceIds).catch((error) => { + console.warn('Failed to fetch legacy devices:', error); + return []; + }) ]).then(([devices, legacyDevices]) => [ ...devices, ...legacyDevices ]) ); }, 100);frontend/src/components/LegacyDeviceList.vue (6)
11-22: Good heading and description for legacy devices sectionThe heading and descriptive text with a documentation link provide clear context to users about legacy devices. This helps users understand what they're seeing and where to find more information.
28-49: Table header setup follows design patternsThe table header structure follows the same pattern as other tables in the application, maintaining UI consistency. I appreciate the tooltip for the "Last Access" column to provide additional context to users.
98-103: Import statements well organizedThe import statements are well-structured, importing only what's needed from external libraries and internal modules.
125-141: Device removal handles missing devices gracefullyThe device removal function properly handles the case where a device might already be missing on the backend by ignoring
NotFoundErrorerrors. This prevents unnecessary error messages for race conditions.
143-149: Device sorting uses reliable creation time comparisonThe function to sort devices properly converts date strings to timestamp values for comparison, ensuring accurate sorting by creation time in descending order (newest first).
115-123:✅ Verification successful
Data fetching implementation uses non-idiomatic pattern
The current implementation uses a property from the
userdatamodule calledmeWithLegacyDevicesAndLastAccess, but checking the PR files, this property isn't defined in the reviewed files. This might cause runtime errors if the property doesn't exist.Please verify that this property exists in the
userdatamodule:
🏁 Script executed:
#!/bin/bash # Check if the property exists in the userdata module grep -r "meWithLegacyDevicesAndLastAccess" frontend/src/Length of output: 644
Verified Implementation:
Our investigation confirms that the propertymeWithLegacyDevicesAndLastAccessis indeed defined infrontend/src/common/userdata.tsas a getter and is used correctly inLegacyDeviceList.vue. No runtime errors should occur, and the current implementation follows the intended design.frontend/src/common/backend.ts (4)
60-60: NewlegacyDeviceflag added to DeviceDtoAdding the
legacyDeviceboolean flag to theDeviceDtotype is a clean way to distinguish between standard and legacy devices in the frontend.
239-243: Deprecated method implementation follows existing patternsThe
listSomeLegacyDevicesmethod follows the same pattern as the standardlistSomemethod, maintaining consistency in the codebase. The deprecation notice correctly suggests using the standard method instead.
250-254: DeprecatedremoveLegacyDevicemethod follows existing patternsThe
removeLegacyDevicemethod follows the same pattern as theremoveDevicemethod, with appropriate error handling. The deprecation notice correctly suggests using the standard method instead.
270-273:❓ Verification inconclusive
PR objective naming concern addressed in code
In the PR objectives, you mentioned that the function name
withLastAccessAndLegacyDeviceswas unattractive and needed a better name. The implementation here usesmeWithLegacyDevicesAndAccesswhich is still quite verbose but more readable.Consider if this method name can be further simplified. The API endpoint name suggests it might be fixed on the backend, but check if a more concise name could be used:
🌐 Web query:
What are good naming conventions for API endpoints that fetch multiple related resources?💡 Result:
When designing REST API endpoints to fetch multiple related resources, adhering to established naming conventions ensures clarity, consistency, and ease of use. Below are key best practices supported by industry standards:
1. Use Nouns and Plural Forms for Collections
- Collections: Use plural nouns for endpoints that return multiple resources (e.g.,
/users,/posts). This aligns with REST principles where endpoints represent resources (nouns) rather than actions (verbs) [1][3][9].- Singular for Singleton Resources: Use singular nouns when addressing a single resource (e.g.,
/users/{id}) [1][10].
2. Hierarchical Structure for Relationships
Represent parent-child relationships with nested paths:
/users/{userId}/posts # Fetch posts by a specific user [1][8] /teams/{teamId}/members # Retrieve members of a team [3]Limit nesting to 2–3 levels to avoid overly complex URIs. For deeper relationships, use query parameters (e.g.,
/posts?author={userId}) [3][8].
3. Consistent Naming Conventions
- Case: Use lowercase letters (e.g.,
/user-profiles, not/UserProfiles) [4][7].- Separators: Hyphens (
-) for multi-word segments (e.g.,/order-history, not/orderHistoryor/order_history) [7][10].- Avoid Special Characters: Exclude spaces, underscores, or file extensions [3][6].
4. Versioning
Include API versions in the path to maintain backward compatibility:
/v1/users /v2/productsThis allows iterative improvements without breaking existing clients [1][4].
5. Filtering, Sorting, and Pagination via Query Parameters
Use query parameters to refine results for related resources:
GET /users?role=admin&sort=name-asc # Filter and sort [1][6] GET /posts?category=tech&page=2 # Paginate results [1][8]Avoid embedding filters or actions in the URI path (e.g.,
/users/adminsis less flexible than/users?role=admin) [6][8].
6. Avoid Anti-Patterns
- Verbs in URIs: Replace
/getUserswithGET /users[1][9].- Deep Nesting: Prefer
/users/{id}/posts?status=draftover/users/{id}/posts/drafts[3][8].- Mixed Conventions: Stick to one naming style (e.g., kebab-case) across all endpoints [4][7].
Examples of Well-Structured Endpoints
Use Case Endpoint HTTP Method Fetch all users /v1/usersGETGet posts by a user /v1/users/{id}/postsGETFilter active orders /v1/orders?status=activeGETPaginated products /v1/products?page=3&limit=20GETBy following these conventions, APIs become intuitive, scalable, and easier to maintain, reducing developer friction and improving integration efficiency.
Citations:
- 1: https://dev.to/msnmongare/best-practices-for-naming-api-endpoints-2n5o
- 2: https://help.mulesoft.com/s/question/0D52T00006Yl6XPSAZ/looking-for-some-advice-as-far-as-raml-and-what-should-be-bundled-together-and-rest-endpoint-naming-conventions
- 3: https://www.theserverside.com/video/Top-REST-API-URL-naming-convention-standards
- 4: https://blog.dreamfactory.com/best-practices-for-naming-rest-api-endpoints
- 5: https://nordicapis.com/10-best-practices-for-naming-api-endpoints/
- 6: https://www.moesif.com/blog/technical/api-development/The-Ultimate-Guide-to-REST-API-Naming-Convention/
- 7: https://pratapsharma.io/api-naming-convention/
- 8: https://stackoverflow.blog/2020/03/02/best-practices-for-rest-api-design/
- 9: https://github.com/saifaustcse/api-best-practices
- 10: https://restfulapi.net/resource-naming/
- 11: https://cloud.google.com/apis/design/naming_convention
ACTION: Revisit function naming for clarity
The updated function name
meWithLegacyDevicesAndAccessis more descriptive than the originalwithLastAccessAndLegacyDevices—which is an improvement—but it remains somewhat verbose relative to common API naming conventions. As noted in established best practices, endpoints (and their corresponding helper methods) should ideally be concise and use clear, noun-based identifiers. Since the backend already marks this method as deprecated (with a recommendation to usemeinstead), please consider whether it’s feasible to simplify the naming further. For example, if the backend can consolidate the legacy details into a single, clear endpoint (like/users/me), the corresponding function could simply be namedmeto enhance clarity and maintainability.
- Verify that a simplified naming strategy aligns with both the backend API design and overall project conventions.
- If the backend update is in progress or planned, ensure that the transition strategy (e.g., deprecation warnings) is cleanly communicated in the code.
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (2)
24-24: Added imports and repository injection for LegacyDevice support.The addition of the LegacyDevice import and repository injection is appropriate for handling legacy devices in the API.
Also applies to: 61-61
166-167: Enhanced getMe to handle last access data and modified device retrieval logic.The method now supports retrieving last access values for devices through the new parameter, and the initialization of deviceDtos as a HashSet properly handles both legacy and regular devices. The reorganized condition flow improves readability.
Also applies to: 169-183
backend/src/test/java/org/cryptomator/hub/api/UsersResourceIT.java (2)
81-105: Enhanced test with proper SQL setup and cleanup for device access validation.The test now properly validates device properties including last access time, IP address, and legacy device status. The test setup and cleanup with SQL statements is well implemented with proper resource management.
107-132: Comprehensive test for legacy device endpoint.This test correctly validates the implementation of the legacy device endpoint, ensuring that legacy devices are properly retrieved with their access information. The SQL setup and cleanup methodology is consistent with the rest of the test class.
frontend/src/i18n/en-US.json (1)
204-213: Added comprehensive localization for legacy device UI components.The new entries provide clear and helpful information about legacy devices, including explanations about compatibility issues and migration paths. The consistency with existing device list entries ensures a uniform user experience.
backend/src/test/java/org/cryptomator/hub/api/DeviceResourceIT.java (5)
64-64: Updated DeviceDto creation to include legacyDevice flag.All instances of DeviceDto creation now include the new legacyDevice parameter set to false, which is appropriate for regular devices.
Also applies to: 145-145, 163-163, 174-174, 195-195, 311-311
236-269: Added tests to validate proper separation of device and legacy device APIs.These tests ensure that regular devices cannot be deleted via the legacy device endpoint and vice versa, maintaining proper API boundaries.
279-300: Added test for successful legacy device deletion.This test properly validates that legacy devices can be deleted through the appropriate endpoint and confirms the deletion was successful.
352-361: Enhanced test to verify device type identification.The test now correctly verifies that regular devices have the legacyDevice flag set to false when retrieved through the API.
363-371: Added test for legacy device retrieval.This test properly validates that the legacy device API returns only legacy devices with the legacyDevice flag set to true.
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java (7)
46-49: Nice import usage for optional and functional interfaces.Bringing in
Optional,Function, andConsumerneatly facilitates generic repository logic for removing devices without duplicating code.
205-212: Generic removal method looks solid.The approach of passing
findById,delete, andgetOwneras functional parameters effectively reduces code duplication and clarifies the removal logic. This is a neat refactor.
222-223: New fields support expanded device data.Adding
@JsonProperty("legacyDevice") boolean legacyDeviceand the newlastAccessTimeis well-aligned with the broader changes to identify and track legacy devices.
226-227: Correct default of non-legacy for standard devices.Explicitly setting
legacyDeviceto false infromEntity(Device)is consistent with the separation from legacy devices.
230-232: Deprecated constructor for legacy devices aligns with the newlegacyDevicefield.Marking this as deprecated helps highlight the eventual phase-out of legacy device handling. The usage of null for
publicKeyanduserPrivateKeysis sensible for legacy devices that don't store this data.
234-238: Consistent false assignment for normal device “with event” constructor.Setting
legacyDeviceto false when aVaultKeyRetrievedEventis present ensures the correct categorization of these devices.
240-244: Appropriate handling for legacy devices with event data.Identifying a device as legacy when combining data from
LegacyDeviceandVaultKeyRetrievedEventis consistent with the rest of the codebase.backend/src/main/java/org/cryptomator/hub/entities/LegacyDevice.java (16)
4-4: Importing Parameters for named queries is appropriate.This aligns with using
Panachein a structured way for parameter injection in query methods.
8-9: Enum usage is well-suited to represent device types.Explicitly assigning
EnumType.STRINGavoids potential confusion if the enum’s ordinal changes in the future.
12-13: Many-to-one ownership mapping clarifies relations.Replacing a simple
ownerIdstring with an actualUserreference significantly improves type safety and navigation in the data model.
14-14: Named queries for device list retrieval are well introduced.Ensuring reusability and clarity by naming query fragments is a solid practice.
17-20: Comprehensive query for fetching all devices in a list.This chunk provides a convenient way to retrieve multiple entities in a single call, matching the usage in
getSomeLegacy.
21-27: Named query for deleting by owner.Facilitates quick removal of all legacy devices belonging to a user. This complements the user-based ownership design.
37-39: Loading theUserlazily is practical.This prevents unnecessary data loading if the
ownerfield is not needed, optimizing performance for large queries.
41-42: Introducing a name field for the device.Storing a human-readable device name fosters a better user experience and aligns with the changes in
DeviceResource.
44-46: Device type ensures consistent categorization.This is consistent with the main
Deviceentity and provides clarity regarding the device’s classification.
48-49: Creation time captures the device’s initialization moment.Storing the timestamp for auditing and reference is beneficial, aligning with the logic in the updated
DeviceDto.
59-61: Getter forownerensures direct access to the user relationship.Switching to an actual user entity clarifies ownership without manually managing user IDs.
63-65: Getter fornameis straightforward.No concerns here—this property is used in logging and the new
DeviceDtologic.
67-68: Getter forcreationTimehelps in data consistency and auditing.Aggregating an explicit creation time in legacy devices aligns these entities with the standard device approach.
71-72: Getter fortypeprovides a consistent interface to determine device types.Aligns with
Device.Type, ensuring unified logic between legacy and current devices.
81-83:findAllInList: Good specialized query method.This method efficiently returns a stream of requested entities, matching the approach used for standard devices.
85-87:deleteByOwnerfosters bulk removal of legacy devices.This method is straightforward and aligns with the new naming and ownership logic.
backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java
Outdated
Show resolved
Hide resolved
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
| INNER JOIN EffectiveVaultAccess perm ON token.id.vaultId = perm.id.vaultId AND device.ownerId = perm.id.authorityId | ||
| WHERE token.id.vaultId = :vaultId AND token.id.deviceId = :deviceId AND device.ownerId = :userId | ||
| INNER JOIN EffectiveVaultAccess perm ON token.id.vaultId = perm.id.vaultId AND device.owner.id = perm.id.authorityId | ||
| WHERE token.id.vaultId = :vaultId AND token.id.deviceId = :deviceId AND device.owner.id = :userId |
There was a problem hiding this comment.
I believe I have tested this and this approach would result in an additional join.
There was a problem hiding this comment.
You are right but I needed to add
hub/backend/src/main/java/org/cryptomator/hub/entities/User.java
Lines 150 to 152 in dcd175a
so that I get the legacy devices of a user in
...adding both, the owner and the ownerId isn't possible as well so I think unfortunately this is not avoidable.
| FROM LegacyDevice d | ||
| WHERE d.id IN :ids | ||
| """) | ||
| @NamedQuery(name = "LegacyDevice.deleteByOwner", query = "DELETE FROM LegacyDevice d WHERE d.owner.id = :userId") |
There was a problem hiding this comment.
for same reason as stated above (if I'm not mistaken):
| @NamedQuery(name = "LegacyDevice.deleteByOwner", query = "DELETE FROM LegacyDevice d WHERE d.owner.id = :userId") | |
| @NamedQuery(name = "LegacyDevice.deleteByOwner", query = "DELETE FROM LegacyDevice d WHERE d.ownerId = :userId") |
| @@ -293,12 +350,24 @@ public void testGetSomeNotExisting() { | |||
| } | |||
|
|
|||
| @Test | |||
| @DisplayName("GET /devices?ids=device2&ids=device3 returns 200 with body containing device2 and device3") | |||
| @DisplayName("GET /devices?ids=device2&ids=device3&ids=legacyDevice1 returns 200 with body containing device2 and device3") | |||
There was a problem hiding this comment.
| @DisplayName("GET /devices?ids=device2&ids=device3&ids=legacyDevice1 returns 200 with body containing device2 and device3") | |
| @DisplayName("GET /devices?ids=device2&ids=device3&ids=legacyDevice1 returns 200 with body containing device2, device3 and legacyDevice1") |
There was a problem hiding this comment.
No, legacyDevice1 is not returned, because it is a legacy device, see https://github.com/cryptomator/hub/pull/331/files/dcd175a8b3e5360a24408fdefdb0fbd3f573f039#diff-67652a0e51b56759c5824de898118a1bdb5cbf03d18b39d6aa77a54a5b38c32bR358
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (1)
184-203: Consider refactoring for duplicated logic.We see that
getMeWithLegacyDevicesAndAccess()mirrors the functionality ofgetMe()but for legacy devices. Having two very similar methods might lead to maintenance overhead. Consider extracting common logic or unifying them behind a single method if feasible.backend/src/main/java/org/cryptomator/hub/entities/LegacyDevice.java (2)
21-27: Named queries for bulk operations.The queries look correct for retrieving or deleting legacy devices by ID or owner. For large datasets, you could ensure an index on these fields to maintain performance.
88-94: Repository methods look solid; consider large-list performance.
findAllInListanddeleteByOwnerfulfill common bulk operations. In scenarios with large input lists, you may want to confirm acceptable performance or implement partial batching.
📜 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(4 hunks)backend/src/main/java/org/cryptomator/hub/api/UsersResource.java(4 hunks)backend/src/main/java/org/cryptomator/hub/entities/LegacyDevice.java(3 hunks)frontend/src/i18n/en-US.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/main/java/org/cryptomator/hub/api/DeviceResource.java
- frontend/src/i18n/en-US.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Tests
🔇 Additional comments (6)
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java (4)
24-24: Looks good importingLegacyDevice.This import enables the new legacy device handling logic in this class.
33-34: OpenAPI parameter imports are clear.These imports cleanly document query parameters for the API.
163-163: Parameter annotation forwithLastAccess.Including a descriptive parameter annotation helps clarify the purpose of
withLastAccessin the generated API docs.
176-179: Conditional logic for device retrieval is well-structured.Initializing
deviceDtosonly in specific if/else blocks (rather than upfront) aligns with the previous reviewer’s suggestion and ensures compile-time checks that all paths are handled.backend/src/main/java/org/cryptomator/hub/entities/LegacyDevice.java (2)
37-52: Confirm immutability ofownerfield.
@JoinColumn(name = "owner_id", updatable = false)prevents changing theownerafter creation. Verify this aligns with the intended design, as it disallows updates to the device’s owner once persisted.
62-72: Getter methods appear consistent and complete.The new getters for
owner,name,type,publickey, andcreationTimeare straightforward and match the entity fields correctly.Also applies to: 74-80
Still to do:
withLastAccessAndLegacyDevicesis a quiet ugly name, find a better oneaudit_event_vault_key_retrieve)