Skip to content

Allow specifying REVOKE after GRANT in the same statement #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
joelynch wants to merge 1 commit into
base: v25.3.7.194-lts-aiven
Choose a base branch
from
Draft

Allow specifying REVOKE after GRANT in the same statement #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 60 additions & 25 deletions src/Interpreters/Access/InterpreterGrantQuery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,17 @@ namespace ServerSetting
namespace
{
/// Extracts access rights elements which are going to be granted or revoked from a query.
/// elements_to_revoke: applied BEFORE the grant (for REPLACE: revoke ALL, or standalone REVOKE).
/// elements_to_revoke_after_grant: applied AFTER the grant (combined syntax revokes → creates partial revokes).
void collectAccessRightsElementsToGrantOrRevoke(
const ASTGrantQuery & query,
AccessRightsElements & elements_to_grant,
AccessRightsElements & elements_to_revoke)
AccessRightsElements & elements_to_revoke,
AccessRightsElements & elements_to_revoke_after_grant)
{
elements_to_grant.clear();
elements_to_revoke.clear();
elements_to_revoke_after_grant.clear();

if (query.is_revoke)
{
Expand All @@ -57,11 +61,15 @@ namespace
/// GRANT WITH REPLACE OPTION
elements_to_grant = query.access_rights_elements;
elements_to_revoke.emplace_back(AccessType::ALL);
/// Explicit revokes from combined syntax are applied after the grant
elements_to_revoke_after_grant = query.access_rights_elements_to_revoke;
}
else
{
/// GRANT
/// GRANT (possibly with embedded REVOKE)
elements_to_grant = query.access_rights_elements;
/// Combined syntax revokes are applied after the grant to create partial revokes
elements_to_revoke_after_grant = query.access_rights_elements_to_revoke;
}
}

Expand Down Expand Up @@ -135,13 +143,19 @@ namespace
const std::vector<UUID> & grantees_from_query,
bool & need_check_grantees_are_allowed,
const AccessRightsElements & elements_to_grant,
AccessRightsElements & elements_to_revoke)
AccessRightsElements & elements_to_revoke,
AccessRightsElements & elements_to_revoke_after_grant)
{
/// Check access rights which are going to be granted.
/// To execute the command GRANT the current user needs to have the access granted with GRANT OPTION.
current_user_access.checkGrantOption(elements_to_grant);

if (current_user_access.hasGrantOption(elements_to_revoke))
/// Combine both revoke lists for permission checking.
AccessRightsElements all_elements_to_revoke;
all_elements_to_revoke.insert(all_elements_to_revoke.end(), elements_to_revoke.begin(), elements_to_revoke.end());
all_elements_to_revoke.insert(all_elements_to_revoke.end(), elements_to_revoke_after_grant.begin(), elements_to_revoke_after_grant.end());

if (current_user_access.hasGrantOption(all_elements_to_revoke))
{
/// Simple case: the current user has the grant option for all the access rights specified for REVOKE.
return;
Expand Down Expand Up @@ -175,20 +189,20 @@ namespace

need_check_grantees_are_allowed = false; /// already checked

if (!elements_to_revoke.empty() && elements_to_revoke[0].is_partial_revoke)
std::for_each(elements_to_revoke.begin(), elements_to_revoke.end(), [&](AccessRightsElement & element) { element.is_partial_revoke = false; });
if (!all_elements_to_revoke.empty() && all_elements_to_revoke[0].is_partial_revoke)
std::for_each(all_elements_to_revoke.begin(), all_elements_to_revoke.end(), [&](AccessRightsElement & element) { element.is_partial_revoke = false; });
AccessRights access_to_revoke;
access_to_revoke.grant(elements_to_revoke);
access_to_revoke.grant(all_elements_to_revoke);
access_to_revoke.makeIntersection(all_granted_access);

/// Build more accurate list of elements to revoke, now we use an intersection of the initial list of elements to revoke
/// and all the granted access rights to these grantees.
bool grant_option = !elements_to_revoke.empty() && elements_to_revoke[0].grant_option;
elements_to_revoke.clear();
bool grant_option = !all_elements_to_revoke.empty() && all_elements_to_revoke[0].grant_option;
all_elements_to_revoke.clear();
for (auto & element_to_revoke : access_to_revoke.getElements())
{
if (!element_to_revoke.is_partial_revoke && (element_to_revoke.grant_option || !grant_option))
elements_to_revoke.emplace_back(std::move(element_to_revoke));
all_elements_to_revoke.emplace_back(std::move(element_to_revoke));
}

/// Additional check for REVOKE
Expand All @@ -207,7 +221,7 @@ namespace
return;

/// Technically, this check always fails if `containsWithGrantOption` returns `false`. But we still call it to get a nice exception message.
current_user_access.checkGrantOption(elements_to_revoke);
current_user_access.checkGrantOption(all_elements_to_revoke);
}

/// Checks if the current user has enough roles granted with admin option to grant or revoke specified roles.
Expand Down Expand Up @@ -278,10 +292,11 @@ namespace
/// This function is less accurate than checkGrantOption() because it cannot use any information about
/// access rights the grantees currently have (due to those grantees are located on multiple nodes,
/// we just don't have the full information about them).
AccessRightsElements getRequiredAccessForExecutingOnCluster(const AccessRightsElements & elements_to_grant, const AccessRightsElements & elements_to_revoke)
AccessRightsElements getRequiredAccessForExecutingOnCluster(const AccessRightsElements & elements_to_grant, const AccessRightsElements & elements_to_revoke, const AccessRightsElements & elements_to_revoke_after_grant)
{
auto required_access = elements_to_grant;
required_access.insert(required_access.end(), elements_to_revoke.begin(), elements_to_revoke.end());
required_access.insert(required_access.end(), elements_to_revoke_after_grant.begin(), elements_to_revoke_after_grant.end());
std::for_each(required_access.begin(), required_access.end(), [&](AccessRightsElement & element) { element.grant_option = true; });
return required_access;
}
Expand Down Expand Up @@ -320,16 +335,23 @@ namespace
T & grantee,
const AccessRightsElements & elements_to_grant,
const AccessRightsElements & elements_to_revoke,
const AccessRightsElements & elements_to_revoke_after_grant,
const std::vector<UUID> & roles_to_grant,
const RolesOrUsersSet & roles_to_revoke,
bool admin_option)
{
/// Step 1: Pre-grant revoke (for REPLACE: revoke ALL, or standalone REVOKE).
if (!elements_to_revoke.empty())
grantee.access.revoke(elements_to_revoke);

/// Step 2: Grant.
if (!elements_to_grant.empty())
grantee.access.grant(elements_to_grant);

/// Step 3: Post-grant revoke (for combined GRANT ... REVOKE ... syntax → creates partial revokes).
if (!elements_to_revoke_after_grant.empty())
grantee.access.revoke(elements_to_revoke_after_grant);

if (!roles_to_revoke.empty())
{
if (admin_option)
Expand All @@ -352,38 +374,44 @@ namespace
IAccessEntity & grantee,
const AccessRightsElements & elements_to_grant,
const AccessRightsElements & elements_to_revoke,
const AccessRightsElements & elements_to_revoke_after_grant,
const std::vector<UUID> & roles_to_grant,
const RolesOrUsersSet & roles_to_revoke,
bool admin_option)
{
if (auto * user = typeid_cast<User *>(&grantee))
updateGrantedAccessRightsAndRolesTemplate(*user, elements_to_grant, elements_to_revoke, roles_to_grant, roles_to_revoke, admin_option);
updateGrantedAccessRightsAndRolesTemplate(*user, elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant, roles_to_grant, roles_to_revoke, admin_option);
else if (auto * role = typeid_cast<Role *>(&grantee))
updateGrantedAccessRightsAndRolesTemplate(*role, elements_to_grant, elements_to_revoke, roles_to_grant, roles_to_revoke, admin_option);
updateGrantedAccessRightsAndRolesTemplate(*role, elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant, roles_to_grant, roles_to_revoke, admin_option);
}

template <typename T>
void grantCurrentGrantsTemplate(
T & grantee,
const AccessRights & rights_to_grant,
const AccessRightsElements & elements_to_revoke)
const AccessRightsElements & elements_to_revoke,
const AccessRightsElements & elements_to_revoke_after_grant)
{
if (!elements_to_revoke.empty())
grantee.access.revoke(elements_to_revoke);

grantee.access.makeUnion(rights_to_grant);

if (!elements_to_revoke_after_grant.empty())
grantee.access.revoke(elements_to_revoke_after_grant);
}

/// Grants current user's grants with grant options to specified user.
void grantCurrentGrants(
IAccessEntity & grantee,
const AccessRights & new_rights,
const AccessRightsElements & elements_to_revoke)
const AccessRightsElements & elements_to_revoke,
const AccessRightsElements & elements_to_revoke_after_grant)
{
if (auto * user = typeid_cast<User *>(&grantee))
grantCurrentGrantsTemplate(*user, new_rights, elements_to_revoke);
grantCurrentGrantsTemplate(*user, new_rights, elements_to_revoke, elements_to_revoke_after_grant);
else if (auto * role = typeid_cast<Role *>(&grantee))
grantCurrentGrantsTemplate(*role, new_rights, elements_to_revoke);
grantCurrentGrantsTemplate(*role, new_rights, elements_to_revoke, elements_to_revoke_after_grant);
}

/// Calculates all available rights to grant with current user intersection.
Expand Down Expand Up @@ -415,13 +443,14 @@ namespace
{
AccessRightsElements elements_to_grant;
AccessRightsElements elements_to_revoke;
collectAccessRightsElementsToGrantOrRevoke(query, elements_to_grant, elements_to_revoke);
AccessRightsElements elements_to_revoke_after_grant;
collectAccessRightsElementsToGrantOrRevoke(query, elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant);

std::vector<UUID> roles_to_grant;
RolesOrUsersSet roles_to_revoke;
collectRolesToGrantOrRevoke(query, roles_to_grant, roles_to_revoke);

updateGrantedAccessRightsAndRoles(grantee, elements_to_grant, elements_to_revoke, roles_to_grant, roles_to_revoke, query.admin_option);
updateGrantedAccessRightsAndRoles(grantee, elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant, roles_to_grant, roles_to_revoke, query.admin_option);
}
}

Expand All @@ -433,11 +462,14 @@ BlockIO InterpreterGrantQuery::execute()

query.replaceCurrentUserTag(getContext()->getUserName());
query.access_rights_elements.eraseNotGrantable();
query.access_rights_elements_to_revoke.eraseNotGrantable();

if (!query.access_rights_elements.sameOptions())
throw Exception(ErrorCodes::LOGICAL_ERROR, "Elements of an ASTGrantQuery are expected to have the same options");
if (!query.access_rights_elements.empty() && query.access_rights_elements[0].is_partial_revoke && !query.is_revoke)
throw Exception(ErrorCodes::LOGICAL_ERROR, "A partial revoke should be revoked, not granted");
if (!query.access_rights_elements_to_revoke.sameOptions())
throw Exception(ErrorCodes::LOGICAL_ERROR, "Revoke elements of an ASTGrantQuery are expected to have the same options");

auto & access_control = getContext()->getAccessControl();
auto current_user_access = getContext()->getAccess();
Expand All @@ -447,7 +479,8 @@ BlockIO InterpreterGrantQuery::execute()
/// Collect access rights and roles we're going to grant or revoke.
AccessRightsElements elements_to_grant;
AccessRightsElements elements_to_revoke;
collectAccessRightsElementsToGrantOrRevoke(query, elements_to_grant, elements_to_revoke);
AccessRightsElements elements_to_revoke_after_grant;
collectAccessRightsElementsToGrantOrRevoke(query, elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant);

std::vector<UUID> roles_to_grant;
RolesOrUsersSet roles_to_revoke;
Expand All @@ -457,15 +490,17 @@ BlockIO InterpreterGrantQuery::execute()
String current_database = getContext()->getCurrentDatabase();
elements_to_grant.replaceEmptyDatabase(current_database);
elements_to_revoke.replaceEmptyDatabase(current_database);
elements_to_revoke_after_grant.replaceEmptyDatabase(current_database);
query.access_rights_elements.replaceEmptyDatabase(current_database);
query.access_rights_elements_to_revoke.replaceEmptyDatabase(current_database);

/// Executing on cluster.
if (!query.cluster.empty())
{
if (query.current_grants)
throw Exception(ErrorCodes::BAD_ARGUMENTS, "GRANT CURRENT GRANTS can't be executed on cluster.");

auto required_access = getRequiredAccessForExecutingOnCluster(elements_to_grant, elements_to_revoke);
auto required_access = getRequiredAccessForExecutingOnCluster(elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant);
checkAdminOptionForExecutingOnCluster(*current_user_access, roles_to_grant, roles_to_revoke);
current_user_access->checkGranteesAreAllowed(grantees);
DDLQueryOnClusterParams params;
Expand All @@ -476,7 +511,7 @@ BlockIO InterpreterGrantQuery::execute()
/// Check if the current user has corresponding access rights granted with grant option.
bool need_check_grantees_are_allowed = true;
if (!query.current_grants)
checkGrantOption(access_control, *current_user_access, grantees, need_check_grantees_are_allowed, elements_to_grant, elements_to_revoke);
checkGrantOption(access_control, *current_user_access, grantees, need_check_grantees_are_allowed, elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant);

/// Check if the current user has corresponding roles granted with admin option.
checkAdminOption(access_control, *current_user_access, grantees, need_check_grantees_are_allowed, roles_to_grant, roles_to_revoke, query.admin_option);
Expand Down Expand Up @@ -543,9 +578,9 @@ BlockIO InterpreterGrantQuery::execute()
current_user_access->checkAccess(AccessType::PROTECTED_ACCESS_MANAGEMENT);
auto clone = entity->clone();
if (query.current_grants)
grantCurrentGrants(*clone, new_rights, elements_to_revoke);
grantCurrentGrants(*clone, new_rights, elements_to_revoke, elements_to_revoke_after_grant);
else
updateGrantedAccessRightsAndRoles(*clone, elements_to_grant, elements_to_revoke, roles_to_grant, roles_to_revoke, query.admin_option);
updateGrantedAccessRightsAndRoles(*clone, elements_to_grant, elements_to_revoke, elements_to_revoke_after_grant, roles_to_grant, roles_to_revoke, query.admin_option);
return clone;
};

Expand Down
60 changes: 60 additions & 0 deletions src/Interpreters/Access/InterpreterShowGrantsQuery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,60 @@ namespace ErrorCodes

namespace
{
/// Merges standalone REVOKE ASTGrantQuery nodes (partial revokes) into a preceding
/// GRANT ASTGrantQuery node's access_rights_elements_to_revoke field, producing
/// the combined "GRANT ... REVOKE ... TO ..." syntax.
void mergeRevokeIntoGrant(ASTs & grant_queries)
{
/// Find the last GRANT node for each grant_option value (false / true).
/// Partial revokes that share the same grant_option are merged into it.
std::shared_ptr<ASTGrantQuery> last_grant_no_option;
std::shared_ptr<ASTGrantQuery> last_grant_with_option;

ASTs merged;
merged.reserve(grant_queries.size());

for (auto & ast : grant_queries)
{
auto & query = ast->as<ASTGrantQuery &>();

if (!query.is_revoke)
{
/// It's a GRANT node — track it as a merge target.
bool go = !query.access_rights_elements.empty() && query.access_rights_elements[0].grant_option;
if (go)
last_grant_with_option = std::static_pointer_cast<ASTGrantQuery>(ast);
else
last_grant_no_option = std::static_pointer_cast<ASTGrantQuery>(ast);
merged.push_back(ast);
}
else
{
/// It's a REVOKE node (partial revoke). Try to merge into a preceding GRANT.
bool go = !query.access_rights_elements.empty() && query.access_rights_elements[0].grant_option;
auto & target = go ? last_grant_with_option : last_grant_no_option;

if (target)
{
/// Merge: move the revoke elements into the GRANT node's revoke list.
for (auto & elem : query.access_rights_elements)
{
elem.is_partial_revoke = false; /// Clear the flag — it's now an explicit revoke in the combined syntax
target->access_rights_elements_to_revoke.emplace_back(std::move(elem));
}
/// Don't add this REVOKE node to merged — it's absorbed.
}
else
{
/// No preceding GRANT to merge into — keep the standalone REVOKE.
merged.push_back(ast);
}
}
}

grant_queries = std::move(merged);
}

void getGrantsFromAccess(
ASTs & res,
const AccessRights & access,
Expand Down Expand Up @@ -69,6 +123,12 @@ namespace

current_query->access_rights_elements.emplace_back(std::move(element));
}

/// Post-processing: merge partial-revoke REVOKE nodes into preceding GRANT nodes
/// to produce combined "GRANT ... REVOKE ... TO ..." output.
/// The elements are sorted: grants before partial-revokes (for the same full_name),
/// and alphabetically by full_name. So a GRANT ON *.* always precedes REVOKE ON system.*.
mergeRevokeIntoGrant(res);
}

template <typename T>
Expand Down
10 changes: 10 additions & 0 deletions src/Parsers/Access/ASTGrantQuery.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ void ASTGrantQuery::formatImpl(WriteBuffer & ostr, const FormatSettings & settin
throw Exception(ErrorCodes::LOGICAL_ERROR, "Elements of an ASTGrantQuery are expected to have the same options");
if (!access_rights_elements.empty() && access_rights_elements[0].is_partial_revoke && !is_revoke)
throw Exception(ErrorCodes::LOGICAL_ERROR, "A partial revoke should be revoked, not granted");
if (!access_rights_elements_to_revoke.empty() && !access_rights_elements_to_revoke.sameOptions())
throw Exception(ErrorCodes::LOGICAL_ERROR, "Revoke elements of an ASTGrantQuery are expected to have the same options");
bool grant_option = !access_rights_elements.empty() && access_rights_elements[0].grant_option;

formatOnCluster(ostr, settings);
Expand Down Expand Up @@ -82,6 +84,13 @@ void ASTGrantQuery::formatImpl(WriteBuffer & ostr, const FormatSettings & settin
access_rights_elements.formatElementsWithoutOptions(ostr, settings.hilite);
}

/// Format the embedded REVOKE clause (combined GRANT ... REVOKE ... TO ... syntax)
if (!access_rights_elements_to_revoke.empty())
{
ostr << " " << (settings.hilite ? hilite_keyword : "") << "REVOKE" << (settings.hilite ? hilite_none : "") << " ";
access_rights_elements_to_revoke.formatElementsWithoutOptions(ostr, settings.hilite);
}

ostr << (settings.hilite ? IAST::hilite_keyword : "") << (is_revoke ? " FROM " : " TO ")
<< (settings.hilite ? IAST::hilite_none : "");
grantees->format(ostr, settings);
Expand All @@ -102,6 +111,7 @@ void ASTGrantQuery::formatImpl(WriteBuffer & ostr, const FormatSettings & settin
void ASTGrantQuery::replaceEmptyDatabase(const String & current_database)
{
access_rights_elements.replaceEmptyDatabase(current_database);
access_rights_elements_to_revoke.replaceEmptyDatabase(current_database);
}


Expand Down
Loading