Fix wrong access type for SYSTEM CLEANUP and VIRTUAL PARTS UPDATE ON CLUSTER

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: security, backwards-compatibility, testing-strategy | Difficulty: mid
Link: https://github.com/ClickHouse/ClickHouse/pull/110289

The problem

The ON CLUSTER versions of SYSTEM CLEANUP and SYSTEM VIRTUAL PARTS UPDATE required the wrong privilege: they checked for SYSTEM PULLING REPLICATION LOG instead of their dedicated access types (SYSTEM CLEANUP and SYSTEM VIRTUAL PARTS UPDATE). This meant that users with the correct, dedicated privilege were denied when using the ON CLUSTER form, while users with the unrelated SYSTEM PULLING REPLICATION LOG privilege were allowed.

Why it's non-trivial

A naive fix might only patch the privilege for one code path or overlook the distinction between table-scoped and global (no-table) forms. The correct fix must ensure that both the table-specific and global ON CLUSTER commands use the right privilege, and that the regression test covers all relevant cases (including negative tests for users with only the old, incorrect privilege).

The approach

1. Correct privilege checks in the interpreter:

The main logic change is in InterpreterSystemQuery::getRequiredAccessForDDLOnCluster(). The code previously checked for the wrong access type; the fix replaces it with the correct one for both table-scoped and global (no-table) forms:

         case Type::START_CLEANUP:
         {
             if (!query.table)
-                required_access.emplace_back(AccessType::SYSTEM_PULLING_REPLICATION_LOG);
+                required_access.emplace_back(AccessType::SYSTEM_CLEANUP);
             else
-                required_access.emplace_back(AccessType::SYSTEM_PULLING_REPLICATION_LOG, query.getDatabase(), query.getTable());
+                required_access.emplace_back(AccessType::SYSTEM_CLEANUP, query.getDatabase(), query.getTable());
             break;
         }

This changes the required privilege for SYSTEM START/STOP CLEANUP ON CLUSTER to SYSTEM_CLEANUP.

         case Type::START_VIRTUAL_PARTS_UPDATE:
         {
             if (!query.table)
-                required_access.emplace_back(AccessType::SYSTEM_PULLING_REPLICATION_LOG);
+                required_access.emplace_back(AccessType::SYSTEM_VIRTUAL_PARTS_UPDATE);
             else
-                required_access.emplace_back(AccessType::SYSTEM_PULLING_REPLICATION_LOG, query.getDatabase(), query.getTable());
+                required_access.emplace_back(AccessType::SYSTEM_VIRTUAL_PARTS_UPDATE, query.getDatabase(), query.getTable());
             break;
         }

This does the same for SYSTEM START/STOP VIRTUAL PARTS UPDATE ON CLUSTER, switching to SYSTEM_VIRTUAL_PARTS_UPDATE.

2. Add regression tests for all access paths:

A new shell test script (04499_system_cluster_access_types.sh) and reference output file were added. The test:

Creates users with the correct and incorrect privileges.
Asserts that users with the dedicated privilege succeed, and those with only the old privilege are denied.
Covers both table-scoped and global (no-table) forms for both commands.
Handles the private nature of VIRTUAL PARTS UPDATE (which is not implemented in open-source builds) by checking for the expected error after the access check.

Example from the test:

# Holder of SYSTEM CLEANUP is allowed (command runs on MergeTree).
run --user "$cleanup_user" --query "SYSTEM STOP CLEANUP ON CLUSTER $cluster $CLICKHOUSE_DATABASE.$table" >/dev/null || exit 1
echo "ok"

Verifies that the correct privilege allows the command.

# Holder of only SYSTEM PULLING REPLICATION LOG is denied both commands.
run --user "$wrong_user" --query "SYSTEM STOP CLEANUP ON CLUSTER $cluster $CLICKHOUSE_DATABASE.$table -- { serverError ACCESS_DENIED }"

Verifies that the old, incorrect privilege is no longer sufficient.

From the review thread

| a | Deterministic repro? | Yes. Buggy binary: user with SYSTEM CLEANUP (or VIRTUAL PARTS UPDATE) + CLUSTER is denied the ON CLUSTER form, error demands SYSTEM PULLING REPLICATION LOG. Fixed binary: allowed. | | b | Root cause explained? | getRequiredAccessForDDLOnCluster() emitted SYSTEM_PULLING_REPLICATION_LOG for the CLEANUP and VIRTUAL_PARTS_UPDATE cases; local paths use SYSTEM_CLEANUP / SYSTEM_VIRTUAL_PARTS_UPDATE. | | c | Fix matches root cause? | Yes. The two DDL-on-cluster cases now emit the dedicated access types. | | d | Test intent preserved / new tests? | New test 04499_system_cluster_access_types.sh: positive paths assert success/expected serverError; negative path (only PULLING REPLICATION LOG) asserts ACCESS_DENIED. | | e | Both directions? | Yes. Fails on buggy binary (build ef300bff), passes on fixed (build c51d7aba). | | f | Fix general, not narrow? | Audited every case in getRequiredAccessForDDLOnCluster(). These two are the only remaining mismatches; PREWARM PRIMARY INDEX CACHE was already fixed by 2c0a4c01. | | g | Generalizes across inputs? | Covers both table-scoped and no-table forms; both STOP and START share the case. | | h | Backward compatible? | Backward-INCOMPATIBLE by design: the ON CLUSTER form now requires the dedicated grant. Requested by azat on #108956. | | i | Invariants preserved? | Local and DDL-on-cluster access checks now agree; no other contract touched. |

Takeaways

Always ensure privilege checks are consistent between local and distributed (ON CLUSTER) execution paths.
Regression tests for access control should cover both positive (allowed) and negative (denied) cases, including all privilege variants and command forms.
When fixing security or RBAC bugs, audit all similar code paths to avoid leaving related inconsistencies unfixed.