Add SMB support#650
Conversation
SailReal
left a comment
SailReal
left a comment
There was a problem hiding this comment.
Thank you very much for the contribution 💚!
Before looking any further into the PR, it looks like your added dependency https://github.com/hierynomus/smbj/tags is pretty unmaintained. Do you know any more details about it?
|
Hi there, Thank you for your help. I chose smbj primarily because it provides the necessary support for SMB 2 and SMB 3, and it seemed like the most reliable choice to get modern SMB support off the ground for the app. Regarding the maintenance, while the last tagged release was in early 2024, the repository remains active there was a commit last week, which suggests it is still active. That said, if you have a different option in mind, then I can look into switching it. I’ve enabled maintainer write access for this pull request, so any maintainer can make direct adjustments if needed, or I can update it based on your recommendations. I have tested this pull request out myself over SMB, and I am now able to unlock and update a vault via SMB by creating a text file with a written comment. The newly created text file with written text shows up when the vault is unlocked again using the desktop app. Folder create and file upload also works without issue. Furthermore, I have recreated the SMB icons as best as I can using existing assets from the source code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe change adds SMB as a cloud type with domain modeling, database mapping, SMBJ repository operations, factory wiring, authentication, presentation models, add/edit UI flows, validation, persistence, resources, and icons. Project configuration adds SMB dependencies, Gradle toolchain settings, IDE metadata, and build updates. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
presentation/src/main/java/org/cryptomator/presentation/ui/adapter/CloudSettingsAdapter.kt (1)
44-50: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReset recycled username state for SMB rows.
When a holder previously displayed a logged-in provider, the SMB branch leaves
cloudUsernamevisible with the previous row’s username. Clear and hide it before thewhenblock, while the fallback branch explicitly shows it when appropriate.Proposed fix
val cloudModel = getItem(position) binding.cloudImage.setImageResource(cloudModel.cloudType().cloudImageResource) +binding.cloudUsername.text = null +binding.cloudUsername.visibility = View.GONE when (cloudModel.cloudType()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@presentation/src/main/java/org/cryptomator/presentation/ui/adapter/CloudSettingsAdapter.kt` around lines 44 - 50, In the holder-binding logic around the cloud type when block, reset the recycled cloudUsername view before evaluating cloudModel.cloudType(): clear its text and hide it by default. Update the fallback branch to explicitly show and populate cloudUsername when that provider requires displaying a username, while preserving the existing provider-specific names.presentation/src/main/java/org/cryptomator/presentation/presenter/AuthenticateCloudPresenter.kt (1)
67-76: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftImplement SMB authentication before enabling this strategy.
SmbAuthStrategy.resumed()unconditionally callsfailAuthentication, so every existing SMB connection fails when the normalAuthenticateCloudPresenterflow is used. The PR’s SMB vault-access path therefore cannot authenticate; wire this strategy to the SMB connection/authentication use case and return the authenticated cloud on success.Also applies to: 401-410
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@presentation/src/main/java/org/cryptomator/presentation/presenter/AuthenticateCloudPresenter.kt` around lines 67 - 76, Implement SMB authentication in SmbAuthStrategy before keeping it enabled in AuthenticateCloudPresenter.strategies. Replace the unconditional failAuthentication behavior in SmbAuthStrategy.resumed() with the SMB connection/authentication use case, and return the authenticated cloud on success while preserving failure handling for unsuccessful authentication. Ensure both normal authentication and the SMB vault-access flow use this path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.idea/deploymentTargetSelector.xml:
- Line 10: Remove the machine-specific physical device selection identified by
DeviceId from deploymentTargetSelector.xml, preferably deleting the shared IDE
state file from version control or clearing the device entry before committing.
In
`@data/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.kt`:
- Around line 128-136: Update SmbCloudContentRepository.exists so only SMB
not-found conditions return false; let connection, authentication, and other
non-not-found exceptions propagate to the caller. Preserve the existing
fileExists/folderExists checks within withDiskShare and narrow the catch around
this logic accordingly.
- Around line 250-256: Update SmbCloudContentRepository.read and its copy flow
to emit the download lifecycle through progressAware, including the started
state and incremental progress while bytes are copied. Mirror the established
callback behavior in write(), using the transferred byte count and preserving
the existing stream and resource handling.
- Around line 217-246: Update write so data.open(context) is resolved and
validated before ds.openFile creates or overwrites the destination; fail
immediately when no input stream is available, then use the non-null stream for
copying and preserve the existing progress and file-info behavior.
In `@domain/src/main/java/org/cryptomator/domain/SmbCloud.java`:
- Around line 54-56: Update SmbCloud.configurationMatches to compare the domain
field in addition to url and username, while continuing to exclude password so
password changes do not create a distinct connection.
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/SmbAddOrChangePresenter.kt`:
- Around line 113-121: Update onCloudAuthenticated and save so finishWithResult
is invoked only from the save operation’s onSuccess callback, after persistence
completes. Handle the save handler’s failure callback without reporting
successful setup, preserving the existing result payload and ensuring
asynchronous write failures reach the caller.
In
`@presentation/src/main/java/org/cryptomator/presentation/ui/activity/CryptomatorVariantsActivity.kt`:
- Around line 21-27: Move the hardcoded variant text assigned to
tvLiteSupported, tvLiteUnsupported, tvFdroidCustomSupported,
tvFdroidCustomUnsupported, and tvWebsiteSupported into complete string resources
in strings.xml. Replace each literal in CryptomatorVariantsActivity with its
corresponding resource reference, preserving the existing wording and
variant-specific lists.
In
`@presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SmbAddOrChangeFragment.kt`:
- Around line 77-82: Update the password extraction in the SMB input flow around
smbAddOrChangePresenter.checkUserInput to preserve the exact contents of
passwordEditText, removing trim() only for the password while retaining trimming
for urlPort, username, and domain.
In `@presentation/src/main/res/drawable/smb_vault.xml`:
- Around line 6-15: Keep presentation/src/main/res/drawable/smb_vault.xml lines
6-15 unchanged as the unselected styling. Update
presentation/src/main/res/drawable/smb_vault_selected.xml lines 6-15 to use the
distinct selected-state styling established by the other vault icons, including
the appropriate selected colors or visual treatment.
In `@presentation/src/main/res/layout/fragment_setup_smb.xml`:
- Around line 2-79: Wrap the root form content in a vertical ScrollView or
NestedScrollView so all SMB fields, including domain_edit_text and
create_cloud_button, remain reachable on compact or landscape screens when the
keyboard is visible. Preserve the existing LinearLayout orientation, sizing,
padding, and child controls within the scroll container.
---
Outside diff comments:
In
`@presentation/src/main/java/org/cryptomator/presentation/presenter/AuthenticateCloudPresenter.kt`:
- Around line 67-76: Implement SMB authentication in SmbAuthStrategy before
keeping it enabled in AuthenticateCloudPresenter.strategies. Replace the
unconditional failAuthentication behavior in SmbAuthStrategy.resumed() with the
SMB connection/authentication use case, and return the authenticated cloud on
success while preserving failure handling for unsuccessful authentication.
Ensure both normal authentication and the SMB vault-access flow use this path.
In
`@presentation/src/main/java/org/cryptomator/presentation/ui/adapter/CloudSettingsAdapter.kt`:
- Around line 44-50: In the holder-binding logic around the cloud type when
block, reset the recycled cloudUsername view before evaluating
cloudModel.cloudType(): clear its text and hide it by default. Update the
fallback branch to explicitly show and populate cloudUsername when that provider
requires displaying a username, while preserving the existing provider-specific
names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ef45cc2-274f-474d-a03c-f8f231f774f4
⛔ Files ignored due to path filters (1)
presentation/src/main/res/drawable-xxhdpi/smb.pngis excluded by!**/*.png
📒 Files selected for processing (41)
.idea/appInsightsSettings.xml.idea/deploymentTargetSelector.xml.idea/misc.xml.idea/planningMode.xmlbuildsystem/dependencies.gradledata/build.gradledata/src/apkStorePlaystore/java/org/cryptomator/data/cloud/CloudContentRepositoryFactories.javadata/src/fdroidAccrescent/java/org/cryptomator/data/cloud/CloudContentRepositoryFactories.javadata/src/lite/java/org/cryptomator/data/cloud/CloudContentRepositoryFactories.javadata/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.ktdata/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepositoryFactory.javadata/src/main/java/org/cryptomator/data/cloud/smb/SmbFile.ktdata/src/main/java/org/cryptomator/data/cloud/smb/SmbFolder.ktdata/src/main/java/org/cryptomator/data/cloud/smb/SmbNode.ktdata/src/main/java/org/cryptomator/data/db/mappers/CloudEntityMapper.javadomain/src/main/java/org/cryptomator/domain/CloudType.ktdomain/src/main/java/org/cryptomator/domain/SmbCloud.javadomain/src/main/java/org/cryptomator/domain/usecases/cloud/ConnectToSmb.javagradle.propertiesgradle/wrapper/gradle-wrapper.propertiespresentation/src/main/AndroidManifest.xmlpresentation/src/main/java/org/cryptomator/presentation/di/component/ActivityComponent.javapresentation/src/main/java/org/cryptomator/presentation/intent/SmbAddOrChangeIntent.javapresentation/src/main/java/org/cryptomator/presentation/model/CloudTypeModel.ktpresentation/src/main/java/org/cryptomator/presentation/model/SmbCloudModel.ktpresentation/src/main/java/org/cryptomator/presentation/model/mappers/CloudModelMapper.ktpresentation/src/main/java/org/cryptomator/presentation/presenter/AuthenticateCloudPresenter.ktpresentation/src/main/java/org/cryptomator/presentation/presenter/CloudConnectionListPresenter.ktpresentation/src/main/java/org/cryptomator/presentation/presenter/CloudSettingsPresenter.ktpresentation/src/main/java/org/cryptomator/presentation/presenter/SmbAddOrChangePresenter.ktpresentation/src/main/java/org/cryptomator/presentation/ui/activity/CryptomatorVariantsActivity.ktpresentation/src/main/java/org/cryptomator/presentation/ui/activity/SmbAddOrChangeActivity.ktpresentation/src/main/java/org/cryptomator/presentation/ui/activity/view/SmbAddOrChangeView.ktpresentation/src/main/java/org/cryptomator/presentation/ui/adapter/CloudSettingsAdapter.ktpresentation/src/main/java/org/cryptomator/presentation/ui/bottomsheet/CloudConnectionSettingsBottomSheet.ktpresentation/src/main/java/org/cryptomator/presentation/ui/fragment/SmbAddOrChangeFragment.ktpresentation/src/main/res/drawable/smb_vault.xmlpresentation/src/main/res/drawable/smb_vault_selected.xmlpresentation/src/main/res/layout/fragment_setup_smb.xmlpresentation/src/main/res/values/strings.xmlsettings.gradle
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
data/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.kt (2)
90-130: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftReconnecting for every single filesystem operation is expensive.
withDiskShareopens a brand-newSMBClient, TCP connection, session (with full NTLM/Kerberos authentication), and tree connect for every call — and it's invoked independently byexists,list,create/mkdirs,move,write,read, anddelete. Browsing a folder or unlocking a vault typically issues several sequential calls (e.g.,listfollowed by per-entryexistschecks), so each one pays a full negotiate+authenticate+tree-connect round trip. This will noticeably slow down normal usage compared to reusing a single authenticated share/session for the repository's lifetime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.kt` around lines 90 - 130, Refactor withDiskShare so SMBClient, connection, authenticated session, and DiskShare are established once and reused across repository operations instead of recreated per call. Preserve the existing authentication, credential-error mapping, resource cleanup, and action invocation behavior, while ensuring the shared resources are closed when the repository lifecycle ends.
90-119: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound SMB connect and IO timeouts in
withDiskShare.
SMBClient()currently uses the library default config, whose socket timeout defaults to infinite, whileSmbConfigdoes not expose a direct connect timeout in the builder. At least setSmbConfig.builder.withTimeout(...).withSoTimeout(...).build()and wire a boundedSocketFactory/custom connection handling for the TCP connect phase so unreachable SMB hosts or frozen operations don’t block every filesystem call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.kt` around lines 90 - 119, Update withDiskShare to construct SMBClient with an SmbConfig configured via builder.withTimeout(...) and withSoTimeout(...) using bounded durations, and provide bounded TCP connect handling through the supported SocketFactory/custom connection mechanism. Replace the unconfigured SMBClient() creation while preserving the existing authentication, share-connection, and resource-lifecycle behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@data/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.kt`:
- Around line 339-372: Update deleteRecursive to track failures from child ds.rm
calls, recursive deleteRecursive calls, and both ds.rmdir attempts, retaining
the relevant exception when possible. Propagate the failure to its caller
instead of swallowing it so delete reports unsuccessful removal when any nested
file or directory remains undeleted.
- Around line 42-49: Update throwWrappedIfRequired so STATUS_ACCESS_DENIED and
STATUS_NETWORK_ACCESS_DENIED are mapped to WrongCredentialsException only for
authenticate() and connectShare() failures, not for general file or folder
operations. Preserve the existing mappings for path-not-found and other
credential-specific statuses, and let resource-level permission errors propagate
as NetworkConnectionException or the established permission error.
---
Outside diff comments:
In
`@data/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.kt`:
- Around line 90-130: Refactor withDiskShare so SMBClient, connection,
authenticated session, and DiskShare are established once and reused across
repository operations instead of recreated per call. Preserve the existing
authentication, credential-error mapping, resource cleanup, and action
invocation behavior, while ensuring the shared resources are closed when the
repository lifecycle ends.
- Around line 90-119: Update withDiskShare to construct SMBClient with an
SmbConfig configured via builder.withTimeout(...) and withSoTimeout(...) using
bounded durations, and provide bounded TCP connect handling through the
supported SocketFactory/custom connection mechanism. Replace the unconfigured
SMBClient() creation while preserving the existing authentication,
share-connection, and resource-lifecycle behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d734cb56-400d-4db8-aee7-3b30b86b5e8c
📒 Files selected for processing (11)
.idea/deploymentTargetSelector.xmldata/src/main/java/org/cryptomator/data/cloud/smb/SmbCloudContentRepository.ktdomain/src/main/java/org/cryptomator/domain/SmbCloud.javapresentation/src/main/java/org/cryptomator/presentation/presenter/AuthenticateCloudPresenter.ktpresentation/src/main/java/org/cryptomator/presentation/presenter/SmbAddOrChangePresenter.ktpresentation/src/main/java/org/cryptomator/presentation/ui/activity/CryptomatorVariantsActivity.ktpresentation/src/main/java/org/cryptomator/presentation/ui/adapter/CloudSettingsAdapter.ktpresentation/src/main/java/org/cryptomator/presentation/ui/fragment/SmbAddOrChangeFragment.ktpresentation/src/main/res/drawable/smb_vault_selected.xmlpresentation/src/main/res/layout/fragment_setup_smb.xmlpresentation/src/main/res/values/strings.xml
💤 Files with no reviewable changes (1)
- .idea/deploymentTargetSelector.xml
🚧 Files skipped from review as they are similar to previous changes (6)
- presentation/src/main/res/layout/fragment_setup_smb.xml
- presentation/src/main/java/org/cryptomator/presentation/ui/activity/CryptomatorVariantsActivity.kt
- presentation/src/main/java/org/cryptomator/presentation/ui/adapter/CloudSettingsAdapter.kt
- domain/src/main/java/org/cryptomator/domain/SmbCloud.java
- presentation/src/main/java/org/cryptomator/presentation/ui/fragment/SmbAddOrChangeFragment.kt
- presentation/src/main/java/org/cryptomator/presentation/presenter/SmbAddOrChangePresenter.kt
2 participants
This pull request aims to add SMB support into the app and closes issue #85
Currently, got the UI and ability to input SMB details. But do need to work on the SMB icon and further testing ensure SMB works correctly.