Add CHECK GRANT query #68885

Unalian · 2024-08-26T12:51:42Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add CHECK GRANT query to check whether the current user/role has been granted the specific privilege and whether the corresponding table/column exists in the memory.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing):

Allow: All Required Checks
Allow: Stateless tests
Allow: Stateful tests
Allow: Integration Tests
Allow: Performance tests
Allow: All Builds
Allow: batch 1, 2 for multi-batch jobs
Allow: batch 3, 4, 5, 6 for multi-batch jobs

Exclude: Style check
Exclude: Fast test
Exclude: All with ASAN
Exclude: All with TSAN, MSAN, UBSAN, Coverage
Exclude: All with aarch64, release, debug

Run only fuzzers related jobs (libFuzzer fuzzers, AST fuzzers, etc.)
Exclude: AST fuzzers

robot-ch-test-poll · 2024-08-26T15:03:00Z

This is an automated comment for commit 1f7be09 with description of existing statuses. It's updated for the latest CI running

✅ Click here to open a full report in a separate page

Successful checks

Check name	Description	Status
Builds	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

Unalian · 2024-09-09T12:09:21Z

Please check the pr. The stateless test failure seems to be not related to my codes. @vitlibar @alexey-milovidov

vitlibar · 2024-09-13T11:20:58Z

docs/en/sql-reference/statements/check-grant.md

+title: "CHECK GRANT Statement"
+---
+
+The `CHECK GRANT` query is used to check whether the current user/role has been granted a specific privilege, and whether the corresponding table/column exists in the memory.


@Unalian Why do you think CHECK GRANT should check that the corresponding table/column exists in the memory?

@Unalian Why do you think CHECK GRANT should check that the corresponding table/column exists in the memory?

Depends on the describe of the issue.

For example, if a table doesn't exist then

DROP TABLE IF EXISTS db.mytable

still works, and that requires privilege GRANT DROP TABLE ON db.mytable.

But what is the point of checking the table's existence in CHECK GRANTS in this case?

CHECK GRANT DROP TABLE ON db.mytable

Perhaps we need either a setting to enable/disable checking existence in CHECK GRANT or just separate commands to check grants only or to check existence only. We already have the EXISTS TABLE command, so I think we can have EXISTS COLUMNS as well.

For example, if a table doesn't exist then

DROP TABLE IF EXISTS db.mytable

still works, and that requires privilege GRANT DROP TABLE ON db.mytable.

But what is the point of checking

CHECK GRANT DROP TABLE ON db.mytable

in this case?

CHECK GRANT DROP TABLE ON db.mytable

will return if the user has privilege to DROP TABLE ON db.mytable and the db.mytable exists.

CHECK GRANT DROP TABLE ON db.mytable
will return if the user has privilege to DROP TABLE ON db.mytable and the db.mytable exists

Yes, but this is not what some users might expect from command CHECK GRANT DROP TABLE ON db.mytable in this case.

Perhaps we need either a setting to enable/disable checking existence in CHECK GRANT or separate commands to check grants only or to check existence only. We already have the EXISTS TABLE command, so I think we can have EXISTS COLUMNS as well.

I got your point. Separating the commands to check grants only or to check existence only may be better. Actually, that is what I am thinking about during coding(#67772 (comment)). I will change the logic here (remove the existence check in check grant). Then, maybe add EXISTS COLUMNS just like EXISTS TABLE as well.

vitlibar · 2024-09-23T11:27:36Z

src/Interpreters/Access/InterpreterCheckGrantQuery.cpp

+                if (!founded)
+                {
+                    /// Column not found.
+                    return executeQuery("SELECT 0 AS CHECK_GRANT", getContext(), QueryFlags{.internal = true}).second;


Calling executeQuery is a quite heavy way to just return 0 as a result.

It's much better to just build the result block here from scratch - in the same way how it's built in InterpreterExistsQuery:

return QueryPipeline(std::make_shared<SourceFromSingleChunk>(Block{{ ColumnUInt8::create(1, result), std::make_shared<DataTypeUInt8>(), "result" }}));

Calling executeQuery is a quite heavy way to just return 0 as a result.

It's much better to just build the result block here from scratch - in the same way how it's built in InterpreterExistsQuery:

return QueryPipeline(std::make_shared<SourceFromSingleChunk>(Block{{ ColumnUInt8::create(1, result), std::make_shared<DataTypeUInt8>(), "result" }}));

Got it.

…certain access rights elements && the elements exist).

…nt; Update the way to return result.

vitlibar · 2024-10-01T11:41:42Z

src/Interpreters/Access/InterpreterCheckGrantQuery.cpp

+    }
+    BlockIO res;
+    res.pipeline = QueryPipeline(
+        std::make_shared<SourceFromSingleChunk>(Block{{ColumnUInt8::create(1, 1), std::make_shared<DataTypeUInt8>(), "result"}}));


You can use user_is_granted right in the arguments of ColumnUInt8::create(), there is no need to repeat the same code again:

BlockIO res; res.pipeline = QueryPipeline( std::make_shared<SourceFromSingleChunk>(Block{{ColumnUInt8::create(1, user_is_granted), std::make_shared<DataTypeUInt8>(), "result"}}));

vitlibar · 2024-10-01T11:43:56Z

src/Interpreters/Access/InterpreterCheckGrantQuery.cpp

+    AccessRightsElements elements_to_check_grant;
+    collectAccessRightsElementsToGrantOrRevoke(query, elements_to_check_grant);


These two lines can be replaced with just one:

Suggested change

AccessRightsElements elements_to_check_grant;

collectAccessRightsElementsToGrantOrRevoke(query, elements_to_check_grant);

const AccessRightsElements & elements_to_check_grant = query.access_rights_elements;

and function collectAccessRightsElementsToGrantOrRevoke is not needed in this file.

…ateless-test case about check grant wildcard

vitlibar · 2024-10-22T10:51:40Z

src/Parsers/Access/ASTCheckGrantQuery.cpp

+
+    settings.ostr << " ";
+
+    formatElementsWithoutOptions(access_rights_elements, settings);


Why can't you use

settings.ostr << access_rights_element.toString();

here instead of calling this function formatElementsWithoutOptions() which looks like a lot of copy-paste?

Why can't you use

settings.ostr << access_rights_element.toString();

here instead of calling this function formatElementsWithoutOptions() which looks like a lot of copy-paste?

The function does some format works in formatFunction(). Suppose I use settings.ostr << access_rights_element.toString(); here, such query's format will be like (which is created by Parser):

:) check grant select(col,col2) on tb, alter on tb2 CHECK GRANT SELECT(col, col2) ON tb, GRANT ALTER TABLE, ALTER VIEW ON tb2 Query id: 5f3927b0-8d99-4db2-a702-67e53955476c ┌─result─┐ 1. │ 1 │ └────────┘ 1 row in set. Elapsed: 0.001 sec.

However, the correct response should be just like GRANT keyword:

:) check grant select(col,col2) on tb, alter on tb2 CHECK GRANT SELECT(col, col2) ON tb, ALTER TABLE, ALTER VIEW ON tb2 Query id: b48aabc2-2b6e-45ed-a36a-7802a00e3852 ┌─result─┐ 1. │ 1 │ └────────┘ 1 row in set. Elapsed: 0.001 sec.

I will pollish the formatFunction here.

The main problem with your functions is that they look like a lot of copy-pasted code which isn't good. Perhaps you could get rid of at least formatColumnNames & formatONClause?

vitlibar · 2024-10-22T10:58:16Z

src/Parsers/Access/ParserCheckGrantQuery.cpp

+    }
+
+    bool parseAccessFlagsWithColumns(IParser::Pos & pos, Expected & expected,
+                                     std::vector<std::pair<AccessFlags, Strings>> & access_and_columns)


Please move common functions such as parseAccessFlags, parseColumnNames, parseAccessFlagsWithColumns, parseElements to some separate source file (src/Parser/Access/parseAccessRightsElements.h/cpp), where both ParserCheckGrantQuery.cpp and ParserGrantQuery.cpp could use it.

Please move common functions such as parseAccessFlags, parseColumnNames, parseAccessFlagsWithColumns, parseElements to some separate source file (src/Parser/Access/parseAccessRightsElements.h/cpp), where both ParserCheckGrantQuery.cpp and ParserGrantQuery.cpp could use it.

Got it.

I decided to that by myself
#72103

I decided to that by myself #72103

I decided to that by myself #72103

Amazing! I'm so sorry I haven't updated the code in time because I've been so busy lately :(. Thank you for your work.

alexey-milovidov · 2025-01-10T00:21:45Z

@Unalian @vitlibar, a bug exists #74414

nikitamikhaylov added the can be tested Allows running workflows for external contributors label Aug 26, 2024

robot-ch-test-poll added the pr-feature Pull request with new product feature label Aug 26, 2024

vitlibar self-assigned this Sep 6, 2024

Unalian force-pushed the feat-67772 branch from 579f9d1 to 9f8a298 Compare September 8, 2024 08:19

vitlibar reviewed Sep 13, 2024

View reviewed changes

vitlibar reviewed Sep 23, 2024

View reviewed changes

Unalian added 13 commits September 30, 2024 11:48

[feature] Add 'check grant' to see if (current user has been granted …

394317b

…certain access rights elements && the elements exist).

[test] Add test case

bae1aef

[fix] Add formatImpl in AST

c290085

[doc] Add doc

75dc880

[fix] Fix include format

a7d6935

[fix] Fix style check

26cab16

[Fix] Fix code style

a5d977b

[Update] Use stateless test instead of integration test

639560b

[rm] Remove integration test

636a5cc

[Fix] Add source env build in stateless test

61f2a2d

[Update] Remove check for existance of tables/columns, just check gra…

9e44636

…nt; Update the way to return result.

[Update] Add wildcard check for check grant

f41c60d

[Test] Add test case for check grant

de6f2b0

Unalian force-pushed the feat-67772 branch from 7385042 to de6f2b0 Compare October 1, 2024 07:40

vitlibar reviewed Oct 1, 2024

View reviewed changes

Unalian added 2 commits October 2, 2024 01:19

[Perf] Perf the codes for check grant

e522977

[Doc/Test] Update doc about check grant wildcard; Add more case in st…

1f7be09

…ateless-test case about check grant wildcard

Unalian force-pushed the feat-67772 branch from 5159071 to 1f7be09 Compare October 2, 2024 00:11

vitlibar reviewed Oct 22, 2024

View reviewed changes

vitlibar approved these changes Nov 19, 2024

View reviewed changes

vitlibar added this pull request to the merge queue Nov 19, 2024

Merged via the queue into ClickHouse:master with commit 353ff95 Nov 19, 2024
196 of 198 checks passed

vitlibar mentioned this pull request Nov 19, 2024

Get rid of code duplication after adding CHECK GRANT #72103

Merged

robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Nov 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CHECK GRANT query #68885

Add CHECK GRANT query #68885

Unalian commented Aug 26, 2024

robot-ch-test-poll commented Aug 26, 2024 •

edited by robot-clickhouse-ci-2

Loading

Unalian commented Sep 9, 2024

vitlibar Sep 13, 2024

Unalian Sep 13, 2024

vitlibar Sep 17, 2024 •

edited

Loading

vitlibar Sep 17, 2024 •

edited

Loading

Unalian Sep 17, 2024

vitlibar Sep 17, 2024

Unalian Sep 17, 2024 •

edited

Loading

vitlibar Sep 23, 2024 •

edited

Loading

Unalian Sep 23, 2024

vitlibar Oct 1, 2024

vitlibar Oct 1, 2024

vitlibar Oct 22, 2024

Unalian Oct 22, 2024

vitlibar Oct 23, 2024 •

edited

Loading

vitlibar Oct 22, 2024 •

edited

Loading

Unalian Oct 22, 2024

vitlibar Nov 19, 2024

Unalian Nov 19, 2024

alexey-milovidov commented Jan 10, 2025

		AccessRightsElements elements_to_check_grant;
		collectAccessRightsElementsToGrantOrRevoke(query, elements_to_check_grant);

	AccessRightsElements elements_to_check_grant;
	collectAccessRightsElementsToGrantOrRevoke(query, elements_to_check_grant);
	const AccessRightsElements & elements_to_check_grant = query.access_rights_elements;


		settings.ostr << " ";

		formatElementsWithoutOptions(access_rights_elements, settings);

Add CHECK GRANT query #68885

Add CHECK GRANT query #68885

Conversation

Unalian commented Aug 26, 2024

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

CI Settings (Only check the boxes if you know what you are doing):

robot-ch-test-poll commented Aug 26, 2024 • edited by robot-clickhouse-ci-2 Loading

Unalian commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitlibar Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

vitlibar Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Unalian Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

vitlibar Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitlibar Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

vitlibar Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexey-milovidov commented Jan 10, 2025

robot-ch-test-poll commented Aug 26, 2024 •

edited by robot-clickhouse-ci-2

Loading

vitlibar Sep 17, 2024 •

edited

Loading

vitlibar Sep 17, 2024 •

edited

Loading

Unalian Sep 17, 2024 •

edited

Loading

vitlibar Sep 23, 2024 •

edited

Loading

vitlibar Oct 23, 2024 •

edited

Loading

vitlibar Oct 22, 2024 •

edited

Loading