Add SharePoint support #402
Conversation
…and add SharePoint as CloudProvider
[ci skip]
…tGraphAccountMigration
WalkthroughThis pull request transitions the project’s cloud integration from OneDrive to Microsoft Graph. It introduces new classes, protocols, view controllers, and view models to support Microsoft Graph and SharePoint authentication flows, including URL validation, drive listing, and error localization. The changes update authentication coordinators, modify delegation in folder creation and vault management, and enhance account handling by replacing OneDrive-specific logic with Microsoft Graph–oriented implementations. Additionally, dependency versions and package configurations have been updated. Database migration functions and keychain services are revised to accommodate Microsoft Graph account storage, while related test cases and localization files have been adjusted accordingly. Overall, the modifications refactor configuration, error handling, and UI elements to reflect the new Microsoft Graph integration across the application. Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 4
🧹 Nitpick comments (33)
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderType.swift (1)
13-22: Add documentation for each enum caseConsider adding documentation comments for each case in the enum to explicitly document their purpose, especially for the new
microsoftGraphcase. This would help developers understand the available options and the context in which they should be used.public enum CloudProviderType: Codable, Equatable, Hashable, DatabaseValueConvertible { + /// Box cloud storage provider case box + /// Dropbox cloud storage provider case dropbox + /// Google Drive cloud storage provider case googleDrive + /// Local file system storage case localFileSystem(type: LocalFileSystemType) + /// Microsoft Graph API (supports OneDrive and SharePoint) case microsoftGraph(type: MicrosoftGraphType) + /// pCloud storage provider case pCloud + /// Amazon S3 compatible storage case s3(type: S3Type) + /// WebDAV storage case webDAV(type: WebDAVType) }Cryptomator/MicrosoftGraph/MicrosoftGraphAuthenticatorError+Localization.swift (2)
1-12: Consider updating the copyright yearThe copyright year is set to 2025, which appears to be incorrect as the current year is 2024.
-// Copyright © 2025 Skymatic GmbH. All rights reserved. +// Copyright © 2024 Skymatic GmbH. All rights reserved.
13-22: Add documentation comments for clarityThe extension implements
LocalizedErrorprotocol correctly, but adding documentation comments would improve code maintainability.+/// Extension to provide localized error descriptions for Microsoft Graph authentication errors extension MicrosoftGraphAuthenticatorError: LocalizedError { + /// Returns a localized description for the error public var errorDescription: String? { switch self { case .missingAccountIdentifier: return nil case .serverDeclinedScopes: return LocalizedString.getValue("microsoftGraphAuthenticator.error.serverDeclinedScopes") } } }Cryptomator/MicrosoftGraph/SharePointAuthenticating.swift (1)
12-16: Add documentation to the protocol and methodsFor better code maintainability and understanding, consider adding documentation comments to the protocol and its methods.
+/// Protocol defining the authentication flow for SharePoint protocol SharePointAuthenticating: AnyObject { + /// Sets the SharePoint site URL and presents the authentication flow + /// - Parameters: + /// - siteURL: The URL of the SharePoint site + /// - viewController: The view controller from which to present the authentication UI func setSiteURL(_ siteURL: URL, from viewController: UIViewController) + /// Called when authentication is successful with the provided credential + /// - Parameter credential: The SharePoint credential from a successful authentication + /// - Throws: Error if the authentication cannot be completed func authenticated(_ credential: SharePointCredential) throws + /// Cancels the ongoing authentication process func cancel() }FileProviderExtensionUI/RootViewController.swift (1)
73-73: Update error log message to reference Microsoft GraphThe error message still refers to OneDrive even though the code now uses Microsoft Graph.
- DDLogError("Setting up OneDrive failed with error: \(error)") + DDLogError("Setting up Microsoft Graph failed with error: \(error)")Cryptomator/MicrosoftGraph/SharePointDriveListViewModel.swift (2)
13-50: Well-structured view model with clear responsibilitiesThe implementation follows good MVVM patterns with proper separation of concerns. The view model correctly manages the SharePoint drive listing functionality using promises for asynchronous operations and provides appropriate callbacks for changes and errors.
Consider adding a method to stop listening for changes to prevent potential memory leaks:
+ func stopListenForChanges() { + changeListener = nil + errorListener = nil + }Additionally, you might want to enhance error handling with more specific error types to provide better feedback to users:
private func fetchDrives(for siteIdentifier: String) { discovery.fetchSharePointDrives(for: siteIdentifier).then { drives in self.drives = drives self.changeListener?() }.catch { error in + // Convert common errors to more specific types + let convertedError: Error + switch error { + case CloudProviderError.unauthorized: + convertedError = NSLocalizedString("sharePoint.error.unauthorized", comment: "") + case CloudProviderError.itemNotFound: + convertedError = NSLocalizedString("sharePoint.error.siteNotFound", comment: "") + default: + convertedError = error + } - self.errorListener?(error) + self.errorListener?(convertedError) } }
5-5: Creation date appears to be in the futureThe file header shows a creation date of "03.12.24", which appears to be in the future if using MM.DD.YY format. Consider updating to the correct date or using a format that avoids confusion.
Cryptomator/MicrosoftGraph/SharePointCredential.swift (1)
5-5: Creation date appears to be in the futureThe file header shows a creation date of "12.03.25", which appears to be in the future. Consider updating to the correct date or using a format that avoids confusion.
SharedResources/Assets.xcassets/sharepoint-selected.imageset/Contents.json (1)
1-23: Consider aligning asset folder name with image filenames.The asset folder is named "sharepoint-selected" but the actual image files are named "sharepoint-vault-selected". This slight naming inconsistency might cause confusion during future maintenance.
Cryptomator/Common/ChooseFolder/ChooseFolderViewModel.swift (2)
14-14: Protocol constraint update requires class conformanceBy making
ChooseFolderViewModelProtocolinherit fromAnyObject, all conforming types must now be classes, not structures or enums. This is a breaking change for any existing struct implementations of this protocol.Consider adding a comment explaining why reference semantics are required for this protocol.
22-22: New protocol method requires implementation in all conforming typesAdding the
addItemmethod to the protocol requires all conforming types to implement this method, which could potentially break existing implementations.Consider providing a default implementation for this method in a protocol extension to maintain backward compatibility.
Cryptomator/MicrosoftGraph/SharePointAuthenticationCoordinator.swift (1)
33-42: Enhance error handling for better user experienceWhile the authentication process is well-implemented, the error handling could be improved to provide more specific feedback to users.
Consider categorizing errors and providing more user-friendly messages for common authentication issues (e.g., network errors, invalid credentials) instead of using a generic error handler.
func setSiteURL(_ siteURL: URL, from viewController: UIViewController) { MicrosoftGraphAuthenticator.authenticate(from: viewController, for: .sharePoint).then { credential in self.showDriveList(credential: credential, siteURL: siteURL) }.catch { error in guard case CocoaError.userCancelled = error else { - self.handleError(error, for: self.navigationController) + switch error { + case let networkError as URLError where networkError.code == .notConnectedToInternet: + self.handleError(AuthenticationError.noInternetConnection, for: self.navigationController) + case let authError as MSALError: + self.handleError(AuthenticationError.fromMSAL(authError), for: self.navigationController) + default: + self.handleError(error, for: self.navigationController) + } return } } }Cryptomator/Common/ChooseFolder/ChooseFolderViewController.swift (1)
209-209: Empty implementation in mock classThe mock implementation of
addItemis empty, which may not accurately simulate the behavior of a real implementation that would update the items array and trigger UI updates.Consider implementing this method in the mock to better reflect the actual behavior:
-func addItem(_ item: CloudItemMetadata) {} +func addItem(_ item: CloudItemMetadata) { + // In a real implementation, this would add the item and notify listeners + // For preview purposes, we could log or perform some visual feedback + print("Item would be added: \(item.name)") +}Cryptomator/MicrosoftGraph/SharePointDriveListViewController.swift (1)
5-5: Fix creation dateThe file header shows a creation date of December 3, 2024, but we're currently in March 2025.
-// Created by Majid Achhoud on 03.12.24. +// Created by Majid Achhoud on 03.03.25.Cryptomator/MicrosoftGraph/OneDriveAuthenticator.swift (1)
17-34:❓ Verification inconclusive
Verify account linking logic for OneDrive authentication
The code properly handles credential management with good error handling. However, consider adding a comment explaining why you're first saving the CloudProviderAccount before the MicrosoftGraphAccount, as this sequential dependency is critical.
Additionally, the catch block for SQLITE_CONSTRAINT errors properly handles the case where a Microsoft Graph account with the same credential already exists, but it might be helpful to add more specific logging to aid in debugging.
public static func authenticate(from viewController: UIViewController, cloudProviderAccountManager: CloudProviderAccountManager, microsoftGraphAccountManager: MicrosoftGraphAccountManager) -> Promise<CloudProviderAccount> { return MicrosoftGraphAuthenticator.authenticate(from: viewController, for: .oneDrive).then { credential -> CloudProviderAccount in let accountUID = UUID().uuidString let cloudProviderAccount = CloudProviderAccount(accountUID: accountUID, cloudProviderType: .microsoftGraph(type: .oneDrive)) - try cloudProviderAccountManager.saveNewAccount(cloudProviderAccount) // Make sure to save this first, because Microsoft Graph account has a reference to the Cloud Provider account. + // Save CloudProviderAccount first because MicrosoftGraphAccount references it via accountUID + try cloudProviderAccountManager.saveNewAccount(cloudProviderAccount) do { let microsoftGraphAccount = MicrosoftGraphAccount(accountUID: accountUID, credentialID: credential.identifier, type: .oneDrive) try microsoftGraphAccountManager.saveNewAccount(microsoftGraphAccount) return cloudProviderAccount } catch let dbError as DatabaseError where dbError.resultCode == .SQLITE_CONSTRAINT { + // This credential is already linked to another account, retrieve that one instead try cloudProviderAccountManager.removeAccount(with: accountUID) let existingMicrosoftGraphAccount = try microsoftGraphAccountManager.getAccount(credentialID: credential.identifier, driveID: nil, type: .oneDrive) return try cloudProviderAccountManager.getAccount(for: existingMicrosoftGraphAccount.accountUID) } catch { throw error } } }Run the following script to check if there are other similar patterns in the codebase for handling duplicate credentials:
🏁 Script executed:
#!/bin/bash # Check for similar error handling patterns in other authenticator files rg -A 10 -B 2 "catch.*DatabaseError.*SQLITE_CONSTRAINT" --type swiftLength of output: 4155
Clarify Dependency and Enhance Debug Logging in OneDrive Authentication
The OneDrive authentication flow currently manages account linking and duplicate credential handling correctly, but it would benefit from improved inline documentation and logging. Please consider the following:
Document Sequential Dependency:
Add an inline comment explaining that the CloudProviderAccount is saved first because the MicrosoftGraphAccount later references it via the accountUID. This clarification is critical to understanding the need for the save ordering.Augment Error Logging:
In the catch block handlingSQLITE_CONSTRAINTerrors, include more detailed logging (for example, logging the specific error context or credential identifier). This will make debugging issues involving duplicate credentials easier.Ensure Consistency:
Similar error handling patterns exist in other authenticators (such as in SharePointAuthenticator), so ensure that any improvements here are consistent across the codebase.Proposed Diff Snippet Update:
public static func authenticate(from viewController: UIViewController, cloudProviderAccountManager: CloudProviderAccountManager, microsoftGraphAccountManager: MicrosoftGraphAccountManager) -> Promise<CloudProviderAccount> { return MicrosoftGraphAuthenticator.authenticate(from: viewController, for: .oneDrive).then { credential -> CloudProviderAccount in let accountUID = UUID().uuidString let cloudProviderAccount = CloudProviderAccount(accountUID: accountUID, cloudProviderType: .microsoftGraph(type: .oneDrive)) - try cloudProviderAccountManager.saveNewAccount(cloudProviderAccount) // Make sure to save this first, because Microsoft Graph account has a reference to the Cloud Provider account. + // Save CloudProviderAccount first because MicrosoftGraphAccount references it via accountUID + try cloudProviderAccountManager.saveNewAccount(cloudProviderAccount) do { let microsoftGraphAccount = MicrosoftGraphAccount(accountUID: accountUID, credentialID: credential.identifier, type: .oneDrive) try microsoftGraphAccountManager.saveNewAccount(microsoftGraphAccount) return cloudProviderAccount } catch let dbError as DatabaseError where dbError.resultCode == .SQLITE_CONSTRAINT { + // Existing MicrosoftGraphAccount already linked; log detailed information here for debugging duplicate credential issues try cloudProviderAccountManager.removeAccount(with: accountUID) let existingMicrosoftGraphAccount = try microsoftGraphAccountManager.getAccount(credentialID: credential.identifier, driveID: nil, type: .oneDrive) return try cloudProviderAccountManager.getAccount(for: existingMicrosoftGraphAccount.accountUID) } catch { throw error } } }CryptomatorTests/AddLocalVault/AddLocalVaultViewModelTestCase.swift (1)
59-61: Consider returning a default mock account or indicating it is intentionally unimplemented.
Currently, this method always throwsMockError.notMocked, which may lead to untested paths or unexpected failures in tests relying on a successful retrieval of an account. You could return a defaultCloudProviderAccountif your test scenario requires it, or raise visibility around the intentional “not implemented” status.Cryptomator/AppDelegate.swift (1)
44-47: Update error message to match Microsoft Graph changes.
Even though this block correctly initializes Microsoft Graph via MSAL, the error message at line 49 still references OneDrive. Consider updating it to avoid confusion:- DDLogError("Setting up OneDrive failed with error: \(error)") + DDLogError("Setting up Microsoft Graph failed with error: \(error)")Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultCoordinator.swift (1)
216-216: Empty method implementation may need follow-up or removal.
The newly introducedshowCreateNewFolderfunction is empty. If folder creation is meant to be supported here, consider implementing the logic or removing the method if it’s unused.Cryptomator/MicrosoftGraph/SharePointAuthenticator.swift (2)
14-16: Static property risks potential reference cycles.
Storing the coordinator in a static property can lead to strong reference cycles if the coordinator, in turn, references this class. Consider holding it locally within theauthenticatemethod or using a weak reference pattern if feasible.
17-42: Consider using a database transaction for account creation to avoid partial states.
If an unexpected error arises after saving theCloudProviderAccountbut before saving theMicrosoftGraphAccount, you risk leaving partial data in the database. A single transaction or more centralized error handling could help maintain data integrity:public static func authenticate(...) -> Promise<CloudProviderAccount> { ... return sharePointCoordinator.pendingAuthentication.then { credential -> CloudProviderAccount in + // Possible approach: + try DatabaseQueue.shared.inTransaction { db in + try cloudProviderAccountManager.saveNewAccount(...) + try microsoftGraphAccountManager.saveNewAccount(...) + return .commit + } } ... }CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderAccountDBManager.swift (1)
48-56: Consider using consistent error types.The implementation now throws
MicrosoftGraphAccountError.accountNotFoundErrorinstead of the more genericCloudProviderAccountError.accountNotFoundError. This could create inconsistencies if other cloud providers still use the generic error type.guard let account = account else { - throw MicrosoftGraphAccountError.accountNotFoundError + throw CloudProviderAccountError.accountNotFoundError }Cryptomator/MicrosoftGraph/EnterSharePointURLViewModel.swift (2)
14-16: Ensure robust error feedback for invalid URLs.The
SharePointURLValidationError.invalidURLerror case is too generic. Providing a more descriptive message or additional context (e.g., a localized error string indicating why the URL is invalid) could improve the user’s understanding, especially in UI alerts.
31-31: Consider dynamic default URL.Hardcoding
"https://"might not always align with custom SharePoint domain requirements. If your application needs to support various patterns (e.g., intranet subdomains), consider making this default configurable via user preferences or environment settings.Cryptomator/MicrosoftGraph/EnterSharePointURLViewController.swift (3)
14-16: Inject a coordinator fallback or guard.The coordinator reference is weak, but code paths (e.g.
nextButtonClicked) assume it’s present. Consider a fallback or an early exit strategy to handle nil coordinator scenarios gracefully (e.g., immediate dismiss or alert).
23-32: Prevent repeated subscription.Subscribing to
lastReturnButtonPressedinviewDidLoadis fine, but ensure no multiple event subscriptions occur if the view might reload. If the class remains in memory across transitions, re-subscribing can trigger duplicated events. A quick fix would be to set up the subscription ininitor guard if a subscription already exists.
34-42: Handle invalid URL scenario gracefully.When
viewModel.getValidatedSharePointURL()fails, the controller callscoordinator.handleError(...). Consider offering the user more guidance in the UI about what makes a valid URL, rather than simply showing a generic error. This can be done via a dedicated error label or an alert with a localized explanation.Cryptomator/AddVault/CreateNewVault/CreateNewVaultCoordinator.swift (1)
50-51: Refine user cancellation handling.The
guard case CocoaError.userCancelled = error else { ... }logic is appropriate. Consider logging the cancellation event for better analytics or user support, particularly if cancellations frequently occur at this authentication step.Cryptomator/Common/CloudAuthenticator.swift (1)
30-50: Comprehensive authentication switch.Covering all cases, including
.microsoftGraph(type: .oneDrive)and.microsoftGraph(type: .sharePoint), keeps the code symmetrical. If you anticipate new variants (e.g., specialized SharePoint site types), consider an extensible approach or plugin model for future expansions.Cryptomator/Common/CloudAccountList/AccountListViewModel.swift (2)
132-144: Handle URL schemes more robustly.
Removing only"https://"may miss other schemes (e.g.,"http://","ftp://", etc.). Consider using URL parsing (e.g., extracting the host) for an even cleaner display label.
215-242: Consider refactoring large methods.
startListenForChanges()is lengthy, which can impact readability and maintainability. Consider splitting responding actions into smaller dedicated methods.CryptomatorCommon/Sources/CryptomatorCommonCore/MicrosoftGraph/MicrosoftGraphAccountDBManager.swift (1)
58-96: Evaluate concurrency and duplication checks.
- If multiple threads write simultaneously, consider concurrency safeguards or transaction logic.
- When saving new accounts via
saveNewAccount(_:), confirm whether you want to overwrite duplicates or block them.CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorDatabase.swift (2)
213-239: Consider adding documentation for the migration functionSimilar to how
s3DisplayNameMigrationhas documentation explaining its purpose, consider adding documentation formicrosoftGraphAccountMigrationto clarify what it does and why. This will help future maintainers understand the migration process, especially since it's transforming OneDrive accounts to Microsoft Graph accounts.Example documentation:
/** Migrates the database to support Microsoft Graph accounts. This migration: 1. Creates a new table for storing Microsoft Graph account details 2. Converts existing OneDrive accounts to Microsoft Graph accounts 3. Updates related tables to reference the new account UIDs */
227-238: Consider adding error logging during migrationThe migration process performs several critical operations (updates across multiple tables, insertions) but lacks error logging. If a failure occurs during these operations, it might be difficult to diagnose the issue.
for row in rows { let oldAccountUID: String = row["accountUID"] // which is the `credentialID` let newAccountUID = UUID().uuidString let newCloudProviderType = CloudProviderType.microsoftGraph(type: .oneDrive).databaseValue + DDLogInfo("Migrating OneDrive account \(oldAccountUID) to Microsoft Graph account \(newAccountUID)") try db.execute(sql: "UPDATE cloudProviderAccounts SET accountUID = ?, cloudProviderType = ? WHERE accountUID = ?", arguments: [newAccountUID, newCloudProviderType, oldAccountUID]) try db.execute(sql: "UPDATE vaultAccounts SET delegateAccountUID = ? WHERE delegateAccountUID = ?", arguments: [newAccountUID, oldAccountUID]) try db.execute(sql: "UPDATE accountListPosition SET accountUID = ?, cloudProviderType = ? WHERE accountUID = ?", arguments: [newAccountUID, newCloudProviderType, oldAccountUID]) let newMicrosoftGraphType = MicrosoftGraphType.oneDrive.databaseValue try db.execute(sql: "INSERT INTO microsoftGraphAccounts (accountUID, credentialID, driveID, siteURL, type) VALUES (?, ?, NULL, NULL, ?)", arguments: [newAccountUID, oldAccountUID, newMicrosoftGraphType]) }
📜 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 (54)
Cryptomator.xcodeproj/project.pbxproj(21 hunks)Cryptomator.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved(8 hunks)Cryptomator/AddVault/CreateNewVault/CreateNewVaultChooseFolderViewController.swift(1 hunks)Cryptomator/AddVault/CreateNewVault/CreateNewVaultCoordinator.swift(5 hunks)Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultChooseFolderViewController.swift(1 hunks)Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultCoordinator.swift(3 hunks)Cryptomator/AppDelegate.swift(2 hunks)Cryptomator/Common/ChooseFolder/ChooseFolderViewController.swift(2 hunks)Cryptomator/Common/ChooseFolder/ChooseFolderViewModel.swift(2 hunks)Cryptomator/Common/ChooseFolder/FolderChoosing.swift(1 hunks)Cryptomator/Common/CloudAccountList/AccountListViewController.swift(1 hunks)Cryptomator/Common/CloudAccountList/AccountListViewModel.swift(5 hunks)Cryptomator/Common/CloudAuthenticator.swift(4 hunks)Cryptomator/Common/CloudProviderType+Localization.swift(3 hunks)Cryptomator/Common/UIImage+CloudProviderType.swift(5 hunks)Cryptomator/MicrosoftGraph/EnterSharePointURLViewController.swift(1 hunks)Cryptomator/MicrosoftGraph/EnterSharePointURLViewModel.swift(1 hunks)Cryptomator/MicrosoftGraph/MicrosoftGraphAuthenticatorError+Localization.swift(1 hunks)Cryptomator/MicrosoftGraph/OneDriveAuthenticator.swift(1 hunks)Cryptomator/MicrosoftGraph/SharePointAuthenticating.swift(1 hunks)Cryptomator/MicrosoftGraph/SharePointAuthenticationCoordinator.swift(1 hunks)Cryptomator/MicrosoftGraph/SharePointAuthenticator.swift(1 hunks)Cryptomator/MicrosoftGraph/SharePointCredential.swift(1 hunks)Cryptomator/MicrosoftGraph/SharePointDriveListViewController.swift(1 hunks)Cryptomator/MicrosoftGraph/SharePointDriveListViewModel.swift(1 hunks)Cryptomator/MicrosoftGraph/SharePointURLValidationError+Localization.swift(1 hunks)Cryptomator/S3/S3AuthenticationViewModel.swift(3 hunks)Cryptomator/Settings/SettingsCoordinator.swift(3 hunks)Cryptomator/VaultDetail/MoveVault/MoveVaultCoordinator.swift(1 hunks)Cryptomator/VaultDetail/VaultDetailInfoFooterViewModel.swift(2 hunks)Cryptomator/VaultDetail/VaultDetailUnlockCoordinator.swift(1 hunks)Cryptomator/VaultDetail/VaultDetailViewController.swift(1 hunks)Cryptomator/WebDAV/WebDAVAuthenticationCoordinator.swift(1 hunks)Cryptomator/WebDAV/WebDAVAuthenticationViewModel.swift(0 hunks)CryptomatorCommon/Package.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorDatabase.swift(3 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorKeychain.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderAccountDBManager.swift(2 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderDBManager.swift(4 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderType.swift(1 hunks)CryptomatorCommon/Sources/CryptomatorCommonCore/MicrosoftGraph/MicrosoftGraphAccountDBManager.swift(1 hunks)CryptomatorCommon/Tests/CryptomatorCommonCoreTests/Manager/CloudProviderAccountManagerTests.swift(1 hunks)CryptomatorIntents/Common/VaultOptionsProvider.swift(1 hunks)CryptomatorIntents/SaveFileIntentHandler.swift(2 hunks)CryptomatorTests/AddLocalVault/AddLocalVaultViewModelTestCase.swift(1 hunks)CryptomatorTests/S3AuthenticationViewModelTests.swift(2 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)
- Cryptomator/WebDAV/WebDAVAuthenticationViewModel.swift
🔇 Additional comments (102)
CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorKeychain.swift (1)
28-28: LGTM! Well-structured addition for Microsoft Graph support.This addition of the
microsoftGraphkeychain follows the established pattern of other cloud services in the codebase and aligns perfectly with the PR objective of adding SharePoint support via Microsoft Graph.CryptomatorIntents/Common/VaultOptionsProvider.swift (1)
34-35: Appropriate refactoring of cloud provider access methodThe change from directly fetching the cloud provider type to retrieving the entire account object is consistent with the broader shift in the codebase toward an account-based approach. This refactoring provides better support for Microsoft Graph and SharePoint integration, and offers more flexibility if additional account properties are needed in the future.
The error handling remains consistent, maintaining the original try-catch pattern for handling potential errors during account retrieval.
Also applies to: 38-38
Cryptomator/WebDAV/WebDAVAuthenticationCoordinator.swift (2)
48-48: Good standardization to use system-defined error type.Replacing the custom
WebDAVAuthenticationError.userCanceledwith the standardCocoaError(.userCancelled)improves consistency with iOS system standards and aligns error handling across different authentication methods in the app.
56-56: Appropriate error standardization for user dismissal.Using
CocoaError(.userCancelled)here maintains consistency with the error handling in thecancel()method above, providing a unified approach to handling user-initiated cancellations throughout the authentication flow.FileProviderExtension/FileProviderExtension.swift (1)
33-36: LGTM! Microsoft Graph integration looks properly implemented.The Microsoft Graph configuration setup follows the same pattern as other cloud services in this file. The MSAL client application is correctly initialized with the appropriate client ID, redirect URI, and keychain sharing group.
Cryptomator/Common/CloudProviderType+Localization.swift (3)
9-9: Import added to support Microsoft Graph integration.The import of
CryptomatorCloudAccessCoreprovides access to theMicrosoftGraphTypeenum that's used in the extension below. This aligns with the PR objective of adding SharePoint support through Microsoft Graph.
22-25: Clean implementation of Microsoft Graph localization.Good improvements:
- Parameter renaming from
localFileSystemTypetotypeimproves code consistency- The new
.microsoftGraph(type)case follows the same pattern as local file system, delegating to the type's own localization methodThe implementation properly supports both OneDrive and SharePoint as part of the Microsoft Graph integration.
47-56: Well-structured extension for Microsoft Graph types.This extension properly handles localization for both OneDrive and SharePoint, providing the appropriate display names. The implementation is clean and follows the same pattern as the other provider type extensions.
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderType.swift (2)
9-9: New import for cloud access functionalityThe addition of the
CryptomatorCloudAccessCoreimport suggests this module contains implementations related to the new Microsoft Graph integration. This is a good modular approach to separating cloud access logic.
18-18:✅ Verification successful
Strategic transition from OneDrive to Microsoft Graph
This change replaces the previous
oneDrivecase with the more versatilemicrosoftGraph(type: MicrosoftGraphType), which aligns with the PR objective to add SharePoint support. The Microsoft Graph API provides a unified interface for both OneDrive and SharePoint services.
🏁 Script executed:
#!/bin/bash # Let's check the definition of MicrosoftGraphType enum rg -A 10 "enum MicrosoftGraphType" --type swiftLength of output: 49
I'll run another verification to search for any occurrence of
MicrosoftGraphTypeacross the repository since the previous search didn't yield any results. This will help us confirm that the new case references a valid type. Below is the additional shell script:Once you review the output of this search, please verify manually that
MicrosoftGraphTypeis indeed defined and imported as expected. If the type isn’t found, we may need to double-check if it's defined in a different module or file.Rewritten Review Comment:
Attention: Verify MicrosoftGraphType Definition
The change replacing the previous
oneDrivecase withmicrosoftGraph(type: MicrosoftGraphType)aligns with the PR objective to support SharePoint via the Microsoft Graph API. This update provides a unified interface for OneDrive and SharePoint. However, please verify thatMicrosoftGraphTypeis defined in the project and is imported correctly where needed.
- Confirm that
MicrosoftGraphTypeis declared (e.g., as an enum) in the repository.- Ensure that any necessary updates or imports for
MicrosoftGraphTypeare present.
🏁 Script executed:
#!/bin/bash # Search for any occurrence of 'MicrosoftGraphType' in the repository's Swift files rg "MicrosoftGraphType" --type swiftLength of output: 1961
Microsoft Graph API Integration Confirmed
The updated
microsoftGraph(type: MicrosoftGraphType)case aligns with the PR objective of unifying OneDrive and SharePoint support through the Microsoft Graph API. Our repository search confirms thatMicrosoftGraphTypeis defined and utilized consistently across multiple modules (e.g., inMicrosoftGraphAccountDBManager.swift,UIImage+CloudProviderType.swift, etc.). No additional modifications or imports are necessary.
- Verified usage of
MicrosoftGraphTypein multiple Swift files.- Change meets the strategic objective for the API consolidation.
README.md (1)
26-27: Updated environment variables to support Microsoft GraphThe environment variables have been correctly updated from OneDrive to Microsoft Graph to support the SharePoint integration. This change aligns with the PR objective of adding SharePoint support.
FileProviderExtensionUI/RootViewController.swift (1)
68-71: Microsoft Graph configuration correctly implementedThe Microsoft Graph configuration has been properly set up to replace the previous OneDrive configuration.
Cryptomator/S3/S3AuthenticationViewModel.swift (1)
55-55: Consistent error type renaming and implementationThe error type has been renamed from
S3AuthenticationViewModelErrortoS3AuthenticationErrorthroughout the file, maintaining the same functionality while providing a cleaner naming convention that focuses on the domain rather than the implementation component.Also applies to: 59-59, 81-81, 96-100, 102-113
fastlane/scripts/create-cloud-access-secrets.sh (1)
30-32: LGTM: Proper transition from OneDrive to Microsoft Graph secretsThe changes appropriately replace OneDrive-specific secrets with Microsoft Graph equivalents, following the same pattern used for other cloud providers. This aligns with the PR's objective of adding SharePoint support through Microsoft Graph integration.
Cryptomator/MicrosoftGraph/SharePointCredential.swift (1)
12-16: Clean and well-defined credential structureThe
SharePointCredentialstruct is appropriately designed with the necessary properties for SharePoint authentication and drive access.SharedResources/Assets.xcassets/sharepoint-vault.imageset/Contents.json (1)
1-23: Asset added to support SharePoint integration.The addition of this SharePoint vault icon asset at multiple resolutions (1x, 2x, 3x) follows the standard iOS asset catalog format and supports the new SharePoint functionality being introduced.
SharedResources/Assets.xcassets/sharepoint.imageset/Contents.json (1)
1-23: Properly configured SharePoint icon asset.The SharePoint icon asset is correctly defined with appropriate scaling variants (1x, 2x, 3x) and follows iOS asset catalog conventions. This will support the new SharePoint integration UI.
Cryptomator/VaultDetail/VaultDetailViewController.swift (1)
126-126: Error handling updated to use standard system error.Good change replacing the custom
VaultDetailUnlockError.userCanceledwith the standardCocoaError.userCancelled. This simplifies error handling by leveraging system-provided error types instead of maintaining custom ones.Cryptomator/MicrosoftGraph/SharePointURLValidationError+Localization.swift (1)
13-22:✅ Verification successful
Implementation looks good!
The extension properly conforms
SharePointURLValidationErrortoLocalizedErrorand provides localized error messages for both error cases. This approach matches best practices for error localization in Swift applications.Ensure the localization keys exist in your localization files:
🏁 Script executed:
#!/bin/bash # Check if the localization keys exist in the string files rg -l "sharePoint.urlValidation.error.emptyURL" --type-add 'strings:*.strings' rg -l "sharePoint.urlValidation.error.invalidURL" --type-add 'strings:*.strings'Length of output: 399
Approval Confirmed: Localized Strings Exist
The localization keys "sharePoint.urlValidation.error.emptyURL" and "sharePoint.urlValidation.error.invalidURL" were found in the SharedResources/en.lproj/Localizable.strings file, and the implementation in Cryptomator/MicrosoftGraph/SharePointURLValidationError+Localization.swift conforms to the expected best practices. No further changes are necessary.
Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultChooseFolderViewController.swift (1)
106-106: Protocol conformance addition looks good.This empty implementation properly maintains conformance with the updated
ChooseFolderViewModelProtocolwhich now requires anaddItemmethod.Cryptomator/Common/CloudAccountList/AccountListViewController.swift (1)
145-145: Good addition of Microsoft Graph provider types.The change correctly updates the
supportsEditingmethod to include the new Microsoft Graph provider types (OneDrive and SharePoint), maintaining consistent behavior by returningfalsefor these types, just like the other cloud providers.Cryptomator/VaultDetail/VaultDetailUnlockCoordinator.swift (2)
47-47: Good use of standard system error.Replacing the custom error with
CocoaError(.userCancelled)improves code maintainability by using standard system errors for user cancellation.
55-55: Good use of standard system error.Consistent use of
CocoaError(.userCancelled)here as well. This change ensures uniform error handling for user cancellation throughout the application.Cryptomator/AddVault/CreateNewVault/CreateNewVaultChooseFolderViewController.swift (1)
76-76: New protocol method implementation added for Microsoft Graph supportThis implementation adds the newly required
addItemmethod to fulfill theChooseFolderViewModelProtocol. While the mock implementation is empty (which is appropriate for testing), this change properly supports the SharePoint integration work described in the PR.CryptomatorCommon/Package.swift (1)
29-29:❓ Verification inconclusive
Major version update for cloud-access-swift package
Updating from 1.12.0 to 2.0.0 is a significant change that likely introduces breaking changes according to semantic versioning. This update is necessary for the Microsoft Graph and SharePoint integration work.
🏁 Script executed:
#!/bin/bash # Verify if there are any areas in the codebase that might not be compatible with the new package version # Look for any imports of OneDrive specific classes that might no longer exist rg "import.*OneDrive" --type swiftLength of output: 36
Review Update: Verify Cloud-Access-Swift Upgrade Compatibility
The update to cloud-access-swift from 1.12.0 to 2.0.0 is a significant change as it indicates a major version update. A search for OneDrive-specific imports using the command did not return any results; however, given the potential for breaking changes in a major version bump, please ensure to manually verify all related integration points (especially those tied to Microsoft Graph and SharePoint) to confirm compatibility.
- Confirm that no other parts of the codebase depend on APIs that might have been altered or removed in v2.0.0.
- Manually review usage of cloud-access-swift functionalities to ensure that no hidden dependencies (other than OneDrive) are affected.
Cryptomator/VaultDetail/VaultDetailInfoFooterViewModel.swift (1)
62-67: OneDrive replaced with Microsoft Graph implementationThe code correctly handles Microsoft Graph account retrieval and username extraction, maintaining the same pattern as other cloud providers while transitioning from OneDrive to Microsoft Graph integration.
CryptomatorTests/S3AuthenticationViewModelTests.swift (2)
108-109: Error type renamed from S3AuthenticationViewModelError to S3AuthenticationErrorTest cases have been properly updated to use the new error type name, maintaining consistency with the renamed error enumeration.
123-124: Error type renamed from S3AuthenticationViewModelError to S3AuthenticationErrorTest cases have been properly updated to use the new error type name, maintaining consistency with the renamed error enumeration.
CryptomatorIntents/SaveFileIntentHandler.swift (2)
134-135: Appropriate replacement of OneDrive with Microsoft GraphThe code correctly updates the
CloudProviderTypeenum to use.microsoftGraph(type)instead of.oneDrive, maintaining the same pattern as other cloud provider types by delegating asset name resolution to the type-specific extension.
157-166: Well-implemented MicrosoftGraphType extensionThe new extension for
MicrosoftGraphTypefollows the same design pattern as the existingLocalFileSystemTypeextension and correctly handles both OneDrive and SharePoint cases, providing appropriate asset names for each.Cryptomator/Common/ChooseFolder/FolderChoosing.swift (1)
15-15:✅ Verification successful
Protocol method signature changed - verify all implementations
The
showCreateNewFoldermethod in theFolderChoosingprotocol has been modified to include an additional parameterdelegate: ChooseFolderViewModelProtocol?. While the parameter is optional (which helps with backward compatibility), this is a breaking change that requires updating all implementing classes.Run the following script to verify all implementations of the
FolderChoosingprotocol have been updated:
🏁 Script executed:
#!/bin/bash # Find all implementations of the FolderChoosing protocol and check if they've been updated echo "Looking for implementations of the FolderChoosing protocol..." rg -p "class\s+\w+.*?:\s*(?:\w+,\s*)*FolderChoosing(?:,\s*\w+)*" -g "*.swift" echo -e "\nChecking implementations of showCreateNewFolder method..." rg -p "func\s+showCreateNewFolder\s*\(\s*parentPath\s*:" -g "*.swift" echo -e "\nVerifying delegate parameter is included in all implementations..." rg -p "func\s+showCreateNewFolder\s*\(\s*parentPath\s*:.*?delegate\s*:" -g "*.swift"Length of output: 2294
Protocol method signature update verified across implementations
All implementations of the
FolderChoosingprotocol'sshowCreateNewFoldermethod have been successfully updated to include the optionaldelegate: ChooseFolderViewModelProtocol?parameter. The following files have been confirmed:
Cryptomator/VaultDetail/MoveVault/MoveVaultCoordinator.swift(line 66)Cryptomator/AddVault/CreateNewVault/CreateNewVaultCoordinator.swift(line 143)Cryptomator/Common/ChooseFolder/FolderChoosing.swift(line 15)Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultCoordinator.swift(line 216)No further modifications are required.
CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorDatabase.swift (6)
95-97: Migration properly registeredThe new migration for Microsoft Graph accounts is correctly registered with the database migrator with a descriptive name.
213-239:✅ Verification successful
Well-structured Microsoft Graph account migration
The migration function is comprehensive and handles several important aspects:
- Creates the new
microsoftGraphAccountstable with appropriate columns and constraints- Adds a unique index with a WHERE clause for handling null driveIDs
- Migrates existing OneDrive accounts to Microsoft Graph format
- Updates related records in other tables to maintain referential integrity
There's a good use of parameter binding in SQL statements to prevent SQL injection.
The migration performs updates to several related tables. Let's verify the completeness of the migration by checking if there are any other tables that might reference OneDrive accounts:
🏁 Script executed:
#!/bin/bash # Check if there are other tables that might reference OneDrive accounts # that are not updated in the migration echo "Looking for references to cloudProviderAccounts in database schema..." rg "references\(\"cloudProviderAccounts\"" -g "*.swift" echo -e "\nChecking for other potential references to accountUID in SQL statements..." rg "accountUID.*cloudProviderAccounts" -g "*.swift" | grep -v "microsoftGraphAccountMigration"Length of output: 1631
Migration Verified: No Additional OneDrive References Found
The migration code in
microsoftGraphAccountMigrationis robust and comprehensive:
- It creates the new
microsoftGraphAccountstable with the correct columns, constraints, and a unique index for rows with a nulldriveID.- It migrates existing OneDrive accounts by updating records in
cloudProviderAccounts,vaultAccounts, andaccountListPositionappropriately.- A repository-wide search confirms that the only references to
cloudProviderAccountsare within this file, with no additional tables or SQL statements referencing OneDrive accounts missed by the migration.- Use of parameter binding in SQL statements is correctly preventing injection attacks.
214-221: The table creation and constraints look goodThe table definition for
microsoftGraphAccountsincludes appropriate columns for Microsoft Graph account attributes and properly references thecloudProviderAccountstable with a cascade delete constraint. The unique constraints are also correctly defined.
222-226: Good use of conditional unique constraint for null driveIDThe unique index creation for
credentialIDandtypewhendriveIDis null is a good approach. This ensures uniqueness where needed while allowing multiple records with the same credential for different drives.
95-97:❓ Verification inconclusive
Verify migration versioning and order
The migration is properly registered with the migrator. However, ensure that this migration is executed in the correct order relative to other migrations and that all existing installations will properly migrate.
Run this script to verify the migration ordering:
🏁 Script executed:
#!/bin/bash # Check migration versioning across the codebase echo "Checking migrations in CryptomatorDatabase.swift" grep -A 20 "static var migrator: DatabaseMigrator" $(find . -name "CryptomatorDatabase.swift") echo "Checking for version references elsewhere in the codebase" rg "microsoftGraphAccountMigration" --type swiftLength of output: 1459
Migration Order Confirmed – Verify Runtime Behavior on Existing Installs
Static analysis confirms the registration order in
CryptomatorCommon/Sources/CryptomatorCommonCore/CryptomatorDatabase.swiftis as follows:
"v1""v2""s3DisplayNameMigration""initialHubSupport""microsoftGraphAccountMigration"This ordering shows that the Microsoft Graph Account migration is correctly placed after the prior migrations. No stray version references or alternative definitions were detected elsewhere in the codebase.
Please confirm via integration testing that this ordering ensures a smooth migration path for existing installations.
227-238:❓ Verification inconclusive
Verify data consistency after migration
The migration converts OneDrive accounts to Microsoft Graph accounts and updates multiple tables. It's important to verify that all references are properly updated and that no data inconsistencies occur.
Consider adding a consistency check at the end of the migration to verify that all references have been properly updated.
🌐 Web query:
What are common pitfalls when migrating data between cloud providers?💡 Result:
Migrating data between cloud providers introduces unique challenges that can undermine the process if not addressed. Key pitfalls include security risks, unexpected costs, and technical complexities, but proactive strategies can mitigate these issues effectively.
1. Security and Compliance Risks
Data exposure during transfer and misconfigured access controls are critical concerns. Migrating sensitive data across cloud environments increases vulnerability to breaches, especially if encryption or identity management is mishandled[1][2][7]. Compliance with regulations like GDPR or HIPAA becomes more complex when data crosses jurisdictional boundaries[1][15].
Solution: Use end-to-end encryption for data in transit and at rest. Implement robust identity and access management (IAM) policies and audit configurations pre- and post-migration[2][7][15].2. Vendor Lock-In
Proprietary tools, data formats, and APIs from the original cloud provider can create dependency, complicating transitions[2][8][10]. For example, AWS-specific services may not integrate seamlessly with Google Cloud.
Solution: Prioritize open-source or multi-cloud-compatible tools. Adopt containerization (e.g., Kubernetes) and avoid provider-specific services for critical workflows[4][10][11].3. Data Loss and Corruption
Transferring large datasets between providers risks data loss or corruption due to network interruptions or incompatible storage systems[7][12][16].
Solution: Conduct incremental migrations with thorough backups. Validate data integrity using checksums and perform phased testing[6][12][16].4. Cost Overruns
Unexpected expenses arise from data egress fees, retooling applications, and prolonged parallel infrastructure runs[2][5][8]. For instance, AWS charges for data transfers out to another provider.
Solution: Use cost calculators to estimate egress fees and optimize data compression. Schedule migrations during off-peak hours to reduce bandwidth costs[5][8][14].5. Compatibility and Performance Issues
Legacy applications or dependencies on origin-cloud services (e.g., Azure Active Directory) may fail in the new environment[3][11][16]. Latency can also spike if data centers are geographically distant[7][18].
Solution: Refactor applications to use cloud-agnostic services. Test performance in staging environments and leverage CDNs to mitigate latency[3][11][16].6. Inadequate Planning and Stakeholder Alignment
Rushing migration without a detailed roadmap leads to scope creep, missed deadlines, and miscommunication between teams[4][5][13].
Solution: Develop a phased migration plan with clear milestones. Involve business units early to align priorities and assign accountability[4][12][13].7. Skill Gaps and Tooling Shortfalls
Lack of expertise in multi-cloud architectures or automation tools can stall migrations[2][5][9].
Solution: Partner with certified cloud architects or use managed migration services. Invest in cross-training teams on multi-cloud tools like Terraform[9][10][13].8. Downtime and Service Disruption
Unplanned downtime during cutover phases disrupts operations and impacts revenue[4][14][18].
Solution: Use hybrid cloud models to maintain redundancy. Schedule migrations during low-traffic periods and implement rollback plans[4][7][14].By addressing these pitfalls through rigorous planning, leveraging cross-cloud tools, and prioritizing security, organizations can ensure smoother transitions between providers while minimizing risks and costs.
Citations:
- 1: https://www.infosysbpm.com/blogs/business-transformation/7-pitfalls-to-avoid-for-seamless-cloud-migration.html
- 2: https://www.ivanti.com/blog/cloud-migration-benefits-risks
- 3: https://www.baass.com/blog/common-pitfalls-of-cloud-migration-and-how-to-avoid-them
- 4: https://aws.amazon.com/blogs/mt/designing-a-successful-cloud-migration-top-five-pitfalls-and-how-to-avoid-a-stall/
- 5: https://www.hbs.net/blog/cloud-migration-challenges
- 6: https://www.aztechit.co.uk/blog/cloud-migration-mistakes
- 7: https://atlan.com/cloud-migration-challenges/
- 8: https://bluexp.netapp.com/blog/cloud-migration-strategy-challenges-and-steps
- 9: https://www.datadynamicsinc.com/blog-clear-skies-ahead-how-to-avoid-the-top-5-cloud-migration-pitfalls-and-ensure-a-successful-journey/
- 10: https://nordcloud.com/blog/8-cloud-migration-pitfalls-you-must-avoid/
- 11: https://www.digitalocean.com/resources/articles/cloud-migration-challenges
- 12: https://www.teradata.com/insights/data-platform/4-common-data-migration-mistakes-and-how-to-avoid-them
- 13: https://www.bcg.com/publications/2024/overcoming-cloud-migration-challenges
- 14: https://lawsontalk.com/5-pitfalls-to-know-when-migrating-to-the-cloud-t4510.html
- 15: https://www.ibm.com/think/insights/cloud-migration-challenges
- 16: https://www.cloudficient.com/blog/10-common-data-migration-challenges-and-how-to-overcome-them
- 17: https://www.checkpoint.com/cyber-hub/cloud-security/what-is-cloud-migration/top-5-cloud-migration-challenges/
- 18: https://stratusgrid.com/blog/cloud-migration-challenges
- 19: https://www.vmware.com/topics/cloud-migration
Action Required: Ensure Robust Post-Migration Consistency Checks
The migration converts OneDrive accounts to Microsoft Graph accounts and updates several tables. To further mitigate common pitfalls—such as data loss, orphaned records, or mismatches during data migration—please consider adding explicit consistency checks, for example:
- Verify References: After processing all rows, run queries that confirm no legacy OneDrive entries remain in tables like
cloudProviderAccounts,vaultAccounts, andaccountListPosition.- Example Check:
let orphanCount: Int = try Row.fetchOne(db, sql: "SELECT COUNT(*) as count FROM cloudProviderAccounts WHERE cloudProviderType = ?", arguments: [LegacyCloudProviderType.oneDrive.databaseValue])?["count"] ?? 0 assert(orphanCount == 0, "Legacy OneDrive records still exist after migration")- Confirm Update Counts: Ensure the number of updated rows in each table matches the expected migration count to detect any discrepancies.
These steps will help prevent potential pitfalls such as data corruption or orphan records during the migration.
Cryptomator/VaultDetail/MoveVault/MoveVaultCoordinator.swift (2)
66-66: Updated method signature to match protocol changesThe
showCreateNewFoldermethod signature has been correctly updated to match the new protocol requirement, adding thedelegate: ChooseFolderViewModelProtocol?parameter.
70-70: Delegate properly passed to child coordinatorThe delegate is correctly passed to the child
AuthenticatedFolderCreationCoordinator, enabling it to communicate folder creation events back to the delegate.Cryptomator/Common/ChooseFolder/ChooseFolderViewModel.swift (1)
67-71: Good implementation for dynamic item managementThe
addItemimplementation correctly adds the new item, maintains the sorted order, and notifies observers of the change through thechangeListener.This will enable better integration with SharePoint by allowing dynamic updates to the folder contents.
CryptomatorCommon/Tests/CryptomatorCommonCoreTests/Manager/CloudProviderAccountManagerTests.swift (3)
26-27: API change reflected in testsThe test has been updated to use
getAccount(for:)instead of what appears to be a previous method likegetCloudProviderType(for:), correctly accessing the cloud provider type through the account object.This change aligns with the shift toward retrieving complete account objects rather than just specific properties.
34-35: Consistent API usageUsing the same
getAccount(for:)method here maintains consistency with the previous test case.
37-37: Error handling updated for API changeError handling in the test has been updated to match the new API, ensuring that the appropriate error is thrown when an account is not found.
Cryptomator/MicrosoftGraph/SharePointAuthenticationCoordinator.swift (5)
16-21: New coordinator for SharePoint authenticationThe
SharePointAuthenticationCoordinatorclass conforms to the necessary protocols and properly manages a promise for pending authentication.The use of a weak reference to the parent coordinator helps prevent memory leaks in the coordinator hierarchy.
26-31: Appropriate initialization of authentication flowThe
startmethod properly initializes the authentication flow by creating and presenting the view controller for entering a SharePoint URL.
44-49: Appropriate drive list presentationThe
showDriveListmethod correctly initializes the drive list view controller with the necessary credentials and site URL.
51-59: Proper promise resolutionBoth
authenticatedandcancelmethods appropriately fulfill or reject the pending authentication promise and then clean up by callingclose().
61-64: Appropriate resource cleanupThe
closemethod correctly dismisses the view controller and notifies the parent coordinator that the child has finished.Cryptomator/Common/ChooseFolder/ChooseFolderViewController.swift (1)
84-84: ViewModel now passed as delegate for improved collaborationThe
viewModelis now passed as a delegate to theshowCreateNewFoldermethod, which will allow for better integration between folder creation and folder listing.This change enables newly created folders to be dynamically added to the view through the
addItemmethod.Cryptomator/Common/UIImage+CloudProviderType.swift (5)
9-9: LGTM: Added import for Microsoft Graph supportThe import for
CryptomatorCloudAccessCoreis necessary to use theMicrosoftGraphTypeenum.
28-31: Update to support Microsoft Graph typesThe
cloudProviderTypecase handling has been properly updated to support the new Microsoft Graph type alongside the existing local file system type.
54-61: LGTM: New method to handle Microsoft Graph vault iconsGood implementation of a helper method that returns the appropriate vault icon asset name based on the Microsoft Graph type (OneDrive or SharePoint).
72-75: LGTM: Consistent update for storage iconsThe storage icon handling has been updated consistently with the vault icon implementation to support Microsoft Graph types.
95-102: LGTM: New method to handle Microsoft Graph storage iconsGood implementation of a helper method that returns the appropriate storage icon asset name based on the Microsoft Graph type (OneDrive or SharePoint).
Cryptomator/MicrosoftGraph/SharePointDriveListViewController.swift (5)
14-21: LGTM: Well-structured view controller initializationThe view controller is properly set up with a weak coordinator reference and view model dependency injection.
23-38: LGTM: Appropriate viewDidLoad implementationThe view controller's
viewDidLoadmethod properly sets up the UI components, registers cells, and starts listening for data changes with appropriate error handling.
40-45: LGTM: Good refresh control implementationThe refresh control implementation follows best practices by showing the loading indicator immediately and setting the appropriate content offset.
84-90: Efficient but commented workaround for header capitalizationThe method to prevent header titles from being displayed in uppercase is well-implemented, though it's worth noting this is a workaround for a UIKit limitation.
92-100: LGTM: Appropriate drive selection handlingThe table view selection handler correctly creates a SharePoint credential and notifies the coordinator with proper error handling.
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderDBManager.swift (4)
62-63: LGTM: Enhanced account retrieval logicGood improvement to get the complete account object rather than just the cloud provider type, which provides more flexibility.
Also applies to: 64-64
80-84: LGTM: Well-implemented Microsoft Graph provider supportThe Microsoft Graph support is implemented correctly, retrieving the appropriate account and credentials, and constructing the provider with the necessary parameters.
113-115: LGTM: Consistent implementation in background session providerThe changes to the background session provider method match those in the regular provider method, maintaining consistency.
131-135: LGTM: Background session provider for Microsoft Graph implemented correctlyThe Microsoft Graph background session provider is implemented correctly, following the pattern established for other cloud providers in the same method.
CryptomatorTests/AddLocalVault/AddLocalVaultViewModelTestCase.swift (1)
67-68: No issues - straightforward property addition.
ThesavedAccountsarray provides a convenient in-memory store for new accounts. For test scenarios, this approach is sufficiently simple; no concurrency or additional synchronization is necessary as long as you don’t require thread-safe interactions.Cryptomator/AppDelegate.swift (2)
83-83: Returning a standard CocoaError for user cancellation is appropriate.
Switching toCocoaError(.userCancelled)aligns better with platform conventions and ensures consistent handling of cancellation scenarios across the codebase.
90-91: Correct integration of Microsoft Graph callback handling.
This logic properly delegates the returned URL to MSAL for Microsoft Graph sign-in flows. This is a recommended approach for handling redirect callbacks.Cryptomator/AddVault/OpenExistingVault/OpenExistingVaultCoordinator.swift (2)
28-28: Good extension of cloud providers to include Microsoft Graph variants.
By adding.microsoftGraph(type: .oneDrive)and.microsoftGraph(type: .sharePoint), you ensure future expansions are streamlined and handle new provider types consistently.
51-56: Graceful handling of user cancellation.
Guarding forCocoaError.userCancelledcleanly differentiates normal cancellations from real errors and prevents showing unintended error messages to the user.SharedResources/en.lproj/Localizable.strings (2)
157-158: Well-structured localization string for Microsoft Graph authentication error.The added error message is clear and helpful for users encountering permission issues with Microsoft Graph.
216-224: Comprehensive SharePoint localization strings added.The SharePoint-related strings are well-organized and provide clear guidance for users. The URL format instructions are particularly helpful. All strings follow the existing naming conventions and structure.
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderAccountDBManager.swift (1)
37-39: Method signature appropriately changed to provide the full account.The protocol now returns the entire
CloudProviderAccountobject instead of just the type, providing more flexibility for clients of this protocol.Cryptomator/Settings/SettingsCoordinator.swift (3)
10-11: Added necessary import for cloud access functionality.The addition of the
CryptomatorCloudAccessCoreimport supports the new Microsoft Graph integration.
53-54: SharePoint options added to cloud provider list.The implementation now includes both OneDrive and SharePoint as Microsoft Graph provider types, enhancing the available cloud options for users.
123-128: Improved error handling during authentication.The authentication process now properly handles errors, ignoring user cancellations while properly processing other errors. This makes the user experience more robust.
Cryptomator.xcodeproj/project.pbxproj (11)
392-392: New SharePoint authentication coordinator addedThis file adds a new SharePoint authentication coordinator class to manage the Microsoft Graph authentication flow for SharePoint integration.
401-401: OneDrive authenticator implementation addedA dedicated authenticator class for OneDrive via Microsoft Graph has been added to the project structure.
402-402: SharePoint credential model addedNew SharePoint credential model to hold SharePoint-specific authentication data.
403-403: Added SharePoint URL validation error localizationLocalization support for SharePoint URL validation errors, enhancing user experience during authentication.
427-427: Added Microsoft Graph error localizationLocalization support for Microsoft Graph authenticator errors to provide user-friendly error messages.
428-428: SharePoint authenticator implementation addedNew authenticator class for handling SharePoint-specific authentication processes.
442-443: Added SharePoint URL entry UI componentsAdded view controller and view model for SharePoint URL entry screen, allowing users to input their SharePoint site URL.
444-444: Added SharePoint authentication protocolProtocol defining the authentication interface for SharePoint, enabling clean separation of concerns.
445-446: Added SharePoint drive listing UI componentsView controller and view model for displaying and selecting available SharePoint drives, completing the SharePoint connection flow.
2043-2059: Organized Microsoft Graph components into a dedicated groupAll SharePoint and Microsoft Graph related files are properly organized into a dedicated group in the project structure, following good organization practices.
2526-2526: Updated URL schemes to include Microsoft Graph authenticationThe script for adding URL schemes now includes support for Microsoft Graph redirect URI, which is necessary for the OAuth authentication flow.
Cryptomator/MicrosoftGraph/EnterSharePointURLViewModel.swift (1)
43-48: Request domain verification.Where feasible, verifying that the domain indeed corresponds to a valid SharePoint site (e.g., checking deduced tenant name or site path) can prevent user confusion later in the flow. You might implement an async check if the environment supports it.
Cryptomator/AddVault/CreateNewVault/CreateNewVaultCoordinator.swift (3)
27-27: Microsoft Graph integration looks good.Adding
.microsoftGraph(type: .oneDrive)and.microsoftGraph(type: .sharePoint)broadens provider support in line with the PR objective to add SharePoint support. The approach is consistent with existing patterns for enumerating cloud providers.
143-147: Delegate injection for folder creation.Exposing
delegateinshowCreateNewFolderimproves modularity—folder creation updates can be handled by distinct view models. However, ensure you address potential retain cycles if the delegate references the coordinator. A weak delegate reference is already set, which is good practice.
172-193: Delegate-based update of newly created folder.Using
delegate?.addItem(...)ensures the parent flow model is updated promptly. If concurrency or sync issues may arise (e.g., multiple items added at once), ensure the delegate method is thread-safe or queued to the main thread for UI updates. This approach otherwise looks solid.Cryptomator/Common/CloudAuthenticator.swift (2)
21-28: Centralized account management.Introducing
microsoftGraphAccountManagerensures a single source for Microsoft Graph–related account data, aligning with established patterns for other providers. It helps maintain a clearer separation of concerns between general cloud accounts and provider-specific logic.
131-166: Graceful multi-account deauthentication.The code checks if multiple accounts for the same
credentialIDexist before fully deauthenticating. This is essential for shared credential usage. Confirm that upstream logic can handle partial deauthentication states correctly (such as when one account is removed but another remains).Cryptomator/Common/CloudAccountList/AccountListViewModel.swift (4)
60-67: Consider logging or handling failures when retrieving and creating account cell content.
Currently, errors fromgetAccount(for:)orcreateAccountCellContent(for:)are discarded bycompactMap { try? ... }, causing any failed retrieval or creation to be silently skipped. If this is intentional, please ensure clear user communication (e.g., logging). Otherwise, consider more robust error handling for missing or invalid data.
98-100: Placeholder approach is acceptable for initial data retrieval.
Returning a placeholder for.microsoftGraphis consistent with other placeholder-based flows. The logic is straightforward, though you might later unify it with a fully fetched representation if needed.
146-161: Verify edge cases when fetching drive details.
IffetchDrive(for:)fails or returns a null drive name, the chain yields an error or the placeholder text"<unknown-drive-name>". Ensure test coverage or fallback logic aligns with the expected UX in these edge cases.
243-269: Microsoft Graph refresh logic mirrors other providers.
The.microsoftGraphcase is consistent with the dropbox/box approach. This uniform handling is beneficial for maintainability and clarity.Cryptomator.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
8-9:✅ Verification successful
Confirm new pinned dependency versions are valid and secure.
Multiple dependencies have been upgraded to newer versions. Please verify that the updated versions are free from breaking changes, known security issues, or major regressions.Also applies to: 17-18, 53-54, 71-72, 98-99, 161-162, 188-189, 215-216, 251-252
🌐 Web query:
Are these pinned versions known to contain any security vulnerabilities or breaking changes? "appauth-ios 1.7.6" "asn1 2.7.0" "bigint 1.21.0" "cloud-access-swift 2.0.0" "digest 1.11.0" "microsoft-authentication-library-for-objc 1.5.1" "pcloud-sdk-swift 3.2.3" "swift-log 1.6.3" "xctest-dynamic-overlay 1.5.2"💡 Result:
The pinned versions of the listed libraries were analyzed using available security advisories and changelogs. Here's the breakdown:
appauth-ios 1.7.6
- Security: No known vulnerabilities reported in the changelog [13].
- Changes: Fixes an issue with
OIDExternalUserAgentIOSCustomBrowseron iOS 10+ and updates test runners. No breaking changes noted.OpenSSL-related Libraries (ASN.1, Digest)
- Potential Risks: OpenSSL vulnerabilities (e.g., CVE-2024-0727, CVE-2023-24011) in PKCS12 parsing and S/MIME validation [2] could indirectly affect libraries relying on older OpenSSL versions. Ensure these libraries use OpenSSL ≥3.0.7 or patched versions.
pcloud-sdk-swift 3.2.3
- Security: No vulnerabilities reported in the documentation [4]. Uses secure authentication flows (e.g.,
ASWebAuthenticationSession).- Breaking Changes: The SDK requires iOS 13+ and macOS 10.15+ for SPM integration.
swift-log 1.6.3
- Security: No direct vulnerabilities found, but [7] highlights risks in related Swift frameworks (e.g.,
swift-nio-http2). Ensure dependencies are updated.Other Libraries (bigint, cloud-access-swift, xctest-dynamic-overlay)
- No security advisories or breaking changes identified in the provided sources.
Key Recommendations
- Update OpenSSL Dependencies: Mitigate risks from CVEs like CVE-2024-0727 by using OpenSSL ≥3.0.7.
- Monitor Advisories: Track libraries without explicit vulnerability data (e.g.,
microsoft-authentication-library-for-objc).- Test Compatibility: Verify iOS/macOS version requirements for
pcloud-sdk-swiftandappauth-ios.No critical vulnerabilities were found in the pinned versions, but proactive updates are advised for OpenSSL-dependent components.
Citations:
- 1: https://securityaffairs.com/166390/mobile-2/apple-ios-17-6-and-ipados-17-6.html
- 2: https://github.com/chnzzh/OpenSSL-CVE-lib
- 3: https://0xn3va.gitbook.io/cheat-sheets/web-application/oauth-2.0-vulnerabilities
- 4: https://github.com/pCloud/pcloud-sdk-swift
- 5: https://www.techtimes.com/articles/306929/20240730/apple-urges-iphone-users-update-ios-17-6-asap-why.htm
- 6: https://www.cisa.gov/news-events/bulletins/sb21-256
- 7: https://swift.org/support/security.html
- 8: https://docs.checkmarx.com/en/34965-68643-scan.html
- 9: https://pub.dev/packages/flutter_appauth/changelog
- 10: https://backstage.forgerock.com/knowledge/advisories/article/a64088600
- 11: https://confluence.atlassian.com/security/cve-2023-22515-broken-access-control-vulnerability-in-confluence-data-center-and-server-1295682276.html
- 12: https://www.tencentcloud.com/document/product/1047/34281
- 13: https://github.com/openid/AppAuth-iOS/blob/master/CHANGELOG.md
- 14: https://explore.alas.aws.amazon.com
- 15: https://www.cisa.gov/news-events/bulletins/sb25-006
- 16: https://www.cisa.gov/news-events/bulletins/sb25-034
- 17: https://preside.io/ChangeLog.txt
- 18: https://docs.konghq.com/gateway/changelog/
- 19: https://forums.swift.org/c/server/security-updates/50
- 20: https://github.com/pCloud/pcloud-sdk-swift/security
Dependency Versions Verified – Confirm Security & Compatibility
The upgraded dependency versions in the Package.resolved file have been analyzed against available security advisories and changelogs. Overall, no critical vulnerabilities or breaking changes were found for the following libraries:
- appauth-ios 1.7.6: No known vulnerabilities or breaking changes.
- bigint, cloud-access-swift, xctest-dynamic-overlay: No security issues or regressions identified.
- swift-log 1.6.3: No direct vulnerabilities detected; however, remain aware of related framework risks.
- microsoft-authentication-library-for-objc 1.5.1: No immediate issues, but continue to monitor future advisories.
- pcloud-sdk-swift 3.2.3: Secure implementation noted; verify that your project meets its iOS (13+) and macOS (10.15+) requirements.
Points to Note:
- ASN.1 2.7.0 and Digest 1.11.0:
There are potential risks linked to older OpenSSL vulnerabilities (e.g., CVE-2024-0727, CVE-2023-24011). Please confirm these libraries are using a patched OpenSSL version (≥3.0.7) to mitigate any indirect risks.Ensure that your integration tests cover platform compatibility and that any OpenSSL-dependent components are correctly updated. This verification applies to the package resolution lines at 8–9, as well as 17–18, 53–54, 71–72, 98–99, 161–162, 188–189, 215–216, and 251–252.
CryptomatorCommon/Sources/CryptomatorCommonCore/MicrosoftGraph/MicrosoftGraphAccountDBManager.swift (6)
1-8: No issues with file header changes.
9-13: Imported modules appear consistent.
14-35: Consider additional validation or constraints for stored fields.
ForMicrosoftGraphAccount, verifyingaccountUIDuniqueness or adding checks for invalid URLs might help avoid data inconsistencies, especially with user-provided site URLs.
37-45: Confirm primary key constraints for PersistableRecord usage.
Ensure the database schema designatesaccountUIDas a primary key (or a unique constraint) so that.save(db)behaves as expected (INSERT vs. UPDATE).
47-49: Compact error enum is sufficient.
The error caseaccountNotFoundErroris clearly defined and minimal.
51-56: Protocol coverage looks good.
Defines essential operations for managing Microsoft Graph accounts in a straightforward manner.
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderType.swift
Show resolved
Hide resolved
CryptomatorCommon/Sources/CryptomatorCommonCore/Manager/CloudProviderType.swift
Show resolved
Hide resolved
No description provided.