Skip to content

Add a management realm and limit the raised PHP limits (stacked on #60) - #62

Open
somethingwithproof wants to merge 4 commits into
fix/review-hardeningfrom
fix/management-realm
Open

somethingwithproof wants to merge 4 commits into
fix/review-hardeningfrom
fix/management-realm

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

fix: add a management realm and limit the raised PHP limits (2.8)

This PR is stacked on #60 and must merge after it. Open it with base fix/review-hardening, which is at c81fe12; the branch is fix/management-realm, rebased onto that commit and holding three commits. It bumps the plugin version from 2.7 (set in #60) to 2.8 and adds a step to the version-change upgrade that #60 reorders, so it cannot be applied on its own. When #60 is merged, retarget this PR to main; no further rebase is needed because the branch already contains #60's last commit.

Description

Two leftovers from the review that #60 does not cover:

  1. Importing and deleting share the viewing realm. One realm, "Plugin -> MySQL Slow Log Viewer", covered viewing, uploading and deleting. This PR adds a second realm, "Plugin -> MySQL Slow Log Management". Importing, uploading and deleting now need it in addition to the viewing realm.
  2. memory_limit and the execution time were unlimited for every request to the page. They are now raised only for the import form, an upload and a delete.

Related issue

No open issue covers these. This follows the review findings noted in #60.

Behaviour

  • A user who holds only the viewing realm can still read the list, details, query and chart pages. For that user the list no longer shows the Import link, the row checkboxes or the actions menu, and a request to the import form, an upload or a delete is refused with a message and a redirect to the list.
  • Management fails closed: if the management realm is not registered, nobody can import or delete.
  • Upgrade from an older version. The upgrade registers the new realm and grants it to every user and every group that holds the viewing realm, so nobody loses import or delete when upgrading. It runs once, on the way up from a version older than 2.8; the version is recorded only after it succeeds, so a failure is logged and retried. A later change by an administrator is not undone, because the grant does not run again at 2.8 or later.
  • Fresh install. Both realms are registered. Cacti grants them to the administrator account; everyone else gets nothing until an administrator assigns the realms in User Management.
  • Limits. The import form, upload and import requests get an unlimited execution time and memory limit (the form describes that override to the user, so it has to apply to the form itself). A delete gets an unlimited execution time only. All other requests keep the limits the web server is configured with. The page size was bounded in Harden the slow log import and pages, and fix review findings #60, so the details list no longer needs the override.

How has this been tested?

  • Unit tests: ManagementRealmTest (realm id, allowed and denied, fail closed, install registers both realms, the grants and their failure cases), RequestLimitsTest (each action), and three added cases in SlowlogCheckUpgradeTest (grant before the version is recorded when upgrading from 2.7, no grant at 2.8 or later, version left unchanged when the grant fails).
  • End-to-end tests against an installed Cacti and MariaDB:
    • AccessFlowTest: a user with the viewing realm only reads the list but sees no Import link, checkboxes or actions, and a delete post, an upload post and the import form are all refused; users holding both realms directly or through a group see the controls and can delete; revoking the management realm stops deleting while viewing continues.
    • RealmFlowTest: a 2.7 install is taken through the upgrade (in a scratch copy of the plugin directory); the viewer and the grouped user are granted the realm, a user with another realm is not, an administrator's narrowing is not undone at the current version, and a run from 2.7 grants again.
    • The views, delete and upgrade tests use users that hold the management realm where they upload or delete.
  • Patch coverage against Harden the slow log import and pages, and fix review findings #60's branch: 100 percent (30 of 30 changed lines). New-code duplication on my detector: 0.0 percent.
  • Time zones: the whole suite, with E2E, also passed with PHP's date.timezone set to America/New_York and to UTC (513 passed, 1 skipped each) against MariaDB 11.8.9 in UTC. This PR adds no time handling, so the only interaction with Harden the slow log import and pages, and fix review findings #60's time zone fix is that its tests run on top of it.

CI-order runs (plugin installed with plugin_manage.php --install --enable, then Pest with SLOWLOG_E2E=1, then the patch gate):

Core With E2E Without E2E tests/bin intact cacti.log errors
Cacti develop (1.3 dev tree) 513 passed, 1 skipped 407 passed, 107 skipped yes 0
Cacti 1.2.x (local branch at 1.2.32) 513 passed, 1 skipped 407 passed, 107 skipped yes 0

The 1.2.x tree is a local branch of that line, not a fresh clone, and Pest ran from the develop checkout's vendor directory. The skip-when-log-not-writable path was not exercised because the containers run as root.

Risk

  • Group membership added after the upgrade. A user who joins a group that holds the viewing realm after the upgrade does not get the management realm; the administrator assigns it. This is the point of the split.
  • Custom realm assignments. The upgrade copies the viewing grants as they are at that moment. A site that restricts users some other way (for example by editing user_auth_realm outside Cacti) is not covered.
  • Realm numbering. The new realm takes the next free plugin realm id. The id is looked up by name at run time, never assumed.
  • The new realm is registered under a pseudo file name, slowlog_manage.php. No page has that name; Cacti only needs a name to list the realm.
  • One extra plugin_realms lookup on the list page and on the pages that check management. Not timed.

Rollback

Revert the three commits (5019a6f, 27a3c99, c7fcc3e). The extra realm row stays in plugin_realms and the grants stay in user_auth_realm and user_auth_group_realm; with the old code they have no effect and can be removed in User Management. Nothing in the data tables changes. If the recorded version is already 2.8, set it back to 2.7 on the Plugin Management page or the upgrade check will not run again.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (a user who relied on the viewing realm alone to import or delete loses that after a fresh install; upgrades keep it)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation. (CHANGELOG updated; the README is updated separately in docs/readme-verified.)
  • I have updated the documentation repository accordingly.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@sonarqubecloud

Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant