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 👍 |
📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe recent updates to Samba enhance user configurability for folder access related to Home Assistant. 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
|
@frenck - Trying not to be a bother - but I did want to follow up on the status of this PR as it stands today, and path forward for it. In the end, I am mainly looking for some updated direction for keeping this moving, when you have a few min to review. If the larger philosophical conversation on what to enable up front is still ongoing, happy to circle back to it, and look to make adjustments to accommodate - but I would like to confirm that before making that type of structural change to the PR. If there are other changes desired, let me know. If you would prefer I follow up with someone else for a review before doing another round of review directly, let me know. While I don't want it to languish, I also realize the general priority for this PR (or lack thereof) - so, any time in the next few weeks that works for you (I will ping back if no response in the next two weeks or so) If others have feedback or would like to discuss more generally, also more than happy to dig in on that (@Eskander , tagging you because you had provided some comments earlier, in case you have ongoing or other feedback) |
This comment was marked as abuse.
This comment was marked as abuse.
|
Hey there. I didn't find this PR in time and made my own version of something similar but a lot more flexible. My version allows full customization of which shares are accessible by which user of multiple, and also allows removing shares entirely. Sadly, lists of objects aren't formatted nicely by the home assistant add-on options page. I wanted to throw this out there as a potential option. I'm willing to collaborate with you @as-kholin on this, if home-assistant devs think this could be a viable way forward. I did this for the following advantages:
As you can see, the core idea is to have 2 lists: users and shares. The shares list contains a deny list per share, which denies certain users access (by not adding them to smb.conf "valid users" for that share). I hope you consider whether this could be a way forward, as it offers significant advantages over the simple list of enabled shares. I will link my fork of this repo shortly, so you can see and compare. |
|
Here is the relevant commit in my fork |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
samba/config.yaml (2)
56-57:enabled_sharesschema ensures valid configurations.The regex pattern effectively validates the share names, ensuring only the predefined options are accepted. The case-insensitive matching is a good usability feature.
For improved readability, consider breaking the regex pattern into multiple lines:
enabled_shares: - "match(^(?i:(addons|addon_configs|backup|config| media|share|ssl))$)"This change would make the pattern easier to read and maintain without affecting functionality.
63-63: Newdebugoption added for troubleshooting.The addition of an optional
debugboolean field is a good practice for facilitating troubleshooting.To enhance usability:
- Add a brief comment explaining the purpose and effect of enabling debug mode.
- Consider adding documentation that describes how to use this debug option and what additional information it provides when enabled.
samba/translations/en.yaml (1)
14-21: LGTM! Consider minor wording improvement.The new
enabled_sharesconfiguration option aligns well with the PR objectives, allowing users to selectively enable specific folders for sharing. The description is clear and enforces security by restricting to only allowed values.Consider rewording the phrase "Any you wish to add must be typed" to improve clarity. A suggestion:
- List of file shares to make available. Any you wish to add must be typed. + List of file shares to make available. Select from the allowed values listed above.samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run (1)
41-45: LGTM: Debug logging feature added with a security considerationThe new debug logging feature is a valuable addition for troubleshooting. It appropriately checks the 'debug' setting before logging.
Consider adding a warning about potential exposure of sensitive information when debug logging is enabled. You might want to redact or mask sensitive data (like passwords) before logging the configuration.
samba/CHANGELOG.md (1)
2-5: Approve changes with a minor suggestion.The changelog entry for version 12.4.0 accurately reflects the new features added, which align with the PR objectives. The format is consistent with the rest of the file.
To improve clarity, consider adding a brief explanation of the impact of these changes, especially regarding the ability to enable/disable specific shares.
Consider expanding the first bullet point slightly:
- Add the ability to enable and disable specific shares + Add the ability to enable and disable specific shares, improving user control over folder accesssamba/DOCS.md (4)
15-18: Improve clarity in step 2.The new step aligns well with the PR objectives. To enhance clarity:
Consider rewording step 2 as follows:
-2. Review the enabled shares. Disable any you do not plan to use (they can be re-enabled later) +2. Review the enabled shares. Disable any shares you do not plan to use. You can re-enable them later if needed.This change improves readability and maintains a direct, authoritative tone.
44-51: LGTM. Consider adding a brief description.The new
enabled_sharesconfiguration option aligns perfectly with the PR objectives. It provides users with the ability to selectively enable specific folders.Consider adding a brief description above the list to improve clarity:
enabled_shares: # List of shares to enable. Remove or comment out any shares you don't want to be accessible. - addons - addon_configs - backup - config - media - share - sslThis addition would help users understand the purpose of this configuration option at a glance.
79-82: LGTM. Consider expanding the description.The description for the
enabled_sharesoption is clear and concise. To provide users with more context:Consider expanding the description as follows:
### Option: `enabled_shares` (required) List of Samba shares that will be accessible. Only the shares listed here will be exposed by the Samba server. Remove or comment out any shares you don't want to be accessible.This expanded description provides more context about the option's functionality and aligns with the PR objectives of enhancing user control over shared folders.
102-108: Approve with minor suggestions.The description for the
debugoption is informative and aligns with the coding guidelines. Here are some suggestions for improvement:
- Address the grammatical issue in line 105:
-know what you would be wanting to do with this information, not necessary. +know what you want to do with this information, not necessary.
- Simplify the last line for consistency with other option descriptions:
-Optional config. If not defined, then it will default to `false`. +Defaults to `false`.These changes will enhance readability and maintain consistency with the rest of the document.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~105-~105: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...nerally, unless you know what you would be wanting to do with this information, not necess...(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
samba/rootfs/usr/share/tempio/smb.gtpl (1)
Line range hint
29-118: Consider refactoring share definitions to reduce code duplicationThe share definitions for
config,addons,addon_configs,ssl,share,backup, andmediaare largely identical except for the share names and paths. Repeating similar code can lead to maintenance challenges and potential errors in the future. To enhance maintainability and readability, consider iterating over theenabled_sharesand dynamically generating the share configurations.Here's an example of how you might refactor the template:
{{ range $share := .enabled_shares }} [{{ $share }}] browseable = yes writeable = yes path = /{{ $share }} valid users = {{ $.username }} force user = root force group = root veto files = /{{ $.veto_files | join "/" }}/ delete veto files = {{ if gt (len $.veto_files) 0 }}yes{{ else }}no{{ end }} {{ end }}If the share names do not directly correspond to the directory names, you might need to map them accordingly. This approach reduces repetition and simplifies future updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- samba/CHANGELOG.md (1 hunks)
- samba/DOCS.md (5 hunks)
- samba/config.yaml (3 hunks)
- samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run (2 hunks)
- samba/rootfs/usr/share/tempio/smb.gtpl (8 hunks)
- samba/translations/en.yaml (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
samba/CHANGELOG.md (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.samba/DOCS.md (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.samba/config.yaml (6)
Pattern
*/**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern
*/**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern
*/**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern
*/**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern
*/**(html|markdown|md): do not comment on HTML used for icons
Pattern
*/**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
🪛 LanguageTool
samba/DOCS.md
[uncategorized] ~105-~105: This verb may not be in the correct tense. Consider changing the tense to fit the context better.
Context: ...nerally, unless you know what you would be wanting to do with this information, not necess...(AI_EN_LECTOR_REPLACEMENT_VERB_TENSE)
🔇 Additional comments (11)
samba/config.yaml (4)
2-2: Version update is appropriate.The version bump from 12.3.2 to 12.4.0 correctly reflects the minor feature additions in this update.
30-37: Newenabled_sharesoption enhances configurability.The addition of the
enabled_sharesoption aligns well with the PR objectives, allowing users to selectively enable specific folders for sharing. The list comprehensively covers important directories.Regarding the past comment about using "allow" instead of "enable":
The term "enabled_shares" is clear and consistent with typical configuration terminology. It accurately conveys that these shares are actively made available, which is more precise than "allowed_shares" in this context.
Line range hint
39-43: Improved formatting inveto_fileslist.The corrected indentation in the
veto_fileslist enhances readability and maintains consistent formatting throughout the configuration file.
Line range hint
1-63: Overall, these changes significantly enhance the Samba add-on's configurability.The updates align well with the PR objectives, providing users with more control over shared folders while maintaining security. The new
enabled_sharesoption and corresponding schema validation improve both flexibility and robustness. The addition of a debug option will aid in troubleshooting.These changes effectively address the issues raised in #3696, #3775, and #3551 by allowing selective folder sharing and improving overall manageability of the Samba add-on.
samba/translations/en.yaml (2)
32-34: LGTM! Valuable debugging option added.The new
debugconfiguration option is a great addition. It provides users with the ability to log the resulting Samba configuration, which will be invaluable for troubleshooting and verifying settings.
Line range hint
1-34: Verify if the past typographical error has been addressed.A previous review comment pointed out a typographical error in the description of the "addons" configuration, where "Configurations" was misspelled as "Configuraitons". Please verify if this has been corrected in the relevant part of the file.
✅ Verification successful
Typographical error 'Configuraitons' has been resolved in
samba/translations/en.yaml.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the presence of the typographical error in the file. # Test: Search for both the correct and incorrect spellings echo "Checking for the correct spelling 'Configurations':" rg --type yaml "Enable SMB access to the Add-on Configurations folder" samba/translations/en.yaml echo "Checking for the incorrect spelling 'Configuraitons':" rg --type yaml "Enable SMB access to the Add-on Configuraitons folder" samba/translations/en.yamlLength of output: 441
Script:
#!/bin/bash # Description: Search for the incorrect spelling 'Configuraitons' in the en.yaml file. echo "Searching for 'Configuraitons' in samba/translations/en.yaml:" rg --type yaml "Configuraitons" samba/translations/en.yamlLength of output: 197
samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run (2)
15-16: LGTM: New configuration requirement addedThe addition of the
enabled_sharesconfiguration requirement aligns with the PR objectives. It ensures that users explicitly specify which folders to share, enhancing security and user control.
35-36: LGTM: Improved configuration generation with case normalizationThe addition of
ascii_downcaseto theenabled_sharesvalues enhances consistency in share names. This is a good practice to prevent potential issues with case-sensitive systems.To ensure this change doesn't unexpectedly affect existing setups, please run the following verification:
samba/DOCS.md (2)
64-64: LGTM. Debug option added.The addition of the
debugoption with a default value offalseis a good enhancement. It provides users with the ability to enable additional logging when needed, while keeping it disabled by default for normal operation.
Line range hint
1-134: Overall documentation update is well-executed.The changes to the Samba share add-on documentation effectively communicate the new configuration options and align well with the PR objectives. The additions of
enabled_sharesanddebugoptions are clearly explained, enhancing user control over shared folders and providing better debugging capabilities.The document maintains a consistent style and follows the coding guidelines for the most part. The minor suggestions provided in the previous comments will further improve clarity and consistency.
Great job on updating the documentation to reflect these important changes!
samba/rootfs/usr/share/tempio/smb.gtpl (1)
Line range hint
29-40: Verify the path for the[config]shareThe
[config]share is set with the path/homeassistant. Please verify that this is the correct path for the Home Assistant configuration directory. Typically, the configuration directory is located at/config. Using an incorrect path might prevent users from accessing the intended files.
Fixes #3696, fixes #3775,resolves #3551.
Adds the ability to disable specific shares from the Samba addon config.
Summary by CodeRabbit
New Features
Documentation
Chores