Correct revoke for the partially granted rights. #61115
Conversation
robot-clickhouse
added
the
pr-improvement
|
This is an automated comment for commit 10e91c5 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
|
| @@ -178,6 +178,21 @@ namespace | |||
| elements_to_revoke.emplace_back(std::move(element_to_revoke)); | |||
| } | |||
There was a problem hiding this comment.
Above your code in this function you can see the code starting with the comment:
/// Special case for the command REVOKE: it's possible that the current user doesn't have
/// the access granted with GRANT OPTION but it's still ok because the roles or users
/// from whom the access rights will be revoked don't have the specified access granted either.
///
/// For example, to execute
/// GRANT ALL ON mydb.* TO role1
/// REVOKE ALL ON *.* FROM role1
/// the current user needs to have the grants only on the 'mydb' database.
Why isn't it enough?
There was a problem hiding this comment.
Because there is no access element that can atomically represent GRANT ALL ON *.*; REVOKE ALL ON mydb.*;
Therefore, if we try to execute REVOKE ALL ON *.* from the user with the exact same rights, element_to_revoke will contain only REVOKE ALL ON *.* and the request will fail.
There was a problem hiding this comment.
Thank you for the explanation, I see it now.
| /// | ||
| /// the query `REVOKE SELECT ON *.* FROM user1` executed by user2 should succeed. | ||
| if (current_user_access.getAccessRights()->containsWithGrantOption(access_to_revoke)) | ||
| return; |
There was a problem hiding this comment.
What is the point of calling current_user_access.checkGrantOption(elements_to_revoke) at the end of this function now? I mean it seems you've already checked everything in containsWithGrantOption().
There was a problem hiding this comment.
Actually, the only reason is that I want to reuse the nice exception message from the checkGrantOption function.
There was a problem hiding this comment.
Let's move throwing of this nice exception message to a separate function then maybe?
|
We definitely need a test for that. |
| @@ -178,6 +178,21 @@ namespace | |||
| elements_to_revoke.emplace_back(std::move(element_to_revoke)); | |||
| } | |||
|
|
|||
| /// Additional check for REVOKE | |||
There was a problem hiding this comment.
It feels logically more right for this code to appear a bit earlier - before we calculate elements_to_revoke.
| else | ||
| { | ||
| if (!child->contains(node)) | ||
| return false; |
There was a problem hiding this comment.
When you'll be adding a test please make sure you add test cases for all these paths of evaluation.
Oops, I had written some tests but forgot to |
|
AST fuzzer - #56640 |
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Improvements for the access checks, allowing to revoke of unpossessed rights in case the target user doesn't have the revoking grants either.
Example:
In the example above, both
user1anduser2don't have theSELECT ON system.*permission, but theREVOKE ALLquery will succeed.Closes #58837
Documentation entry for user-facing changes