You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "empiredan (via GitHub)" <gi...@apache.org> on 2023/06/14 07:29:45 UTC
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
empiredan commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1228941458
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -216,9 +216,9 @@ void ranger_resource_policy_manager::start()
std::chrono::milliseconds(1));
}
-bool ranger_resource_policy_manager::allowed(const int rpc_code,
- const std::string &user_name,
- const std::string &database_name)
+access_control_result ranger_resource_policy_manager::allowed(const int rpc_code,
+ const std::string &user_name,
+ const std::string &database_name)
Review Comment:
Could be declared as `const`, while both `_global_policies_lock` and `_database_policies_lock` could be declared `mutable`.
##########
src/runtime/test/ranger_resource_policy_manager_test.cpp:
##########
@@ -193,27 +193,142 @@ TEST(ranger_resource_policy_manager_test, ranger_resource_policy_serialized_test
{
access_type ac_type;
std::string user_name;
- bool expected_result;
- } tests[] = {{access_type::kRead, "user", false}, {access_type::kRead, "user1", true},
- {access_type::kWrite, "user1", true}, {access_type::kCreate, "user1", false},
- {access_type::kDrop, "user1", false}, {access_type::kList, "user1", true},
- {access_type::kMetadata, "user1", false}, {access_type::kControl, "user1", false},
- {access_type::kRead, "user2", true}, {access_type::kWrite, "user2", false},
- {access_type::kCreate, "user2", false}, {access_type::kDrop, "user2", false},
- {access_type::kList, "user2", true}, {access_type::kMetadata, "user2", false},
- {access_type::kControl, "user2", false}, {access_type::kRead, "user3", false},
- {access_type::kWrite, "user3", false}, {access_type::kCreate, "user3", false},
- {access_type::kDrop, "user3", false}, {access_type::kList, "user3", true},
- {access_type::kMetadata, "user3", false}, {access_type::kControl, "user3", false},
- {access_type::kRead, "user4", true}, {access_type::kWrite, "user4", false},
- {access_type::kCreate, "user4", false}, {access_type::kDrop, "user4", false},
- {access_type::kList, "user4", true}, {access_type::kMetadata, "user4", false},
- {access_type::kControl, "user4", false}};
+ policy_check_type check_type;
+ policy_check_status expected_result;
+ } tests[] = {
Review Comment:
How about providing some simple description for each case ?
##########
src/runtime/ranger/ranger_resource_policy_manager.h:
##########
@@ -74,9 +74,10 @@ class ranger_resource_policy_manager
// When using Ranger for ACL, periodically pull policies from Ranger service.
void start();
- // Return true if the 'user_name' is allowed to access 'database_name' via 'rpc_code'.
- bool
- allowed(const int rpc_code, const std::string &user_name, const std::string &database_name);
+ // Return 'access_control_result::kAllowed' if the 'user_name' is allowed to access
+ // 'database_name' via 'rpc_code'.
+ access_control_result
+ allowed(const int rpc_code, const std::string &user_name, const std::string &app_name);
Review Comment:
Why did `database_name` changed to `app_name` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@pegasus.apache.org
For additional commands, e-mail: dev-help@pegasus.apache.org