From f6a56db19ea985d34d7416404d52d68ac5b03f33 Mon Sep 17 00:00:00 2001 From: Marin Tomic Date: Wed, 29 Aug 2018 09:18:51 +0200 Subject: [PATCH] Small cleanup of auth queries Summary: Changed GRANT ROLE to SET ROLE. Now it is `SET ROLE FOR user TO ROLE` instead of `GRANT ROLE role TO user`. It makes more sense because our users can only have 1 role. Changed REVOKE ROLE to CLEAR ROLE. Now it is `CLEAR ROLE FOR user` instead of `REVOKE ROLE role FOR user`. REVOKE ROLE would throw exception if user was not a member of role. CLEAR ROLE clears the role whatever it is. I find that the latter makes more sense combined with SET ROLE. Changed `SHOW ROLE FOR USER user` to `SHOW ROLE FOR user`. Changed `SHOW USERS FOR ROLE role` to `SHOW USERS FOR role`. Reviewers: mferencevic, teon.banek, buda Reviewed By: mferencevic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D1572 --- src/query/frontend/ast/ast.capnp | 6 +-- src/query/frontend/ast/ast.cpp | 24 ++++----- src/query/frontend/ast/ast.hpp | 6 +-- .../frontend/ast/cypher_main_visitor.cpp | 21 ++++---- .../frontend/ast/cypher_main_visitor.hpp | 12 ++--- .../opencypher/grammar/MemgraphCypher.g4 | 18 +++---- .../opencypher/grammar/MemgraphCypherLexer.g4 | 1 + src/query/plan/operator.cpp | 20 +++---- src/query/plan/operator.lcp | 12 ++--- tests/integration/auth/checker.cpp | 2 +- tests/integration/auth/runner.py | 14 ++--- tests/unit/cypher_main_visitor.cpp | 54 ++++++++++--------- 12 files changed, 92 insertions(+), 98 deletions(-) diff --git a/src/query/frontend/ast/ast.capnp b/src/query/frontend/ast/ast.capnp index 70b76ea28..60fcc206c 100644 --- a/src/query/frontend/ast/ast.capnp +++ b/src/query/frontend/ast/ast.capnp @@ -407,12 +407,12 @@ struct AuthQuery { setPassword @4; dropUser @5; showUsers @6; - grantRole @7; - revokeRole @8; + setRole @7; + clearRole @8; grantPrivilege @9; denyPrivilege @10; revokePrivilege @11; - showGrants @12; + showPrivileges @12; showRoleForUser @13; showUsersForRole @14; } diff --git a/src/query/frontend/ast/ast.cpp b/src/query/frontend/ast/ast.cpp index 01b63f55b..2c4aa1a87 100644 --- a/src/query/frontend/ast/ast.cpp +++ b/src/query/frontend/ast/ast.cpp @@ -2099,11 +2099,11 @@ void AuthQuery::Save(capnp::AuthQuery::Builder *builder, case Action::SHOW_USERS: builder->setAction(capnp::AuthQuery::Action::SHOW_USERS); break; - case Action::GRANT_ROLE: - builder->setAction(capnp::AuthQuery::Action::GRANT_ROLE); + case Action::SET_ROLE: + builder->setAction(capnp::AuthQuery::Action::SET_ROLE); break; - case Action::REVOKE_ROLE: - builder->setAction(capnp::AuthQuery::Action::REVOKE_ROLE); + case Action::CLEAR_ROLE: + builder->setAction(capnp::AuthQuery::Action::CLEAR_ROLE); break; case Action::GRANT_PRIVILEGE: builder->setAction(capnp::AuthQuery::Action::GRANT_PRIVILEGE); @@ -2114,8 +2114,8 @@ void AuthQuery::Save(capnp::AuthQuery::Builder *builder, case Action::REVOKE_PRIVILEGE: builder->setAction(capnp::AuthQuery::Action::REVOKE_PRIVILEGE); break; - case Action::SHOW_GRANTS: - builder->setAction(capnp::AuthQuery::Action::SHOW_GRANTS); + case Action::SHOW_PRIVILEGES: + builder->setAction(capnp::AuthQuery::Action::SHOW_PRIVILEGES); break; case Action::SHOW_ROLE_FOR_USER: builder->setAction(capnp::AuthQuery::Action::SHOW_ROLE_FOR_USER); @@ -2192,11 +2192,11 @@ void AuthQuery::Load(const capnp::Tree::Reader &base_reader, case capnp::AuthQuery::Action::SHOW_USERS: action_ = Action::SHOW_USERS; break; - case capnp::AuthQuery::Action::GRANT_ROLE: - action_ = Action::GRANT_ROLE; + case capnp::AuthQuery::Action::SET_ROLE: + action_ = Action::SET_ROLE; break; - case capnp::AuthQuery::Action::REVOKE_ROLE: - action_ = Action::REVOKE_ROLE; + case capnp::AuthQuery::Action::CLEAR_ROLE: + action_ = Action::CLEAR_ROLE; break; case capnp::AuthQuery::Action::GRANT_PRIVILEGE: action_ = Action::GRANT_PRIVILEGE; @@ -2207,8 +2207,8 @@ void AuthQuery::Load(const capnp::Tree::Reader &base_reader, case capnp::AuthQuery::Action::REVOKE_PRIVILEGE: action_ = Action::REVOKE_PRIVILEGE; break; - case capnp::AuthQuery::Action::SHOW_GRANTS: - action_ = Action::SHOW_GRANTS; + case capnp::AuthQuery::Action::SHOW_PRIVILEGES: + action_ = Action::SHOW_PRIVILEGES; break; case capnp::AuthQuery::Action::SHOW_ROLE_FOR_USER: action_ = Action::SHOW_ROLE_FOR_USER; diff --git a/src/query/frontend/ast/ast.hpp b/src/query/frontend/ast/ast.hpp index c3883e844..669428d7d 100644 --- a/src/query/frontend/ast/ast.hpp +++ b/src/query/frontend/ast/ast.hpp @@ -2356,12 +2356,12 @@ class AuthQuery : public Clause { SET_PASSWORD, DROP_USER, SHOW_USERS, - GRANT_ROLE, - REVOKE_ROLE, + SET_ROLE, + CLEAR_ROLE, GRANT_PRIVILEGE, DENY_PRIVILEGE, REVOKE_PRIVILEGE, - SHOW_GRANTS, + SHOW_PRIVILEGES, SHOW_ROLE_FOR_USER, SHOW_USERS_FOR_ROLE }; diff --git a/src/query/frontend/ast/cypher_main_visitor.cpp b/src/query/frontend/ast/cypher_main_visitor.cpp index cee8fd84a..43abecd10 100644 --- a/src/query/frontend/ast/cypher_main_visitor.cpp +++ b/src/query/frontend/ast/cypher_main_visitor.cpp @@ -365,23 +365,22 @@ antlrcpp::Any CypherMainVisitor::visitShowUsers( /** * @return AuthQuery* */ -antlrcpp::Any CypherMainVisitor::visitGrantRole( - MemgraphCypher::GrantRoleContext *ctx) { +antlrcpp::Any CypherMainVisitor::visitSetRole( + MemgraphCypher::SetRoleContext *ctx) { AuthQuery *auth = storage_.Create(); - auth->action_ = AuthQuery::Action::GRANT_ROLE; - auth->role_ = ctx->role->accept(this).as(); + auth->action_ = AuthQuery::Action::SET_ROLE; auth->user_ = ctx->user->accept(this).as(); + auth->role_ = ctx->role->accept(this).as(); return auth; } /** * @return AuthQuery* */ -antlrcpp::Any CypherMainVisitor::visitRevokeRole( - MemgraphCypher::RevokeRoleContext *ctx) { +antlrcpp::Any CypherMainVisitor::visitClearRole( + MemgraphCypher::ClearRoleContext *ctx) { AuthQuery *auth = storage_.Create(); - auth->action_ = AuthQuery::Action::REVOKE_ROLE; - auth->role_ = ctx->role->accept(this).as(); + auth->action_ = AuthQuery::Action::CLEAR_ROLE; auth->user_ = ctx->user->accept(this).as(); return auth; } @@ -463,10 +462,10 @@ antlrcpp::Any CypherMainVisitor::visitPrivilege( /** * @return AuthQuery* */ -antlrcpp::Any CypherMainVisitor::visitShowGrants( - MemgraphCypher::ShowGrantsContext *ctx) { +antlrcpp::Any CypherMainVisitor::visitShowPrivileges( + MemgraphCypher::ShowPrivilegesContext *ctx) { AuthQuery *auth = storage_.Create(); - auth->action_ = AuthQuery::Action::SHOW_GRANTS; + auth->action_ = AuthQuery::Action::SHOW_PRIVILEGES; auth->user_or_role_ = ctx->userOrRole->accept(this).as(); return auth; } diff --git a/src/query/frontend/ast/cypher_main_visitor.hpp b/src/query/frontend/ast/cypher_main_visitor.hpp index 3b10922a9..38843482b 100644 --- a/src/query/frontend/ast/cypher_main_visitor.hpp +++ b/src/query/frontend/ast/cypher_main_visitor.hpp @@ -135,7 +135,8 @@ class CypherMainVisitor : public antlropencypher::MemgraphCypherBaseVisitor { /** * @return Query* */ - antlrcpp::Any visitExplainQuery(MemgraphCypher::ExplainQueryContext *ctx) override; + antlrcpp::Any visitExplainQuery( + MemgraphCypher::ExplainQueryContext *ctx) override; /** * @return Query* @@ -235,13 +236,12 @@ class CypherMainVisitor : public antlropencypher::MemgraphCypherBaseVisitor { /** * @return AuthQuery* */ - antlrcpp::Any visitGrantRole(MemgraphCypher::GrantRoleContext *ctx) override; + antlrcpp::Any visitSetRole(MemgraphCypher::SetRoleContext *ctx) override; /** * @return AuthQuery* */ - antlrcpp::Any visitRevokeRole( - MemgraphCypher::RevokeRoleContext *ctx) override; + antlrcpp::Any visitClearRole(MemgraphCypher::ClearRoleContext *ctx) override; /** * @return AuthQuery* @@ -269,8 +269,8 @@ class CypherMainVisitor : public antlropencypher::MemgraphCypherBaseVisitor { /** * @return AuthQuery* */ - antlrcpp::Any visitShowGrants( - MemgraphCypher::ShowGrantsContext *ctx) override; + antlrcpp::Any visitShowPrivileges( + MemgraphCypher::ShowPrivilegesContext *ctx) override; /** * @return AuthQuery* diff --git a/src/query/frontend/opencypher/grammar/MemgraphCypher.g4 b/src/query/frontend/opencypher/grammar/MemgraphCypher.g4 index 0e7b1a303..c11908966 100644 --- a/src/query/frontend/opencypher/grammar/MemgraphCypher.g4 +++ b/src/query/frontend/opencypher/grammar/MemgraphCypher.g4 @@ -11,13 +11,13 @@ memgraphCypherKeyword : cypherKeyword | AUTH | BATCH | BATCHES + | CLEAR | DATA | DENY | DROP | FOR | FROM | GRANT - | GRANTS | IDENTIFIED | INTERVAL | K_TEST @@ -60,12 +60,12 @@ authQuery : createRole | setPassword | dropUser | showUsers - | grantRole - | revokeRole + | setRole + | clearRole | grantPrivilege | denyPrivilege | revokePrivilege - | showGrants + | showPrivileges | showRoleForUser | showUsersForRole ; @@ -87,9 +87,9 @@ dropUser : DROP USER user=userOrRoleName ; showUsers : SHOW USERS ; -grantRole : GRANT ROLE role=userOrRoleName TO user=userOrRoleName ; +setRole : SET ROLE FOR user=userOrRoleName TO role=userOrRoleName; -revokeRole : REVOKE ROLE role=userOrRoleName FROM user=userOrRoleName ; +clearRole : CLEAR ROLE FOR user=userOrRoleName ; grantPrivilege : GRANT ( ALL PRIVILEGES | privileges=privilegeList ) TO userOrRole=userOrRoleName ; @@ -102,11 +102,11 @@ privilege : CREATE | DELETE | MATCH | MERGE | SET privilegeList : privilege ( ',' privilege )* ; -showGrants : SHOW GRANTS FOR userOrRole=userOrRoleName ; +showPrivileges : SHOW PRIVILEGES FOR userOrRole=userOrRoleName ; -showRoleForUser : SHOW ROLE FOR USER user=userOrRoleName ; +showRoleForUser : SHOW ROLE FOR user=userOrRoleName ; -showUsersForRole : SHOW USERS FOR ROLE role=userOrRoleName ; +showUsersForRole : SHOW USERS FOR role=userOrRoleName ; streamQuery : createStream | dropStream diff --git a/src/query/frontend/opencypher/grammar/MemgraphCypherLexer.g4 b/src/query/frontend/opencypher/grammar/MemgraphCypherLexer.g4 index 7154141ca..9accae45f 100644 --- a/src/query/frontend/opencypher/grammar/MemgraphCypherLexer.g4 +++ b/src/query/frontend/opencypher/grammar/MemgraphCypherLexer.g4 @@ -14,6 +14,7 @@ ALTER : A L T E R ; AUTH : A U T H ; BATCH : B A T C H ; BATCHES : B A T C H E S ; +CLEAR : C L E A R ; DATA : D A T A ; DENY : D E N Y ; DROP : D R O P ; diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index fff022bbd..69e7d77c0 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -3258,7 +3258,7 @@ std::vector AuthHandler::OutputSymbols(const SymbolTable &) const { case AuthQuery::Action::SHOW_ROLE_FOR_USER: return {role_symbol_}; - case AuthQuery::Action::SHOW_GRANTS: + case AuthQuery::Action::SHOW_PRIVILEGES: return {privilege_symbol_, effective_symbol_, details_symbol_}; case AuthQuery::Action::CREATE_USER: @@ -3266,8 +3266,8 @@ std::vector AuthHandler::OutputSymbols(const SymbolTable &) const { case AuthQuery::Action::SET_PASSWORD: case AuthQuery::Action::CREATE_ROLE: case AuthQuery::Action::DROP_ROLE: - case AuthQuery::Action::GRANT_ROLE: - case AuthQuery::Action::REVOKE_ROLE: + case AuthQuery::Action::SET_ROLE: + case AuthQuery::Action::CLEAR_ROLE: case AuthQuery::Action::GRANT_PRIVILEGE: case AuthQuery::Action::DENY_PRIVILEGE: case AuthQuery::Action::REVOKE_PRIVILEGE: @@ -3447,7 +3447,7 @@ class AuthHandlerCursor : public Cursor { return true; } - case AuthQuery::Action::GRANT_ROLE: { + case AuthQuery::Action::SET_ROLE: { std::lock_guard lock(auth.WithLock()); auto user = auth.GetUser(self_.user()); if (!user) { @@ -3467,20 +3467,12 @@ class AuthHandlerCursor : public Cursor { return false; } - case AuthQuery::Action::REVOKE_ROLE: { + case AuthQuery::Action::CLEAR_ROLE: { std::lock_guard lock(auth.WithLock()); auto user = auth.GetUser(self_.user()); if (!user) { throw QueryRuntimeException("User '{}' doesn't exist!", self_.user()); } - auto role = auth.GetRole(self_.role()); - if (!role) { - throw QueryRuntimeException("Role '{}' doesn't exist!", self_.role()); - } - if (user->role() != role) { - throw QueryRuntimeException("User '{}' isn't a member of role '{}'!", - self_.user(), self_.role()); - } user->ClearRole(); auth.SaveUser(*user); return false; @@ -3529,7 +3521,7 @@ class AuthHandlerCursor : public Cursor { return false; } - case AuthQuery::Action::SHOW_GRANTS: { + case AuthQuery::Action::SHOW_PRIVILEGES: { if (!grants_) { std::lock_guard lock(auth.WithLock()); auto user = auth.GetUser(self_.user_or_role()); diff --git a/src/query/plan/operator.lcp b/src/query/plan/operator.lcp index 428015424..de6c90aa4 100644 --- a/src/query/plan/operator.lcp +++ b/src/query/plan/operator.lcp @@ -1978,19 +1978,19 @@ and returns true, once.") "AuthQuery::Action" '(create-role drop-role show-roles create-user set-password - drop-user show-users grant-role - revoke-role grant-privilege + drop-user show-users set-role + clear-role grant-privilege deny-privilege revoke-privilege - show-grants show-role-for-user + show-privileges show-role-for-user show-users-for-role)) :capnp-load (lcp:capnp-load-enum "::query::capnp::AuthQuery::Action" "AuthQuery::Action" '(create-role drop-role show-roles create-user set-password - drop-user show-users grant-role - revoke-role grant-privilege + drop-user show-users set-role + clear-role grant-privilege deny-privilege revoke-privilege - show-grants show-role-for-user + show-privileges show-role-for-user show-users-for-role))) (user "std::string" :reader t) (role "std::string" :reader t) diff --git a/tests/integration/auth/checker.cpp b/tests/integration/auth/checker.cpp index e8a789337..8d6294d89 100644 --- a/tests/integration/auth/checker.cpp +++ b/tests/integration/auth/checker.cpp @@ -33,7 +33,7 @@ int main(int argc, char **argv) { } try { - auto ret = client.Execute("SHOW GRANTS FOR user", {}); + auto ret = client.Execute("SHOW PRIVILEGES FOR user", {}); const auto &records = ret.records; uint64_t count_got = 0; for (const auto &record : records) { diff --git a/tests/integration/auth/runner.py b/tests/integration/auth/runner.py index 9385a5190..1e6e0a63c 100755 --- a/tests/integration/auth/runner.py +++ b/tests/integration/auth/runner.py @@ -110,11 +110,11 @@ QUERIES = [ ("AUTH",) ), ( - "GRANT ROLE test_role TO test_user", + "SET ROLE FOR test_user TO test_role", ("AUTH",) ), ( - "REVOKE ROLE test_role FROM test_user", + "CLEAR ROLE FOR test_user", ("AUTH",) ), ( @@ -130,15 +130,15 @@ QUERIES = [ ("AUTH",) ), ( - "SHOW GRANTS FOR test_user", + "SHOW PRIVILEGES FOR test_user", ("AUTH",) ), ( - "SHOW ROLE FOR USER test_user", + "SHOW ROLE FOR test_user", ("AUTH",) ), ( - "SHOW USERS FOR ROLE test_role", + "SHOW USERS FOR test_role", ("AUTH",) ), @@ -299,9 +299,9 @@ def execute_test(memgraph_binary, tester_binary, checker_binary): user_perm, ", role ", role_perm, "user mapped to role:", mapped, " ~~\033[0m") if mapped: - execute_admin_queries(["GRANT ROLE role TO user"]) + execute_admin_queries(["SET ROLE FOR user TO role"]) else: - execute_admin_queries(["REVOKE ROLE role FROM user"]) + execute_admin_queries(["CLEAR ROLE FOR user"]) user_prep = "FROM" if user_perm == "REVOKE" else "TO" role_prep = "FROM" if role_perm == "REVOKE" else "TO" execute_admin_queries([ diff --git a/tests/unit/cypher_main_visitor.cpp b/tests/unit/cypher_main_visitor.cpp index df39d83e6..15c51731c 100644 --- a/tests/unit/cypher_main_visitor.cpp +++ b/tests/unit/cypher_main_visitor.cpp @@ -1971,24 +1971,26 @@ TYPED_TEST(CypherMainVisitorTest, ShowUsers) { "", "", {}, {}); } -TYPED_TEST(CypherMainVisitorTest, GrantRole) { - ASSERT_THROW(TypeParam("GRANT ROLE"), SyntaxException); - ASSERT_THROW(TypeParam("GRANT ROLE role"), SyntaxException); - ASSERT_THROW(TypeParam("GRANT ROLE role TO"), SyntaxException); - ASSERT_THROW(TypeParam("GRANT ROLE TO user"), SyntaxException); - check_auth_query("GRANT ROLE role TO user", - AuthQuery::Action::GRANT_ROLE, "user", "role", "", +TYPED_TEST(CypherMainVisitorTest, SetRole) { + ASSERT_THROW(TypeParam("SET ROLE"), SyntaxException); + ASSERT_THROW(TypeParam("SET ROLE user"), SyntaxException); + ASSERT_THROW(TypeParam("SET ROLE FOR user"), SyntaxException); + ASSERT_THROW(TypeParam("SET ROLE FOR user TO"), SyntaxException); + check_auth_query("SET ROLE FOR user TO role", + AuthQuery::Action::SET_ROLE, "user", "role", "", + {}, {}); + check_auth_query("SET ROLE FOR user TO null", + AuthQuery::Action::SET_ROLE, "user", "null", "", {}, {}); } -TYPED_TEST(CypherMainVisitorTest, RevokeRole) { - ASSERT_THROW(TypeParam("REVOKE ROLE"), SyntaxException); - ASSERT_THROW(TypeParam("REVOKE ROLE role"), SyntaxException); - ASSERT_THROW(TypeParam("REVOKE ROLE role FROM"), SyntaxException); - ASSERT_THROW(TypeParam("REVOKE ROLE FROM user"), SyntaxException); - check_auth_query("REVOKE ROLE role FROM user", - AuthQuery::Action::REVOKE_ROLE, "user", "role", - "", {}, {}); +TYPED_TEST(CypherMainVisitorTest, ClearRole) { + ASSERT_THROW(TypeParam("CLEAR ROLE"), SyntaxException); + ASSERT_THROW(TypeParam("CLEAR ROLE user"), SyntaxException); + ASSERT_THROW(TypeParam("CLEAR ROLE FOR user TO"), SyntaxException); + check_auth_query("CLEAR ROLE FOR user", + AuthQuery::Action::CLEAR_ROLE, "user", "", "", + {}, {}); } TYPED_TEST(CypherMainVisitorTest, GrantPrivilege) { @@ -2042,28 +2044,28 @@ TYPED_TEST(CypherMainVisitorTest, RevokePrivilege) { AuthQuery::Privilege::STREAM}); } -TYPED_TEST(CypherMainVisitorTest, ShowGrants) { - ASSERT_THROW(TypeParam("SHOW GRANTS FOR"), SyntaxException); - check_auth_query("SHOW GRANTS FOR user", - AuthQuery::Action::SHOW_GRANTS, "", "", "user", +TYPED_TEST(CypherMainVisitorTest, ShowPrivileges) { + ASSERT_THROW(TypeParam("SHOW PRIVILEGES FOR"), SyntaxException); + check_auth_query("SHOW PRIVILEGES FOR user", + AuthQuery::Action::SHOW_PRIVILEGES, "", "", "user", {}, {}); - ASSERT_THROW(TypeParam("SHOW GRANTS FOR user1, user2"), SyntaxException); + ASSERT_THROW(TypeParam("SHOW PRIVILEGES FOR user1, user2"), SyntaxException); } TYPED_TEST(CypherMainVisitorTest, ShowRoleForUser) { - ASSERT_THROW(TypeParam("SHOW ROLE FOR USER"), SyntaxException); - check_auth_query("SHOW ROLE FOR USER user", + ASSERT_THROW(TypeParam("SHOW ROLE FOR "), SyntaxException); + check_auth_query("SHOW ROLE FOR user", AuthQuery::Action::SHOW_ROLE_FOR_USER, "user", "", "", {}, {}); - ASSERT_THROW(TypeParam("SHOW ROLE FOR USER user1, user2"), SyntaxException); + ASSERT_THROW(TypeParam("SHOW ROLE FOR user1, user2"), SyntaxException); } TYPED_TEST(CypherMainVisitorTest, ShowUsersForRole) { - ASSERT_THROW(TypeParam("SHOW USERS FOR ROLE"), SyntaxException); - check_auth_query("SHOW USERS FOR ROLE role", + ASSERT_THROW(TypeParam("SHOW USERS FOR "), SyntaxException); + check_auth_query("SHOW USERS FOR role", AuthQuery::Action::SHOW_USERS_FOR_ROLE, "", "role", "", {}, {}); - ASSERT_THROW(TypeParam("SHOW USERS FOR ROLE role1, role2"), SyntaxException); + ASSERT_THROW(TypeParam("SHOW USERS FOR role1, role2"), SyntaxException); } TYPED_TEST(CypherMainVisitorTest, CreateStream) {