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.
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
base: master
Are you sure you want to change the base?
Samba: Update Samba add-on to allow selectively enabling folders #3701
Changes from 18 commits
d2fc1ef
ae11735
a3105fc
6f4c772
09ef073
537522e
8a0d55d
7d313cb
8d6ccb5
40ccb71
f1417d3
372c2a7
e3e3dd7
8e9b5bd
d020619
d62e9db
dae9c99
26c70a7
acc72b5
58b754e
1093201
0f8f768
86699ce
a8b41c3
ed18112
5d6903b
0ed4082
f8322c4
15a5626
da6295e
9bc5fda
7b4c04d
89bd885
62492fb
6f8282f
9f62db3
25b4758
e830809
11acf7b
47495b8
9fb8782
a940a04
0a1bc6d
f3d3e59
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.