Hub support #560
Hub support #560
Conversation
WalkthroughThe recent changes encompass a comprehensive update across multiple files, primarily enhancing the functionality related to hub vault management and cryptographic operations. Key modifications include the introduction of new dependencies in the Gradle files, such as New exception classes have been introduced to manage specific error conditions related to hub operations, enhancing the robustness of error handling throughout the application. Additionally, several dialog interfaces and layout files have been created to facilitate user interactions for hub device setup and related notifications. The 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 35
🧹 Outside diff range and nitpick comments (82)
domain/src/main/java/org/cryptomator/domain/UnverifiedVaultConfig.kt (1)
6-6: LGTM! Consider adding documentation and validation.The open class design is appropriate for extensibility, particularly for the hub-specific vault configurations. However, a few improvements could enhance the robustness:
Consider adding:
- KDoc documentation explaining:
- Purpose of each property
- Valid format/values for
vaultFormat- Expected JWT format
- Property validation:
open class UnverifiedVaultConfig( - open val jwt: String, - open val keyId: URI, - open val vaultFormat: Int + open val jwt: String, + open val keyId: URI, + open val vaultFormat: Int ) : Serializable { + init { + require(jwt.isNotBlank()) { "JWT token cannot be blank" } + require(vaultFormat > 0) { "Vault format must be positive" } + } }
- Consider using nullable types or sealed ranges if appropriate for your use case:
open val jwt: String?, // If JWT is optional open val vaultFormat: Int = 1 // If there's a default formatdomain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultIsArchivedException.java (1)
5-10: Consider enhancing the exception with documentation and additional constructors.The exception could be more useful with the following improvements:
- Add Javadoc explaining when this exception is thrown
- Add constructors with message and cause parameters for better error context
Consider applying this enhancement:
public class HubVaultIsArchivedException extends BackendException { + private static final long serialVersionUID = 1L; + + /** + * Thrown when attempting to perform operations on a Hub vault that has been archived. + */ public HubVaultIsArchivedException() { super(); } + + /** + * @param message the detail message + */ + public HubVaultIsArchivedException(String message) { + super(message); + } + + /** + * @param message the detail message + * @param cause the cause of this exception + */ + public HubVaultIsArchivedException(String message, Throwable cause) { + super(message, cause); + } }domain/src/main/java/org/cryptomator/domain/exception/hub/HubUserSetupRequiredException.java (1)
7-9: Consider adding additional constructors for better error reporting.The current implementation only provides a default constructor. Consider adding constructors that accept a message and/or cause parameter for more detailed error reporting.
public class HubUserSetupRequiredException extends BackendException { public HubUserSetupRequiredException() { super(); } + + public HubUserSetupRequiredException(String message) { + super(message); + } + + public HubUserSetupRequiredException(String message, Throwable cause) { + super(message, cause); + } + + public HubUserSetupRequiredException(Throwable cause) { + super(cause); + } }domain/src/main/java/org/cryptomator/domain/exception/hub/HubDeviceSetupRequiredException.java (2)
5-11: Add JavaDoc documentation to clarify exception usageWhile the basic implementation is correct, adding documentation would improve clarity for developers:
- When this exception is thrown
- What conditions trigger it
- How to handle it
Add documentation like this:
+/** + * Exception thrown when a hub device requires setup before it can be used. + * This typically occurs when attempting to perform hub operations with an + * unregistered or improperly configured device. + */ public class HubDeviceSetupRequiredException extends BackendException {
7-9: Consider adding constructors with message and cause parametersThe current implementation only provides a no-arg constructor. Consider adding additional constructors for better error context:
public HubDeviceSetupRequiredException() { super(); } + +public HubDeviceSetupRequiredException(String message) { + super(message); +} + +public HubDeviceSetupRequiredException(String message, Throwable cause) { + super(message, cause); +}domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultAccessForbiddenException.java (2)
5-5: Add class-level documentation.Consider adding Javadoc to describe when this exception is thrown and provide examples of scenarios that trigger it. This will help other developers understand its purpose and proper usage.
+/** + * Exception thrown when access to a Hub vault is denied due to insufficient permissions + * or invalid access rights. This typically occurs when a user attempts to access + * a vault they are not authorized to use. + */ public class HubVaultAccessForbiddenException extends BackendException {
7-9: Consider adding additional constructors for better error handling.The current implementation only provides a no-args constructor. Consider adding constructors that accept a message and/or cause for more detailed error reporting.
public HubVaultAccessForbiddenException() { super(); } + +/** + * Constructs a new exception with the specified detail message. + * + * @param message the detail message + */ +public HubVaultAccessForbiddenException(String message) { + super(message); +} + +/** + * Constructs a new exception with the specified detail message and cause. + * + * @param message the detail message + * @param cause the cause of this exception + */ +public HubVaultAccessForbiddenException(String message, Throwable cause) { + super(message, cause); +}domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidSetupCodeException.java (1)
5-5: Consider adding serialVersionUIDFor better serialization support, consider adding a serialVersionUID field.
public class HubInvalidSetupCodeException extends BackendException { + private static final long serialVersionUID = 1L;domain/src/main/java/org/cryptomator/domain/exception/hub/HubLicenseUpgradeRequiredException.java (1)
5-11: Add serialVersionUID for serialization compatibility.Since this exception extends
BackendExceptionwhich likely implementsSerializable, adding aserialVersionUIDwould ensure consistent serialization across different versions of the application.public class HubLicenseUpgradeRequiredException extends BackendException { + private static final long serialVersionUID = 1L; public HubLicenseUpgradeRequiredException() { super(); } }domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidVersionException.java (1)
5-11: Consider enhancing the exception with documentation and version details.To improve maintainability and developer experience, consider:
- Adding Javadoc explaining when this exception is thrown and how to handle it
- Including version information (required vs actual) in the exception
Here's a suggested implementation:
package org.cryptomator.domain.exception.hub; import org.cryptomator.domain.exception.BackendException; +/** + * Thrown when attempting to interact with a Hub instance that has an incompatible version. + */ public class HubInvalidVersionException extends BackendException { + private final String requiredVersion; + private final String actualVersion; + + /** + * Creates a new HubInvalidVersionException. + * + * @param message The error message + * @param requiredVersion The minimum required Hub version + * @param actualVersion The actual Hub version encountered + */ + public HubInvalidVersionException(String message, String requiredVersion, String actualVersion) { + super(message); + this.requiredVersion = requiredVersion; + this.actualVersion = actualVersion; + } + + public String getRequiredVersion() { + return requiredVersion; + } + + public String getActualVersion() { + return actualVersion; + } }domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultOperationNotSupportedException.java (1)
5-5: Add class documentationPlease add Javadoc to describe when this exception is thrown and provide examples of unsupported operations. This will help other developers understand when to use this exception.
+/** + * Exception thrown when attempting an operation that is not supported for Hub vaults. + * For example, when trying to perform operations that are only available for local vaults + * or when attempting to use features not yet implemented for Hub vaults. + */ public class HubVaultOperationNotSupportedException extends BackendException {domain/src/main/java/org/cryptomator/domain/exception/hub/HubDeviceAlreadyRegisteredForOtherUserException.java (1)
7-9: Consider enhancing the constructor to provide more context.The current implementation only provides a default constructor. Consider adding constructors that accept a message and/or cause to provide more detailed error information.
Consider adding these constructors:
public HubDeviceAlreadyRegisteredForOtherUserException() { super(); } + +public HubDeviceAlreadyRegisteredForOtherUserException(String message) { + super(message); +} + +public HubDeviceAlreadyRegisteredForOtherUserException(String message, Throwable cause) { + super(message, cause); +}domain/src/main/java/org/cryptomator/domain/exception/hub/HubAuthenticationFailedException.java (1)
5-15: Consider enhancing the exception class with additional features.The exception class could benefit from the following improvements:
- Add JavaDoc documentation explaining when this exception is thrown
- Include a constructor that accepts a message string
- Add serialVersionUID for serialization support
Here's the suggested implementation:
package org.cryptomator.domain.exception.hub; import org.cryptomator.domain.exception.BackendException; +/** + * Thrown when authentication with Cryptomator Hub fails. + * This can occur during initial authentication or when refreshing credentials. + */ public class HubAuthenticationFailedException extends BackendException { + private static final long serialVersionUID = 1L; public HubAuthenticationFailedException() { super(); } public HubAuthenticationFailedException(Exception e) { super(e); } + public HubAuthenticationFailedException(String message) { + super(message); + } + + public HubAuthenticationFailedException(String message, Exception e) { + super(message, e); + } }domain/src/main/java/org/cryptomator/domain/UnverifiedHubVaultConfig.kt (1)
5-23: Consider separating authentication configuration from vault configuration.The class currently handles both vault configuration and OAuth/authentication configuration. Consider splitting these responsibilities into separate classes to improve maintainability and follow the Single Responsibility Principle:
HubAuthConfigfor auth-related propertiesUnverifiedHubVaultConfigfocusing only on vault-specific propertiesExample structure:
data class HubAuthConfig( val clientId: String, val authEndpoint: String, val tokenEndpoint: String, val authSuccessUrl: String, val authErrorUrl: String, val apiBaseUrl: String?, val devicesResourceUrl: String, ) class UnverifiedHubVaultConfig( override val jwt: String, override val keyId: URI, override val vaultFormat: Int, val authConfig: HubAuthConfig, ) : UnverifiedVaultConfig(jwt, keyId, vaultFormat)presentation/src/main/java/org/cryptomator/presentation/ui/activity/view/UnlockVaultView.kt (1)
17-21: Consider adding KDoc documentation for the new Hub methodsThe new Hub-related methods are well-named and follow the existing pattern, but would benefit from documentation explaining:
- The purpose of each dialog
- The expected user interactions
- The relationship between parameters (e.g., vaultModel and unverifiedVaultConfig)
Add KDoc comments like this:
+ /** + * Shows dialog for creating a new Hub device for vault access + * @param vaultModel The vault requiring device creation + * @param unverifiedVaultConfig The unverified Hub vault configuration + */ fun showCreateHubDeviceDialog(vaultModel: VaultModel, unverifiedVaultConfig: UnverifiedHubVaultConfig) + /** + * Shows dialog when user setup is required for Hub access + * @param unverifiedVaultConfig The unverified Hub vault configuration + */ fun showHubUserSetupRequiredDialog(unverifiedHubVaultConfig: UnverifiedHubVaultConfig) + /** + * Shows dialog when user needs to upgrade their Hub license + */ fun showHubLicenseUpgradeRequiredDialog() + /** + * Shows dialog when user lacks permission to access the Hub vault + */ fun showHubVaultAccessForbiddenDialog() + /** + * Shows dialog when attempting to access an archived Hub vault + */ fun showHubVaultIsArchivedDialog()domain/src/main/java/org/cryptomator/domain/repository/HubRepository.kt (2)
6-7: Add KDoc documentation for the interface.Consider adding documentation to explain the interface's purpose, responsibilities, and its role in Hub support.
+/** + * Repository interface for interacting with Cryptomator Hub. + * Handles vault key management, user authentication, device registration, + * and configuration retrieval operations. + */ interface HubRepository {
6-29: Consider architectural improvements for better separation of concerns.The current interface combines different responsibilities (user management, device management, vault management). Consider splitting this into more focused interfaces:
HubUserRepositoryfor user-related operationsHubDeviceRepositoryfor device managementHubVaultRepositoryfor vault-specific operationsAlso, consider adding version compatibility checks since the Hub API level is part of the configuration.
Would you like me to provide an example of how to split these interfaces?
data/src/main/java/org/cryptomator/data/repository/RepositoryModule.java (1)
35-39: Fix typo in method nameThe method name
provideHubRepositoryRepositorycontains a duplicate "Repository" word. This should be corrected to maintain consistency with other provider methods in this class.Apply this change:
- public HubRepository provideHubRepositoryRepository(HubRepositoryImpl hubRepository) { + public HubRepository provideHubRepository(HubRepositoryImpl hubRepository) {domain/src/main/java/org/cryptomator/domain/usecases/vault/CreateHubDevice.java (2)
10-11: Add class-level documentation.Consider adding Javadoc to document:
- The purpose of this use case
- Why the class is package-private
- The relationship with HubRepository
- Potential exceptions that might be thrown
@UseCase +/** + * Use case for registering a new device with Cryptomator Hub. + * Package-private to ensure access only through the use case factory. + * + * @throws BackendException when device registration fails + * @throws HubAuthenticationFailedException when authentication fails + * @throws HubDeviceAlreadyRegisteredForOtherUserException when device is already registered + * @throws HubInvalidSetupCodeException when setup code is invalid + */ class CreateHubDevice {
1-32: Consider architectural improvements.
- Consider implementing the Command pattern with an undo operation for rollback scenarios
- Add a result object to provide more detailed feedback about the device creation
- Consider using a builder pattern for complex object construction
Example Result object:
public class HubDeviceCreationResult { private final String deviceId; private final Instant createdAt; private final String status; // ... constructor and getters }data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudProvider.java (1)
22-22: Add Javadoc for the new unlock method.The new unlock method introduces Hub-specific parameters using JWE format. Consider adding Javadoc to explain:
- The purpose and format of
vaultKeyJweanduserKeyJwe- When this unlock method should be used vs. the password-based methods
- The expected format of the JWE tokens
Example documentation:
/** * Unlocks a vault using Hub-provided JWE format keys. * * @param vault The vault to unlock * @param unverifiedVaultConfig The vault configuration * @param vaultKeyJwe The vault key in JWE format * @param userKeyJwe The user key in JWE format * @param cancelledFlag Flag to cancel the operation * @return The unlocked vault * @throws BackendException If the unlock operation fails */presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultAccessForbiddenDialog.kt (3)
13-16: Consider adding KDoc documentationAdding KDoc documentation to the interface and method would improve API clarity for consumers.
+/** + * Callback interface for handling Hub vault access forbidden dialog events + */ interface Callback { + /** + * Called when the vault access forbidden dialog is finished, + * either by clicking the neutral button or pressing back + */ fun onVaultAccessForbiddenDialogFinished() }
33-34: Document or implement the empty setupView methodThe empty
setupView()method should either be documented if intentionally empty or implemented if functionality is missing.+ /** + * View setup is not required for this dialog as all configuration + * is handled in setupDialog + */ public override fun setupView() { }🧰 Tools
🪛 detekt (1.23.7)
[warning] 33-34: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
1-42: Consider enhancing error handling robustnessTo improve the dialog's reliability, consider:
- Adding null safety checks for the dialog reference
- Handling edge cases (e.g., multiple dismissal attempts)
- Adding logging for debugging purposes
🧰 Tools
🪛 detekt (1.23.7)
[warning] 33-34: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubLicenseUpgradeRequiredDialog.kt (2)
13-16: Consider adding KDoc documentationAdding KDoc documentation to the interface and its method would improve maintainability and help other developers understand the expected behavior.
+/** + * Callback interface for handling Hub license upgrade dialog events. + */ interface Callback { + /** + * Called when the Hub license upgrade dialog is finished, either by user action or system events. + */ fun onHubLicenseUpgradeRequiredDialogFinished() }
37-42: Consider adding KDoc documentation to the factory methodAdding documentation would help clarify the purpose and usage of this factory method.
companion object { + /** + * Creates a new instance of HubLicenseUpgradeRequiredDialog. + * @return A new dialog instance ready to be shown. + */ fun newInstance(): HubLicenseUpgradeRequiredDialog { return HubLicenseUpgradeRequiredDialog() } }presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultArchivedDialog.kt (5)
13-16: Add KDoc documentation for the interface and method.Consider adding documentation to explain when this callback is triggered and its purpose in the context of Hub vault archival.
+/** + * Callback interface to handle Hub vault archived dialog completion. + */ interface Callback { + /** + * Called when the Hub vault archived dialog is dismissed either by + * clicking the neutral button or pressing back. + */ fun onHubVaultArchivedDialogFinished() }
19-21: Remove unnecessary line continuation comments.The builder chain can be more readable without the `//' comments.
- builder // - .setTitle(R.string.dialog_hub_vault_archived_title) // - .setNeutralButton(getString(R.string.dialog_hub_vault_archived_positive_button)) { _: DialogInterface, _: Int -> callback?.onHubVaultArchivedDialogFinished() } + builder + .setTitle(R.string.dialog_hub_vault_archived_title) + .setNeutralButton(getString(R.string.dialog_hub_vault_archived_positive_button)) { _: DialogInterface, _: Int -> + callback?.onHubVaultArchivedDialogFinished() + }
22-30: Improve key event handling logic.The current implementation of
OnKeyListenercould be simplified and made more idiomatic.- .setOnKeyListener { _, keyCode, _ -> - if (keyCode == KeyEvent.KEYCODE_BACK) { - dialog?.dismiss() - callback?.onHubVaultArchivedDialogFinished() - true - } else { - false - } - } + .setOnKeyListener { _, keyCode, event -> + when { + keyCode == KeyEvent.KEYCODE_BACK && event.action == KeyEvent.ACTION_UP -> { + dialog?.dismiss() + callback?.onHubVaultArchivedDialogFinished() + true + } + else -> false + } + }
34-38: Simplify view setup using Kotlin scope functions.The code can be more concise using Kotlin's
as?andapplyfunctions.- public override fun setupView() { - super.onStart() - val dialog = dialog as AlertDialog? - dialog?.setCanceledOnTouchOutside(false) - } + public override fun setupView() { + super.onStart() + (dialog as? AlertDialog)?.apply { + setCanceledOnTouchOutside(false) + } + }
40-45: Add KDoc for the factory method.Document the factory method to explain its purpose and usage.
companion object { + /** + * Creates a new instance of HubVaultArchivedDialog. + * @return A new dialog instance for displaying Hub vault archived message. + */ fun newInstance(): HubVaultArchivedDialog { return HubVaultArchivedDialog() } }presentation/src/main/java/org/cryptomator/presentation/di/component/ApplicationComponent.java (1)
40-40: LGTM! Well-integrated Hub repository declaration.The
hubRepository()method follows the established pattern for repository injection and is properly scoped at the application level. The integration with RepositoryModule ensures proper dependency provision.The placement of this repository at the application component level is appropriate as it will likely be needed across different features related to Hub functionality.
domain/src/main/java/org/cryptomator/domain/repository/CloudRepository.java (3)
44-45: Consider maintaining consistency with Optional parameterThe
unverifiedVaultConfigparameter is required here, while other unlock methods useOptional<UnverifiedVaultConfig>. This inconsistency might force implementers to handle null checks differently.Consider this alternative signature:
-Cloud unlock(Vault vault, UnverifiedVaultConfig unverifiedVaultConfig, String vaultKeyJwe, String userKeyJwe, Flag cancelledFlag) throws BackendException; +Cloud unlock(Vault vault, Optional<UnverifiedVaultConfig> unverifiedVaultConfig, String vaultKeyJwe, String userKeyJwe, Flag cancelledFlag) throws BackendException;
44-45: Add Javadoc for the new unlock methodThe new method introduces JWE parameters which would benefit from documentation explaining their format, content, and purpose, especially since this appears to be part of the Hub authentication flow.
Consider adding documentation:
+/** + * Unlocks a vault using Hub authentication. + * + * @param vault The vault to unlock + * @param unverifiedVaultConfig The vault configuration + * @param vaultKeyJwe The vault key in JWE format + * @param userKeyJwe The user key in JWE format + * @param cancelledFlag Flag to indicate cancellation + * @return The unlocked cloud + * @throws BackendException if the unlock operation fails + */ Cloud unlock(Vault vault, UnverifiedVaultConfig unverifiedVaultConfig, String vaultKeyJwe, String userKeyJwe, Flag cancelledFlag) throws BackendException;
44-45: Consider refactoring unlock method variantsThe interface now has three unlock methods with similar signatures but different authentication mechanisms (password-based vs JWE-based). This might indicate a need for a more flexible design.
Consider these architectural improvements:
- Create an
UnlockCredentialsinterface with implementations for different authentication methods:
PasswordCredentialsHubJweCredentials- Consolidate the unlock methods into a single method accepting
UnlockCredentialsThis would make the interface more maintainable and extensible for future authentication methods.
presentation/src/main/java/org/cryptomator/presentation/model/ProgressStateModel.kt (1)
50-50: LGTM with a minor suggestion on constant ordering.The new constant follows the established patterns and integrates well with the existing progress states. Consider grouping it with other creation-related constants (like
CREATING_VAULT,CREATING_FOLDER) for better code organization.Apply this diff to improve constant grouping:
val CREATING_FOLDER = ProgressStateModel("FOLDER", text(R.string.dialog_progress_creating_folder)) val CREATING_TEXT_FILE = ProgressStateModel("FILE", text(R.string.dialog_progress_creating_text_file)) +val CREATING_HUB_DEVICE = ProgressStateModel("HUB_DEVICE", text(R.string.dialog_progress_creating_hub_device_setup)) val UNLOCKING_VAULT = ProgressStateModel("VAULT", text(R.string.dialog_progress_unlocking_vault)) val CHANGING_PASSWORD = ProgressStateModel("PASSWORD", text(R.string.dialog_progress_change_password)) val CREATING_VAULT = ProgressStateModel("VAULT", text(R.string.dialog_progress_creating_vault)) -val CREATING_HUB_DEVICE = ProgressStateModel("HUB_DEVICE", text(R.string.dialog_progress_creating_hub_device_setup)) val UNKNOWN = ProgressStateModel("UNKNOWN_MIMETYPE", text(R.string.dialog_progress_please_wait))util/build.gradle (1)
56-57: Consider security configurations for OAuth flowsSince this module will be handling JWT tokens as part of Hub support (likely OAuth flows):
- Ensure ProGuard/R8 rules are properly configured to prevent JWT classes from being obfuscated
- Consider adding security-related Android manifest configurations (e.g., network security config, cleartext traffic policy)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubUserSetupRequiredDialog.kt (2)
18-23: Add KDoc documentation for the Callback interface.Consider adding documentation to explain:
- When each callback is triggered
- The expected behavior when implementing these callbacks
- The significance of the UnverifiedHubVaultConfig parameter
Example documentation:
/** * Interface defining callback methods for Hub user setup actions */ interface Callback { /** * Called when the user chooses to proceed with Hub profile setup * @param unverifiedVaultConfig The configuration of the vault pending verification */ fun onGoToHubProfileClicked(unverifiedVaultConfig: UnverifiedHubVaultConfig) /** * Called when the user cancels the Hub setup process */ fun onCancelHubUserSetupClicked() }
57-57: Make the constant name more descriptive.The constant
VAULT_CONFIG_ARGcould be more specific to indicate it's for unverified vault configuration.- private const val VAULT_CONFIG_ARG = "vaultConfig" + private const val UNVERIFIED_VAULT_CONFIG_ARG = "unverifiedVaultConfig"presentation/src/main/res/layout/dialog_create_hub_device.xml (1)
2-9: Consider using ConstraintLayout instead of RelativeLayoutConstraintLayout is the recommended layout for complex views as it provides better performance and maintainability. It would be particularly beneficial here for handling the relationships between the input fields and included layouts.
<androidx.core.widget.NestedScrollView xmlns:android="http://schemas.android.com/apk/res/android" android:layout_width="match_parent" android:layout_height="match_parent"> - <RelativeLayout + <androidx.constraintlayout.widget.ConstraintLayout android:layout_width="match_parent" android:layout_height="match_parent" android:padding="@dimen/activity_vertical_margin">presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt (4)
27-28: Consider using Parcelable instead of Serializable for better type safety and performanceThe callback method uses Serializable parameters (
VaultModelandUnverifiedHubVaultConfig). Consider implementing Parcelable instead, as it's more type-safe and performs better on Android.
46-54: Simplify back button handling using early returnThe back button handling can be simplified using an early return pattern for better readability.
- dialog.setOnKeyListener { _, keyCode, _ -> - if (keyCode == KeyEvent.KEYCODE_BACK) { - dialog.dismiss() - callback?.onCreateHubDeviceCanceled() - true - } else { - false - } - } + dialog.setOnKeyListener { _, keyCode, _ -> + if (keyCode != KeyEvent.KEYCODE_BACK) return@setOnKeyListener false + dialog.dismiss() + callback?.onCreateHubDeviceCanceled() + true + }
85-86: Add documentation for empty overrideThe empty
setupView()method is likely required by the parent class. Add a comment explaining why it's empty or remove it if not required.- override fun setupView() { - } + override fun setupView() { + // No additional view setup required as it's handled in onStart + }🧰 Tools
🪛 detekt (1.23.7)
[warning] 85-86: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
108-115: Consider using Fragment arguments delegationThe fragment arguments handling can be simplified using Kotlin's property delegation with the Fragment KTX library.
// At class level private val vaultModel: VaultModel by lazy { requireArguments().getSerializable(VAULT_ARG) as VaultModel } private val unverifiedVaultConfig: UnverifiedHubVaultConfig by lazy { requireArguments().getSerializable(VAULT_CONFIG_ARG) as UnverifiedHubVaultConfig }data/src/main/java/org/cryptomator/data/repository/CloudRepositoryImpl.java (2)
112-118: Add documentation for the new unlock method.The new unlock method appears to be part of the Hub support feature. Please add Javadoc explaining:
- The purpose of this method
- The expected format/content of vaultKeyJwe and userKeyJwe parameters
- The specific exceptions that might be thrown
Example:
/** * Unlocks a vault using Hub authentication. * * @param vault The vault to unlock * @param unverifiedVaultConfig The vault configuration * @param vaultKeyJwe The vault key in JWE format * @param userKeyJwe The user key in JWE format * @param cancelledFlag Flag to cancel the operation * @return Decrypted view of the vault * @throws BackendException if unlock fails * @throws HubAuthenticationFailedException if Hub authentication fails * @throws HubVaultAccessForbiddenException if access is forbidden */
114-116: Consider adding parameter validation and specific error handling.The method could benefit from:
- Validation of JWE parameters before delegation
- More specific exception handling for Hub-related failures
Consider this implementation:
public Cloud unlock(Vault vault, UnverifiedVaultConfig unverifiedVaultConfig, String vaultKeyJwe, String userKeyJwe, Flag cancelledFlag) throws BackendException { + if (vaultKeyJwe == null || vaultKeyJwe.isEmpty() || userKeyJwe == null || userKeyJwe.isEmpty()) { + throw new IllegalArgumentException("JWE parameters cannot be null or empty"); + } + try { Vault vaultWithVersion = cryptoCloudFactory.unlock(vault, unverifiedVaultConfig, vaultKeyJwe, userKeyJwe, cancelledFlag); return decryptedViewOf(vaultWithVersion); + } catch (BackendException e) { + // Rethrow specific Hub-related exceptions + throw e; + } }presentation/src/main/java/org/cryptomator/presentation/exception/ExceptionHandlers.kt (1)
82-86: Consider implementing a Hub-specific error handling strategyAs Hub support grows, consider implementing a dedicated
HubExceptionHandlerclass to encapsulate all Hub-related error handling logic. This would:
- Improve maintainability by centralizing Hub error handling
- Allow for Hub-specific error recovery strategies
- Make it easier to add new Hub-related error cases in the future
Example approach:
class HubExceptionHandler : ExceptionHandler { override fun handle(view: View, e: Throwable): Boolean { return when (e) { is HubAuthenticationFailedException -> handleAuthError(view, e) is HubInvalidVersionException -> handleVersionError(view, e) // ... other Hub-specific handlers else -> false } } private fun handleAuthError(view: View, e: HubAuthenticationFailedException): Boolean { // Hub-specific authentication error recovery logic } // ... other handling methods }data/build.gradle (1)
199-200: Consider grouping OAuth/JWT related dependenciesFor better organization, consider grouping this dependency with other authentication-related dependencies (like
msgraphAuth) under a comment section.implementation dependencies.jsonWebToken + // OAuth & JWT implementation dependencies.joseJwt + // cloud playstoreImplementation dependencies.dropboxCoredata/src/main/java/org/cryptomator/data/cloud/crypto/MasterkeyCryptoCloudProvider.kt (1)
129-131: Consider enhancing the error message for better clarity.The implementation correctly enforces separation of concerns by throwing an
IllegalStateExceptionfor hub-based unlock attempts on a password-based vault provider. However, the error message could be more descriptive.Consider this more descriptive error message:
- throw IllegalStateException("Password based vaults do not support hub unlock") + throw IllegalStateException("This vault is configured for password-based authentication and does not support Cryptomator Hub authentication")presentation/src/main/res/values/strings.xml (2)
533-541: Consider renaming the setup code related string identifiers.While the user-facing strings consistently use "Account Key", the string identifiers still use "setup_code". Consider renaming these identifiers for better code maintainability:
- <string name="dialog_create_hub_device_setup_code_label">Account Key</string> - <string name="dialog_create_hub_device_setup_code_hint">Your Account Key is required...</string> - <string name="dialog_create_hub_device_setup_code_empty">Account Key can\'t be empty.</string> + <string name="dialog_create_hub_device_account_key_label">Account Key</string> + <string name="dialog_create_hub_device_account_key_hint">Your Account Key is required...</string> + <string name="dialog_create_hub_device_account_key_empty">Account Key can\'t be empty.</string>
544-544: Remove extra quotation marks.There are unnecessary quotation marks at the end of these hint messages:
- <string name="dialog_hub_vault_access_forbidden_hint">Your user has not yet been authorized to access this vault. Ask the vault owner to authorize it."</string> + <string name="dialog_hub_vault_access_forbidden_hint">Your user has not yet been authorized to access this vault. Ask the vault owner to authorize it.</string> - <string name="dialog_hub_license_upgrade_required_hint">Your Cryptomator Hub instance has an invalid license. Please inform a Hub administrator to upgrade or renew the license."</string> + <string name="dialog_hub_license_upgrade_required_hint">Your Cryptomator Hub instance has an invalid license. Please inform a Hub administrator to upgrade or renew the license.</string>Also applies to: 548-548
domain/src/main/java/org/cryptomator/domain/usecases/vault/UnlockHubVault.java (2)
17-17: MakeHUB_MINIMUM_VERSIONa static constantConstants should be declared as
static finalto prevent unnecessary instance copies.Apply this diff to update the declaration:
- private final int HUB_MINIMUM_VERSION = 1; + private static final int HUB_MINIMUM_VERSION = 1;
24-29: SimplifycancelledFlaginitialization using a lambda expressionSince
Flagis a functional interface, you can use a lambda expression to simplify the code.Apply this diff to simplify the code:
- private final Flag cancelledFlag = new Flag() { - @Override - public boolean get() { - return cancelled; - } - }; + private final Flag cancelledFlag = () -> cancelled;data/src/main/java/org/cryptomator/data/cloud/crypto/HubkeyCryptoCloudProvider.kt (4)
49-51: Check cancellation flag earlier to avoid unnecessary processingCurrently, the cancellation flag is checked after parsing JWE objects and decrypting the master key. To improve efficiency and responsiveness, consider checking the
cancelledFlagat the beginning of the method to avoid unnecessary operations if the unlock process has been canceled.Apply this change:
// ...
26-28: UseUnsupportedOperationExceptionfor unsupported methodsThe methods
create,unlock,createUnlockToken,isVaultPasswordValid, andchangePasswordcurrently throwIllegalStateExceptionto indicate that certain operations are not supported. Consider usingUnsupportedOperationException, which is more appropriate for signaling that a method is not implemented or not supported.Apply this diff to replace
IllegalStateExceptionwithUnsupportedOperationException:Also applies to: 31-33, 36-38, 62-64, 72-74, 89-91
62-64: Clarify exception messages to reflect the specific unsupported operationsThe exception messages in
createUnlockToken,isVaultPasswordValid, andchangePasswordmethods currently state "Hub cannot unlock vaults using password", which may not accurately describe the unsupported operation. Updating the exception messages to clearly indicate the specific unsupported functionality enhances code readability and maintainability.Example changes:
// In createUnlockToken method // In isVaultPasswordValid method // In changePassword methodAlso applies to: 72-74, 89-91
66-69: Restrict visibility ofcryptorFormethodThe
cryptorFormethod is marked with a comment "// Visible for testing", suggesting it is intended for internal use or testing purposes. Consider reducing its visibility tointernalorprivateto encapsulate the implementation details and prevent unintended usage from outside the class.Apply this change to restrict the method's visibility:
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudFactory.java (1)
96-98: Rename parameter for clarity.The parameter name
unverifiedVaultConfigOptionalmay be misleading since it is not anOptional. Consider renaming it tounverifiedVaultConfigto improve readability.Apply this diff to rename the parameter:
-private CryptoCloudProvider cryptoCloudProvider(UnverifiedVaultConfig unverifiedVaultConfigOptional) { - return cryptoCloudProvider(Optional.of(unverifiedVaultConfigOptional)); +private CryptoCloudProvider cryptoCloudProvider(UnverifiedVaultConfig unverifiedVaultConfig) { + return cryptoCloudProvider(Optional.of(unverifiedVaultConfig)); }presentation/src/main/java/org/cryptomator/presentation/ui/activity/UnlockVaultActivity.kt (4)
34-39: Consider ordering implemented interfaces for better readabilityFor improved readability and maintainability, consider ordering the implemented interfaces alphabetically or grouping related callbacks together. This makes it easier for other developers to navigate the code.
121-140: Refactor dialog methods to reduce code duplicationThe
show*Dialogmethods share a similar structure. Refactoring these methods by creating a generic function can reduce code duplication and enhance maintainability.You could introduce a generic method like this:
private fun showHubDialog(dialog: DialogFragment) { showDialog(dialog) }Then update the dialog methods:
override fun showCreateHubDeviceDialog(vaultModel: VaultModel, unverifiedVaultConfig: UnverifiedHubVaultConfig) { showHubDialog(CreateHubDeviceDialog.newInstance(vaultModel, unverifiedVaultConfig)) } override fun showHubUserSetupRequiredDialog(unverifiedHubVaultConfig: UnverifiedHubVaultConfig) { showHubDialog(HubUserSetupRequiredDialog.newInstance(unverifiedHubVaultConfig)) } // Apply the same pattern to other dialog methods
157-184: Simplify hub callback methods to enhance maintainabilityThe callback methods related to hub dialogs have similar patterns. Consider consolidating these methods into a single handler or using a unified interface to reduce boilerplate code.
For example, you could define a generic callback interface:
interface HubDialogCallback { fun onPositiveAction(data: Any?) fun onNegativeAction() }Implement this interface in your activity:
class UnlockVaultActivity : BaseActivity<ActivityUnlockVaultBinding>(...), HubDialogCallback { // ... override fun onPositiveAction(data: Any?) { // Handle positive action with the provided data } override fun onNegativeAction() { finish() } }Update your dialogs to use this generic callback:
class CreateHubDeviceDialog : DialogFragment() { // ... private lateinit var callback: HubDialogCallback override fun onAttach(context: Context) { super.onAttach(context) callback = context as HubDialogCallback } // Call the callback methods where appropriate }
5-5: Verify the necessity of the new importEnsure that
UnverifiedHubVaultConfigis utilized in the code. If it's not being used, consider removing the import to keep the code clean.util/src/main/java/org/cryptomator/util/crypto/HubDeviceCryptor.java (3)
49-219: Enhance documentation with Javadoc commentsAdding Javadoc comments to the
HubDeviceCryptorclass and its public methods will improve understandability and assist other developers in using the class correctly.Consider documenting:
- The purpose of each public method.
- The expected inputs and outputs.
- Any thrown exceptions and under what conditions they are thrown.
127-140: Handle potential empty key field scenarioThe variable
keyBytesis initialized as an empty array, which may lead to issues if thekeyFieldis not present in the payload. Ensure that the absence of the key is properly handled.Consider adding a check for the presence of the key:
if (fields.get(keyField) instanceof String key) { keyBytes = Base64.getDecoder().decode(key); return rawKeyFactory.apply(keyBytes); } else { + throw new MasterkeyLoadingFailedException("JWE payload missing required key field: " + keyField); }
62-70: Specify explicit key generation parametersWhen generating the key pair, ensure that all parameters are explicitly set to avoid unintended defaults, which may affect security.
Consider specifying the provider for the
KeyPairGenerator:var keyPairGenerator = KeyPairGenerator.getInstance(KeyProperties.KEY_ALGORITHM_EC, DEFAULT_KEYSTORE_NAME); +keyPairGenerator.initialize(parameterSpec, new SecureRandom()); keyPairGenerator.generateKeyPair();Also, ensure that a secure random number generator is used.
data/src/main/java/org/cryptomator/data/repository/HubRepositoryImpl.java (12)
103-103: Handle specific HTTP errors such as Unauthorized (401)In the
getUsermethod, responses with HTTP status code401 Unauthorizedare not specifically handled. This could lead to a genericFatalBackendExceptionwithout clear context for the caller. Consider handling401 Unauthorizedexplicitly to provide more meaningful feedback.Apply this diff to handle the
401 Unauthorizedstatus code:if (response.isSuccessful() && response.body() != null) { JSONObject jsonObject = new JSONObject(response.body().string()); return new UserDto(jsonObject.getString("id"), jsonObject.getString("name"), jsonObject.getString("publicKey"), jsonObject.getString("privateKey"), jsonObject.getString("setupCode")); } + else if (response.code() == HttpURLConnection.HTTP_UNAUTHORIZED) { + throw new HubAuthenticationFailedException(); + } throw new FatalBackendException("Failed to load user, bad response code " + response.code());
129-130: WrapIOExceptionwithBackendExceptionConsistent exception handling is important for maintainability. Replace the thrown
FatalBackendExceptionwith a more generalBackendExceptionor a specific exception to better represent the error context.Apply this diff:
} catch (IOException | JSONException e) { - throw new FatalBackendException("Failed to load device", e); + throw new BackendException("Failed to load device", e); }
154-155: Correct exception message when building JSON objectThe exception message "Failed to parse user private key" is misleading in this context. The error occurs while building the JSON object for the device data, not parsing the user private key.
Apply this diff to correct the exception message:
} catch (JSONException e) { - throw new FatalBackendException("Failed to parse user private key", e); + throw new FatalBackendException("Failed to build device JSON object", e); }
171-172: Adjust exception message for device creation failureIn the
createDevicemethod, the exception message refers to loading the device, which is inaccurate. It should reflect that the failure occurred during device creation.Apply this diff to update the exception message:
default: - throw new FatalBackendException("Failed to load device with response code " + response.code()); + throw new FatalBackendException("Failed to create device with response code " + response.code());
189-190: Correct exception messages ingetConfigmethodThe exception messages mention "device" instead of "config," which could be confusing. Update them to accurately reflect the context.
Apply this diff to correct the exception messages:
} else { - throw new FatalBackendException("Failed to load device, response code good but no body"); + throw new FatalBackendException("Failed to load config, response code good but no body"); } ... throw new FatalBackendException("Failed to load config with response code " + response.code());
93-107: Avoid potential NullPointerException and improve JSON parsingWhen parsing the response in
getUser, if required JSON fields are missing,getStringwill throw aJSONException. It might be better to check for the presence of keys and handle missing fields gracefully.Consider modifying the code to validate JSON fields:
JSONObject jsonObject = new JSONObject(response.body().string()); + if (!jsonObject.has("id") || !jsonObject.has("name") || !jsonObject.has("publicKey") || !jsonObject.has("privateKey") || !jsonObject.has("setupCode")) { + throw new FatalBackendException("Incomplete user data received"); + } return new UserDto( jsonObject.getString("id"), jsonObject.getString("name"), jsonObject.getString("publicKey"), jsonObject.getString("privateKey"), jsonObject.getString("setupCode") );
68-89: Refactor repetitive error handling codeThe try-catch blocks and response code handling logic are similar across multiple methods. Consider refactoring this logic into a helper method to reduce code duplication and enhance maintainability.
As an example, you could create a method to handle responses:
private String handleResponse(Response response) throws BackendException { switch (response.code()) { case HttpURLConnection.HTTP_OK: if (response.body() != null) { return response.body().string(); } else { throw new FatalBackendException("Response code OK but body is null"); } // Handle other status codes... default: throw new FatalBackendException("Unexpected response code " + response.code()); } }Then use this helper in your methods.
115-128: Handle additional HTTP status codesIn the
getDevicemethod, onlyHTTP_OKandHTTP_NOT_FOUNDare explicitly handled. Consider adding handling for other relevant status codes such asHTTP_UNAUTHORIZED(401) andHTTP_FORBIDDEN(403) to provide clearer error messages and improve error handling.Apply this diff to handle additional status codes:
switch (response.code()) { case HttpURLConnection.HTTP_OK: // Existing code... break; + case HttpURLConnection.HTTP_UNAUTHORIZED: + throw new HubAuthenticationFailedException(); + case HttpURLConnection.HTTP_FORBIDDEN: + throw new HubAccessForbiddenException(); case HttpURLConnection.HTTP_NOT_FOUND: throw new HubDeviceSetupRequiredException(); // Rest of the code... }
43-55: Consider Dependency Injection forOkHttpClientandHubDeviceCryptorFor better testability and adherence to the Dependency Injection principle, consider injecting
OkHttpClientandHubDeviceCryptorrather than instantiating them within the constructor.Modify the constructor to accept these dependencies:
@Inject -public HubRepositoryImpl(Context context) { - this.httpClient = new OkHttpClient.Builder() - // Existing builder code... - .build(); - this.hubDeviceCryptor = HubDeviceCryptor.getInstance(); +public HubRepositoryImpl(Context context, OkHttpClient httpClient, HubDeviceCryptor hubDeviceCryptor) { + this.httpClient = httpClient; + this.hubDeviceCryptor = hubDeviceCryptor; }This allows for easier testing and more flexible configuration.
57-60: ReuseHttpLoggingInterceptorinstanceCurrently, a new
HttpLoggingInterceptoris created each timehttpLoggingInterceptoris called. Since the interceptor is stateless, you can create it once and reuse it, reducing overhead.Refactor by initializing the interceptor once:
+ private final Interceptor httpLoggingInterceptor; ... public HubRepositoryImpl(Context context) { this.httpClient = new OkHttpClient.Builder() - .addInterceptor(httpLoggingInterceptor(context)) + .addInterceptor(httpLoggingInterceptor) // Existing builder code... .build(); + this.httpLoggingInterceptor = createHttpLoggingInterceptor(); this.hubDeviceCryptor = HubDeviceCryptor.getInstance(); } -private Interceptor httpLoggingInterceptor(Context context) { +private Interceptor createHttpLoggingInterceptor() { HttpLoggingInterceptor.Logger logger = message -> Timber.tag("OkHttp").d(message); return new HttpLoggingInterceptor(logger); }
185-195: Improve exception messages and error handling ingetConfigThe exception messages reference "device" instead of "config," which can be misleading. Also, consider handling specific HTTP error codes for better clarity.
Apply this diff:
if (response.isSuccessful()) { if (response.body() != null) { JSONObject jsonObject = new JSONObject(response.body().string()); return new ConfigDto(jsonObject.getInt("apiLevel")); } else { - throw new FatalBackendException("Failed to load device, response code good but no body"); + throw new FatalBackendException("Failed to load config, response code OK but body is null"); } } else { - throw new FatalBackendException("Failed to load device with response code " + response.code()); + throw new FatalBackendException("Failed to load config with response code " + response.code()); }Additionally, handle specific HTTP status codes if applicable.
148-154: Ensure consistent date formattingWhen putting the "creationTime" into the JSON object, ensure that the date format aligns with the backend expectations, such as using ISO 8601 format with UTC timezone.
Verify that
Instant.now().toString()provides the correct format, or consider using a formatter.dto.put("creationTime", Instant.now().toString()); + // Ensure the date is in the correct format, e.g., + dto.put("creationTime", ZonedDateTime.now(ZoneOffset.UTC).format(DateTimeFormatter.ISO_INSTANT));util/src/androidTest/java/org/cryptomator/util/crypto/HubDeviceCryptorTest.java (3)
67-69: Ensure test assertions provide informative failure messagesThe
assertThatmethod from Hamcrest provides more expressive assertion messages. However, combining it with JUnit 5 requires importingorg.hamcrest.MatcherAssert. Ensure that the assertion imports are consistent with the testing framework.Update the import:
- import static org.junit.Assert.assertThat; + import static org.hamcrest.MatcherAssert.assertThat;Alternatively, if switching entirely to JUnit 5, consider using
assertEqualsfor simplicity:- assertThat(deviceId, is("F82D0F002724A2916C5695016A17A7E8A3092FE99E0BF65B44998630330C54CA")); + assertEquals("F82D0F002724A2916C5695016A17A7E8A3092FE99E0BF65B44998630330C54CA", deviceId);
142-142: Simplify exception assertions using static importsIn the test methods, you're using
Assertions.assertThrows, but you've already statically importedassertThrows. For consistency and readability, you can use the statically imported method directly.Update the assertions:
- Assertions.assertThrows(HubDeviceCryptor.InvalidJweKeyException.class, () -> HubDeviceCryptor.decryptUserKey(userKeyJwe, "WRONG_SETUP_CODE")); + assertThrows(HubDeviceCryptor.InvalidJweKeyException.class, () -> HubDeviceCryptor.decryptUserKey(userKeyJwe, "WRONG_SETUP_CODE"));Apply similar changes in other test methods where applicable.
Also applies to: 153-156, 176-176
99-108: Handle potential exceptions in cryptographic operationsWhile the test assumes successful decryption and comparison of keys, it's good practice to handle potential exceptions that may arise during cryptographic operations, such as
ParseExceptionorInvalidKeySpecException.Add appropriate exception declarations to the test method signature or handle them within the method.
- public void testDecryptMasterkeyUsingDeviceKey() throws ParseException { + public void testDecryptMasterkeyUsingDeviceKey() throws Exception {Or handle exceptions using try-catch blocks if specific handling is required.
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (2)
241-264: Reduce code duplication betweenhubAuthenticationUnlockandhubAuthenticationCreateDeviceBoth methods share similar logic for handling authentication responses and token exchanges.
Extract the shared logic into a separate method to improve maintainability:
private fun handleHubAuthenticationResponse( result: ActivityResult, onSuccess: (String) -> Unit, onError: (Throwable) -> Unit ) { if (result.isResultOk) { val resp = AuthorizationResponse.fromIntent(result.intent()) if (resp != null) { hubAuthService?.performTokenRequest(resp.createTokenExchangeRequest()) { token, ex -> token?.accessToken?.let { onSuccess(it) } ?: onError(HubAuthenticationFailedException(ex ?: Exception("Unknown error during token exchange"))) } } else { val ex = AuthorizationException.fromIntent(result.intent()) onError(HubAuthenticationFailedException(ex)) } } else { onError(HubAuthenticationFailedException()) } }Then update the original methods to use this helper:
fun hubAuthenticationUnlock(result: ActivityResult, vault: Vault, unverifiedHubVaultConfig: UnverifiedHubVaultConfig) { handleHubAuthenticationResponse(result, { accessToken -> // Existing success logic }, { error -> showErrorAndFinish(error) }) }
Line range hint
287-290: Unutilized methodonWindowFocusChangedwithFIXMEcommentThe method
onWindowFocusChangedis marked with aFIXMEcomment and isn't used anywhere in the class.Consider removing this method if it's obsolete or implement its functionality if needed. Would you like assistance in refactoring or determining its necessity?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (48)
buildsystem/dependencies.gradle(5 hunks)data/build.gradle(1 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudFactory.java(4 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudProvider.java(1 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoConstants.kt(1 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/HubkeyCryptoCloudProvider.kt(1 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/MasterkeyCryptoCloudProvider.kt(1 hunks)data/src/main/java/org/cryptomator/data/cloud/crypto/VaultConfig.kt(2 hunks)data/src/main/java/org/cryptomator/data/repository/CloudRepositoryImpl.java(1 hunks)data/src/main/java/org/cryptomator/data/repository/HubRepositoryImpl.java(1 hunks)data/src/main/java/org/cryptomator/data/repository/RepositoryModule.java(2 hunks)domain/src/main/java/org/cryptomator/domain/UnverifiedHubVaultConfig.kt(1 hunks)domain/src/main/java/org/cryptomator/domain/UnverifiedVaultConfig.kt(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubAuthenticationFailedException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubDeviceAlreadyRegisteredForOtherUserException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubDeviceSetupRequiredException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidSetupCodeException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidVersionException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubLicenseUpgradeRequiredException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubUserSetupRequiredException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultAccessForbiddenException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultIsArchivedException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultOperationNotSupportedException.java(1 hunks)domain/src/main/java/org/cryptomator/domain/repository/CloudRepository.java(1 hunks)domain/src/main/java/org/cryptomator/domain/repository/HubRepository.kt(1 hunks)domain/src/main/java/org/cryptomator/domain/usecases/vault/CreateHubDevice.java(1 hunks)domain/src/main/java/org/cryptomator/domain/usecases/vault/UnlockHubVault.java(1 hunks)presentation/build.gradle(2 hunks)presentation/src/main/java/org/cryptomator/presentation/di/component/ApplicationComponent.java(2 hunks)presentation/src/main/java/org/cryptomator/presentation/exception/ExceptionHandlers.kt(2 hunks)presentation/src/main/java/org/cryptomator/presentation/model/ProgressStateModel.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt(10 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/activity/UnlockVaultActivity.kt(5 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/activity/view/UnlockVaultView.kt(2 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubLicenseUpgradeRequiredDialog.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubUserSetupRequiredDialog.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultAccessForbiddenDialog.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultArchivedDialog.kt(1 hunks)presentation/src/main/res/layout/dialog_create_hub_device.xml(1 hunks)presentation/src/main/res/layout/dialog_hub_license_upgrade_required.xml(1 hunks)presentation/src/main/res/layout/dialog_hub_user_setup_required.xml(1 hunks)presentation/src/main/res/layout/dialog_hub_vault_access_forbidden.xml(1 hunks)presentation/src/main/res/layout/dialog_hub_vault_archived.xml(1 hunks)presentation/src/main/res/values/strings.xml(2 hunks)util/build.gradle(2 hunks)util/src/androidTest/java/org/cryptomator/util/crypto/HubDeviceCryptorTest.java(1 hunks)util/src/main/java/org/cryptomator/util/crypto/HubDeviceCryptor.java(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- presentation/src/main/res/layout/dialog_hub_license_upgrade_required.xml
- presentation/src/main/res/layout/dialog_hub_user_setup_required.xml
- presentation/src/main/res/layout/dialog_hub_vault_access_forbidden.xml
- presentation/src/main/res/layout/dialog_hub_vault_archived.xml
🧰 Additional context used
🪛 detekt (1.23.7)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt
[warning] 85-86: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubLicenseUpgradeRequiredDialog.kt
[warning] 34-35: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultAccessForbiddenDialog.kt
[warning] 33-34: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🪛 Gitleaks (8.21.2)
util/src/androidTest/java/org/cryptomator/util/crypto/HubDeviceCryptorTest.java
41-41: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (44)
domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultIsArchivedException.java (1)
1-11: LGTM! The basic exception implementation is correct.
The exception class is appropriately placed in the hub-specific package and extends the correct parent class.
domain/src/main/java/org/cryptomator/domain/exception/hub/HubUserSetupRequiredException.java (2)
1-11: LGTM! The basic implementation looks good.
The exception class is well-placed in the hub-specific exception package and correctly extends BackendException.
1-11: Verify exception handling in Hub-related components.
Let's verify how this exception is being caught and handled in the codebase.
✅ Verification successful
Let me try a different search strategy to understand how this exception is used in the codebase.
Exception handling is properly implemented across the codebase
The HubUserSetupRequiredException is appropriately thrown and handled:
- Thrown in
HubRepositoryImplwhen server returns status code 449, indicating user setup is required - Caught and handled in
UnlockVaultPresenterwhich shows a user setup dialog viaview?.showHubUserSetupRequiredDialog(unverifiedHubVaultConfig) - Part of a comprehensive Hub-related exception handling hierarchy alongside other Hub exceptions
- Proper error messages are defined in the presentation layer's
ExceptionHandlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of HubUserSetupRequiredException to ensure proper error handling
# Search for catch blocks handling this exception
echo "Searching for exception handling..."
rg -A 3 "catch.*HubUserSetupRequiredException"
# Search for throws declarations
echo "Searching for methods that throw this exception..."
rg "throws.*HubUserSetupRequiredException"
# Search for potential UI components that might need to handle this
echo "Searching for related UI components..."
fd -e java -e kt | rg -l "Hub.*Dialog|Hub.*Activity|Hub.*Fragment"
Length of output: 449
Script:
#!/bin/bash
# Search for any references to HubUserSetupRequiredException
echo "Searching for all references to HubUserSetupRequiredException..."
rg "HubUserSetupRequiredException"
# Search for classes that might throw this exception
echo "Searching for Hub-related service/repository classes..."
fd -e java -e kt | rg -l "Hub.*Service|Hub.*Repository"
# Search for any Hub-related error handling
echo "Searching for Hub-related error handling patterns..."
rg -A 5 "Hub.*Exception"
Length of output: 35230
domain/src/main/java/org/cryptomator/domain/exception/hub/HubDeviceSetupRequiredException.java (1)
1-11: LGTM - Basic implementation is correct
The exception class:
- Correctly extends BackendException
- Is properly placed in the hub-specific exception package
- Follows Java exception handling patterns
domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultAccessForbiddenException.java (1)
5-9: LGTM! The basic implementation is correct.
The exception class is appropriately placed in the domain layer and extends the correct parent class.
domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidSetupCodeException.java (1)
1-11: Verify exception handling implementation
Let's verify the exception handling patterns and usage in the codebase.
domain/src/main/java/org/cryptomator/domain/exception/hub/HubLicenseUpgradeRequiredException.java (1)
1-11: Verify exception handling in Hub-related workflows.
This exception seems to be part of a larger Hub feature implementation. Ensure proper exception handling is implemented in the UI layer to show appropriate upgrade messages to users.
✅ Verification successful
Let me check the content of the HubLicenseUpgradeRequiredDialog to verify proper exception handling.
Let me check the string resources to verify the user-facing messages in the dialog.
Exception handling for Hub license upgrade is properly implemented
The verification shows a complete implementation of the exception handling flow:
- The exception is caught in
UnlockVaultPresenterand mapped toshowHubLicenseUpgradeRequiredDialog() - A dedicated dialog
HubLicenseUpgradeRequiredDialogis implemented with proper user messaging - Clear user-facing messages are defined in string resources explaining the license upgrade requirement
- The exception is properly thrown from the data layer (
HubRepositoryImpl) based on HTTP response codes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper exception handling implementation
# Expected results: Find UI components handling this exception and verify error messages
# Check for exception handling in activities/fragments
rg -t java -t kotlin "catch.*HubLicenseUpgradeRequiredException" -A 5
# Look for related dialog implementations
fd -e java -e kt . | rg -i "hub.*dialog|dialog.*hub"
Length of output: 605
Script:
#!/bin/bash
# Check the implementation of HubLicenseUpgradeRequiredDialog
cat "presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubLicenseUpgradeRequiredDialog.kt"
# Search for where this dialog is being used
rg "HubLicenseUpgradeRequiredDialog" -l
# Look for exception handling in view models or presenters
rg "HubLicenseUpgradeRequiredException" -A 5
Length of output: 6182
Script:
#!/bin/bash
# Check the dialog's string resources
rg "dialog_hub_license_upgrade_required" presentation/src/main/res/values/strings.xml
# Check UnlockVaultView implementation to verify the dialog showing method
cat "presentation/src/main/java/org/cryptomator/presentation/ui/activity/view/UnlockVaultView.kt"
Length of output: 1672
domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidVersionException.java (1)
5-10: LGTM! The implementation is clean and follows good practices.
The exception class is appropriately placed in the domain layer and follows proper inheritance hierarchy.
domain/src/main/java/org/cryptomator/domain/exception/hub/HubAuthenticationFailedException.java (1)
1-15: LGTM! The basic implementation follows Java exception handling patterns.
The exception class is properly structured and placed in the appropriate package hierarchy.
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoConstants.kt (1)
9-9: LGTM! Well-defined scheme constant.
The HUB_SCHEME constant follows the established pattern and aligns well with the existing MASTERKEY_SCHEME constant.
presentation/src/main/java/org/cryptomator/presentation/ui/activity/view/UnlockVaultView.kt (2)
3-3: Verify the UnverifiedHubVaultConfig import usage
The new import UnverifiedHubVaultConfig is correctly used in the new Hub-related methods.
Let's verify this is the only place using this class:
✅ Verification successful
Import of UnverifiedHubVaultConfig is correctly used and consistent across the codebase
The import is used consistently throughout the Hub-related components:
- Defined in
domain/src/main/java/org/cryptomator/domain/UnverifiedHubVaultConfig.kt - Used in view interfaces, activities, dialogs, presenters, and repositories
- All usages are related to Hub vault configuration and authentication flows
- The import in UnlockVaultView.kt is necessary for the Hub-related method signatures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of UnverifiedHubVaultConfig across the codebase
rg "UnverifiedHubVaultConfig" --type kotlin
Length of output: 6111
17-21: Verify error handling in implementing classes
The interface methods appear to handle various error scenarios for Hub vaults appropriately. However, we should ensure implementing classes handle these scenarios gracefully.
Let's check the implementing classes:
✅ Verification successful
Error handling for Hub vaults is properly implemented
The verification shows that UnlockVaultActivity implements all Hub-related error scenarios appropriately:
- Each method properly delegates to a corresponding dialog using
showDialog - All required dialog classes are properly imported
- The presenter (
UnlockVaultPresenter) correctly maps specific Hub exceptions to the appropriate dialog methods - The implementation is consistent and follows a uniform pattern for error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find implementations of UnlockVaultView
ast-grep --pattern 'class $_ : $$$UnlockVaultView$$$'
Length of output: 55
Script:
#!/bin/bash
# Let's try a different approach using ripgrep to find implementations
rg "class.*implements.*UnlockVaultView|class.*:.*UnlockVaultView" -l
Length of output: 70
Script:
#!/bin/bash
# Also search for Kotlin files that might contain UnlockVaultView implementations
fd -e kt | xargs rg "UnlockVaultView"
Length of output: 767
Script:
#!/bin/bash
# Let's also check where these Hub-related methods are called
rg "show(CreateHubDeviceDialog|HubUserSetupRequiredDialog|HubLicenseUpgradeRequiredDialog|HubVaultAccessForbiddenDialog|HubVaultIsArchivedDialog)"
Length of output: 2693
Script:
#!/bin/bash
# Let's check the implementation in UnlockVaultActivity to see how these dialogs are handled
rg -A 10 "fun show(CreateHubDeviceDialog|HubUserSetupRequiredDialog|HubLicenseUpgradeRequiredDialog|HubVaultAccessForbiddenDialog|HubVaultIsArchivedDialog)" "presentation/src/main/java/org/cryptomator/presentation/ui/activity/UnlockVaultActivity.kt"
Length of output: 1302
Script:
#!/bin/bash
# Let's also check for any dialog-related imports in the Activity
rg "^import.*Dialog" "presentation/src/main/java/org/cryptomator/presentation/ui/activity/UnlockVaultActivity.kt"
Length of output: 756
domain/src/main/java/org/cryptomator/domain/repository/HubRepository.kt (1)
1-5: LGTM! Clean package structure and minimal imports.
The interface is correctly placed in the domain layer and follows clean architecture principles with minimal dependencies.
data/src/main/java/org/cryptomator/data/repository/RepositoryModule.java (1)
5-5: LGTM!
The import statement is correctly placed and follows the existing import pattern.
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudProvider.java (1)
22-22: Verify the consistency of UnverifiedVaultConfig handling.
The new method takes UnverifiedVaultConfig as a direct parameter while other methods use Optional<UnverifiedVaultConfig>. This inconsistency might indicate that Hub vaults always require a config, but this should be verified.
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultAccessForbiddenDialog.kt (3)
1-11: LGTM! Well-structured class declaration
The class is properly structured with appropriate use of data binding, inheritance, and generics.
36-41: LGTM! Clean factory method implementation
The companion object provides a clear and concise factory method following Kotlin conventions.
18-31:
Fix return value handling in setOnKeyListener
The lambda's return value handling is incorrect. The boolean return value should be the last expression in both branches.
.setOnKeyListener { _, keyCode, _ ->
if (keyCode == KeyEvent.KEYCODE_BACK) {
dialog?.dismiss()
callback?.onVaultAccessForbiddenDialogFinished()
- true
+ true
} else {
- false
+ false
}
-}
+}Likely invalid or redundant comment.
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubLicenseUpgradeRequiredDialog.kt (1)
1-11: LGTM: Class structure and imports are well-organized
The class follows Kotlin best practices with proper use of generics and data binding.
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultArchivedDialog.kt (2)
1-11: LGTM! Clean class structure and proper use of imports.
The class is well-organized with appropriate inheritance from BaseDialog and proper use of data binding.
1-46: Verify dialog resources and integration.
Let's ensure all required resources exist and the dialog is properly integrated with the Hub support feature.
✅ Verification successful
Dialog resources and integration are properly set up
The verification confirms:
- Required string resources exist in
strings.xml:dialog_hub_vault_archived_titleis defined with text "Vault archived"dialog_hub_vault_archived_positive_buttonreferences another string resource
- Dialog is properly integrated in
UnlockVaultActivitywhich:- Implements the
HubVaultArchivedDialog.Callback - Shows the dialog via
showDialog(HubVaultArchivedDialog.newInstance()) - Handles the callback
onHubVaultArchivedDialogFinished()
- Implements the
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify string resources and dialog usage
# Check if required string resources exist
echo "Checking string resources..."
rg "dialog_hub_vault_archived_title|dialog_hub_vault_archived_positive_button" "presentation/src/main/res/values/strings.xml"
# Check dialog usage in the codebase
echo "Checking dialog usage..."
rg "HubVaultArchivedDialog" --type kotlin
Length of output: 2107
presentation/src/main/java/org/cryptomator/presentation/di/component/ApplicationComponent.java (1)
12-12: LGTM!
The import is properly placed and maintains the existing import organization.
util/build.gradle (2)
Line range hint 59-68: LGTM! Good test coverage setup
The addition of junitParams and mockitoAndroid for instrumentation tests complements the existing unit test setup, enabling comprehensive test coverage for both unit and integration tests.
56-57: Verify security implications of the JWT library
The addition of joseJwt aligns with the Hub support feature. However, JWT libraries require careful version management due to potential security implications.
Let's verify the JWT library version and check for known vulnerabilities:
✅ Verification successful
JWT library version 9.47 is secure and up-to-date
The JWT library version 9.47 is higher than all vulnerable versions found in security advisories:
- Latest HIGH severity issue affects versions < 9.37.2
- All other vulnerabilities affect much older versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check JWT library version and known vulnerabilities
# Check the version defined in dependencies.gradle
echo "Checking JWT version in dependencies.gradle:"
rg "joseJwtVersion" -A 1
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.nimbusds:nimbus-jose-jwt") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1911
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubUserSetupRequiredDialog.kt (3)
1-14: LGTM! Well-structured dialog class implementation.
The class follows Kotlin best practices with proper use of generics, view binding, and annotations.
1-66: Verify integration with other Hub-related components.
Let's verify how this dialog integrates with other Hub-related components and ensure consistent error handling.
✅ Verification successful
Based on the examination of the Hub-related dialogs and their integration in the UnlockVaultPresenter, I can now provide a final response:
Integration with Hub components is properly implemented and consistent
The dialog follows the established patterns seen in other Hub-related dialogs:
- Consistent error handling through the Callback interface
- Proper integration with UnlockVaultPresenter for Hub authentication flow
- Follows the same dialog setup pattern as HubVaultAccessForbiddenDialog and HubLicenseUpgradeRequiredDialog
- Proper handling of back button and touch outside events
- Consistent implementation of the BaseDialog pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other Hub-related dialogs and their usage patterns
echo "Checking Hub-related dialog implementations..."
rg -l "HubUserSetupRequiredDialog|Hub.*Dialog" --type kotlin
echo "Checking Hub-related error handling patterns..."
ast-grep --pattern 'class $_ : BaseDialog<$_, $_> {
$$$
interface Callback {
$$$
}
$$$
}'
Length of output: 1126
Script:
#!/bin/bash
# Let's examine the implementation of other Hub-related dialogs to verify consistency
echo "Checking HubVaultAccessForbiddenDialog implementation..."
cat presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubVaultAccessForbiddenDialog.kt
echo -e "\nChecking HubLicenseUpgradeRequiredDialog implementation..."
cat presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubLicenseUpgradeRequiredDialog.kt
echo -e "\nChecking usage in UnlockVaultPresenter..."
cat presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Length of output: 26607
25-40: Review button configuration in dialog setup.
The positive button is configured with neutral button text and behavior, which might be confusing:
- The method
setPositiveButtonis used but the string resource suggests it's a neutral button - The positive button click handler is empty
Let's verify the string resource and button usage:
Consider using setNeutralButton instead of setPositiveButton for better semantic clarity:
- .setPositiveButton(getString(R.string.dialog_hub_user_setup_required_neutral_button)) { _: DialogInterface, _: Int -> }
+ .setNeutralButton(getString(R.string.dialog_hub_user_setup_required_neutral_button)) { _: DialogInterface, _: Int -> }presentation/src/main/res/layout/dialog_create_hub_device.xml (1)
60-74: Verify visibility handling for progress and error views
Both progress and error views are positioned in the same location. Ensure proper visibility handling in the corresponding dialog class to prevent overlapping.
✅ Verification successful
Visibility handling is properly managed through base class
The CreateHubDeviceDialog inherits from BaseProgressErrorDialog which properly manages the visibility of progress and error views. From the earlier search results, we can see that BaseProgressErrorDialog implements the following visibility logic:
- When showing error: progress view is set to GONE and error view to VISIBLE
- When showing progress: error view is set to GONE and progress view to VISIBLE
This ensures that the views will never be visible simultaneously, preventing any overlap issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the dialog class that manages these views
ast-grep --pattern 'class $_ {
$$$
@+id/ll_dialog_progress
$$$
@+id/ll_dialog_error
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Search for Kotlin/Java files that reference these view IDs
rg -l "R.id.ll_dialog_progress|ll_dialog_progress|R.id.ll_dialog_error|ll_dialog_error" --type kotlin --type java
# Search for visibility handling patterns in those files
rg "visibility|View.VISIBLE|View.GONE|View.INVISIBLE" --type kotlin --type java -A 2 -B 2
Length of output: 37256
Script:
#!/bin/bash
# Search for the dialog class that manages the create hub device dialog
rg -l "dialog_create_hub_device" --type kotlin --type java
# Then search for visibility handling in those files
rg "ll_dialog_progress|ll_dialog_error" -A 5 -B 5 --type kotlin --type java
Length of output: 227
Script:
#!/bin/bash
# Look at the content of CreateHubDeviceDialog.kt to see how it handles these views
cat presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt
# Also check if it inherits from BaseProgressErrorDialog which we saw in earlier results
rg "class CreateHubDeviceDialog.*BaseProgressErrorDialog" --type kotlin
Length of output: 4576
data/src/main/java/org/cryptomator/data/repository/CloudRepositoryImpl.java (1)
114-117: Verify integration with existing unlock methods.
The implementation follows the same pattern as other unlock methods, which is good. However, we should verify that this new unlock path properly integrates with the existing vault management flow.
✅ Verification successful
Let me gather more specific information about the unlock implementation and its integration.
Based on the gathered information, I can now provide the final response:
The new unlock implementation is properly integrated with the existing vault management flow
The implementation follows the established pattern consistently:
- It correctly delegates to
cryptoCloudFactory.unlock()like other unlock methods - It uses the same
decryptedViewOf()pattern for the return value - The method is properly declared in the
CloudRepositoryinterface - Error handling follows the same
BackendExceptionpattern
The code shows proper integration with both the interface contract and the underlying CryptoCloudFactory implementation, maintaining consistency with existing unlock flows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components that might need updates for the new unlock flow
echo "Searching for related unlock implementations..."
rg "unlock.*vault.*config" --type java
echo "Checking for potential Hub-related error handling..."
rg "Hub.*Exception" --type java
echo "Verifying lock/unlock lifecycle handling..."
ast-grep --pattern 'class $_ implements CloudRepository {
$$$
unlock($$$) {
$$$
}
$$$
}'
Length of output: 7915
Script:
#!/bin/bash
# Check for all unlock method implementations and their usage
echo "Searching for unlock method implementations..."
ast-grep --pattern 'unlock($$$) {
$$$
}'
echo "Checking for unlock method calls..."
rg "unlock\(" --type java -A 2
echo "Checking for CryptoCloudFactory usage..."
rg "cryptoCloudFactory" --type java -A 2
Length of output: 15541
presentation/src/main/java/org/cryptomator/presentation/exception/ExceptionHandlers.kt (2)
15-19: LGTM! Well-organized imports
The new Hub-related exception imports follow the established package structure and maintain good separation of concerns.
82-86: Verify string resources and consider documentation
While the new Hub-related exception handlers follow the existing pattern, consider these improvements:
- Verify that all referenced string resources exist and contain appropriate error messages
- Consider adding documentation to explain specific scenarios these handlers address
- Review if the handler order affects exception handling precedence
Let's verify the string resources:
Consider adding KDoc comments to document these handlers:
// Example documentation:
/**
* Handles Hub-specific exceptions:
* - HubVaultOperationNotSupportedException: When an operation is not supported for Hub vaults
* - HubAuthenticationFailedException: When Hub authentication fails
* - HubDeviceAlreadyRegisteredForOtherUserException: When attempting to register a device already linked to another user
* - HubInvalidSetupCodeException: When the provided Hub setup code is invalid
* - HubInvalidVersionException: When there's a version mismatch between client and Hub
*/
private fun setupHandlers() {
// ... existing code ...
}✅ Verification successful
String resources verified and documentation suggestion is valid
All referenced string resources exist in presentation/src/main/res/values/strings.xml with appropriate error messages that match their corresponding exception handlers:
error_hub_vault_operation_not_supportederror_hub_authentication_failederror_hub_device_already_register_for_other_usererror_hub_invalid_setup_codeerror_hub_invalid_version
The suggestion to add KDoc comments would indeed improve code maintainability by documenting the specific scenarios these handlers address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the referenced string resources exist in the strings.xml files
# Search for the string resource definitions
echo "Searching for Hub-related string resources..."
rg -A 1 'name="error_hub_' 'presentation/src/main/res/values/strings.xml'
Length of output: 715
data/build.gradle (1)
199-200: Verify security of nimbus-jose-jwt dependency
The addition of joseJwt dependency is appropriate for Hub support implementation. However, let's verify the security of the specified version.
✅ Verification successful
The project uses nimbus-jose-jwt version 9.47, but there is a HIGH severity vulnerability (Denial of Service) affecting versions < 9.37.2 that was published on February 11, 2024. While the current version (9.47) is above the vulnerable range, it's important to note that this is a security-critical dependency used for JWT operations. The version appears to be up-to-date and not affected by any known vulnerabilities.
- ✅ Version 9.47 is above all vulnerable version ranges found in security advisories
- ✅ The dependency is consistently used across data and util modules
- ℹ️ The dependency is properly defined in the central dependencies.gradle file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and latest versions of nimbus-jose-jwt
# Check Maven Central for latest versions
curl -s "https://search.maven.org/solrsearch/select?q=g:com.nimbusds+AND+a:nimbus-jose-jwt&rows=5&wt=json" | jq '.response.docs[].v'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "com.nimbusds:nimbus-jose-jwt") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1597
Script:
#!/bin/bash
# Check the actual version specified in the build files
rg -A1 "joseJwt" --type gradle
Length of output: 514
presentation/build.gradle (2)
178-178: LGTM! AppAuth is a secure choice for OAuth.
The AppAuth library is a well-maintained, security-focused choice for implementing OAuth 2.0 flows. It follows OAuth 2.0 best practices and provides PKCE support by default.
Let's verify the AppAuth version for known vulnerabilities:
✅ Verification successful
Let me verify the specific version being used to ensure completeness.
AppAuth 0.11.1 is secure with no known vulnerabilities
The verification confirms:
- AppAuth version 0.11.1 is being used
- No security vulnerabilities were found in the GitHub Security Advisory Database for this package
- This is a recent stable version of the library
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check AppAuth version for security advisories
# Check for security advisories in AppAuth
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "net.openid:appauth") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 353
Script:
#!/bin/bash
# Check the AppAuth version specified in the build files
rg -g "*.gradle*" "appauth" -A 3 -B 3
Length of output: 1679
51-53: LGTM! Verify redirect URI configuration.
The manifest placeholder for OAuth redirect scheme is correctly configured. The scheme matches the application ID which follows security best practices.
Let's verify the redirect URI configuration is consistent across the app:
✅ Verification successful
LGTM! The OAuth redirect scheme configuration is properly implemented.
The verification confirms that:
- The manifest placeholder
appAuthRedirectSchemewith valueorg.cryptomator.androidmatches the Hub redirect URL defined inCryptoConstants.HUB_REDIRECT_URL - The redirect URL is properly used in the authorization flow in
UnlockVaultPresenter.kt - No conflicting redirect URI schemes are found in the codebase
- The manifest contains appropriate intent filters for handling OAuth redirects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded redirect URIs that might conflict
# with the configured scheme
# Search for redirect URI strings
rg -i "redirect.*uri|redirect.*url" --type xml
rg -i "org\.cryptomator\.android://"
Length of output: 87
Script:
#!/bin/bash
# Let's try a broader search for OAuth/redirect related configurations
# Search in all file types including Kotlin/Java source files
rg -i "redirect.*uri|redirect.*url|oauth" -A 3
# Also search for any AppAuth configuration code
ast-grep --pattern 'AuthorizationServiceConfiguration($$$)'
# Search for any AndroidManifest.xml files that might contain intent filters
fd "AndroidManifest.xml" --exec grep -A 5 -i "intent-filter"
Length of output: 33258
buildsystem/dependencies.gradle (3)
114-114: LGTM: Testing dependencies are up-to-date
The mockito-android version (5.14.2) is recent and aligns well with other testing dependencies in the project.
Also applies to: 182-182
148-148: Verify dependency compatibility with existing libraries
Let's verify that the new JWT and OAuth dependencies don't conflict with existing authentication libraries (e.g., jsonWebToken, msgraphAuth).
Also applies to: 172-172
✅ Verification successful
No dependency conflicts detected with AppAuth integration
After analyzing the dependencies in the Gradle files, I can confirm that there are no conflicts between the AppAuth library and the existing authentication libraries. The codebase uses:
- AppAuth (
net.openid:appauth:0.11.1) for OpenID Connect - MSAL (
com.microsoft.identity.client:msal:4.0.5) for Microsoft authentication - Auth0 JWT (
com.auth0:java-jwt:4.4.0) for JWT handling
These libraries serve different purposes and are commonly used together in Android applications:
- AppAuth handles OAuth/OpenID Connect flows
- MSAL is specific to Microsoft authentication
- Auth0 JWT is for JWT token handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential dependency conflicts
# Search for other JWT/OAuth related dependencies
rg -l "jwt|oauth|auth" --type gradle
# Check build.gradle files for potential conflicts
fd -e gradle | xargs cat | grep -E "implementation.*('|\").*:(jwt|oauth|auth)"
Length of output: 208
Script:
#!/bin/bash
# Let's examine the content of the identified Gradle files to check for authentication-related dependencies
echo "=== Content of buildsystem/dependencies.gradle ==="
cat buildsystem/dependencies.gradle
echo -e "\n=== Content of presentation/build.gradle ==="
cat presentation/build.gradle
echo -e "\n=== Content of presentation/prebuild.gradle ==="
cat presentation/prebuild.gradle
Length of output: 21760
67-70: Verify security implications of JWT and OAuth dependencies
The new dependencies are critical for Hub authentication. Let's verify their versions for known vulnerabilities and latest stable releases.
✅ Verification successful
Let me gather more information about the AppAuth dependency since the PyPI check wasn't successful (AppAuth is actually an Android library, not a Python package).
Dependencies are using latest stable versions and have no known vulnerabilities
The verification shows that both dependencies are using their latest stable versions:
- nimbus-jose-jwt 9.47 (latest on Maven Central)
- AppAuth-Android 0.11.1 (latest on Maven Central)
No security vulnerabilities were found in GitHub's security advisory database for either package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and latest versions
# Check PyPI for latest versions of appauth
curl -s https://pypi.org/pypi/appauth/json | jq '.info.version'
# Check Maven Central for latest versions of nimbus-jose-jwt
curl -s "https://search.maven.org/solrsearch/select?q=g:com.nimbusds+AND+a:nimbus-jose-jwt&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion'
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "nimbus-jose-jwt") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 575
Script:
#!/bin/bash
# Check Maven Central for latest versions of AppAuth-Android
curl -s "https://search.maven.org/solrsearch/select?q=g:net.openid+AND+a:appauth&rows=1&wt=json" | jq -r '.response.docs[0].latestVersion'
# Check for security advisories for AppAuth
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "appauth") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 490
presentation/src/main/res/values/strings.xml (1)
47-51: LGTM! Error messages are clear and comprehensive.
The error messages provide good coverage for Hub-related error scenarios and maintain consistency with the existing error message style.
data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoCloudFactory.java (4)
24-24: Import of HUB_SCHEME is appropriate for new hub functionality.
The addition of HUB_SCHEME import is necessary to support the new hub schemes.
44-44: Refactored method call improves clarity.
Using masterkeyCryptoCloudProvider().create(location, password); enhances readability by explicitly indicating the use of the master key provider.
72-74: Addition of new unlock method aligns with hub support.
The new unlock method provides the capability to unlock vaults using JWE keys, which is essential for the hub integration.
92-94: Introduction of masterkeyCryptoCloudProvider method enhances code reuse.
Creating a dedicated masterkeyCryptoCloudProvider() method promotes code reuse and improves encapsulation.
util/src/main/java/org/cryptomator/util/crypto/HubDeviceCryptor.java (1)
196-201: Verify consistent encoding for device IDs
The getDeviceId method encodes the hashed key using uppercase hexadecimal. Ensure consistency with other components that may expect device IDs in a specific format (e.g., lowercase).
Please confirm that the device ID format aligns with the expected format in other system components.
util/src/androidTest/java/org/cryptomator/util/crypto/HubDeviceCryptorTest.java (1)
181-188: Verify the correctness of key encryption and decryption
In the testEncryptAndDecryptUserKey method, ensure that the encryption and decryption processes are accurately validating the key material.
Run the following script to check for any discrepancies in the key encoding:
✅ Verification successful
Let me check the implementation of the encryption and decryption methods to ensure they're properly handling the key material.
Let me try a different approach to find the implementation using ripgrep.
Key encryption and decryption implementation is correctly verified
The test correctly validates the key material by:
- Using
assertArrayEqualsto compare the encoded bytes of the original private key with the decrypted key - The implementation in
HubDeviceCryptorshows proper encryption using ECDH-ES with A256GCM and secure key handling with zero-filling of sensitive data
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the encoded keys before and after encryption/decryption are identical.
# Extract encoded keys from the test method
fd 'HubDeviceCryptorTest.java' -x grep -A 10 'testEncryptAndDecryptUserKey' {}
# Ensure that the byte arrays of the private keys match
# Expected result: The byte arrays are identical, confirming successful encryption and decryption.
Length of output: 487
Script:
#!/bin/bash
# Check the implementation of HubDeviceCryptor
ast-grep --pattern 'encryptUserKey($$$) {
$$$
}'
# Also check the decryption implementation
ast-grep --pattern 'decryptUserKey($$$) {
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Search for HubDeviceCryptor class implementation
rg -l "class HubDeviceCryptor" --type java
# Once found, show the content of the file
fd "HubDeviceCryptor.java" -x cat {}
Length of output: 8338
domain/src/main/java/org/cryptomator/domain/exception/hub/HubVaultAccessForbiddenException.java
Show resolved
Hide resolved
domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidSetupCodeException.java
Show resolved
Hide resolved
...n/src/main/java/org/cryptomator/domain/exception/hub/HubLicenseUpgradeRequiredException.java
Show resolved
Hide resolved
...c/main/java/org/cryptomator/domain/exception/hub/HubVaultOperationNotSupportedException.java
Show resolved
Hide resolved
...va/org/cryptomator/domain/exception/hub/HubDeviceAlreadyRegisteredForOtherUserException.java
Show resolved
Hide resolved
util/src/androidTest/java/org/cryptomator/util/crypto/HubDeviceCryptorTest.java
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Outdated
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Copilot reviewed 33 out of 48 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- buildsystem/dependencies.gradle: Language not supported
- data/build.gradle: Language not supported
- data/src/main/java/org/cryptomator/data/cloud/crypto/CryptoConstants.kt: Language not supported
- data/src/main/java/org/cryptomator/data/cloud/crypto/HubkeyCryptoCloudProvider.kt: Language not supported
- data/src/main/java/org/cryptomator/data/cloud/crypto/MasterkeyCryptoCloudProvider.kt: Language not supported
- data/src/main/java/org/cryptomator/data/cloud/crypto/VaultConfig.kt: Language not supported
- domain/src/main/java/org/cryptomator/domain/UnverifiedHubVaultConfig.kt: Language not supported
- domain/src/main/java/org/cryptomator/domain/UnverifiedVaultConfig.kt: Language not supported
- domain/src/main/java/org/cryptomator/domain/exception/hub/HubAuthenticationFailedException.java: Evaluated as low risk
- domain/src/main/java/org/cryptomator/domain/exception/hub/HubDeviceAlreadyRegisteredForOtherUserException.java: Evaluated as low risk
- domain/src/main/java/org/cryptomator/domain/exception/hub/HubDeviceSetupRequiredException.java: Evaluated as low risk
- domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidSetupCodeException.java: Evaluated as low risk
- domain/src/main/java/org/cryptomator/domain/exception/hub/HubInvalidVersionException.java: Evaluated as low risk
- domain/src/main/java/org/cryptomator/domain/exception/hub/HubLicenseUpgradeRequiredException.java: Evaluated as low risk
- domain/src/main/java/org/cryptomator/domain/exception/hub/HubUserSetupRequiredException.java: Evaluated as low risk
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
domain/src/main/java/org/cryptomator/domain/exception/vaultconfig/VaultConfigLoadException.java (1)
11-13: Consider adding documentation about error handling strategy.Since this exception can now wrap any
Throwable, it would be helpful to document the types of errors that are expected to be wrapped.Add Javadoc to clarify the usage:
+/** + * Thrown when loading a vault configuration fails. + * This can happen due to various reasons including: + * - JWT verification failures + * - Hub authentication issues + * - File system access problems + * @param message The error message + * @param e The underlying cause + */ public VaultConfigLoadException(String message, Throwable e) { super(message, e); }presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt (3)
36-44: Extract button click handler logic into a separate methodThe click listener contains multiple operations and could benefit from being extracted into a dedicated method for better readability and maintainability.
createDeviceButton?.setOnClickListener { + handleCreateDeviceClick() +} + +private fun handleCreateDeviceClick() { val vaultModel = requireArguments().getSerializable(VAULT_ARG) as VaultModel val unverifiedVaultConfig = requireArguments().getSerializable(VAULT_CONFIG_ARG) as UnverifiedHubVaultConfig if (valid(binding.etDeviceName.text.toString(), binding.etSetupCode.text.toString())) { showProgress(ProgressModel(ProgressStateModel.CREATING_HUB_DEVICE)) callback?.onCreateHubDeviceClicked(vaultModel, unverifiedVaultConfig, binding.etDeviceName.text.toString(), binding.etSetupCode.text.toString()) onWaitForResponse(binding.etDeviceName) } -}
64-76: Consider adding more robust input validationThe current validation only checks for empty fields. Consider adding:
- Maximum length validation for device name
- Character validation (e.g., no special characters)
- Setup code format validation
private fun valid(name: String, setupCode: String): Boolean { return when { name.isEmpty() -> { showError(R.string.dialog_create_hub_device_name_empty) false } + name.length > MAX_DEVICE_NAME_LENGTH -> { + showError(R.string.dialog_create_hub_device_name_too_long) + false + } + !name.matches(DEVICE_NAME_PATTERN) -> { + showError(R.string.dialog_create_hub_device_name_invalid_chars) + false + } setupCode.isEmpty() -> { showError(R.string.dialog_create_hub_device_setup_code_empty) false } + !setupCode.matches(SETUP_CODE_PATTERN) -> { + showError(R.string.dialog_create_hub_device_setup_code_invalid) + false + } else -> true } }
85-86: Add documentation for empty methodAdd a comment explaining why this method is intentionally empty, as it's required by the superclass but not needed in this implementation.
override fun setupView() { + // Intentionally empty - required by BaseDialog but not needed in this implementation }🧰 Tools
🪛 detekt (1.23.7)
[warning] 85-86: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
data/src/main/java/org/cryptomator/data/cloud/crypto/VaultConfig.kt (1)
103-109: Consider adding URI scheme validation.While the method correctly validates URI syntax, it might be beneficial to add scheme validation for security purposes.
Consider this enhancement:
private fun parseUri(uriValue: Map<String, Any>, fieldName: String): URI { val uriString = uriValue[fieldName] as? String ?: throw VaultConfigLoadException("Missing or invalid '$fieldName' claim in JWT header") - return try { - URI.create(uriString) + val uri = try { + URI.create(uriString) + } catch (e: IllegalArgumentException) { + throw VaultConfigLoadException("Invalid '$fieldName' URI: ${e.message}", e) + } + + // Validate URI scheme + if (!uri.scheme.equals("https", ignoreCase = true)) { + throw VaultConfigLoadException("Invalid scheme for '$fieldName' URI: must be HTTPS") + } + return uri - } catch (e: IllegalArgumentException) { - throw VaultConfigLoadException("Invalid '$fieldName' URI: ${e.message}", e) - } }presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (2)
155-169: Consider using more specific progress states for Hub authentication.While the implementation correctly handles different vault actions, using
ProgressModel.GENERICcould be improved to provide more detailed feedback to users during the authentication process.Consider creating Hub-specific progress states:
-view?.showProgress(ProgressModel.GENERIC) +view?.showProgress(ProgressModel(ProgressStateModel.HUB_AUTHENTICATION))
234-266: Reduce code duplication in Hub authentication flows.The Hub device creation flow shares significant code with the unlock flow, particularly in handling authentication responses and token exchange.
Consider extracting the common authentication logic into a shared method:
private fun handleHubAuthResponse( result: ActivityResult, onTokenReceived: (String) -> Unit, onError: (Throwable) -> Unit ) { if (result.isResultOk) { val resp = AuthorizationResponse.fromIntent(result.intent()) if (resp != null) { hubAuthService?.performTokenRequest(resp.createTokenExchangeRequest()) { token, ex -> token?.accessToken?.let(onTokenReceived) ?: onError(HubAuthenticationFailedException(ex)) } } else { onError(HubAuthenticationFailedException( AuthorizationException.fromIntent(result.intent()) )) } } else { onError(HubAuthenticationFailedException()) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
data/src/main/java/org/cryptomator/data/cloud/crypto/VaultConfig.kt(2 hunks)data/src/main/java/org/cryptomator/data/repository/HubRepositoryImpl.java(1 hunks)domain/src/main/java/org/cryptomator/domain/UnverifiedHubVaultConfig.kt(1 hunks)domain/src/main/java/org/cryptomator/domain/exception/vaultconfig/VaultConfigLoadException.java(1 hunks)presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt(10 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt(1 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubUserSetupRequiredDialog.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- data/src/main/java/org/cryptomator/data/repository/HubRepositoryImpl.java
- domain/src/main/java/org/cryptomator/domain/UnverifiedHubVaultConfig.kt
- presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubUserSetupRequiredDialog.kt
🧰 Additional context used
📓 Learnings (2)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (2)
Learnt from: SailReal
PR: cryptomator/android#560
File: presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt:223-224
Timestamp: 2024-11-26T16:40:40.575Z
Learning: In `UnlockVaultPresenter.kt`, within the `hubAuthenticationUnlock` method, it's acceptable to pass `null` as the cause to `HubAuthenticationFailedException` when `ex` is `null`, as the exception class handles null causes appropriately.
Learnt from: SailReal
PR: cryptomator/android#560
File: presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt:159-161
Timestamp: 2024-11-26T16:39:17.044Z
Learning: In `UnlockVaultPresenter`, avoid eagerly initializing `hubAuthService` to prevent unnecessary instantiation and disposal when it's not used.
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt (1)
Learnt from: SailReal
PR: cryptomator/android#560
File: presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubLicenseUpgradeRequiredDialog.kt:34-35
Timestamp: 2024-11-26T16:16:40.691Z
Learning: When subclassing `BaseDialog`, we must implement the abstract method `setupView()`, even if the implementation is empty.
🪛 detekt (1.23.7)
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt
[warning] 85-86: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🔇 Additional comments (6)
domain/src/main/java/org/cryptomator/domain/exception/vaultconfig/VaultConfigLoadException.java (1)
11-13: LGTM! The generalization of the exception parameter makes sense.
The change from JWTVerificationException to Throwable provides more flexibility for Hub support while maintaining the exception handling pattern.
Let's verify the existing usage patterns to ensure this change doesn't break existing error handling:
✅ Verification successful
The change to use Throwable is appropriate and safe
Based on the code analysis:
- The
VaultConfigLoadExceptionis used primarily inVaultConfig.ktto wrap various error conditions during vault config loading and verification - The exception is caught in
ExceptionHandlers.ktwith a generic error message (R.string.error_vault_config_loading) - The change from
JWTVerificationExceptiontoThrowablealigns with actual usage, as the code wraps different types of exceptions:- IllegalArgumentException (for URI parsing)
- JWTVerificationException (for JWT verification)
- Other potential errors from Hub integration
The generalization doesn't impact existing error handling patterns since the exception is handled uniformly at the presentation layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing catch blocks and throws of VaultConfigLoadException
# to ensure the change doesn't break existing error handling patterns
# Search for catch blocks handling VaultConfigLoadException
echo "Checking catch blocks:"
rg -A 2 "catch.*VaultConfigLoadException"
# Search for places throwing VaultConfigLoadException
echo "Checking throw statements:"
rg "throw new VaultConfigLoadException"
Length of output: 203
Script:
#!/bin/bash
# Let's try a broader search to understand the usage context
# First, let's find all files that reference VaultConfigLoadException
echo "Files referencing VaultConfigLoadException:"
rg -l "VaultConfigLoadException"
# Let's also look for any code that might be constructing this exception
# using a more lenient pattern
echo -e "\nException construction patterns:"
rg -B 2 -A 2 "new VaultConfigLoadException"
# Let's also check the class hierarchy to understand the exception structure
echo -e "\nChecking the exception class definition and hierarchy:"
rg -B 3 -A 3 "class VaultConfigLoadException"
Length of output: 1547
Script:
#!/bin/bash
# Let's check the actual usage in the files we found
echo "Checking VaultConfig.kt implementation:"
rg -B 5 -A 5 "VaultConfigLoadException" data/src/main/java/org/cryptomator/data/cloud/crypto/VaultConfig.kt
echo -e "\nChecking exception handling in ExceptionHandlers.kt:"
rg -B 5 -A 5 "VaultConfigLoadException" presentation/src/main/java/org/cryptomator/presentation/exception/ExceptionHandlers.kt
# Let's also check if there were any JWT related exceptions before
echo -e "\nChecking for any JWT related code:"
rg -l "JWT|jwt"
Length of output: 6393
presentation/src/main/java/org/cryptomator/presentation/ui/dialog/CreateHubDeviceDialog.kt (2)
104-116: LGTM! Factory method follows best practices
The companion object implementation follows Android fragment creation best practices with proper argument bundling.
27-28: Verify Serializable implementation of parameter types
The callback methods use serializable parameters (VaultModel and UnverifiedHubVaultConfig). Ensure these classes properly implement Serializable to prevent runtime exceptions.
✅ Verification successful
Based on the results from our verification attempts, we can see that:
VaultModelproperly implementsSerializableUnverifiedHubVaultConfigappears to be a data class with no explicit serialization implementation
Let me try one final check to be thorough:
Serialization implementation is properly handled
Both parameters are properly handled for serialization:
VaultModeldirectly implementsSerializableUnverifiedHubVaultConfigextendsUnverifiedVaultConfigwhich implementsSerializable
The serialization concern is properly addressed in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if VaultModel and UnverifiedHubVaultConfig implement Serializable
ast-grep --pattern 'class $CLASS implements $$$Serializable$$$' \
&& echo "Checking class implementations..." \
&& rg "class (VaultModel|UnverifiedHubVaultConfig).*implements.*Serializable" -A 2
Length of output: 224
Script:
#!/bin/bash
# Let's try a different approach to check these classes and their inheritance
echo "Searching for VaultModel class definition..."
rg "class VaultModel" -A 5
echo -e "\nSearching for UnverifiedHubVaultConfig class definition..."
rg "class UnverifiedHubVaultConfig" -A 5
echo -e "\nSearching for Parcelable implementations (common in Android for serialization)..."
rg "class (VaultModel|UnverifiedHubVaultConfig).*:.*Parcelable" -A 5
echo -e "\nChecking imports for serialization-related classes..."
rg "^import.*\.(Serializable|Parcelable)" --type kotlin
Length of output: 3735
Script:
#!/bin/bash
# Let's check if UnverifiedHubVaultConfig implements Serializable through its parent class
echo "Checking UnverifiedHubVaultConfig parent class..."
rg "class UnverifiedHubVaultConfig.*extends|implements|:" -B 2
echo -e "\nChecking UnverifiedVaultConfig (potential parent) implementation..."
rg "interface UnverifiedVaultConfig|class UnverifiedVaultConfig" -A 2
Length of output: 145513
Script:
#!/bin/bash
# Let's check UnverifiedVaultConfig interface and its inheritance
echo "Checking UnverifiedVaultConfig interface..."
rg "interface UnverifiedVaultConfig" -A 5 --type kotlin
echo -e "\nChecking if UnverifiedHubVaultConfig implements UnverifiedVaultConfig..."
rg "class UnverifiedHubVaultConfig.*:.*UnverifiedVaultConfig" -A 5 --type kotlin
Length of output: 398
Script:
#!/bin/bash
# Check if UnverifiedHubVaultConfig extends any class or implements any interface
echo "Checking UnverifiedHubVaultConfig full definition..."
rg "class UnverifiedHubVaultConfig" -B 2 -A 10 --type kotlin
echo -e "\nChecking for any base UnverifiedVaultConfig class or interface..."
rg "UnverifiedVaultConfig" -B 2 -A 2 --type kotlin
Length of output: 38528
data/src/main/java/org/cryptomator/data/cloud/crypto/VaultConfig.kt (2)
10-10: LGTM!
The import is correctly placed and necessary for the new hub support functionality.
86-90: LGTM! Robust error handling for keyId parsing.
The added try-catch block with specific exception handling improves the robustness of the code.
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (1)
67-69: LGTM! Proper initialization and cleanup of Hub-related components.
The initialization of Hub-related components follows best practices:
- Lazy initialization of
hubAuthServiceto prevent unnecessary instantiation - Proper cleanup in
destroyed()method - Correct unsubscription of use cases
Also applies to: 80-80, 94-94, 605-607
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Outdated
Show resolved
Hide resolved
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (2)
155-169: Consider extracting the Hub scheme check into a constantThe scheme check could be made more maintainable by extracting it into a constant.
+ private companion object { + private const val HUB_SCHEME_PREFIX = "hub" + } - } else if (unverifiedVaultConfig.isPresent && unverifiedVaultConfig.get().keyId.scheme.startsWith(CryptoConstants.HUB_SCHEME)) { + } else if (unverifiedVaultConfig.isPresent && unverifiedVaultConfig.get().keyId.scheme.startsWith(HUB_SCHEME_PREFIX)) {
234-265: Consider using specific progress states for Hub operationsThe generic progress state could be made more specific for better user feedback.
+ private companion object { + private const val PROGRESS_HUB_DEVICE_CREATION = "HUB_DEVICE_CREATION" + } - view?.showProgress(ProgressModel.GENERIC) + view?.showProgress(ProgressModel(PROGRESS_HUB_DEVICE_CREATION))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt(10 hunks)presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubUserSetupRequiredDialog.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- presentation/src/main/java/org/cryptomator/presentation/ui/dialog/HubUserSetupRequiredDialog.kt
🧰 Additional context used
📓 Learnings (1)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (3)
Learnt from: SailReal
PR: cryptomator/android#560
File: presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt:177-191
Timestamp: 2024-11-26T18:05:58.758Z
Learning: The default implementation of `AuthorizationRequest.Builder` in the AppAuth library already generates and sets the state, nonce, and code verifier using `AuthorizationManagementUtil` and `CodeVerifierUtil`, so it's unnecessary to manually set them.
Learnt from: SailReal
PR: cryptomator/android#560
File: presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt:223-224
Timestamp: 2024-11-26T16:40:40.575Z
Learning: In `UnlockVaultPresenter.kt`, within the `hubAuthenticationUnlock` method, it's acceptable to pass `null` as the cause to `HubAuthenticationFailedException` when `ex` is `null`, as the exception class handles null causes appropriately.
Learnt from: SailReal
PR: cryptomator/android#560
File: presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt:159-161
Timestamp: 2024-11-26T16:39:17.044Z
Learning: In `UnlockVaultPresenter`, avoid eagerly initializing `hubAuthService` to prevent unnecessary instantiation and disposal when it's not used.
🔇 Additional comments (4)
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt (4)
3-13: LGTM: Clean integration of Hub-related dependencies
The new imports and constructor parameters are well-organized and properly integrated with the existing codebase.
Also applies to: 22-28, 68-69
80-80: LGTM: Proper lifecycle management of hubAuthService
The implementation correctly follows the lazy initialization pattern for hubAuthService and properly handles its disposal in the destroyed() method.
Also applies to: 94-94
177-232: LGTM: Robust Hub authentication implementation
The implementation correctly leverages AppAuth's built-in security features and handles various error scenarios appropriately.
605-607: LGTM: Proper resource cleanup
The new use cases are correctly added to the unsubscription list in the initialization block.
presentation/src/main/java/org/cryptomator/presentation/presenter/UnlockVaultPresenter.kt
Show resolved
Hide resolved
This adds support for Cryptomator Hub 🎉