Skip to content

fix(install): run privileged user/ownership setup via sudo when installer is non-root (#753) - #754

Merged
jaylfc merged 1 commit into
devfrom
fix/install-sudo-cmd-helpers
Jun 10, 2026
Merged

fix(install): run privileged user/ownership setup via sudo when installer is non-root (#753)#754
jaylfc merged 1 commit into
devfrom
fix/install-sudo-cmd-helpers

Conversation

@jaylfc

@jaylfc jaylfc commented Jun 10, 2026

Copy link
Copy Markdown
Owner

On a non-root run where have_root_or_sudo succeeds (passwordless sudo, the default on Radxa/Armbian SBC images), ensure_taos_user called useradd and usermod bare without any privilege elevation, and set_data_dir_ownership called chown -R and chmod the same way, silently discarding errors via 2>/dev/null || true. The service unit was still stamped with User=taos, so the first start failed with 217/USER on a fresh box, or reproduced the #723 CHDIR restart loop on a box with a prior root install where the repair chown/chmod never actually ran.

Both functions now compute local sudo_cmd the same way install_linux_systemd_system already does (empty when already root, "sudo" otherwise), and prefix every privileged call with it. The chown -R and parent-walk chmod o+x failures are no longer silenced: if either fails, a visible warn tells the user the service will not start and to re-run with sudo. The chmod 0700/0600 tightening calls also receive the prefix. Closes #753; hardens the #723 scenario.

Summary by CodeRabbit

  • Bug Fixes

    • Improved privilege handling during installation to ensure proper user and group management.
    • Tightened permissions on data directories and credential files for enhanced security.
  • Chores

    • Enhanced error handling and warnings during system setup.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a33f345-08ce-414a-bef1-9c4f746d08bf

📥 Commits

Reviewing files that changed from the base of the PR and between 5fac4b2 and 2c6b110.

📒 Files selected for processing (1)
  • scripts/install-server.sh

📝 Walkthrough

Walkthrough

The installer script scripts/install-server.sh improves privilege handling by introducing root-aware sudo_cmd conditionals in two functions: ensure_taos_user now uses sudo_cmd for user creation and group additions; set_data_dir_ownership wraps chown and chmod operations through sudo_cmd with explicit failure warnings and stricter permission constraints.

Changes

Privilege Handling and Permissions Hardening

Layer / File(s) Summary
User and group management with root-safe execution
scripts/install-server.sh
ensure_taos_user defines conditional sudo_cmd and applies it to useradd and two usermod group-add calls, replacing unconditional non-root execution patterns.
Directory ownership and permissions with sudo_cmd
scripts/install-server.sh
set_data_dir_ownership routes recursive chown through sudo_cmd with explicit warning behavior; wraps all chmod operations (parent-directory traversal and data/credential-file permission tightening) through sudo_cmd.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

  • jaylfc/taOS#753: This PR directly implements the root-safe privilege handling fixes by threading $sudo_cmd into ensure_taos_user and set_data_dir_ownership functions and removing silent chown/chmod failures.

Possibly related PRs

  • jaylfc/taOS#724: Both PRs modify set_data_dir_ownership to adjust parent-directory permissions; this PR additionally introduces root-safe sudo_cmd wrapping for all privileged operations.
  • jaylfc/taOS#677: Both PRs modify ensure_taos_user and set_data_dir_ownership to implement root-safe user/group creation and permission management for non-root controller execution.

Poem

🐰 A script once made assumptions bold,
Running commands without care to hold,
But now with sudo_cmd so wise,
Each privilege check the script denies,
Safe and sound, the service glides! 🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding sudo handling to privileged operations in the installer when run as a non-root user, which directly matches the changeset's core objective.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/install-sudo-cmd-helpers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

1 participant