Fix legacy hub compatibility #315
Conversation
For legacy reasons
For legacy reasons
|
Warning Rate limit exceeded@SailReal has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces significant modifications to the Possibly related PRs
Suggested reviewers
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 (
|
|
Just noticed: Still broken for adding/removing members, will fix it tomorrow. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java (1)
22-60: Well-implemented backward compatibility.The constructor effectively handles both new and legacy property names with clear fallback logic. The code is well-documented with comments explaining the dual property support.
However, consider adding validation to ensure that at least one of the paired properties (new or legacy) is provided:
if (ecdhPublicKey != null) { this.ecdhPublicKey = ecdhPublicKey; } else { + if (publicKey == null) { + throw new IllegalArgumentException("Either ecdhPublicKey or publicKey must be provided"); + } this.ecdhPublicKey = publicKey; }frontend/src/components/InitialSetup.vue (1)
300-312: Consider making device type configurable.The implementation securely handles device registration and key submission. However, the device type is hardcoded as 'BROWSER'.
Consider making the device type configurable to support different client types in the future:
-async function submitBrowserKeys(browserKeys: BrowserKeys, me: UserDto, userKeys: UserKeys) { +async function submitBrowserKeys(browserKeys: BrowserKeys, me: UserDto, userKeys: UserKeys, deviceType: string = 'BROWSER') { await backend.users.putMe(me); const jwe = await userKeys.encryptForDevice(browserKeys.keyPair.publicKey); await backend.devices.putDevice({ id: await browserKeys.id(), name: deviceName.value, - type: 'BROWSER', + type: deviceType, publicKey: await browserKeys.encodedPublicKey(), userPrivateKey: jwe, creationTime: new Date() }); userdata.reload(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java(2 hunks)backend/src/main/java/org/cryptomator/hub/api/UsersResource.java(3 hunks)frontend/src/common/backend.ts(1 hunks)frontend/src/common/userdata.ts(2 hunks)frontend/src/components/InitialSetup.vue(1 hunks)frontend/src/components/RegenerateSetupCodeDialog.vue(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (9)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java (3)
14-20: LGTM! Well-structured field declarations.The private final fields are properly encapsulated and follow consistent naming conventions.
77-88: Good use of deprecation annotations.The legacy getter is properly marked with
@Deprecated(forRemoval = true)while maintaining functionality.
95-106: Consistent implementation of private keys getters.The implementation follows the same pattern as the public key getters, maintaining consistency throughout the codebase.
frontend/src/common/userdata.ts (2)
96-99: Property reference updated correctly.The check for user initialization now uses the new
privateKeysproperty name, maintaining consistency with the backend changes.
131-131: Property assignment updated correctly.The assignment in
addEcdsaKeyIfMissingnow uses the newprivateKeysproperty name.frontend/src/components/RegenerateSetupCodeDialog.vue (1)
156-156: Property reference updated correctly.The assignment in
regenerateSetupCodenow uses the newprivateKeysproperty name.frontend/src/common/backend.ts (1)
78-78: Type definition updated correctly.The
UserDtotype now uses the newprivateKeysproperty name, ensuring type safety across the application.frontend/src/components/InitialSetup.vue (2)
280-298: LGTM! Secure key recovery implementation.The key recovery process follows secure practices:
- Trims user input
- Comprehensive error handling
- Uses the updated key decryption method
266-266: LGTM! Verify encryption compatibility.The change from
privateKeytoprivateKeysaligns with the PR objectives for legacy hub compatibility. The encryption process looks secure.Please verify that the encryption process is compatible with both legacy and new hub versions by checking the UserKeys implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for UserKeys implementation to verify encryption compatibility ast-grep --pattern 'class UserKeys { $$$ encryptWithSetupCode($$$) { $$$ } $$$ }'Length of output: 95
I'll execute an alternative search to locate the implementation of the encryption function. This will help us understand whether the compatibility with legacy hubs is maintained and how
encryptWithSetupCodeis implemented.
🏁 Script executed:
#!/bin/bash # Search for the encryptWithSetupCode function in the repository with context rg "encryptWithSetupCode(" -n -C 3 .Length of output: 131
Let's run a refined search for the
encryptWithSetupCodemethod, escaping any special regex characters to get a proper output.
🏁 Script executed:
#!/bin/bash # Use ripgrep with an escaped pattern for searching 'encryptWithSetupCode' in the repository rg "encryptWithSetupCode" -n -C 5 .Length of output: 4927
Encryption Compatibility Confirmed
We’ve verified that the
encryptWithSetupCodemethod is implemented infrontend/src/common/crypto.ts(usingJWEBuilder.pbes2) and is properly tested (see tests infrontend/test/common/crypto.spec.ts). The update fromprivateKeytoprivateKeysin theInitialSetup.vuecomponent correctly integrates this encryption process for both legacy and new hub support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java (2)
22-60: Consider using Optional for more concise null checks.While the constructor implementation is correct, the null checks could be more concise using Optional.
Here's a suggested improvement:
- if (ecdhPublicKey != null) { - this.ecdhPublicKey = ecdhPublicKey; - } else { - this.ecdhPublicKey = publicKey; - } + this.ecdhPublicKey = Optional.ofNullable(ecdhPublicKey).orElse(publicKey); - if (privateKeys != null) { - this.privateKeys = privateKeys; - } else { - this.privateKeys = privateKey; - } + this.privateKeys = Optional.ofNullable(privateKeys).orElse(privateKey);
117-131: Remove redundant ecdhPublicKey parameter in factory method.The
justPublicInfomethod passes the same valueuser.getEcdhPublicKey()twice to the constructor. Since one parameter is for legacy support, this duplication is unnecessary.Here's a suggested improvement:
return new UserDto( user.getId(), user.getName(), user.getPictureUrl(), user.getEmail(), user.getLanguage(), Set.of(), user.getEcdhPublicKey(), - user.getEcdhPublicKey(), + null, // legacy publicKey parameter user.getEcdsaPublicKey(), null, null, null);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java(2 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/api/UserDto.java (2)
14-20: LGTM! Well-structured field declarations.The fields are properly encapsulated as private final, with clear naming and appropriate types.
62-115: LGTM! Well-documented getter methods with proper deprecation notices.The getter methods are properly implemented with:
- Consistent @JsonProperty annotations
- Clear deprecation notices with issue references
- Proper implementation of legacy getters
There was a problem hiding this comment.
If I understand this correctly, deserialization accepts both names but prefers the new one (XOR), while serialization produces duplicate fields using both the new and the old name.
Clients should therefore be compatible as long as
- they ignore unknown fields during deserialization
- they produce JSON that conforms to either the old or the new standard.
This looks solid, however the legacy duplications hurt readability. I would therefore like to see a separate constructor that is used for every non-Jackson-related new UserDto() call.
Furthermore, could you please explain what you mean by
getting the userDto: must not contain devices
?
backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
Outdated
Show resolved
Hide resolved
Before the 1.4.0 changes, it was possible to first create the user and then create the devices. We are using it e.g. in hub-cli. If we now put an ( With b1cb347 we also accept |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java(2 hunks)backend/src/main/java/org/cryptomator/hub/api/UsersResource.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/org/cryptomator/hub/api/UsersResource.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (4)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java (4)
14-20: LGTM! Well-structured field declarations.The fields are properly encapsulated as private final, with clear and consistent naming that supports both new and legacy properties.
22-46: LGTM! Well-implemented constructor with legacy support.The constructor effectively handles both new and legacy properties using ternary operators for null checks. The separate
@JsonPropertyannotations (instead of@JsonAlias) correctly prevent property override issues when both new and legacy properties are provided.
48-67: LGTM! Clean secondary constructor implementation.The secondary constructor provides a clean API for internal use without legacy property handling.
70-122: LGTM! Well-documented getter methods with clear deprecation notices.The getters are properly annotated and include clear deprecation notices with migration guidance for legacy properties.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java (1)
22-46: Consider using this constructor as the primary one.The constructor effectively handles both new and legacy properties with proper validation. However, to improve maintainability, consider making this the primary constructor and having the other constructor delegate to it.
public UserDto( String id, String name, String pictureUrl, String email, String language, Set<DeviceResource.DeviceDto> devices, String ecdhPublicKey, String ecdsaPublicKey, String privateKeys, String setupCode) { - super(id, Type.USER, name, pictureUrl); - this.email = email; - this.language = language; - this.devices = devices; - this.ecdhPublicKey = ecdhPublicKey; - this.ecdsaPublicKey = ecdsaPublicKey; - this.privateKeys = privateKeys; - this.setupCode = setupCode; + this(id, name, pictureUrl, email, language, devices, ecdhPublicKey, null, ecdsaPublicKey, privateKeys, null, setupCode); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (3)
backend/src/main/java/org/cryptomator/hub/api/UserDto.java (3)
14-20: LGTM! Well-structured field declarations.The fields are properly encapsulated as private final, ensuring immutability and proper data hiding.
69-122: LGTM! Well-documented getters with proper deprecation notices.The getter methods are properly implemented with clear deprecation notices and issue tracking links for legacy methods.
124-136: LGTM! Factory method correctly implements public info DTO creation.The
justPublicInfomethod properly creates a UserDto instance with only public information, using the correct parameter order and null values for sensitive data.
This PR fixes using legacy API
privateKey/publicKeyprivateKey/publicKey