Samba: Update Samba add-on to allow selectively enabling folders #3701
Conversation
…o only export the ones selected
… if starting with no folders enabled. Update config page translations for consistency
![home-assistant[bot]](https://avatars.githubusercontent.com/in/97978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi @as-kholin
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
WalkthroughWalkthroughThe recent updates to Samba enhance user configurability for folder access related to HomeAssistant. New options allow for selective exposure of directories such as 'addons', 'backup', and 'media', significantly improving both security and functionality. These enhancements ensure that users can tailor their Samba configurations while maintaining safe defaults. Changes
Assessment against linked 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 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: 0
Outside diff range, codebase verification and nitpick comments (1)
samba/DOCS.md (1)
132-140: Fix the grammatical issue.Remove the comma before "because" to correct the grammatical issue.
- These are considered sensitive, because anyone who gets ahold of both parts can impersonate your HomeAssistant server, + These are considered sensitive because anyone who gets ahold of both parts can impersonate your HomeAssistant server,Tools
LanguageTool
[formatting] ~136-~136: If the ‘because’ clause is essential to the meaning, do not use a comma before the clause.
Context: ...SL keys. These are considered sensitive, because anyone who gets ahold of both parts can...(COMMA_BEFORE_BECAUSE)
home-assistant
bot
added
cla-recheck
cla-signed
and removed
cla-recheck
cla-needed
labels
as-kholin
changed the title
…atch the degree of change to other entries in changelog
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
samba/CHANGELOG.md (1)
5-5: Enhance the breaking change notice with additional context.To provide better clarity for users, consider adding more details about the impact and necessary actions.
- BREAKING CHANGE: Default shares will reduce to media and shared. Other folders will have to be enabled by the end user. + BREAKING CHANGE: Default shares will be limited to "media" and "shared". Users must manually enable any other folders they wish to share. This change enhances security and user control but may require updating your configuration.
…ct benign idmap errors
…nt#3697) * Add support for noris network DNS provider * Fix typographical issue
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
samba/translations/en.yaml (1)
44-47: Consider mentioning that the option is disabled by default.To maintain consistency with other options, mention that the option is disabled by default.
- Enable SMB access to the Media folder + Enable SMB access to the Media folder. + This is disabled by default.
- Updated CHANGELOG.md to remove breaking change notice. - Updated config.yaml and en.yaml verbiage from 'enable' to 'allow'
|
I had addressed or replied to the comments listed. Was trying to hold off on resubmitting until the last conversation was resolved. I am not sure if that is where we are. @frenck, it says you make the conversation resolved - on that basis I am resubmitting for review. Let me know if there is still conversation there, or if we want to dig into other ways to manage those folders, to minimize their impact/risk while also exposing them by default to address the security/convenience gap. |
Fixes #3696. Fixes #3551. Adds 'enable' radio buttons for each share exposed by Samba plugin, to allow user to only enable the specific folders they want to expose.
Attempted to set sane defaults of what to expose. Directly upgrading may be viewed as a breaking change, as it would disable currently exposed folders.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores