[fix] Ensure log files are owned by www-data #627

[VanshajPoonia]

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Closes #627.

Description of Changes

Log files in the OpenWISP log directory (e.g. /opt/openwisp2/log/) could be created as root, preventing the application processes (uWSGI, celery, daphne, Django) that run as www-data from writing to them.

Root causes addressed:

1. The log directory had no owner, so under become: true it was created as root:www_group.
2. load_initial_data ran as root, so Django's RotatingFileHandler created openwisp2.log as root during deployment.
3. The cron jobs were installed in root's crontab, so manage.py log rollovers created root-owned files.
4. Supervisor log files were created by root-owned services (supervisord) on first run.

Changes:

• tasks/django.yml
  • Added owner: "{{ www_user }}" to the log directory task.
  • Added an idempotent task that pre-creates the supervisor + Django log files owned by {{ www_user }}:{{ www_group }}. The list is gated per feature flag, matching the conditions used to install the supervisor programs, so no empty log files are created for disabled features.
  • Ran Load initial data as www_user (matching the existing migrate/collectstatic/compilemessages tasks) so openwisp2.log isn't created as root.
• tasks/cron.yml
  • Installed the cron jobs in the www-data crontab (user: "{{ www_user }}") so manage.py runs — and any log rollover — happen as the application user.
  • Added a cleanup task to remove the obsolete root-owned cron entries on upgraded deployments.
• molecule/resources/verify.yml
  • Added regression tests asserting the log directory and the supervisor/Django log files are owned by www-data, and that the cron jobs are installed in the www-data crontab and not root's.
• CHANGES.md — added a bugfix entry.

Scope note: nginx logs (nginx.access.log/nginx.error.log) are intentionally left untouched, as they are created by the nginx master process running as root, and the existing logrotate.d/openwisp-nginx config already recreates them as www-data.

Testing: validated locally with yamllint, ansible-lint, and ansible-playbook --syntax-check (all pass); the full molecule run (converge + the new verify assertions) runs in CI across all four distros. I have not ticked "manually tested" because the end-to-end molecule run was executed by CI rather than locally.

Screenshot

N/A

[coderabbitai]

📝 Walkthrough

Walkthrough

Cron jobs in tasks/cron.yml now run as www_user and remove obsolete root-owned crontab entries. tasks/django.yml sets the log directory owner, pre-creates expected log files with explicit ownership and permissions, and runs load_initial_data as www_user. molecule/resources/verify.yml adds ownership and crontab regression checks. CHANGES.md records the bugfix.

Estimated code review effort: 2 (Simple) | ~15 minutes

Related issues: #627

Suggested labels: bug, ansible

Suggested reviewers: none identified

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error)

Check name	Status	Explanation	Resolution
General Rules	❌ Error	The new verify play only covers the default happy path; it doesn’t exercise the feature-flag boundary or upgrade cleanup the fix depends on.	Add a Molecule case with key services disabled and an upgrade-path assertion for stale root cron entries and stray log files.

✅ Passed checks (7 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is descriptive, matches the fix, and references issue #627.
Description check	✅ Passed	The PR description follows the template with checklist, issue reference, change summary, and screenshot section.
Linked Issues check	✅ Passed	The code addresses #627 by fixing log ownership, cron user context, cleanup, and regression tests for the affected files.
Out of Scope Changes check	✅ Passed	The added changelog, task updates, and verify tests stay within the issue scope and do not introduce unrelated changes.
Bug Fixes	✅ Passed	The patch fixes root causes (ownership, user context, stale root cron) and adds a deterministic molecule regression verify that fails without it.
Features	✅ Passed	PASS: this PR is a bugfix, not a feature request; it adds regression tests and a changelog entry, so the feature-specific checklist doesn’t apply.
Changes	✅ Passed	CHANGES.md was updated, molecule verify adds regression checks for log ownership and cron users, and there are no UI/API changes or migration gaps.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[nemesifier]

Closing because this PR doesn't follow our PR template. Please reopen following our guidelines, rules and PR templates.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@CHANGES.md`:
- Around line 9-14: The CHANGES.md entry uses RST formatting instead of
Markdown, so the issue reference will not render as a clickable link. Update the
changelog text to use Markdown link syntax and plain inline code styling where
needed, keeping the same content but replacing the RST-style issue reference
near the OpenWISP log directory note. Use the surrounding changelog entry as the
target for the fix.

In `@tasks/django.yml`:
- Around line 27-56: The log-file precreation task is touching feature-specific
logs unconditionally, which creates empty files even when the related workers
are disabled. Update the Ansible task using the existing loop in the Django log
setup so that celery-network.log, celery-monitoring.log, and
celery-firmware-upgrader.log are only included when openwisp2_network_topology,
openwisp2_monitoring, or openwisp2_firmware_upgrader are enabled, while keeping
the always-needed logs (such as openwisp2.log, uwsgi.log, celery.log,
celerybeat.log, and daphne.log) unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 03b1d23a-9a7d-41b1-a981-62176649a436

📥 Commits

Reviewing files that changed from the base of the PR and between fbdab35 and 0918681.

📒 Files selected for processing (3)

• CHANGES.md
• tasks/cron.yml
• tasks/django.yml

📜 Review details

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{yml,yaml,j2}

📄 CodeRabbit inference engine (AGENTS.md)

Avoid unnecessary blank lines inside Jinja, YAML, and shell blocks.

Files:

• tasks/cron.yml
• tasks/django.yml

**/*.{yml,yaml,j2,sh}

📄 CodeRabbit inference engine (AGENTS.md)

Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it.

Files:

• tasks/cron.yml
• tasks/django.yml

**/*.{md,rst,txt}

📄 CodeRabbit inference engine (Custom checks)

**/*.{md,rst,txt}: Changes: If the change affects behavior that is already documented, the documentation must be updated to reflect the new behavior
Features: Documentation must mention the new feature; for heavily UI-related features, a new section or page is appropriate

Files:

• CHANGES.md

🔇 Additional comments (3)

tasks/cron.yml (1)

2-51: LGTM!

tasks/django.yml (2)

16-26: LGTM!

---

257-266: 🗄️ Data Integrity & Integration

load_initial_data.py does not require root — The script only performs Django/database updates and reads PRIVATE_KEY / PUBLIC_KEY from the environment, so running this task as {{ www_user }} is appropriate.

			> Likely an incorrect or invalid review comment.

[nemesifier]

I see the you updated the PR description, reopening, thanks.