Added Box support #34
Conversation
WalkthroughThe CryptomatorCloudAccess project now integrates with Box cloud storage, introducing BoxCloudProvider for various storage operations, BoxError for error handling, BoxItem for representing items, and BoxAuthenticator for authentication. The setup includes new files, organization, and dependency adjustments for the Box SDK. Changes
Possibly related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Actionable comments outside the diff hunks (1)
create-integration-test-secrets-file.sh (1)
Line range hint
3-3: Replacesourcewith.to improve the portability of the script and ensure compatibility with POSIX sh.- source ./.integration-test-secrets.sh + . ./.integration-test-secrets.sh
Tests/CryptomatorCloudAccessIntegrationTests/Box/BoxCloudProviderIntegrationTests.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
You've probably seen that the CI failed: https://github.com/cryptomator/cloud-access-swift/actions/runs/8694502837/job/23843551721
Make sure to fix the formatting issues raised by SwiftFormat and SwiftLint. In the root of the project, I believe you can run:
swiftformat .
swiftlint --fixIf you want, you can also add a pre-commit hook so that you will notice these issues earlier: https://github.com/cryptomator/cloud-access-swift?tab=readme-ov-file#contributing
After that, I'll go through the PR more thoroughly. 😄
# Conflicts: # CryptomatorCloudAccess.xcodeproj/project.pbxproj
# Conflicts: # create-integration-test-secrets-file.sh
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
create-integration-test-secrets-file.sh (1)
Line range hint
3-3: Replace 'source' with '.' for better POSIX compliance.- source ./.integration-test-secrets.sh + . ./.integration-test-secrets.sh
There was a problem hiding this comment.
All in all, I'm quite happy with the implementation. Just did some minor changes between 79bb7cd...eb9a34d.
This is almost ready for a merge, but I'd like to request updated READMEs (both in the root directory and in the integration tests).
We should make sure that the integration tests work reliably with the current BoxCredentialMock implementation, which requires an access token and refresh token. I think you mentioned that there are some issues with that? As a fallback, we can require the usage of a developer token, but I'm not that happy with the 60 minutes limitation, to be honest.
Edit: You can try to set expiresIn to 59 or something lower (I think 60 is the tokenRefreshThreshold) instead of 3600, so that the SDK instantly tries to refresh the token. In that case, I would even argue that we don't need to specify an access token, because you'll always get a new access token, similar to the integration tests of Google Drive and OneDrive.
README.md
Outdated
| let tokenStore = BoxTokenStore() | ||
| let viewController = ... // the presenting `UIViewController` | ||
| BoxAuthenticator.authenticate(credential: credential, from: viewController).then { |
There was a problem hiding this comment.
Creating the BoxCredential is missing here. 😉
| @@ -33,6 +35,21 @@ If you are building via a CI system, set these secret environment variables acco | |||
|
|
|||
| ### How to Get the Secrets | |||
|
|
|||
| #### Box | |||
|
|
|||
| To get the access token for Box, generate a developer token in the Box Developer Portal. For more detailed instructions, check out the [OAuth 2.0 Documentation from Box](https://developer.box.com/guides/authentication/oauth2/). | |||
There was a problem hiding this comment.
Just wanted to mention here that the developer token shouldn't be mixed up with the access token from OAuth. The access token can actually be extracted at the same time when extracting the refresh token, which you've described below.
However, we should experiment with a solution that doesn't require an access token at all, as I've described in a different comment. My guess is that a refresh token would be enough for Box to get a new access token.
…egrate developer token.
There was a problem hiding this comment.
Actionable comments posted: 2
Out of diff range and nitpick comments (1)
create-integration-test-secrets-file.sh (1)
Line range hint
3-3: Replace 'source' with '.' for POSIX compliance.- source ./.integration-test-secrets.sh + . ./.integration-test-secrets.sh
|
|
||
| private static let credential = BoxCredentialMock() | ||
| // swiftlint:disable:next force_try | ||
| private static let cloudProvider = try! BoxCloudProvider(credential: credential) |
There was a problem hiding this comment.
Avoid using 'force_try' in tests to prevent crashes during test execution.
- private static let cloudProvider = try! BoxCloudProvider(credential: credential)
+ private static let cloudProvider = try? BoxCloudProvider(credential: credential)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.
| private static let cloudProvider = try! BoxCloudProvider(credential: credential) | |
| private static let cloudProvider = try? BoxCloudProvider(credential: credential) |
| To obtain the refresh token from Box, it is recommended to extract it from `authenticate` after a successful login. The easiest way to do this is to set a breakpoint inside the `BoxAuthenticator`: | ||
|
|
||
| ```swift | ||
| public static func authenticate(from viewController: UIViewController, tokenStore: TokenStore) -> Promise<(BoxClient, String)> { | ||
| return Promise { | ||
| // ... | ||
| fulfill((client, user.id)) // set breakpoint here | ||
| // ... | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Add detailed comments within the authenticate function to guide users on using breakpoints effectively for extracting the refresh token.
+ // Note: The breakpoint will allow you to inspect the authentication process and extract the refresh token.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.
| To obtain the refresh token from Box, it is recommended to extract it from `authenticate` after a successful login. The easiest way to do this is to set a breakpoint inside the `BoxAuthenticator`: | |
| ```swift | |
| public static func authenticate(from viewController: UIViewController, tokenStore: TokenStore) -> Promise<(BoxClient, String)> { | |
| return Promise { | |
| // ... | |
| fulfill((client, user.id)) // set breakpoint here | |
| // ... | |
| } | |
| ``` | |
| public static func authenticate(from viewController: UIViewController, tokenStore: TokenStore) -> Promise<(BoxClient, String)> { | |
| return Promise { | |
| // ... | |
| fulfill((client, user.id)) // set breakpoint here | |
| // Note: The breakpoint will allow you to inspect the authentication process and extract the refresh token. | |
| // ... | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (4)
Tests/CryptomatorCloudAccessIntegrationTests/README.md (4)
39-39: Clarify the expiration details of the Box developer token to prevent confusion during testing.
Line range hint
47-47: Add a comment in theGoogleDriveAuthenticatorcode snippet to explain the purpose of the breakpoint for new developers.+ // Set a breakpoint here to extract the refresh token after successful authentication
Line range hint
91-91: In thePCloudAuthenticatorcode snippet, clarify what the developer should look for when the breakpoint is hit to make the token extraction process clearer.+ // Set a breakpoint here to capture the PCloudCredential details after successful authorization
Line range hint
170-170: The file should end with a single newline character to adhere to POSIX standards.+
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
CryptomatorCloudAccess.xcodeproj/project.pbxproj (1)
189-200: Review overall project settings for potential optimization.Consider reviewing the overall project settings to optimize build times and resource usage, especially with the addition of new components like Box.
Also applies to: 421-421
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- CryptomatorCloudAccess.xcodeproj/project.pbxproj (18 hunks)
- CryptomatorCloudAccess.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (5 hunks)
- CryptomatorCloudAccess.xcodeproj/xcshareddata/xcschemes/BoxIntegrationTests.xcscheme (1 hunks)
- Package.resolved (1 hunks)
- Package.swift (3 hunks)
- Sources/CryptomatorCloudAccess/Box/BoxAuthenticator.swift (1 hunks)
- Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift (1 hunks)
- Sources/CryptomatorCloudAccess/Box/BoxCredential.swift (1 hunks)
- Sources/CryptomatorCloudAccess/Box/BoxIdentifierCache.swift (1 hunks)
- Sources/CryptomatorCloudAccess/Box/BoxItem.swift (1 hunks)
- Tests/CryptomatorCloudAccessIntegrationTests/Box/BoxCredentialMock.swift (1 hunks)
Files skipped from review due to trivial changes (2)
- CryptomatorCloudAccess.xcodeproj/xcshareddata/xcschemes/BoxIntegrationTests.xcscheme
- Package.resolved
Additional Context Used
Learnings (1)
Sources/CryptomatorCloudAccess/Box/BoxAuthenticator.swift (1)
User: tobihagemann URL: https://github.com/cryptomator/cloud-access-swift/pull/34 Timestamp: 2024-04-16T06:03:14.688Z Learning: The `BoxAuthenticatorError` enum in `BoxAuthenticator.swift` includes a case `invalidContext` for handling scenarios where a `UIViewController` cannot be cast to `ASWebAuthenticationPresentationContextProviding`.
Additional comments not posted (22)
Sources/CryptomatorCloudAccess/Box/BoxIdentifierCache.swift (3)
15-25: The constructor correctly sets up the in-memory database and handles its initialization errors by propagating them. Good use of resource initialization.
33-37: TheaddOrUpdate(_:)method correctly handles database operations with appropriate error propagation.
39-43: Good use of parameterized queries ininvalidate(_:)to prevent SQL injection.Sources/CryptomatorCloudAccess/Box/BoxAuthenticator.swift (1)
22-46: Theauthenticate(from:tokenStorage:)method is well-implemented with safe casting and proper error handling. It also makes good use of modern Swift concurrency features.Sources/CryptomatorCloudAccess/Box/BoxCredential.swift (3)
22-26: The constructorinit(tokenStore:)is correctly implemented with proper initialization of OAuth configuration and client.
28-42: Thedeauthenticate()method correctly handles token revocation with appropriate error handling.
44-61: ThegetUsername()method is well-implemented, handling both the presence and absence of a username correctly, and providing clear error handling.Tests/CryptomatorCloudAccessIntegrationTests/Box/BoxCredentialMock.swift (2)
35-47: The constructor ofBoxCredentialMockis well-implemented, setting up the necessary test environment and initializing the superclass correctly.
49-54: Thedeauthenticate()method inBoxCredentialMockis correctly implemented for testing purposes, setting the client to an invalid token.Sources/CryptomatorCloudAccess/Box/BoxItem.swift (3)
13-22: TheBoxItemstruct is well-defined, with clear properties and appropriate protocol conformances for database operations.
24-50: The extension ofBoxItemis well-implemented, providing initializers for different item types and correctly handling unsupported types by throwing an error.
52-58: The encoding functionality in theBoxItemextension is correctly implemented, ensuring that item properties are properly encoded for database operations.Package.swift (1)
Line range hint
17-46: The package configuration inPackage.swiftis correctly set up, including the necessary dependencies and targets for the integration of the Box SDK.CryptomatorCloudAccess.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved (1)
31-39: Added dependency onbox-swift-sdk-genversion0.2.0.This addition aligns with the PR's objective to integrate Box Cloud support. Ensure that this version of the SDK is compatible with the rest of the project dependencies and the Swift version used.
Sources/CryptomatorCloudAccess/Box/BoxCloudProvider.swift (6)
18-22: Ensure proper initialization ofBoxCloudProvider.The constructor correctly initializes the
credential,identifierCache, andmaxPageSizeproperties. The use ofmax(1, min(maxPageSize, 1000))ensures thatmaxPageSizeremains within a sensible range, which is good for controlling the size of data pages handled by the provider.
24-28: Review the implementation of metadata fetching.The method
fetchItemMetadatauses a promise chain to resolve the item path and then fetch its metadata. This approach is clean and maintains readability while handling asynchronous operations effectively.
36-44: Check preconditions and handle file download correctly.The method
downloadFilecorrectly checks if the URL is a file URL and if the file already exists, returning an error if so. This prevents overwriting existing files unintentionally. The rest of the method handles the download task asynchronously, which is appropriate for network operations.
46-70: Ensure robust file upload handling.The
uploadFilemethod includes comprehensive checks for the file's existence and type before proceeding. The conditional logic aroundreplaceExistingis correctly implemented to throw an error if an attempt is made to replace a folder with a file. The use of promises to chain these checks and the final upload task is well done.
72-82: Validate folder creation logic.The method
createFolderproperly checks if the folder already exists before attempting to create it, which avoids unnecessary API calls and potential errors. The promise chain is used effectively to handle asynchronous operations.
339-373: Review deletion logic and error handling indeleteFile.The
deleteFilemethod includes comprehensive error handling, including specific checks for expired tokens and file not found errors. The use of detailed logging helps in debugging and maintaining the system. The method also updates the cache upon successful deletion, which is crucial for consistency.CryptomatorCloudAccess.xcodeproj/project.pbxproj (2)
189-200: Integration of Box-related files and framework appears correctly implemented.Ensure that all newly added files are correctly referenced and that the
BoxSdkGenframework is properly configured in the build settings.Also applies to: 421-421
189-200: Configuration settings for Box integration are correctly added.Verify that the build configurations for the new Box-related files are correctly set to ensure smooth builds and integration tests.
| func get(_ cloudPath: CloudPath) -> BoxItem? { | ||
| try? inMemoryDB.read { db in | ||
| return try BoxItem.fetchOne(db, key: cloudPath) | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding error logging in the get(_:) method to aid in debugging when database read operations fail.
Introduces Box Cloud as a new cloud provider in cloud-access-swift
This PR integrates Box Cloud into the cloud-access-swift framework. It includes the implementation of the CloudProvider functionalities, OAuth2 authentication, addition of necessary credentials, and the creation of integration tests.
All integration tests are running successfully without any errors.