fix(rbac): access control[manual-only]

[kailixu]

Description

Issue(s)

• Close/close/Fix/fix/Resolve/resolve: Issue Link

Checklist

Please check the items in the checklist if applicable.

• Is the user manual updated?
• Are the test cases passed and automated?
• Is there no significant decrease in test coverage?

[gemini-code-assist]

Code Review

This pull request updates the database privilege check logic to distinguish between rollup and trim operations and enhances the test suite with RSMA creation and rollup privilege tests. However, a large block of existing test cases was inadvertently commented out, which should be restored, and a leftover comment string needs to be removed for code cleanliness.

[Copilot]

Pull request overview

This PR adjusts RBAC enforcement for ROLLUP DATABASE by mapping the privilege check to a dedicated rollup operation (instead of treating it like TRIM DATABASE), and updates the privilege test suite to set up RSMA prerequisites for rollup scenarios.

Changes:

• Update mndProcessTrimDbReq() to check MND_OPER_ROLLUP_DB when trimReq.optrType == TSDB_OPTR_ROLLUP.
• Extend the rollup privilege test to create a stable + RSMA before running ROLLUP DATABASE.
• Comment out large portions of the privilege control test suite (table/row/column/RBAC/system/etc. sections).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
test/cases/25-Privileges/test_priv_control.py	Adds RSMA creation for rollup testing, but also disables many test sections by commenting them out.
source/dnode/mnode/impl/src/mndDb.c	Routes DB privilege checks to ROLLUP vs TRIM based on STrimDbReq.optrType.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

'''BUG20 ''' is a standalone triple-quoted string statement. If this is meant to be a comment/marker, prefer a normal # comment (or a tracked TODO with issue ID) to avoid leaving an unused runtime string literal in the test body.

Suggested change

	'''BUG20 '''
	# BUG20

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

source/libs/executor/src/sysscanoperator.c:5248

• doDestroySysTableScanInfo unconditionally calls tsem_destroy(&pInfo->ready), but createSysTableScanOperatorInfo only calls tsem_init(&pInfo->ready, ...) in the non-local-scan path. For the local-scan cases (TABLES, TAGS, FILESETS), ready is never initialized, so destroying it can fail (and is undefined depending on platform semaphore implementation).

Consider initializing ready for all paths, or guard tsem_destroy behind a flag / pInfo->self > 0 / pInfo->readHandle.mnd != NULL as appropriate.

  SSysTableScanInfo* pInfo = (SSysTableScanInfo*)param;
  int32_t            code = tsem_destroy(&pInfo->ready);
  if (code != TSDB_CODE_SUCCESS) {
    qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(code));
  }
  blockDataDestroy(pInfo->pRes);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In loadSysTableCallback, the code releases the acquired ref (taosReleaseRef) before calling tsem_post(&pInfo->ready). If the operator has already removed its own ref (via taosRemoveRef) while this callback is in-flight, the taosReleaseRef here can drop the refcount to 0 and trigger doDestroySysTableScanInfo, freeing pInfo before tsem_post executes → use-after-free on pInfo->ready.

To make this safe, keep the acquired ref held until after tsem_post (or otherwise guarantee the semaphore outlives pInfo, e.g., post via a separately-owned semaphore / task-owned wait primitive).

Suggested change

	// Release our acquired ref BEFORE posting the semaphore.
	// If we post first, the waiter can race ahead: task completes → taosRemoveRef
	// drops the count to 1, then doDestroyTask frees the task memory pool (which
	// owns pInfo). Our subsequent taosReleaseRef would then drop the count to 0
	// and call doDestroySysTableScanInfo on already-freed memory.
	// By releasing first (count 2→1, destructor not triggered), pInfo remains
	// valid for the tsem_post call below, and doDestroySysTableScanInfo is
	// called only later, inside destroySysScanOperator, when pInfo is still live.
	int32_t refCode = taosReleaseRef(sysTableScanRefPool, pWrapper->sysTableScanId);
	if (refCode != TSDB_CODE_SUCCESS) {
	qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(refCode));
	}
	int32_t res = tsem_post(&pInfo->ready);
	if (res != TSDB_CODE_SUCCESS) {
	qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(res));
	}
	// Keep the acquired ref until after posting the semaphore. The semaphore is
	// embedded in pInfo, so releasing the ref first can drop the refcount to 0
	// and destroy pInfo before tsem_post() dereferences pInfo->ready.
	int32_t res = tsem_post(&pInfo->ready);
	if (res != TSDB_CODE_SUCCESS) {
	qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(res));
	}
	int32_t refCode = taosReleaseRef(sysTableScanRefPool, pWrapper->sysTableScanId);
	if (refCode != TSDB_CODE_SUCCESS) {
	qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(refCode));
	}

[Copilot]

sysTableScanFromMNode previously used tsem_timewait(..., VTB_REF_RPC_TIMEOUT_MS) but now blocks with qSemWait(..., &pInfo->ready) which has no timeout (it wraps tsem_wait). If the RPC callback is never invoked (transport error, dropped response, cancellation edge case), this worker thread can block indefinitely.

If a timeout is still required, consider adding a timed wait variant (e.g., qSemTimeWait) or restoring tsem_timewait and handling TSDB_CODE_TIMEOUT_ERROR explicitly.

[Copilot]

test_priv_control() now comments out the majority of the privilege test suite (database/table/RBAC/system/etc.) and effectively only runs do_system_variable_privileges(). This significantly reduces automated coverage for RBAC/privilege behavior and makes regressions much harder to catch.

If the intent is "manual-only", consider gating the full suite behind an explicit flag (env var / CLI option) or marking individual tests as skipped, rather than commenting out the calls in the main test entrypoint.

[Copilot]

do_system_monitoring_privileges still has inconsistent handling for the SHOW CONNECTIONS negative check: the earlier "without privilege" assertion is commented out via a triple-quoted block, but after revoke the assertion is executed (the '''BUG17''' line is now just a no-op string). This makes the test coverage uneven and the intent unclear.

Consider removing the triple-quote blocks entirely and using a normal comment/skip mechanism so SHOW CONNECTIONS is either tested in both places or explicitly skipped in both.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

source/libs/executor/src/sysscanoperator.c:5247

• doDestroySysTableScanInfo() always calls tsem_destroy(&pInfo->ready), but pInfo->ready is only initialized via tsem_init() in the remote/MNode path. For local-scan operators (e.g. TABLES/TAGS/FILESETS), this may destroy an uninitialized semaphore and can crash or return undefined errors. Track initialization (e.g. a readyInited flag) and only destroy when initialized, or initialize ready for all code paths.

  SSysTableScanInfo* pInfo = (SSysTableScanInfo*)param;
  int32_t            code = tsem_destroy(&pInfo->ready);
  if (code != TSDB_CODE_SUCCESS) {
    qError("%s failed at line %d since %s", __func__, __LINE__, tstrerror(code));
  }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

test_priv_control() now comments out almost the entire privilege test suite (DB/Table/RBAC/System/etc.) and effectively only runs do_information_schema_privileges(). This will drastically reduce regression detection in CI; please avoid disabling tests by commenting out calls—use the framework’s skip/mark mechanism or gate the “manual-only” subset behind an explicit runtime flag/env var while keeping the default run comprehensive.

Suggested change

	# self.do_create_database_privilege()
	# self.do_alter_database_privilege()
	# self.do_drop_database_privilege()
	# self.do_use_database_privilege()
	# self.do_show_databases_privilege()
	# self.do_show_create_database_privilege()
	# self.do_flush_database_privilege()
	# self.do_compact_database_privilege()
	# self.do_trim_database_privilege()
	# self.do_rollup_database_privilege()
	# self.do_scan_database_privilege()
	# self.do_ssmigrate_database_privilege()
	# # # Table privilege tests
	# print("")
	# print("[Table Privileges]")
	# self.do_create_table_privilege()
	# self.do_drop_table_privilege()
	# self.do_alter_table_privilege()
	# self.do_select_privilege()
	# self.do_insert_privilege()
	# self.do_delete_privilege()
	# self.do_select_column_privilege_comprehensive()
	# self.do_insert_column_privilege_comprehensive()
	# self.do_show_create_table_privilege()
	self.do_create_database_privilege()
	self.do_alter_database_privilege()
	self.do_drop_database_privilege()
	self.do_use_database_privilege()
	self.do_show_databases_privilege()
	self.do_show_create_database_privilege()
	self.do_flush_database_privilege()
	self.do_compact_database_privilege()
	self.do_trim_database_privilege()
	self.do_rollup_database_privilege()
	self.do_scan_database_privilege()
	self.do_ssmigrate_database_privilege()
	# # # Table privilege tests
	print("")
	print("[Table Privileges]")
	self.do_create_table_privilege()
	self.do_drop_table_privilege()
	self.do_alter_table_privilege()
	self.do_select_privilege()
	self.do_insert_privilege()
	self.do_delete_privilege()
	self.do_select_column_privilege_comprehensive()
	self.do_insert_column_privilege_comprehensive()
	self.do_show_create_table_privilege()

[Copilot]

sysTableScanFromMNode() switched from a timed wait (tsem_timewait(..., VTB_REF_RPC_TIMEOUT_MS)) to an unbounded qSemWait(...). If the RPC response is dropped or the callback never fires, this worker thread can block indefinitely. Consider restoring a bounded wait (or making qSemWait time-bounded) and handling timeout/cancellation explicitly to avoid query hangs.

[Copilot]

transCheckSysTablePriv() uses a hard-coded bitmask (0x01F8u) for sys-table privilege bits. This is brittle and easy to break if the bit layout changes; please replace it with named constants/macros (or compute from the actual bitfield definitions) so the intent and maintenance contract are explicit.

[Copilot]

System DB detection here relies on dbName[0] being 'i'/'I' to decide information_schema vs performance_schema. This is error-prone (e.g., if dbName is qualified or ever changes format) and duplicates logic elsewhere in the codebase (there are existing helpers like IS_INFORMATION_SCHEMA_DB(...)). Prefer using the existing schema-db detection helpers/macros instead of first-character checks.

[Copilot]

getSysTableMeta() decides which meta set to scan by checking only dbName[0] ('i'/'I' vs else). This is brittle and can misclassify if dbName is ever qualified (e.g. acct.db) or if additional system DBs are introduced. Prefer explicit comparisons/macros for information_schema / performance_schema (or reuse existing helpers) rather than first-character heuristics.

[Copilot]

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

mndBuildSysPrivBatchMask() is implemented inside #ifndef _PRIVILEGE and the file has no #else branch, so when TD_PRIVILEGE adds -D_PRIVILEGE (defines _PRIVILEGE) this function will not be compiled, but it is now referenced from mndConfig.c under TD_ENTERPRISE → potential link failure in enterprise+privilege builds. Also, the current implementation ignores pMnode/pUser/token/privTypes and just returns a full bitmask, which defeats the intended per-privilege filtering.

Consider providing a real implementation for _PRIVILEGE builds (e.g. iterate privTypes and check each via the privilege subsystem) and/or moving the declaration/definition out of the stub-only #ifndef _PRIVILEGE block so it is always available.

[Copilot]

test_priv_control() now comments out the majority of privilege test cases (database/table/RBAC/system/etc.), leaving only do_system_variable_privileges() enabled. This significantly reduces automated coverage for RBAC/access-control behavior and risks letting regressions slip through.

If these cases are meant to be manual-only, consider gating them behind an explicit runtime flag/marker (so CI still runs them when desired) rather than commenting them out, or split manual cases into a separate test file/suite.

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

source/libs/parser/src/parTranslater.c:14659

• Password privilege selection is currently wrong: targetUser is computed but never used, and the code compares authRsp.user with pParCxt->pUser (which should always match). That means altering another user's password will still be treated as "change own password" and checked against PRIV_PASS_ALTER_SELF, bypassing/ignoring PRIV_PASS_ALTER. Compare the current user against the target username instead (and drop the unused targetUser variable).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Same issue as the create path: the code checks mndCheckOperPrivilege(..., MND_OPER_DROP_MOUNT) but the surrounding comment still states the mount operation shares DB privileges. Please align the comment and the actual privilege enforcement to avoid confusion and future regressions.

Suggested change

	// mount operation share the privileges of db
	// drop mount operation requires the dedicated drop-mount privilege

[Copilot]

getShowVarPrivMask() calls catalogGetUserAuth(...), which populates heap-owned members inside SGetUserAuthRsp (hash tables, etc.). The function returns without calling tFreeSGetUserAuthRsp(&authRsp), leaking memory on every SHOW LOCAL VARIABLES execution. Ensure authRsp is freed on all paths (success and failure) after extracting the needed privilege bits.

[Copilot]

The comment says "mount operation share the privileges of db", but the code now checks mndCheckOperPrivilege(..., MND_OPER_CREATE_MOUNT) instead of a DB-scoped privilege check. Either update the comment to reflect the new privilege model, or (if it truly should be DB-scoped) restore a DB privilege check with the correct DB object/context.

Suggested change

	// mount operation share the privileges of db
	// mount creation uses mount-specific operation and grant checks

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

source/libs/parser/src/parTranslater.c:14666

• In translateCheckUserOptsPriv(), the password privilege branch compares authRsp.user with pParCxt->pUser, which will always match because catalogGetUserAuth() was requested for pParCxt->pUser. This makes the code always enforce PRIV_PASS_ALTER_SELF even when changing another user’s password, and also leaves targetUser unused. Compare targetUser against the current user (or pParCxt->pUser) to decide between PRIV_PASS_ALTER vs PRIV_PASS_ALTER_SELF.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

test_priv_control() now comments out almost the entire privilege test suite (DB/table/RBAC/system/function/view/stream/etc.) while the docstring and labels still describe it as a comprehensive CI test. This effectively removes coverage for most privilege checks and could let RBAC regressions slip through CI. If the intent is to make this "manual-only", consider gating sections behind an explicit flag/environment variable or splitting a dedicated, smaller manual test case instead of permanently commenting out the calls.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

source/libs/parser/src/parTranslater.c:14660

• The password-change privilege check compares authRsp.user with pParCxt->pUser, which are both the current user, so it will always take the “self password” path even when altering another user’s password. targetUser is computed but never used. Compare the target username being altered against the current user to decide between PRIV_PASS_ALTER vs PRIV_PASS_ALTER_SELF.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

On Linux this invokes make -f make_partial, but the added file in script/api/ is named makefile_partial. As-is the build step will fail with “No rule to make target 'make_partial'”. Update the filename (or add the expected makefile) so the test can compile passwdTest.c reliably.

Suggested change

	p = subprocess.Popen(f"cd {apiPath} && make -f make_partial", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
	p = subprocess.Popen(f"cd {apiPath} && make -f makefile_partial", shell=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE)

[Copilot]

Pull request overview

Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Linux build path: make -f make_partial refers to a non-existent makefile in script/api (the added file is makefile_partial). This will fail the passwd test on Linux unless there is an untracked make_partial file in the environment. Update the command to point at the actual filename (or rename the makefile to match).

[Copilot]

loadSysTableCallback() stores pMsg->pData into pInfo->pRsp on success, but it does not clear pMsg->pData. In this codebase, callbacks that take ownership typically set pMsg->pData = NULL to prevent the RPC framework / caller cleanup from freeing it after the callback returns. As-is, pInfo->pRsp can become a dangling pointer (use-after-free) or be double-freed when sysTableScanFromMNode() later frees pInfo->pRsp.

[Copilot]

getShowVarPrivMask() returns 0 on catalog/auth lookup failures, which will silently filter out all rows from SHOW LOCAL VARIABLES (enterprise) and make operational issues look like "no variables" instead of surfacing an error. Consider propagating the underlying error back to the caller (so the command fails) or at least logging and falling back to a safe default behavior consistent with the product requirements.

[Copilot]

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

source/libs/parser/src/parTranslater.c:14666

• In translateCheckUserOptsPriv(), the password privilege check compares authRsp.user with pParCxt->pUser, which are effectively the same current user, so the code will always take the “own password” branch and require PRIV_PASS_ALTER_SELF even when altering another user. targetUser is computed but never used. Please compare the current user (pParCxt->pUser) to the target user being altered/created (targetUser) to decide between PRIV_PASS_ALTER vs PRIV_PASS_ALTER_SELF.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.