-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Samba: Update Samba add-on to allow selectively enabling folders #3701
Open
as-kholin
wants to merge
44
commits into
home-assistant:master
Choose a base branch
from
as-kholin:smb_selective_enable
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+79
−5
Open
Changes from 33 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
d2fc1ef
Updating to add option to enable specific shares for exporting, and t…
as-kholin ae11735
Updating translations for descriptive titles for the different folders
as-kholin a3105fc
Update DOCS.md to explain settings. Update init-smbd to block startu…
as-kholin 6f4c772
remove unintentional extra line
as-kholin 09ef073
Updating grammer per coderabbitai
as-kholin 537522e
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 8a0d55d
Incrementing version number
as-kholin 7d313cb
Added Changelog update, and changed the version update in config to m…
as-kholin 8d6ccb5
Updating to account for coderabbitai feedback on CHANGELOG
as-kholin 40ccb71
Rebase to home-assistant/addons PR #3704 - Samba: correct benign idma…
as-kholin f1417d3
Letsencrypt: Add support for noris network DNS provider (#3697)
nana-ska 372c2a7
Rebasing CHANGELOG updates
as-kholin e3e3dd7
Merging in changes from upstream
as-kholin 8e9b5bd
Correct YAMLLing errors in user-facing descriptions due to long lines
as-kholin d020619
Correct typo on user descriptions in translations
as-kholin d62e9db
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin dae9c99
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 26c70a7
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin acc72b5
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 58b754e
Per frenck's feedback:
as-kholin 1093201
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 0f8f768
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 86699ce
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin a8b41c3
Correct missed adjustment when config variable names were changed
as-kholin ed18112
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 5d6903b
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 0ed4082
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin f8322c4
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 15a5626
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin da6295e
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 9bc5fda
Correct unintended Translation adjustment
as-kholin 7b4c04d
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 89bd885
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 62492fb
Merge branch 'home-assistant:master' into smb_selective_enable
as-kholin 6f8282f
Merge remote-tracking branch 'upstream/master' into smb_selective_enable
as-kholin 9f62db3
Revamped selective enable
as-kholin 25b4758
Updated translation to 80 character line limit
as-kholin e830809
Updated documentation based on CodeRabbit Feedback
as-kholin 11acf7b
Merge remote-tracking branch 'upstream/master' into smb_selective_enable
as-kholin 47495b8
Merge remote-tracking branch 'upstream/master' into smb_selective_enable
as-kholin 9fb8782
Merge remote-tracking branch 'upstream/master' into smb_selective_enable
as-kholin a940a04
Merge remote-tracking branch 'upstream/master' into smb_selective_enable
as-kholin 0a1bc6d
Merge remote-tracking branch 'upstream/master' into smb_selective_enable
as-kholin f3d3e59
Merge remote-tracking branch 'upstream/master' into smb_selective_enable
as-kholin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logical reasoning behind this is unclear to me. The biggest use case for this add-on is accessing backups and the Home Assistant configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to add that the current defaults already limit shares to local networks, and require authentication. So reducing the default shares would limit use cases for minimal security benefits (only if home network and samba authentication are both compromised).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fail to see that logic in this case, as it assumes those shares contain no sensitive information, which is very unlikely.
Additionally, the main use case for this add-on is accessing one's Home Assistant configuration; or transferring backups (both directions). The add-on should be set up for that use case out of the box.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with everything you are saying on both the defaults, as well as the use cases. However, the counterpoints I would make are:
When I can access files such as
All falling under what is exposed using default filters in just the config folder, and know at least most of them are in the backups as well - then allowing access to them as the default case feels wider than it should be .
I feel strongly about this, but do not view most of it as a 'hill to die on'. If you disagree that the protection provided by only exposing things that are innocuous by default is worth the additional hassle of users having to turn on the specific shares they want, I will update, as the option is a step better than today, even if the defaults expose more than is ideal.
With that said, I would view the SSL folder as being a very high bar for saying 'we should be exposing this by default'. And, if we are exposing more of the other shares by default, I will want to update to add a few more default veto_files (covering at least the highest profile of the ones listed above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I'm a user interested on seeing this happen.
From this thread, I understand the only drawback was the selection of the 'default enabled' options? In which case I guess the PR dev can easily set the /config and /backup shares as default to support the "accessing one's Home Assistant configuration; or transferring backups" default use case.
Would that unlock this PR to be merged?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has been discussed internally today; the conclusion is that we won't be accepting it in its current state.
We are happy to add the options to enable/disable access to certain shares. However, we will not be accepting new defaults on which of those shares are enabled or disabled at this point.
Some reasoning:
Ideally, we agree, we would love to offer and force a choice to the user (similar like we do with the password setting), however, we don't have the UX abilities to make that happen for this specific case at the moment.
For the above reasoning, we welcome the new feature to disable shares, but we are not willing to accept a new defaults; all shares must be enabled (as is the case now).
../Frenck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. I am going to look at refactoring some of this based on this feedback as well. I will resubmit the PR once I have had a chance to do so and update the defaults (in the next 1-2 weeks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR this is ready for review
In a number of ways, my refactor turned into a rewrite, but I believe that it has accomplished the requested changes, while also accomplishing the minimum goals:
In addition, the update had two further goals:
Looking for any feedback for the path to getting this PR merged from here.