-
-
Notifications
You must be signed in to change notification settings - Fork 961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hub 1.2.x → Hub 1.3.x Legacy Device Migration #3291
Conversation
and `decodeECPublicKey(byte[])`
actual migration still missing due to API discussion
WalkthroughThe updates across several Java classes in the Cryptomator project focus on enhancing the key loading mechanism. The changes include improved URI handling, a shift from Google's BaseEncoding to Java's Base64 for encoding, the introduction of new methods for key encryption and decryption, and the addition of exception handling for key decoding failures. Additionally, the handling of API paths has been centralized, and there is a new approach to device registration and legacy device migration. Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (5)
- src/main/java/org/cryptomator/ui/keyloading/hub/HubConfig.java (3 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/JWEHelper.java (7 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java (6 hunks)
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java (5 hunks)
- src/test/java/org/cryptomator/ui/keyloading/hub/JWEHelperTest.java (3 hunks)
Additional comments: 21
src/main/java/org/cryptomator/ui/keyloading/hub/HubConfig.java (3)
- 3-3: The import of
com.fasterxml.jackson.annotation.JsonIgnore
is correctly added to support the new@JsonIgnore
annotation on theURIs
field.- 22-26: The addition of the
URIs
field with the@JsonIgnore
annotation is consistent with the summary stating that this field should not be included in JSON serialization/deserialization processes.- 50-61: The nested class
URIProcessors
with theAPI
method is added as described in the summary. This method is used to resolve paths relative to the/api/
endpoint, which is a logical change for centralizing URI construction.src/main/java/org/cryptomator/ui/keyloading/hub/JWEHelper.java (7)
- 15-15: The import of
org.cryptomator.cryptolib.api.CryptoException
is correctly added to support the new exception handling in the encryption and decryption methods.- 55-77: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [71-84]
The method
decodeECPrivateKey
is added to decode a DER-encoded EC private key. It correctly throws aKeyDecodeFailedException
if the key cannot be decoded, which aligns with the summary.
- 94-107: The method
decodeECPublicKey
is added to decode a DER-encoded EC public key. It correctly throws aKeyDecodeFailedException
if the key cannot be decoded, which aligns with the summary.- 110-112: The method
encryptVaultKey
is added to encrypt aMasterkey
with a user'sECPublicKey
. This method is part of the new functionality for handling encryption of vault keys.- 114-125: The
encryptKey
method is refactored to useBase64.getEncoder()
instead ofBaseEncoding.base64()
, which is consistent with the summary stating thatBaseEncoding.base64()
usage has been replaced.- 148-153: The
readKey
method is updated to useBase64.getDecoder()
instead ofBaseEncoding.base64()
. This change is consistent with the summary and the overall refactoring to remove theBaseEncoding
import.- 168-172: The new exception class
KeyDecodeFailedException
is added as a subclass ofCryptoException
, providing a more specific exception for key decoding failures.src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java (4)
- 91-91: The usage of
hubConfig.URIs.API
to construct theconfigUri
is consistent with the changes inHubConfig.java
and the summary provided.- 125-125: The usage of
hubConfig.URIs.API
to construct thedeviceUri
is consistent with the changes inHubConfig.java
and the summary provided.- 165-165: The usage of
hubConfig.URIs.API
to construct thevaultKeyUri
is consistent with the changes inHubConfig.java
and the summary provided.- 208-208: The usage of
hubConfig.URIs.API
to construct thelegacyAccessTokenUri
is consistent with the changes inHubConfig.java
and the summary provided.src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java (3)
- 34-46: The addition of new imports (
InterruptedIOException
,UncheckedIOException
,ECPublicKey
,Base64
,List
) is consistent with the new methods added for handling user and device registration and migration of legacy devices.- 130-148: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [117-138]
The modifications to the
register
method, including the retrieval of user data and the migration of legacy devices, are consistent with the PR objectives to support legacy device migration.
- 262-263: The introduction of the
VaultDto
record for parsing JSON responses is consistent with the summary and the new functionality for handling JSON responses during the migration process.src/test/java/org/cryptomator/ui/keyloading/hub/JWEHelperTest.java (4)
- 4-4: The addition of the
Masterkey
import is necessary for the new test methods that involve encryption and decryption of vault keys.- 8-10: The addition of
BeforeAll
andNested
imports is necessary for setting up the nested test class structure and initializing shared test data.- 36-62: The new nested class
Decoders
with methods for DER decoding is added, which is consistent with the changes inJWEHelper.java
and the summary provided.- 175-200: New test methods for encryption and decryption of vault and user keys have been included, which is necessary to ensure the new functionality works as expected.
src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
This adds a new subroutine during Device Registration, which attempts to migrate existing access tokens by re-encrypting them for the authenticated user.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation