Hub improvements #403
Hub improvements #403
Conversation
…and add SharePoint as CloudProvider
WalkthroughThe pull request introduces comprehensive support for SharePoint integration within the Cryptomator iOS application. The changes involve a significant refactoring of cloud provider authentication and management, transitioning from OneDrive-specific implementations to a more generalized Microsoft Graph approach. Key modifications include adding new view controllers and view models for SharePoint URL entry and drive selection, updating authentication mechanisms, and introducing new protocols and managers for handling Microsoft Graph drives. The project now supports SharePoint as a cloud storage provider, allowing users to create and open vaults using SharePoint sites and document libraries. The implementation includes URL validation, drive listing, and integration with existing vault creation and management workflows. Localization support has been added, with new strings for SharePoint-related user interactions. The changes also update various configuration files, asset catalogs, and authentication setup across different components of the application to accommodate the new SharePoint functionality. 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: 6
🔭 Outside diff range comments (1)
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderDBManager.swift (1)
Line range hint
64-105: Refactor provider creation methods to reduce cyclomatic complexityThe functions
createProviderandcreateBackgroundSessionProviderhave cyclomatic complexities of 11, exceeding the recommended maximum of 10. To improve maintainability and adhere to coding standards, consider refactoring these methods:
- Extract provider creation logic: Move the code within each
caseblock into separate helper methods. This will simplify the switch statements and reduce the complexity of the main methods.- Reduce code duplication: Since both methods share similar logic for provider creation, especially for cloud providers like SharePoint and OneDrive, consider creating a common method or utilizing a factory pattern to handle shared logic.
For example, you can refactor the
.sharePointcase as follows:// In createProvider case .sharePoint: let provider = try createSharePointProvider(for: accountUID) return provider // In createBackgroundSessionProvider case .sharePoint: let provider = try createSharePointProvider(for: accountUID, sessionIdentifier: sessionIdentifier) return provider // Helper method private func createSharePointProvider(for accountUID: String, sessionIdentifier: String? = nil) throws -> CloudProvider { let allDrives = try driveManager.getDrivesFromKeychain(for: accountUID) guard let drive = allDrives.first else { throw CloudProviderError.itemNotFound } let (credential, driveID) = try getSharePointCredentialAndDriveIdentifier(for: accountUID, driveID: drive.identifier) if let sessionIdentifier = sessionIdentifier { return try MicrosoftGraphCloudProvider.withBackgroundSession(credential: credential, driveIdentifier: driveID, maxPageSize: maxPageSizeForFileProvider, sessionIdentifier: sessionIdentifier) } else { return try MicrosoftGraphCloudProvider(credential: credential, driveIdentifier: driveID, maxPageSize: .max) } }This refactoring reduces complexity and makes the codebase easier to maintain.
Also applies to: 112-169
🧹 Nitpick comments (20)
Cryptomator/VaultDetail/VaultDetailInfoFooterViewModel.swift (1)
45-45: Consider refactoring to reduce cyclomatic complexity.Instead of disabling the SwiftLint warning, consider extracting the provider-specific username retrieval logic into separate methods to improve maintainability and testability.
Example refactor:
protocol CloudProviderUsernameRetriever { func getUsername(accountUID: String) -> String? } class OneDriveUsernameRetriever: CloudProviderUsernameRetriever { func getUsername(accountUID: String) -> String? { let credential = MicrosoftGraphCredential.createForOneDrive(with: accountUID) return try? credential.getUsername() } } // Similar implementations for other providers...Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swift (1)
68-68: Consider extracting the Hub vault check into a computed property.For better readability and reusability, consider extracting the Hub vault check into a computed property.
class UnlockSectionFooterViewModel: HeaderFooterViewModel { + private var isBiometricAuthenticationSupported: Bool { + return vaultInfo.vaultConfigType != .hub + } // ... existing code ... private static func getTitleText(...) -> String { // ... existing code ... - if vaultInfo.vaultConfigType != .hub, let biometryTypeName = biometryTypeName { + if isBiometricAuthenticationSupported, let biometryTypeName = biometryTypeName {CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift (1)
23-23: Well-structured error handling implementationThe implementation provides a complete error handling flow for archived vaults:
- Defines the state in
HubAuthenticationFlow- Maps HTTP 410 response to the appropriate state
- Propagates the state through the authentication flow
- Properly documents the status codes
While there is a cyclomatic complexity warning, it's justified given the nature of authentication flow handling and the number of different states that need to be managed.
Also applies to: 83-85, 248-249, 313-314
FileProviderExtensionUI/RootViewController.swift (1)
72-72: Update the error message to reflect Microsoft Graph.The error message still references OneDrive despite the transition to Microsoft Graph.
- DDLogError("Setting up OneDrive failed with error: \(error)") + DDLogError("Setting up Microsoft Graph failed with error: \(error)")CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationView.swift (1)
24-25: Great addition of archived vault handling with consistent UI!The implementation successfully addresses Issue #398 by providing clear error handling for archived vaults. The reuse of
CryptomatorErrorWithRefreshViewmaintains UI consistency.However, there's a minor formatting issue to fix:
- case .vaultArchived: - CryptomatorErrorWithRefreshView(headerTitle: LocalizedString.getValue("hubAuthentication.vaultArchived"), onRefresh: { Task { await viewModel.refresh() }}) + case .vaultArchived: + CryptomatorErrorWithRefreshView(headerTitle: LocalizedString.getValue("hubAuthentication.vaultArchived"), onRefresh: { Task { await viewModel.refresh() }})🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 24-24: Case statements should vertically aligned with their closing brace
(switch_case_alignment)
🪛 GitHub Actions: Build
[warning] 24-24: Switch and Case Statement Alignment Violation: Case statements should vertically aligned with their closing brace
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubErrorWithRefreshView.swift (3)
3-14: Add documentation to explain the view's purpose.The implementation looks good and follows SwiftUI best practices. Consider adding documentation to explain when and how to use this view, especially since it's part of the Hub error handling system.
Add a documentation comment:
+/// A view that displays an error message with a refresh button. +/// Used for handling Hub-related errors where retrying the operation might resolve the issue. struct CryptomatorErrorWithRefreshView: View {
5-5: Fix indentation inconsistency.The
onRefreshproperty uses tabs while other lines use spaces for indentation.- var onRefresh: () -> Void + var onRefresh: () -> Void
16-20: Enhance preview with meaningful example data.While the preview implementation is correct, consider using a more contextual example that reflects real error scenarios.
- CryptomatorErrorWithRefreshView(headerTitle: "Example Header Title", onRefresh: {}) + CryptomatorErrorWithRefreshView( + headerTitle: LocalizedString.getValue("hub.error.vaultArchived.title"), + onRefresh: {} + )CryptomatorCommon/Sources/CryptomatorCommonCore/MicrosoftGraph/MicrosoftGraphDriveManager.swift (1)
66-66: SanitizeaccountUIDWhen Constructing Keychain KeysUsing
accountUIDdirectly in keychain key names may lead to unexpected behavior ifaccountUIDcontains special characters. Consider sanitizing or encodingaccountUIDto ensure the keys are safe for use.Also applies to: 80-80
Cryptomator/AddVault/CreateNewVault/CreateNewVaultCoordinator.swift (2)
48-48: Handle Authentication Completion and ErrorsThe closure passed to
thenis empty, which means authentication results and errors are not being handled. Consider processing the authentication result or handling potential errors.Example:
authenticator.authenticate(cloudProviderType, from: viewController).then { result in + switch result { + case .success(let accountInfo): + // Handle successful authentication if necessary + break + case .failure(let error): + // Handle authentication error + handleError(error, for: viewController) + } }
90-90: ReplaceUsing
DDLogfor consistent and configurable logging.Apply this diff to replace the
guard let account = currentSharePointAccount else { - print("No current SharePoint account available") + DDLogError("No current SharePoint account available") return }Cryptomator/AddVault/CreateNewVault/URLValidator.swift (2)
12-23: Consider enhancing error handling with specific error cases.While the error handling is functional, consider adding more specific error cases to distinguish between different validation failures (invalid scheme, invalid host, missing path).
public enum URLValidatorError: Error, Equatable { - case invalidURLFormat + case invalidURLFormat + case invalidScheme + case invalidHost + case invalidPath } extension URLValidatorError: LocalizedError { public var errorDescription: String? { switch self { case .invalidURLFormat: return LocalizedString.getValue("addVault.enterSharePointURL.error.invalidURL") + case .invalidScheme: + return LocalizedString.getValue("addVault.enterSharePointURL.error.invalidScheme") + case .invalidHost: + return LocalizedString.getValue("addVault.enterSharePointURL.error.invalidHost") + case .invalidPath: + return LocalizedString.getValue("addVault.enterSharePointURL.error.invalidPath") } } }
25-42: Consider breaking down validation logic for better maintainability.The validation logic could be split into separate methods for better maintainability and reusability.
public enum URLValidator { + private static func validateScheme(_ url: URL) throws { + guard url.scheme == "https" else { + throw URLValidatorError.invalidScheme + } + } + + private static func validateHost(_ url: URL) throws { + guard let host = url.host, + host.contains(".sharepoint.com") else { + throw URLValidatorError.invalidHost + } + } + + private static func validatePath(_ url: URL) throws { + guard url.path.contains("/sites/") else { + throw URLValidatorError.invalidPath + } + } + public static func validateSharePointURL(urlString: String) throws { guard let url = URL(string: urlString) else { throw URLValidatorError.invalidURLFormat } - guard url.scheme == "https", - let host = url.host, - host.contains(".sharepoint.com") else { - throw URLValidatorError.invalidURLFormat - } - let path = url.path - guard path.contains("/sites/") else { - throw URLValidatorError.invalidURLFormat - } + try validateScheme(url) + try validateHost(url) + try validatePath(url) } }Cryptomator/AddVault/CreateNewVault/EnterSharePointURLViewController.swift (1)
30-39: Consider improving error handling and user feedback.The error handling could be enhanced to provide better user feedback:
- Consider using a dedicated error handling service instead of print statements
- Add error recovery suggestions in the UI
@objc func nextButtonClicked() { guard let coordinator = coordinator else { return } do { let url = try viewModel.getValidatedSharePointURL() coordinator.setSharePointURL(url) } catch { - print("Error validating SharePoint URL: \(error)") + ErrorLogger.shared.log(error, context: "SharePoint URL Validation") coordinator.handleError(error, for: self) } }Cryptomator/Common/CloudAuthenticator.swift (1)
Line range hint
73-104: Consider improving error handling in SharePoint setup.While the implementation is generally good, the error handling could be improved:
- Line 94: Replace
- Line 102: Replace
- Consider adding more specific error types for SharePoint-related failures
- print("No current SharePoint account available") + DDLogError("No current SharePoint account available") - print("Error creating provider: \(error)") + DDLogError("Error creating provider: \(error)")Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultCoordinator.swift (1)
73-104: Replace print statements with proper logging.The error handling uses print statements instead of the logging framework.
- print("No current SharePoint account available") + DDLogError("No current SharePoint account available") - print("Error creating provider: \(error)") + DDLogError("Error creating provider: \(error)")CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderDBManager.swift (1)
34-34: Consider setting 'driveManager' access level to 'private'The property
driveManageris currently declared without an explicit access level modifier, making itinternalby default. IfdriveManageris only used within theCloudProviderDBManagerclass, consider declaring it asprivateto encapsulate implementation details and enhance encapsulation.Cryptomator/AddVault/CreateNewVault/SharePointDriveListViewController.swift (2)
58-58: Consider using SharePoint-specific folder iconThe generic system folder icon might not provide the best user experience. Consider using a SharePoint-specific icon for better visual distinction.
- cell.imageView?.image = UIImage(systemName: "folder") + cell.imageView?.image = UIImage(named: "sharepoint-folder") ?? UIImage(systemName: "folder")
69-69: Add single trailing newlineAdd a single trailing newline at the end of the file to comply with Swift style guidelines.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 69-69: Files should have a single trailing newline
(trailing_newline)
🪛 GitHub Actions: Build
[warning] 69-69: Trailing Newline Violation: Files should have a single trailing newline
Cryptomator/AddVault/CreateNewVault/SharePointDriveListViewModel.swift (1)
46-49: Enhance URL path handling robustnessThe current path manipulation could be more robust. Consider using URL components or path components for safer manipulation.
- var serverRelativePath = urlComponents.path.trimmingCharacters(in: CharacterSet(charactersIn: "/")) - if serverRelativePath.hasPrefix("sites/") { - serverRelativePath = String(serverRelativePath.dropFirst("sites/".count)) - } + let pathComponents = urlComponents.path.components(separatedBy: "/").filter { !$0.isEmpty } + let serverRelativePath = pathComponents.dropFirst().joined(separator: "/")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
SharedResources/Assets.xcassets/sharepoint-selected.imageset/sharepoint-vault-selected.pngis excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint-selected.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint-selected.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint-vault.imageset/sharepoint-vault.pngis excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint-vault.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint-vault.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint.imageset/sharepoint.pngis excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint.imageset/[email protected]is excluded by!**/*.pngSharedResources/Assets.xcassets/sharepoint.imageset/[email protected]is excluded by!**/*.png
📒 Files selected for processing (37)
Cryptomator.xcodeproj/project.pbxproj(8 hunks)Cryptomator/AddVault/CreateNewVault/CreateNewVaultCoordinator.swift(2 hunks)Cryptomator/AddVault/CreateNewVault/EnterSharePointURLViewController.swift(1 hunks)Cryptomator/AddVault/CreateNewVault/EnterSharePointURLViewModel.swift(1 hunks)Cryptomator/AddVault/CreateNewVault/SharePointDriveListViewController.swift(1 hunks)Cryptomator/AddVault/CreateNewVault/SharePointDriveListViewModel.swift(1 hunks)Cryptomator/AddVault/CreateNewVault/SharePointURLSetting.swift(1 hunks)Cryptomator/AddVault/CreateNewVault/URLValidator.swift(1 hunks)Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultCoordinator.swift(3 hunks)Cryptomator/AppDelegate.swift(2 hunks)Cryptomator/Common/CloudAccountList/AccountListViewController.swift(1 hunks)Cryptomator/Common/CloudAccountList/AccountListViewModel.swift(4 hunks)Cryptomator/Common/CloudAuthenticator.swift(3 hunks)Cryptomator/Common/CloudProviderType+Localization.swift(1 hunks)Cryptomator/Common/UIImage+CloudProviderType.swift(2 hunks)Cryptomator/Settings/SettingsCoordinator.swift(1 hunks)Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swift(3 hunks)Cryptomator/VaultDetail/VaultDetailInfoFooterViewModel.swift(3 hunks)Cryptomator/VaultDetail/VaultDetailViewModel.swift(2 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorKeychain.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift(4 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationView.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationViewModel.swift(2 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubDeviceRegisteredSuccessfullyView.swift(0 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubErrorWithRefreshView.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderDBManager.swift(5 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderType.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/MicrosoftGraph/MicrosoftGraphDriveManager.swift(1 hunks)CryptomatorIntents/SaveFileIntentHandler.swift(1 hunks)FileProviderExtension/FileProviderExtension.swift(1 hunks)FileProviderExtensionUI/RootViewController.swift(1 hunks)README.md(1 hunks)SharedResources/Assets.xcassets/sharepoint-selected.imageset/Contents.json(1 hunks)SharedResources/Assets.xcassets/sharepoint-vault.imageset/Contents.json(1 hunks)SharedResources/Assets.xcassets/sharepoint.imageset/Contents.json(1 hunks)SharedResources/en.lproj/Localizable.strings(2 hunks)fastlane/scripts/create-cloud-access-secrets.sh(1 hunks)
💤 Files with no reviewable changes (1)
- CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubDeviceRegisteredSuccessfullyView.swift
✅ Files skipped from review due to trivial changes (3)
- SharedResources/Assets.xcassets/sharepoint.imageset/Contents.json
- SharedResources/Assets.xcassets/sharepoint-vault.imageset/Contents.json
- SharedResources/Assets.xcassets/sharepoint-selected.imageset/Contents.json
🧰 Additional context used
🪛 GitHub Actions: Build
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationViewModel.swift
[warning] 102-110: Switch and Case Statement Alignment Violation: Case statements should vertically aligned with their closing brace
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationView.swift
[warning] 24-24: Switch and Case Statement Alignment Violation: Case statements should vertically aligned with their closing brace
Cryptomator/AddVault/CreateNewVault/SharePointDriveListViewController.swift
[warning] 69-69: Trailing Newline Violation: Files should have a single trailing newline
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift
[warning] 52-52: Cyclomatic Complexity Violation: Function should have complexity 10 or less; currently complexity is 11
[warning] 72-81: Switch and Case Statement Alignment Violation: Case statements should vertically aligned with their closing brace
[warning] 248-248: Switch and Case Statement Alignment Violation: Case statements should vertically aligned with their closing brace
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderDBManager.swift
[warning] 64-64: Cyclomatic Complexity Violation: Function should have complexity 10 or less; currently complexity is 11
[warning] 121-121: Cyclomatic Complexity Violation: Function should have complexity 10 or less; currently complexity is 11
🪛 SwiftLint (0.57.0)
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationView.swift
[Warning] 24-24: Case statements should vertically aligned with their closing brace
(switch_case_alignment)
Cryptomator/AddVault/CreateNewVault/SharePointDriveListViewController.swift
[Warning] 69-69: Files should have a single trailing newline
(trailing_newline)
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift
[Warning] 248-248: Case statements should vertically aligned with their closing brace
(switch_case_alignment)
🔇 Additional comments (43)
Cryptomator/VaultDetail/VaultDetailInfoFooterViewModel.swift (1)
63-64: LGTM! Clean transition to Microsoft Graph credentials.The implementation correctly uses the Microsoft Graph API for OneDrive credential management.
Cryptomator/VaultDetail/UnlockSectionFooterViewModel.swift (1)
Line range hint
1-78: The implementation correctly removes biometric authentication options for Hub vaults.The changes align perfectly with Issue #396 from the PR objectives. The code now checks
vaultInfo.vaultConfigType != .hubbefore displaying biometric authentication options in the footer text, effectively removing Face ID and Touch ID options specifically for Hub vaults while maintaining the functionality for other vault types.CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationViewModel.swift (1)
35-35: LGTM! New state added for archived vaults.The addition of the
vaultArchivedcase aligns with the PR objective to handle HTTP 410 status for archived Hub vaults.CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/CryptomatorHubAuthenticator.swift (3)
23-23: LGTM! New enum case for archived vaultsThe addition of
vaultArchivedcase aligns with the PR objective to handle archived Hub vaults.
83-85: LGTM! Proper handling of archived vault stateThe implementation correctly propagates the archived vault state through the authentication flow.
313-314: LGTM! Well-documented enum caseThe
vaultArchivedcase is properly documented with its corresponding HTTP status code (410).CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorKeychain.swift (1)
Line range hint
1-180: Verify implementation of Hub-related changes.The PR objectives mention removing biometric authentication for Hub vaults, but no changes are made to the biometric authentication logic in this file. Since this file contains the keychain and biometric authentication implementation, it might need additional changes.
Let's verify if there are any Hub-related changes in other files:
FileProviderExtensionUI/RootViewController.swift (1)
67-70: Verify Microsoft Graph configuration parameters.The Microsoft Graph configuration setup looks correct, but please ensure:
- The
clientIdandredirectURIvalues inCloudAccessSecretsare properly configured- The keychain sharing group is correctly set to the main app bundle ID
✅ Verification successful
Microsoft Graph configuration is correctly implemented.
The configuration is consistent across all app components with proper keychain sharing and container identifier settings. The implementation follows iOS best practices for app extensions and authentication handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for Microsoft Graph configuration usage across the codebase rg -A 2 "microsoftGraphClientId|microsoftGraphRedirectURI" --type swiftLength of output: 2023
Script:
#!/bin/bash # Check for CloudAccessSecrets and CryptomatorConstants definitions rg -g '!*.{md,txt}' "CloudAccessSecrets|CryptomatorConstants" --type swift -A 3 # Look for capability-related configuration files fd -e entitlementsLength of output: 11683
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubAuthenticationView.swift (2)
23-23: Clean implementation of access not granted error handling!The implementation provides clear user feedback with a refresh option, following SwiftUI best practices.
Line range hint
1-1: Verify biometric authentication removalThe PR objectives mention removing biometric authentication options (Face ID/Touch ID) for Hub vaults, but this isn't visible in the current file.
Let's verify the complete removal of biometric authentication:
✅ Verification successful
Biometric authentication properly restricted for Hub vaults
The code already correctly implements the removal of biometric authentication for Hub vaults through an explicit check in
VaultDetailViewModel.swiftthat prevents biometric features from being enabled whenvaultConfigTypeis.hub.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining biometric authentication code related to Hub vaults rg -i "(?i)(faceid|touchid|biometric|LAContext)" --type swift # Search for any Hub-related authentication configuration ast-grep --pattern 'class $_ { $$$ .*(bio|face|touch).* $$$ }'Length of output: 28509
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 24-24: Case statements should vertically aligned with their closing brace
(switch_case_alignment)
🪛 GitHub Actions: Build
[warning] 24-24: Switch and Case Statement Alignment Violation: Case statements should vertically aligned with their closing brace
CryptomatorCommon/Sources/CryptomatorCommonCore/Hub/HubErrorWithRefreshView.swift (2)
1-2: LGTM!Clean import structure following SwiftUI conventions.
1-20: Verify integration with Hub error handling system.This view appears to be part of the Hub improvements mentioned in the PR objectives, particularly for handling archived vault errors (HTTP 410). Please ensure that this view is properly integrated with the error handling system and displays localized error messages consistently.
Let's verify the localization keys and error handling integration:
✅ Verification successful
Integration with Hub error handling system verified successfully.
The view is properly integrated with the Hub authentication flow, specifically handling archived vault (HTTP 410) and access denied errors. The refresh functionality and error messages are correctly localized across all supported languages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for localization keys related to Hub errors rg -g '*.strings' -g '*.stringsdict' "hub.error" -A 2 # Check for HTTP 410 error handling implementation rg "410" -A 5Length of output: 8140
Script:
#!/bin/bash # Check for usage of CryptomatorErrorWithRefreshView rg "CryptomatorErrorWithRefreshView" -l # Check for common and hub-related error message localizations rg -g '*.strings' -g '*.stringsdict' "common\.button\.refresh|common\.error|hub\." -A 2 # Check for Hub error view implementations ast-grep --pattern 'CryptomatorErrorWithRefreshView(headerTitle: $_, onRefresh: $_)'Length of output: 12691
Cryptomator/VaultDetail/VaultDetailViewModel.swift (2)
124-127: LGTM! Correctly implements removal of biometric authentication for Hub vaults.The implementation aligns with PR objective #396 by explicitly excluding biometric authentication options for Hub vaults while maintaining them for other vault types.
Let's verify that biometric authentication is consistently handled across the codebase:
✅ Verification successful
Verified: Biometric authentication is consistently excluded for Hub vaults
The implementation is consistent across the codebase. Both files that handle biometric authentication UI (
VaultDetailViewModel.swiftandUnlockSectionFooterViewModel.swift) properly exclude Hub vaults using the same condition pattern.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify biometric authentication handling for Hub vaults # Look for other potential biometric authentication checks that might need similar Hub vault exclusion # Search for biometric-related code rg -l "biometric|FaceID|TouchID|LAContext" | while read -r file; do echo "=== $file ===" rg "hub.*bio|bio.*hub" "$file" || true doneLength of output: 12998
149-149: Verify the usage ofvaultInfoin UnlockSectionFooterViewModel.The addition of
vaultInfoparameter suggests enhanced contextual information in the footer. Please ensure thatUnlockSectionFooterViewModelproperly utilizes this parameter to display relevant Hub vault status messages (e.g., archived vault errors).Let's verify the implementation:
✅ Verification successful
Verified:
vaultInfoparameter is properly utilized in UnlockSectionFooterViewModelThe implementation correctly uses the
vaultInfoparameter to differentiate between hub and non-hub vaults, ensuring appropriate display of biometric unlock options. This change aligns with the proper handling of hub vault features.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of vaultInfo in UnlockSectionFooterViewModel # Search for UnlockSectionFooterViewModel implementation ast-grep --pattern 'class UnlockSectionFooterViewModel { $$$ }' # Look for HTTP 410 handling rg -A 5 "410|archived.*vault|vault.*archived"Length of output: 8894
Script:
#!/bin/bash # Search for UnlockSectionFooterViewModel implementation and its usage of vaultInfo rg -U "class UnlockSectionFooterViewModel.*?\{.*?\}" -A 50 # Search for specific usage of vaultInfo within UnlockSectionFooterViewModel rg "vaultInfo\." -A 3 -B 3Length of output: 26763
Cryptomator/AddVault/CreateNewVault/SharePointURLSetting.swift (1)
12-14: Protocol Definition is AppropriateThe
SharePointURLSettingprotocol is correctly defined and follows Swift conventions.Cryptomator/Common/CloudProviderType+Localization.swift (1)
25-26: Changes appear unrelated to PR objectives.The addition of SharePoint localization seems unrelated to the stated PR objectives of Hub improvements (error handling, text updates, and biometric authentication removal).
Could you clarify if this PR's scope has changed from Hub improvements to SharePoint integration?
Cryptomator/AddVault/CreateNewVault/URLValidator.swift (1)
1-42: Changes appear unrelated to PR objectives.This new file implementing SharePoint URL validation seems unrelated to the stated PR objectives of Hub improvements.
Could you clarify if this PR's scope has changed from Hub improvements to SharePoint integration?
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderType.swift (1)
18-18: Changes appear unrelated to PR objectives.The addition of SharePoint as a cloud provider type seems unrelated to the stated PR objectives of Hub improvements.
Could you clarify if this PR's scope has changed from Hub improvements to SharePoint integration?
Cryptomator/AddVault/CreateNewVault/EnterSharePointURLViewController.swift (1)
1-44: Changes appear unrelated to PR objectives.This new file implementing SharePoint URL input seems unrelated to the stated PR objectives of Hub improvements.
Could you clarify if this PR's scope has changed from Hub improvements to SharePoint integration?
Cryptomator/AppDelegate.swift (1)
90-90: Verify URL scheme handling security.The URL scheme handling for Microsoft Graph authentication needs to be verified for proper source application validation.
Run the following script to check URL scheme declarations:
✅ Verification successful
URL scheme handling for Microsoft Graph authentication is properly secured
The implementation follows security best practices by:
- Using the official Microsoft Authentication Library (MSAL)
- Validating the source application
- Properly registering URL schemes in Info.plist
- Using secure configuration with MSALPublicClientApplicationConfig
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check URL scheme declarations in Info.plist files fd -e plist -x grep -l "CFBundleURLSchemes\|CFBundleURLTypes" {}Length of output: 93
Script:
#!/bin/bash # Check URL scheme configuration in Info.plist echo "=== Info.plist URL Configuration ===" cat Cryptomator/Info.plist echo -e "\n=== URL Handling Implementation ===" # Search for URL handling code rg -A 5 "func.*application.*openURLs|func.*application.*open.*url" --type swift echo -e "\n=== CloudAccessSecrets Implementation ===" # Look for CloudAccessSecrets definition rg -A 3 "CloudAccessSecrets" --type swiftLength of output: 9967
Cryptomator/Common/CloudAuthenticator.swift (3)
47-47: Verify OneDrive functionality after Microsoft Graph migration.The authentication method has been updated to use Microsoft Graph. While the implementation looks correct, ensure that existing OneDrive vaults remain accessible after this change.
54-60: LGTM! SharePoint authentication implementation.The implementation follows the established pattern for cloud provider authentication and properly handles credential management.
Line range hint
64-71: LGTM! Account selection logic.The code correctly handles both SharePoint and non-SharePoint account types, directing them to appropriate flows.
Cryptomator/Common/CloudAccountList/AccountListViewModel.swift (3)
91-91: LGTM! OneDrive credential update.The code correctly uses the new Microsoft Graph credential for OneDrive accounts.
101-103: LGTM! SharePoint account handling.The implementation follows the same pattern as OneDrive, ensuring consistency across Microsoft services.
127-130: LGTM! Microsoft Graph credential handling.The method properly handles Microsoft Graph credentials for both OneDrive and SharePoint.
Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultCoordinator.swift (5)
18-18: LGTM! SharePoint protocol conformance.The coordinator correctly implements the SharePoint URL setting protocol.
23-23: LGTM! SharePoint account state management.The state is properly managed using an optional property.
30-30: LGTM! Cloud provider list update.SharePoint has been correctly added to the list of supported cloud providers.
48-53: LGTM! SharePoint URL handling.The implementation properly handles SharePoint URL entry through a dedicated view controller.
64-71: LGTM! Account selection logic.The code correctly differentiates between SharePoint and other account types.
FileProviderExtension/FileProviderExtension.swift (1)
33-36: LGTM! Microsoft Graph configuration.The configuration properly sets up Microsoft Graph for both OneDrive and SharePoint support, including:
- Correct client ID and redirect URI
- Proper keychain sharing group configuration
- Appropriate setup for the shared container
SharedResources/en.lproj/Localizable.strings (2)
130-130: Improved clarity in authorization message.Changed from "device" to "user" authorization to align with Hub v1.3.0, making it clearer that the authorization is tied to the user rather than the device.
140-140: Good addition of archived vault message.Clear error message for HTTP 410 status code, informing users about archived vaults and providing guidance on next steps.
Cryptomator.xcodeproj/project.pbxproj (1)
436-441: LGTM! Well-structured SharePoint integration.The changes add a well-organized set of files for SharePoint integration following MVVM architecture:
- View/ViewModel pairs for SharePoint URL entry and drive list
- Supporting files for URL validation and settings
- Integration with cloud-access-swift package
Also applies to: 1049-1055
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderDBManager.swift (6)
38-40: Constructor update enhances dependency injection and testabilityAdding
driveManageras a parameter in the initializer with a default value improves the class's flexibility and testability by allowing dependency injection. This makes it easier to inject mock implementations during unit tests.
84-85: Transition to Microsoft Graph APIs for OneDrive is appropriateThe switch to using
MicrosoftGraphCredential.createForOneDriveandMicrosoftGraphCloudProviderfor the.oneDrivecase aligns with the migration to Microsoft Graph APIs. This change ensures consistency and leverages the benefits of the unified API.
93-99: Addition of SharePoint support is well-implementedThe new
.sharePointcase increateProvidercorrectly retrieves the drives from the keychain, handles the absence of drives gracefully by throwingCloudProviderError.itemNotFound, and initializes the provider with the appropriate credential and drive identifier.
142-143: Consistent update to Microsoft Graph APIs in background sessionsThe
.oneDrivecase increateBackgroundSessionProvideris updated to useMicrosoftGraphCredentialandMicrosoftGraphCloudProvider.withBackgroundSession, maintaining consistency with the main provider creation method and the overall API migration.
151-157: SharePoint background session support is correctly integratedThe addition of the
.sharePointcase increateBackgroundSessionProvidermirrors the main provider creation, ensuring that background sessions for SharePoint are properly handled with the correct credentials and session identifiers.
173-179: New helper method improves code organization and readabilityThe
getSharePointCredentialAndDriveIdentifiermethod encapsulates the logic for obtaining the SharePoint credential and drive identifier. This reduces duplication and enhances code readability.Cryptomator/Common/UIImage+CloudProviderType.swift (1)
31-32: LGTM! SharePoint icon handling follows existing patternsThe implementation consistently handles SharePoint icons in both
vaultIconForandstorageIconFormethods, maintaining the established pattern used for other cloud providers.Also applies to: 68-69
Cryptomator/AddVault/CreateNewVault/EnterSharePointURLViewModel.swift (1)
13-72: LGTM! Well-structured implementation with proper error handlingThe implementation demonstrates good practices:
- Clear protocol definition with proper separation of concerns
- Comprehensive error handling with localized messages
- Proper memory management with Combine subscribers
| case .vaultArchived: | ||
| await setState(to: .vaultArchived) | ||
| } |
There was a problem hiding this comment.
Fix case statement alignment.
The case statement needs to be aligned with other cases in the switch block to resolve the pipeline warning.
Apply this diff to fix the alignment:
- case .vaultArchived:
- await setState(to: .vaultArchived)
- }
+ case .vaultArchived:
+ await setState(to: .vaultArchived)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case .vaultArchived: | |
| await setState(to: .vaultArchived) | |
| } | |
| case .vaultArchived: | |
| await setState(to: .vaultArchived) | |
| } |
| case 410: | ||
| return .vaultArchived |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix switch case alignment
The switch cases need to be aligned with their closing braces to comply with the style guidelines and resolve pipeline failures.
Apply this formatting fix:
- case .vaultArchived:
- return .vaultArchived
+ case .vaultArchived:
+ return .vaultArchived
- case 410:
- return .vaultArchived
+ case 410:
+ return .vaultArchivedAlso applies to: 83-85
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 248-248: Case statements should vertically aligned with their closing brace
(switch_case_alignment)
🪛 GitHub Actions: Build
[warning] 248-248: Switch and Case Statement Alignment Violation: Case statements should vertically aligned with their closing brace
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle JSON Decoding Errors Appropriately
Returning nil in the catch block can mask JSON decoding errors. Consider throwing the error instead to aid in debugging issues with corrupted data.
Apply this diff to handle decoding errors:
} catch {
- return nil
+ throw error
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return nil | |
| } | |
| throw error | |
| } |
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault"; | ||
| "hubAuthentication.requireAccountInit.alert.title" = "Action Required"; |
There was a problem hiding this comment.
Simplified authorization message needs completion.
The authorization message appears to be incomplete. The message "To access the vault" lacks context and doesn't provide clear guidance to the user.
Consider expanding the message to provide clear guidance, for example:
-"hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault";
+"hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault, your user needs to be authorized by the vault owner.";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault"; | |
| "hubAuthentication.requireAccountInit.alert.title" = "Action Required"; | |
| "hubAuthentication.deviceRegistration.needsAuthorization.alert.message" = "To access the vault, your user needs to be authorized by the vault owner."; | |
| "hubAuthentication.requireAccountInit.alert.title" = "Action Required"; |
| guard let cell = tableView.dequeueReusableCell(withIdentifier: "SharePointDriveCell", for: indexPath) as? CloudCell else { | ||
| fatalError("Could not dequeue CloudCell") | ||
| } |
There was a problem hiding this comment.
Replace fatalError with proper error handling
Using fatalError for cell dequeue failure is not suitable for production code as it could crash the app. Consider handling this gracefully.
- guard let cell = tableView.dequeueReusableCell(withIdentifier: "SharePointDriveCell", for: indexPath) as? CloudCell else {
- fatalError("Could not dequeue CloudCell")
- }
+ guard let cell = tableView.dequeueReusableCell(withIdentifier: "SharePointDriveCell", for: indexPath) as? CloudCell else {
+ return UITableViewCell() // Fallback to basic cell
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guard let cell = tableView.dequeueReusableCell(withIdentifier: "SharePointDriveCell", for: indexPath) as? CloudCell else { | |
| fatalError("Could not dequeue CloudCell") | |
| } | |
| guard let cell = tableView.dequeueReusableCell(withIdentifier: "SharePointDriveCell", for: indexPath) as? CloudCell else { | |
| return UITableViewCell() // Fallback to basic cell | |
| } |
| print("Invalid SharePoint URL") | ||
| return | ||
| } |
There was a problem hiding this comment.
Improve error handling with user feedback
Currently, errors are only logged to console. Consider propagating these errors to the UI layer to provide meaningful feedback to users.
private func fetchSiteAndDrives() {
guard let urlComponents = URL(string: sharePointURL),
let hostName = urlComponents.host else {
- print("Invalid SharePoint URL")
+ handleError(EnterSharePointURLViewModelError.invalidURL)
return
}
// ...
}
+private func handleError(_ error: Error) {
+ // Notify delegate or use callback to show error in UI
+ errorOccurred?(error)
+}Also applies to: 55-56, 63-64
|
This PR will be very hard to review because the feature branch didn't start from develop but from a different feature branch that hasn't been merged yet. Can you please fix that so that the PR is easier to review and can be merged without relying on the other feature branch? Thank you! |
This PR fixes the following issues:
#398 : Show a clear error message for archived Hub vaults (HTTP 410).
#397 : Update texts to align with Hub 1.3.0 (user keys).
#396 : Remove biometric authentication options for Hub vaults.
Changes
• Added HTTP 410 error handling for archived vaults.
• Updated localized strings for user authorization texts.
• Removed Face ID/Touch ID options for Hub vaults.