-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Adjusted to Hub 1.3.x API #3041
Conversation
# Conflicts: # src/main/java/org/cryptomator/ui/keyloading/hub/CreateDeviceDto.java # src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java
[ci skip]
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.
Please also add some screenshots of the new added/altered screens (/src/main/resoruces/fxml/hub_setup_device.fxml
).
src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/SetupDeviceController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/SetupDeviceController.java
Outdated
Show resolved
Hide resolved
.thenApply(response -> { | ||
if (response.statusCode() == 200) { | ||
var dto = fromJson(response.body()); | ||
return Objects.requireNonNull(dto, "null or empty response body"); |
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.
While this sanity check is not bad, is it necessary? The documention of fasterxmls ObjectReader.read(String, Class)
does not mention to return null.
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.
Bad documentation then. I did encounter null here, when the response body is null or empty.
src/main/java/org/cryptomator/ui/keyloading/hub/ReceivedKey.java
Outdated
Show resolved
Hide resolved
# Conflicts: # src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
leads to invalid class files when built via Maven due to specific combination of javac arguments
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.
I also tested old Hub against this Cryptomator branch and device registration, unlocking and unlocking archived showed the expected results
src/main/java/org/cryptomator/ui/keyloading/hub/HubKeyLoadingModule.java
Outdated
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java
Show resolved
Hide resolved
src/main/java/org/cryptomator/ui/keyloading/hub/RegisterDeviceController.java
Show resolved
Hide resolved
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.
The class org.cryptomator.ui.keyloading.hub.HttpHelper
seems to be unused. If that is true, remove it.
This PR contains adjustments required to ensure compatibility with upcoming changes to Hub (probably version 1.3.0):
At the same time, Cryptomator stays compatible with older Hub versions.
unlock: 200 (via legacy access tokens) or 403
unlock (legacy): ✓
Unlock
Caution
The following information are outdated. See #3287 for details.
With this change, Cryptomator attempts to
GET /api/vaults/{vaultId}/access-token
, which will result in a error 404 on old Hub instances, which will trigger the legacy mechanism:cryptomator/src/main/java/org/cryptomator/ui/keyloading/hub/ReceiveKeyController.java
Line 192 in 6ce34ef
Otherwise, the encrypted vault key is returned from Hub. This can be decrypted using the user's private key, which in its encrypted form is part of the JSON returned by
GET /api/devices/{deviceId}
.The full workflow is shown here:
Device Registration
The device registration uses the same API endpoint as before (
PUT /api/devices/{deviceId}
), however the JSON has new fields, most notably theuserPrivateKey
:cryptomator/src/main/java/org/cryptomator/ui/keyloading/hub/SetupDeviceController.java
Lines 222 to 227 in 6ce34ef
Due to
userPrivateKey
being a required field in the new Hub version, old Cryptomator clients will fail with Error 400 on an attempt to register to new Hub instances.In order to encrypt the user's private key for the new device, we require a two-step operation:
GET /api/users/me
to access a PBES-encrypted JWE containing the user key. The Setup Code is required for decryption.Future Compatibility Improvements
vault.cryptomator
files created by future Hub versions.Add some way to check Hub's API version. If newer than expected, ask the user to update Cryptomator. If older than expected show an corresponding message as well.Edit: Will become a separate PR, should we need to distinguish API Levelsvault.cryptomator
Test Checklist