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)

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.

samba/config.yaml (2)

56-57: enabled_shares schema 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: New debug option added for troubleshooting.

The addition of an optional debug boolean field is a good practice for facilitating troubleshooting.

To enhance usability:

1. Add a brief comment explaining the purpose and effect of enabling debug mode.
2. 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_shares configuration 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 consideration

The 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 access

samba/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_shares configuration 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
  - ssl

This 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_shares option 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 debug option is informative and aligns with the coding guidelines. Here are some suggestions for improvement:

1. 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.

2. 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 duplication

The share definitions for config, addons, addon_configs, ssl, share, backup, and media are 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 the enabled_shares and 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.

Configuration used: CodeRabbit UI
Review profile: CHILL

samba/rootfs/etc/s6-overlay/s6-rc.d/init-smbd/run (1)

37-38: Consider adding share name validation.

While case normalization is good, consider adding validation for share names to prevent issues with special characters or invalid formats.

Consider adding a jq filter to validate share names. Example:

 jq ".interfaces = $(jq -c -n '$ARGS.positional' --args -- "${interfaces[@]}") |
-    .enabled_shares.[] |= ascii_downcase" /data/options.json \
+    .enabled_shares.[] |= (
+      ascii_downcase |
+      select(test(\"^[a-z0-9_-]+$\")) // 
+      error(\"Share names must contain only alphanumeric characters, underscores, and hyphens\")
+    )" /data/options.json \

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

#!/bin/bash
# Description: Verify template handling of enabled_shares
# Look for the template file and its usage of enabled_shares
cat /usr/share/tempio/smb.gtpl 2>/dev/null || echo "Template file not found in expected location"

#!/bin/bash
# Find template files in the repository
fd -e gtpl -e tmpl -e template

# If found, check their content for enabled_shares usage
fd -e gtpl -e tmpl -e template -x cat {}

#!/bin/bash
# Description: Check for existing configurations that might break
# Look for existing Samba configurations that might not have 'enabled_shares'
rg -l 'smb\.conf|samba' config/ || echo "No existing configurations found"

#!/bin/bash
# Let's check for any configuration examples or documentation
fd -e yaml -e json . | xargs rg -l "enabled_shares|share"

# Also check for any documentation or examples
fd -e md . | xargs rg -l "enabled_shares|share"

#!/bin/bash
# Let's check the Samba configuration and documentation for existing usage patterns
cat samba/config.yaml
cat samba/DOCS.md