tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Small cleanup of auth queries

Browse Source 
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
This commit is contained in:
Marin Tomic 2018-08-29 09:18:51 +02:00
parent 7b4196cd7c
commit f6a56db19e
12 changed files with 92 additions and 98 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
src/query/frontend/ast/ast.capnp
View File

@ -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;
}

24
src/query/frontend/ast/ast.cpp
View File

@ -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;

6
src/query/frontend/ast/ast.hpp
View File

@ -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
};

21
src/query/frontend/ast/cypher_main_visitor.cpp
View File

@ -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<AuthQuery>();
auth->action_ = AuthQuery::Action::GRANT_ROLE;
auth->role_ = ctx->role->accept(this).as<std::string>();
auth->action_ = AuthQuery::Action::SET_ROLE;
auth->user_ = ctx->user->accept(this).as<std::string>();
auth->role_ = ctx->role->accept(this).as<std::string>();
return auth;
}
/**
* @return AuthQuery*
*/
antlrcpp::Any CypherMainVisitor::visitRevokeRole(
MemgraphCypher::RevokeRoleContext *ctx) {
antlrcpp::Any CypherMainVisitor::visitClearRole(
MemgraphCypher::ClearRoleContext *ctx) {
AuthQuery *auth = storage_.Create<AuthQuery>();
auth->action_ = AuthQuery::Action::REVOKE_ROLE;
auth->role_ = ctx->role->accept(this).as<std::string>();
auth->action_ = AuthQuery::Action::CLEAR_ROLE;
auth->user_ = ctx->user->accept(this).as<std::string>();
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<AuthQuery>();
auth->action_ = AuthQuery::Action::SHOW_GRANTS;
auth->action_ = AuthQuery::Action::SHOW_PRIVILEGES;
auth->user_or_role_ = ctx->userOrRole->accept(this).as<std::string>();
return auth;
}

12
src/query/frontend/ast/cypher_main_visitor.hpp
View File

@ -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*

18
src/query/frontend/opencypher/grammar/MemgraphCypher.g4
View File

@ -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

1
src/query/frontend/opencypher/grammar/MemgraphCypherLexer.g4
View File

@ -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 ;

20
src/query/plan/operator.cpp
View File

@ -3258,7 +3258,7 @@ std::vector<Symbol> 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<Symbol> 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<std::mutex> 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<std::mutex> 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<std::mutex> lock(auth.WithLock());
auto user = auth.GetUser(self_.user_or_role());

12
src/query/plan/operator.lcp
View File

@ -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)

2
tests/integration/auth/checker.cpp
View File

@ -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) {

14
tests/integration/auth/runner.py
View File

@ -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([

54
tests/unit/cypher_main_visitor.cpp
View File

@ -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<TypeParam>("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<TypeParam>("SET ROLE FOR user TO role",
AuthQuery::Action::SET_ROLE, "user", "role", "",
{}, {});
check_auth_query<TypeParam>("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<TypeParam>("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<TypeParam>("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<TypeParam>("SHOW GRANTS FOR user",
AuthQuery::Action::SHOW_GRANTS, "", "", "user",
TYPED_TEST(CypherMainVisitorTest, ShowPrivileges) {
ASSERT_THROW(TypeParam("SHOW PRIVILEGES FOR"), SyntaxException);
check_auth_query<TypeParam>("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<TypeParam>("SHOW ROLE FOR USER user",
ASSERT_THROW(TypeParam("SHOW ROLE FOR "), SyntaxException);
check_auth_query<TypeParam>("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<TypeParam>("SHOW USERS FOR ROLE role",
ASSERT_THROW(TypeParam("SHOW USERS FOR "), SyntaxException);
check_auth_query<TypeParam>("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) {