MDEV-14443 DENY statement#4761
Conversation
be81b12 to
8c31e0a
Compare
174ddcf to
df02867
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds first-class DENY support to the privilege system, making explicit denies override grants across scopes, with persistence in mysql.global_priv and updated privilege-evaluation plumbing.
Changes:
- Introduces
access_t(allow/deny/subtree) and migrates core privilege-caching/checking codepaths fromprivilege_tbitmasks. - Extends parser grammar to accept
DENY ...andREVOKE DENY ...statements. - Adds comprehensive mysql-test coverage for DENY behavior (hierarchy, roles/PUBLIC, SHOW visibility, persistence/restart, case-sensitivity, JSON validity, etc.).
Reviewed changes
Copilot reviewed 64 out of 65 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/wsrep_utils.cc | Initializes wsrep THD master_access using access_t. |
| sql/wsrep_mysqld.cc | Initializes wsrep THD master_access using access_t. |
| sql/temporary_tables.cc | Sets tmp-table grant privilege using access_t. |
| sql/table.h | Switches GRANT_INFO::privilege to access_t and updates related APIs. |
| sql/table.cc | Updates privilege assignment to access_t; resets want_privilege on reinit. |
| sql/sql_yacc.yy | Adds DENY token and grammar for DENY / REVOKE DENY. |
| sql/sql_show.cc | Updates SHOW/I_S privilege logic to use access_t and new merge semantics. |
| sql/sql_prepare.cc | Updates SHOW GRANTS preparation to use new get_show_user signature. |
| sql/sql_parse.h | Updates embedded-access stubs to set access_t privileges. |
| sql/sql_parse.cc | Migrates check_access/show-access flows to access_t and new merge semantics. |
| sql/sql_db.cc | Updates mysql_change_db_impl and USE-db privilege checks for access_t. |
| sql/sql_class.h | Migrates Security_context and THD::col_access to access_t. |
| sql/sql_class.cc | Initializes/sets Security_context privileges via access_t. |
| sql/sql_base.cc | Uses access_t for temporary table privilege checks in callbacks. |
| sql/sql_alter.cc | Migrates alter-table privilege accumulator variable to access_t. |
| sql/sql_acl.h | Updates ACL APIs to return/accept access_t; adds deny-related parameters/state. |
| sql/sp_head.cc | Migrates routine access checks to access_t. |
| sql/set_var.h | Migrates role access storage to access_t. |
| sql/privilege.h | Adds access_t implementation and helpers. |
| sql/lex.h | Adds DENY keyword symbol mapping. |
| sql/json_table.cc | Sets internal function table privileges using access_t. |
| sql/events.cc | Uses access_t::force_allow() for temporary privilege elevation. |
| sql/event_scheduler.cc | Uses access_t for scheduler THD privileges and temporary elevation. |
| sql/event_data_objects.cc | Uses access_t::force_allow() for temporary privilege elevation. |
| plugin/userstat/table_stats.cc | Uses access_t for access checks in userstat table stats. |
| plugin/userstat/index_stats.cc | Uses access_t for access checks in userstat index stats. |
| plugin/feedback/sender_thread.cc | Initializes feedback thread security context using access_t. |
| libmysqld/lib_sql.cc | Initializes embedded server master_access using access_t. |
| mysql-test/main/deny_use_db.test | Adds tests for USE-db behavior with deny-only entries and deny-all. |
| mysql-test/main/deny_use_db.result | Expected output for deny_use_db. |
| mysql-test/main/deny_storage.test | Tests JSON persistence/rewrites of denies in mysql.global_priv. |
| mysql-test/main/deny_storage.result | Expected output for deny_storage. |
| mysql-test/main/deny_show.test | Tests SHOW commands + I_S visibility interactions with DENY. |
| mysql-test/main/deny_show.result | Expected output for deny_show. |
| mysql-test/main/deny_show_grants.test | Tests SHOW GRANTS output/visibility rules for denies (user/role/PUBLIC). |
| mysql-test/main/deny_show_grants.result | Expected output for deny_show_grants. |
| mysql-test/main/deny_routine.test | Tests routine/package DENY semantics and cleanup behaviors. |
| mysql-test/main/deny_routine.result | Expected output for deny_routine. |
| mysql-test/main/deny_role_public.test | Tests deny propagation via role inheritance and PUBLIC. |
| mysql-test/main/deny_role_public.result | Expected output for deny_role_public. |
| mysql-test/main/deny_role_inherit.test | Tests deny-vs-grant resolution through role DAGs. |
| mysql-test/main/deny_role_inherit.result | Expected output for deny_role_inherit. |
| mysql-test/main/deny_revoke.test | Tests REVOKE DENY across scopes and error cases. |
| mysql-test/main/deny_revoke.result | Expected output for deny_revoke. |
| mysql-test/main/deny_restart.test | Tests deny cache injection/persistence across restart. |
| mysql-test/main/deny_restart.result | Expected output for deny_restart. |
| mysql-test/main/deny_metadata.test | Tests DENY effects on metadata commands (SHOW COLUMNS). |
| mysql-test/main/deny_metadata.result | Expected output for deny_metadata. |
| mysql-test/main/deny_lowercase1.test | Tests deny normalization with lower_case_table_names=1. |
| mysql-test/main/deny_lowercase1.result | Expected output for deny_lowercase1. |
| mysql-test/main/deny_lowercase1.opt | MTR option file enabling lowercase table names. |
| mysql-test/main/deny_lowercase0.test | Tests case-sensitive deny behavior with lower_case_table_names=0. |
| mysql-test/main/deny_lowercase0.result | Expected output for deny_lowercase0. |
| mysql-test/main/deny_json_invalid.test | Tests behavior on malformed denies JSON (warning + suppression). |
| mysql-test/main/deny_json_invalid.result | Expected output for deny_json_invalid. |
| mysql-test/main/deny_hierarchy.test | Tests deny precedence across GLOBAL/DB/TABLE/COLUMN hierarchy. |
| mysql-test/main/deny_hierarchy.result | Expected output for deny_hierarchy. |
| mysql-test/main/deny_global.test | Tests global deny semantics across selects/views/show/subqueries/flush. |
| mysql-test/main/deny_global.result | Expected output for deny_global. |
| mysql-test/main/deny_column.test | Tests column-level deny semantics across reads and DML. |
| mysql-test/main/deny_column.result | Expected output for deny_column. |
| mysql-test/main/deny_admin.test | Tests global administrative privileges with DENY. |
| mysql-test/main/deny_admin.result | Expected output for deny_admin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| access_t acc= acl_get_all3(sctx, db_name->str, 0). | ||
| merge_with_parent(sctx->master_access); | ||
| if (!check_access(thd, SELECT_ACL, db_name->str, &thd->col_access, 0,0,1) || | ||
| sctx->master_access & (DB_ACLS | SHOW_DB_ACL) || | ||
| acl_get_all3(sctx, db_name->str, 0)) | ||
| (acc & (DB_ACLS | SHOW_DB_ACL))) | ||
| #endif |
There was a problem hiding this comment.
This database-visibility check uses operator& on access_t, which evaluates certainly_allowed() and therefore also subtracts deny_subtree. For SHOW/I_S listing you typically want to know if any privilege might be available in the schema (even if some tables/columns are denied), so including deny_subtree can incorrectly hide a database when there are lower-scope DENYs. Consider using acc.maybe_allowed(DB_ACLS | SHOW_DB_ACL) (or an equivalent check on allow/deny bits that ignores subtree denies) here.
| | PROXY_SYM ON user FROM user_list | ||
| { | ||
| if (Lex->stmt_revoke_proxy(thd, $3)) | ||
| MYSQL_YYABORT; | ||
| } |
There was a problem hiding this comment.
Because REVOKE DENY uses revoke_privileges, this PROXY_SYM branch makes REVOKE DENY PROXY ... grammatically valid. If DENY is intended only for privilege bits, consider disallowing the PROXY form under REVOKE DENY (separate rule or explicit parse-time error).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 65 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public: | ||
| Sql_cmd_grant(enum_sql_command command) | ||
| :m_command(command) | ||
| { } | ||
| bool is_revoke() const { return m_command == SQLCOM_REVOKE; } | ||
| enum_sql_command sql_command_code() const override { return m_command; } | ||
| void set_deny(bool value) { m_deny= value; } |
There was a problem hiding this comment.
Sql_cmd_grant::m_deny is introduced but never initialized in the constructor. Even if the parser usually calls set_deny() for GRANT/REVOKE privilege statements, leaving it indeterminate risks UB if any execution path reaches m_deny before initialization (e.g., future grammar changes or other constructors). Initialize m_deny in the Sql_cmd_grant ctor initializer list (typically to false).
| --echo # Miscelaneous GRANT/DENY mix, different levels | ||
| --echo # |
There was a problem hiding this comment.
Spelling: “Miscelaneous” should be “Miscellaneous”.
| # Miscelaneous GRANT/DENY mix, different levels | ||
| # |
There was a problem hiding this comment.
Spelling: “Miscelaneous” should be “Miscellaneous” in the expected output as well, otherwise the updated test text and result will remain inconsistent with correct spelling.
Implements DENY/REVOKE DENY and associated tasks.
sanja-byelkin
left a comment
There was a problem hiding this comment.
Generally looks good. But as a big patch has a lot of small things to fix/check.
| (t == PRIV_TYPE_PACKAGE) || (t == PRIV_TYPE_PACKAGE_BODY); | ||
| } | ||
|
|
||
| static inline int cmplen(size_t len1, size_t len2) |
There was a problem hiding this comment.
now we have
static bool streq_safe(const LEX_CSTRING &a, const LEX_CSTRING &b)
{
if (a.length == 0 || b.length == 0)
return a.length == b.length;
return Lex_ident::streq(a, b);
}
it wold be nice to use only one or at least compare similar if comporison is not way more expensive than equality check
if current version left it would be nice to put assert that if pointer is NULL lenght is also 0.
| s2.str, s2.length); | ||
| } | ||
|
|
||
| static int compare_deny_identity( |
There was a problem hiding this comment.
Here we need function comment
| { | ||
| // set_int_value("deny", deny & (longlong) GLOBAL_ACLS); | ||
| privilege_t out_privs; | ||
| update_deny_entry(ACL_PRIV_TYPE::PRIV_TYPE_GLOBAL, NULL, |
There was a problem hiding this comment.
Result of update_deny_entry() ignored
There was a problem hiding this comment.
Yes, and so are the various already existing set_int() in the function, that do a lot of work. That function returns void. I do not know how to improve on that without refactoring parts that are not part of the patch. The only thing I can do is abort(). If you think it is appropriate, I can do.
| /* Change entry now*/ | ||
| if (revoke_all) | ||
| { | ||
| user_table.remove_all_deny_entries(); |
There was a problem hiding this comment.
Result of remove_all_deny_entries() ignored
| for REVOKE. | ||
| */ | ||
| clear_all_denies(combo); | ||
| apply_all_denies_to_caches(user_table, combo); |
There was a problem hiding this comment.
result of apply_all_denies_to_caches() ignored
| } | ||
| else if (rights) | ||
| { | ||
| // set_int_value("deny", deny & (longlong) GLOBAL_ACLS); |
There was a problem hiding this comment.
Why commented out? (maybe remove)?
| { | ||
| table_buf[len]= '\0'; | ||
| entry.table_or_routine= table_buf; | ||
| } |
There was a problem hiding this comment.
Here probably
else
return 1; // error in json_unescape
| { | ||
| GRANT_NAME *grant= (GRANT_NAME *) my_hash_element(hash, i); | ||
| if (!strcmp(grant->user, combo.user.str) && | ||
| !strcmp(safe_str(grant->host.hostname), combo.host.str)) |
There was a problem hiding this comment.
As I remember host names are case insensetive. is above correct comparison?
|
|
||
| out_privs= new_privs; | ||
|
|
||
| if (!first_entry) |
There was a problem hiding this comment.
Can json.length() > 1 replace first_entry?
There was a problem hiding this comment.
It can, but I'd prefer to keep first_entry. Because it is more obvious for the reader. Reader would need to keep in mind that this json has opening bracket '[' at the start, therefore it is 1 and not 0.
Verify that DENY is correctly replicated to slave. Added to rpl suite, since it runs all 3 modes of replication (stmt,mix,row)
Make sure deny_entry_to_json does not fail in memory allocation, by providing StringBuffer with a large enough statically allocated size DENY_JSON_ENTRY_BUFSIZE, currently 1230 bytes. update_deny_entry() - handle json.append() errors
Implements a DENY statement. A DENY explicitly forbids a privilege regardless of any GRANTs at the same or higher scope.
The syntax of DENY is exactly the same as for GRANT (the type of grant that deals with privileges), only the verb needs to be changed (i.e DENY instead of GRANT).
DENYs can be revoked with special "REVOKE DENY" statement, which is analog to "REVOKE" for grants.
Example:
Roles incl PUBLIC support DENY, the semantics are such that DENY from roles are merged with existing privileges, meaning that both current role's and PUBLIC's DENY apply during various privilege checks.
Implementation notes
privilege_t → access_t
The core change is widening the privilege type from a single bitmask to
access_t, which carries three fields:m_allow_bits,m_deny_bits, andm_deny_subtree(summary of denies from lower scopes). operator& and operator~ are overloaded so that existing privilege-check patterns work mostly unchanged; deny bits are simply sticky and propagate through the hierarchy. operator|= and operator=(privilege_t) are deleted to force explicit use of merge_same_level() (roles/PUBLIC) or merge_with_parent() (global→db→table→column).Storage
DENY entries for all scopes are stored as a "denies" JSON array in the user's existing mysql.global_priv row. On DENY / REVOKE DENY the array is rewritten in-place;
REVOKE ALL,GRANT OPTION ON *.*clears it entirely.Cache injection
At server start / FLUSH PRIVILEGES, deny entries are loaded in a second pass over mysql.global_priv in grant_load (skipped entirely when no denies exist) and injected into the in-memory ACL_USER/ACL_DB/GRANT_TABLE / GRANT_NAME caches. Online DENY updates only the affected cache entry; online REVOKE DENY fully rebuilds the user's deny state from JSON.
SHOW GRANTS behavior
Privileged users (with SELECT privilege to
mysql.*) will see own and other's DENY entries inSHOW GRANTSoutput. Currently there is no way for non-privileged users to see own DENYs.