You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pegasus.apache.org by "WHBANG (via GitHub)" <gi...@apache.org> on 2023/06/07 03:58:13 UTC
[GitHub] [incubator-pegasus] WHBANG opened a new pull request, #1518: Dev/ranger fix
WHBANG opened a new pull request, #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518
### What problem does this PR solve? <!--add issue link with summary if exists-->
https://github.com/apache/incubator-pegasus/issues/1054
### What is changed and how does it work?
This patch fixes the judgment logic when ranger matches policies:
1. Traverse all resource policies
1.1. If the current policy matches deny_condition
1.1.1. does not match any deny_exclude, returns kDenied, and the traversal ends
1.1.2. A deny_exclude is matched, return kPending, and continue to the next policy judgment
1.2. No policy is matched or the return value is kPending, enter 2
2. Traverse all resource policies again
2.1. If the current policy matches allow_condition
2.1.1. does not match any allow_exclude, returns kAllowed, and the traversal ends
2.1.2. An allow_exclude is matched, return kPending, and continue to the next policy judgment
2.2 If the return value is kPending, it will return kDenied
3. dose not match any policy, return kDenied
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1230359291
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,250 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kAllow>(const access_type &ac_type,
+ const std::string &user_name) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ return do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ ac_type, user_name, allow_policies, allow_policies_exclude);
+}
+
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kDeny>(const access_type &ac_type,
+ const std::string &user_name) const
+{
+ return do_policies_check<policy_check_type::kDeny, policy_check_status::kDenied>(
+ ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+template <>
+policy_check_status
+acl_policies::do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
Review Comment:
Upadate the comments.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,250 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kAllow>(const access_type &ac_type,
+ const std::string &user_name) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ return do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ ac_type, user_name, allow_policies, allow_policies_exclude);
+}
+
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kDeny>(const access_type &ac_type,
+ const std::string &user_name) const
+{
+ return do_policies_check<policy_check_type::kDeny, policy_check_status::kDenied>(
+ ac_type, user_name, deny_policies, deny_policies_exclude);
Review Comment:
Same.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,250 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kAllow>(const access_type &ac_type,
+ const std::string &user_name) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ return do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ ac_type, user_name, allow_policies, allow_policies_exclude);
+}
+
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kDeny>(const access_type &ac_type,
+ const std::string &user_name) const
+{
+ return do_policies_check<policy_check_type::kDeny, policy_check_status::kDenied>(
+ ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+template <>
+policy_check_status
+acl_policies::do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
Review Comment:
Same.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,250 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kAllow>(const access_type &ac_type,
+ const std::string &user_name) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ return do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ ac_type, user_name, allow_policies, allow_policies_exclude);
Review Comment:
Now it's not needed to pass `allow_policies` and `allow_policies_exclude`
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,250 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kAllow>(const access_type &ac_type,
+ const std::string &user_name) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ return do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ ac_type, user_name, allow_policies, allow_policies_exclude);
+}
+
+template <>
+policy_check_status
+acl_policies::policies_check<policy_check_type::kDeny>(const access_type &ac_type,
+ const std::string &user_name) const
+{
+ return do_policies_check<policy_check_type::kDeny, policy_check_status::kDenied>(
+ ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+template <>
+policy_check_status
+acl_policies::do_policies_check<policy_check_type::kAllow, policy_check_status::kAllowed>(
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
Review Comment:
Use `allow_policies` and `allow_policies_exclude` directly.
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591223336
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591029527
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591537600
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1592285949
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "WHBANG (via GitHub)" <gi...@apache.org>.
WHBANG commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1226893569
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ // This table matches a policy.
+ matched_database_table_policy database_table_policy(
+ {database_name, table_name, policy.policies});
+ // This table matches the policy whose database is "*".
+ if (policy.database_names.count(database_name) == 0) {
Review Comment:
In this case, the table has already matched the `policy`, so `policy.database_names` must include `database_name` or "*", so if there is no `database_name` in the `policy`, it must be matched by "*".
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591217235
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591543231
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1229132213
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
- continue;
- }
- for (const auto &allow_policy_exclude : allow_policies_exclude) {
- // 2.2. In some 'allow_policies_exclude', it's not allowed.
- if (allow_policy_exclude.match(ac_type, user_name)) {
- return false;
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ auto check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any policy in current resource.
+ check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
+ }
+
+ // The check that does not match any policy in current reosource returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_resource_policy(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ if (match_database_type::kNeed == md_type) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
}
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
Review Comment:
If check_status equals to kDenied, check_type must be kDeny, right? If it is, refact the code like:
```
if (policy_check_status::kDenied == check_status) {
CHECK_TRUE(policy_check_type::kDeny == check_type);
return access_control_result::kDenied;
}
```
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
Review Comment:
This function can be templated too, make both policy_check_type and policy_check_status as template parameters?
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
Review Comment:
How about using template functions:
```
// in .h file
template <policy_check_type check_type>
policy_check_status policies_check(...);
// in .cpp file
template<>
policy_check_status acl_policies::policies_check<policy_check_type::kAllow>(...)
{
return do_policies_check(policy_check_type::kAllow, ac_type, user_name, allow_policies, allow_policies_exclude);
}
template<>
policy_check_status acl_policies::policies_check<policy_check_type::kDeny>(...)
{
return do_policies_check(policy_check_type::kAllow, ac_type, user_name, deny_policies, deny_policies_exclude);
}
// calling
auto check_status = policy.policies.policies_check<check_type>(ac_type, user_name);
```
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
- continue;
- }
- for (const auto &allow_policy_exclude : allow_policies_exclude) {
- // 2.2. In some 'allow_policies_exclude', it's not allowed.
- if (allow_policy_exclude.match(ac_type, user_name)) {
- return false;
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ auto check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any policy in current resource.
+ check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
+ }
+
+ // The check that does not match any policy in current reosource returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_resource_policy(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ if (match_database_type::kNeed == md_type) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
}
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
}
- // 2.3. Not in any 'allow_policies_exclude', it's allowed.
- return true;
+ // When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
+ // 'allow_policies_exclude'.
+ if (policy_check_type::kAllow == check_type &&
Review Comment:
Same to above.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
- continue;
- }
- for (const auto &allow_policy_exclude : allow_policies_exclude) {
- // 2.2. In some 'allow_policies_exclude', it's not allowed.
- if (allow_policy_exclude.match(ac_type, user_name)) {
- return false;
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ auto check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any policy in current resource.
+ check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
+ }
+
+ // The check that does not match any policy in current reosource returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_resource_policy(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ if (match_database_type::kNeed == md_type) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
}
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
}
- // 2.3. Not in any 'allow_policies_exclude', it's allowed.
- return true;
+ // When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
+ // 'allow_policies_exclude'.
+ if (policy_check_type::kAllow == check_type &&
+ policy_check_status::kAllowed == check_status) {
+ return access_control_result::kAllowed;
+ }
+ // In a 'policies' and in a 'policies_exclude' or not match.
+ if (policy_check_status::kPending == check_status ||
+ policy_check_status::kNotMatched == check_status) {
+ continue;
+ }
+ }
+ return access_control_result::kPending;
+}
+
+access_control_result check_ranger_database_table_policy_allowed(
+ const std::vector<matched_database_table_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name)
+{
+ // Check if it is denied by any DATABASE_TABLE policy.
+ auto check_res = do_check_ranger_database_table_policy(
+ policies, ac_type, user_name, policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any DATABASE_TABLE policy.
+ check_res = do_check_ranger_database_table_policy(
+ policies, ac_type, user_name, policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
}
- // 3. Otherwise, it's not allowed.
- return false;
+ // The check that does not match any DATABASE_TABLE policy returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_database_table_policy(const std::vector<matched_database_table_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
+ }
+ // When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
+ // 'allow_policies_exclude'.
+ if (policy_check_type::kAllow == check_type &&
+ policy_check_status::kAllowed == check_status) {
+ return access_control_result::kAllowed;
+ }
+ // In a 'policies' and in a 'policies_exclude' or not match.
+ if (policy_check_status::kPending == check_status ||
Review Comment:
Use CHECK_TRUE as above.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
- continue;
- }
- for (const auto &allow_policy_exclude : allow_policies_exclude) {
- // 2.2. In some 'allow_policies_exclude', it's not allowed.
- if (allow_policy_exclude.match(ac_type, user_name)) {
- return false;
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ auto check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any policy in current resource.
+ check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
+ }
+
+ // The check that does not match any policy in current reosource returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_resource_policy(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ if (match_database_type::kNeed == md_type) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
}
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
}
- // 2.3. Not in any 'allow_policies_exclude', it's allowed.
- return true;
+ // When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
+ // 'allow_policies_exclude'.
+ if (policy_check_type::kAllow == check_type &&
+ policy_check_status::kAllowed == check_status) {
+ return access_control_result::kAllowed;
+ }
+ // In a 'policies' and in a 'policies_exclude' or not match.
+ if (policy_check_status::kPending == check_status ||
+ policy_check_status::kNotMatched == check_status) {
+ continue;
+ }
+ }
+ return access_control_result::kPending;
+}
+
+access_control_result check_ranger_database_table_policy_allowed(
+ const std::vector<matched_database_table_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name)
+{
+ // Check if it is denied by any DATABASE_TABLE policy.
+ auto check_res = do_check_ranger_database_table_policy(
+ policies, ac_type, user_name, policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any DATABASE_TABLE policy.
+ check_res = do_check_ranger_database_table_policy(
+ policies, ac_type, user_name, policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
}
- // 3. Otherwise, it's not allowed.
- return false;
+ // The check that does not match any DATABASE_TABLE policy returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_database_table_policy(const std::vector<matched_database_table_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
+ }
+ // When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
+ // 'allow_policies_exclude'.
+ if (policy_check_type::kAllow == check_type &&
+ policy_check_status::kAllowed == check_status) {
+ return access_control_result::kAllowed;
+ }
Review Comment:
Templatize the code as above.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
- continue;
- }
- for (const auto &allow_policy_exclude : allow_policies_exclude) {
- // 2.2. In some 'allow_policies_exclude', it's not allowed.
- if (allow_policy_exclude.match(ac_type, user_name)) {
- return false;
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ auto check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any policy in current resource.
+ check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
+ }
+
+ // The check that does not match any policy in current reosource returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_resource_policy(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ if (match_database_type::kNeed == md_type) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
}
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
}
- // 2.3. Not in any 'allow_policies_exclude', it's allowed.
- return true;
+ // When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
+ // 'allow_policies_exclude'.
+ if (policy_check_type::kAllow == check_type &&
+ policy_check_status::kAllowed == check_status) {
+ return access_control_result::kAllowed;
+ }
+ // In a 'policies' and in a 'policies_exclude' or not match.
+ if (policy_check_status::kPending == check_status ||
+ policy_check_status::kNotMatched == check_status) {
+ continue;
+ }
Review Comment:
Here check_status must be kPending or kNotMatched, right? refactor as
```
CHECK_TRUE(policy_check_status::kPending == check_status ||
policy_check_status::kNotMatched == check_status);
```
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ // This table matches a policy.
+ matched_database_table_policy database_table_policy(
+ {database_name, table_name, policy.policies});
+ // This table matches the policy whose database is "*".
+ if (policy.database_names.count(database_name) == 0) {
Review Comment:
Let's add a CHECK here, policy.database_names == "*" ?
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ // This table matches a policy.
+ matched_database_table_policy database_table_policy(
+ {database_name, table_name, policy.policies});
+ // This table matches the policy whose database is "*".
+ if (policy.database_names.count(database_name) == 0) {
+ database_table_policy.matched_database_name = "*";
+ }
+ // This table matches the policy whose database table is "*".
+ if (policy.table_names.count(table_name) == 0) {
Review Comment:
Let's add a CHECK here, policy.table_names == "*" ?
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591368064
> Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
I want to disable you.
--
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
[GitHub] [incubator-pegasus] empiredan merged pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
empiredan merged PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591556303
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591219145
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591015023
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] empiredan commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "empiredan (via GitHub)" <gi...@apache.org>.
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1592304990
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591339345
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "WHBANG (via GitHub)" <gi...@apache.org>.
WHBANG commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1226898354
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ // This table matches a policy.
+ matched_database_table_policy database_table_policy(
+ {database_name, table_name, policy.policies});
+ // This table matches the policy whose database is "*".
+ if (policy.database_names.count(database_name) == 0) {
+ database_table_policy.matched_database_name = "*";
+ }
+ // This table matches the policy whose database table is "*".
+ if (policy.table_names.count(table_name) == 0) {
Review Comment:
As described above in database, at least one of `table_name` and * is in the `policy.table_names`.
--
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
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "WHBANG (via GitHub)" <gi...@apache.org>.
WHBANG commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1226893569
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ // This table matches a policy.
+ matched_database_table_policy database_table_policy(
+ {database_name, table_name, policy.policies});
+ // This table matches the policy whose database is "*".
+ if (policy.database_names.count(database_name) == 0) {
Review Comment:
In this case, the table has already matched the `policy`, so `policy.database_names` must include `database_name` or *, so if there is no `database_name` in the `policy`, it must be matched by *.
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1592281335
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1226051323
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +582,45 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
Review Comment:
Add some comments please.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,151 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &policy_exclude : exclude_policies) {
+ if (policy_exclude.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ bool need_match_database,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ for (const auto &policy : policies) {
+ if (need_match_database) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
+ }
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status =
+ policy.policies.policies_check(ac_type, user_name, policy_check_type::kDeny);
+ // In a 'deny_policies' and not in any 'deny_policies_exclude'.
+ if (policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
+ }
+ // In a 'deny_policies' and in a 'deny_policies_exclude' or not match.
+ if (policy_check_status::kPending == check_status ||
+ policy_check_status::kNotMatched == check_status) {
continue;
}
- for (const auto &allow_policy_exclude : allow_policies_exclude) {
- // 2.2. In some 'allow_policies_exclude', it's not allowed.
- if (allow_policy_exclude.match(ac_type, user_name)) {
- return false;
+ }
+
+ // Check if it is allowed by any policy in current resource.
+ for (const auto &policy : policies) {
+ if (need_match_database) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
}
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status =
+ policy.policies.policies_check(ac_type, user_name, policy_check_type::kAllow);
+ // In a 'allow_policies' and not in any 'allow_policies_exclude'.
+ if (policy_check_status::kAllowed == check_status) {
+ return access_control_result::kAllowed;
+ }
+ // In a 'deny_policies' and in a 'deny_policies_exclude' or not match.
+ if (policy_check_status::kPending == check_status ||
+ policy_check_status::kNotMatched == check_status) {
+ continue;
+ }
+ }
Review Comment:
It's duplicate, how about encapsulate it as a function?
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,151 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &policy_exclude : exclude_policies) {
Review Comment:
rename `policy_exclude` to `exclude_policy`
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,151 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &policy_exclude : exclude_policies) {
+ if (policy_exclude.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ bool need_match_database,
Review Comment:
use a enum instead of boolean.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,151 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &policy_exclude : exclude_policies) {
+ if (policy_exclude.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ bool need_match_database,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ for (const auto &policy : policies) {
+ if (need_match_database) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
+ }
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status =
+ policy.policies.policies_check(ac_type, user_name, policy_check_type::kDeny);
+ // In a 'deny_policies' and not in any 'deny_policies_exclude'.
Review Comment:
comments add: or not match any policies?
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +582,45 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ matched_database_table_policy _matched_database_table_policy(
Review Comment:
Don't name a variable with a "_" prefix, it's a naming rule for member variables.
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591148394
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] cr-gpt[bot] commented on pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "cr-gpt[bot] (via GitHub)" <gi...@apache.org>.
cr-gpt[bot] commented on PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#issuecomment-1591213359
Seems you are using me but didn't get OPENAI_API_KEY seted in Variables/Secrets for this repo. you could follow [readme](https://github.com/anc95/ChatGPT-CodeReview) for more information
--
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
[GitHub] [incubator-pegasus] WHBANG commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "WHBANG (via GitHub)" <gi...@apache.org>.
WHBANG commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1226260017
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,151 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &policy_exclude : exclude_policies) {
+ if (policy_exclude.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ bool need_match_database,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ for (const auto &policy : policies) {
+ if (need_match_database) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
+ }
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status =
+ policy.policies.policies_check(ac_type, user_name, policy_check_type::kDeny);
+ // In a 'deny_policies' and not in any 'deny_policies_exclude'.
Review Comment:
uh not needed here
--
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
[GitHub] [incubator-pegasus] acelyc111 commented on a diff in pull request #1518: feat(Ranger): refactor the logic when ranger performs ACL
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on code in PR #1518:
URL: https://github.com/apache/incubator-pegasus/pull/1518#discussion_r1226614225
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,45 +28,178 @@ bool policy_item::match(const access_type &ac_type, const std::string &user_name
return static_cast<bool>(access_types & ac_type) && users.count(user_name) != 0;
}
-bool acl_policies::allowed(const access_type &ac_type, const std::string &user_name) const
+policy_check_status acl_policies::policies_check(const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type) const
{
- // 1. Check if it is not allowed.
- for (const auto &deny_policy : deny_policies) {
- // 1.1. In 'deny_policies'.
- if (!deny_policy.match(ac_type, user_name)) {
+ if (check_type == policy_check_type::kAllow) {
+ return do_policies_check(
+ check_type, ac_type, user_name, allow_policies, allow_policies_exclude);
+ }
+ CHECK(check_type == policy_check_type::kDeny, "");
+ return do_policies_check(check_type, ac_type, user_name, deny_policies, deny_policies_exclude);
+}
+
+policy_check_status
+acl_policies::do_policies_check(const policy_check_type &check_type,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const std::vector<policy_item> &policies,
+ const std::vector<policy_item> &exclude_policies) const
+{
+ for (const auto &policy : policies) {
+ // 1. Doesn't match an allow_policies or a deny_policies.
+ if (!policy.match(ac_type, user_name)) {
continue;
}
- bool in_deny_policies_exclude = false;
- for (const auto &deny_policy_exclude : deny_policies_exclude) {
- if (deny_policy_exclude.match(ac_type, user_name)) {
- in_deny_policies_exclude = true;
- break;
+ // 2. Matches a policy.
+ for (const auto &exclude_policy : exclude_policies) {
+ if (exclude_policy.match(ac_type, user_name)) {
+ // 2.1. Matches an allow/deny_policies_exclude.
+ return policy_check_status::kPending;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ // 2.2. Doesn't match any allow/deny_exclude_policies.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
}
}
+ // 3. Doesn't match any policy.
+ return policy_check_status::kNotMatched;
+}
- // 2. Check if it is allowed.
- for (const auto &allow_policy : allow_policies) {
- // 2.1. In 'allow_policies'.
- if (!allow_policy.match(ac_type, user_name)) {
- continue;
- }
- for (const auto &allow_policy_exclude : allow_policies_exclude) {
- // 2.2. In some 'allow_policies_exclude', it's not allowed.
- if (allow_policy_exclude.match(ac_type, user_name)) {
- return false;
+access_control_result
+check_ranger_resource_policy_allowed(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name)
+{
+ // Check if it is denied by any policy in current resource.
+ auto check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any policy in current resource.
+ check_res = do_check_ranger_resource_policy(policies,
+ ac_type,
+ user_name,
+ md_type,
+ database_name,
+ default_database_name,
+ policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
+ }
+
+ // The check that does not match any policy in current reosource returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_resource_policy(const std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const match_database_type &md_type,
+ const std::string &database_name,
+ const std::string &default_database_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ if (match_database_type::kNeed == md_type) {
+ // Lagacy table not match any database.
+ if (database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(default_database_name) == 0) {
+ continue;
}
+ // New table not match any database.
+ if (!database_name.empty() && policy.database_names.count("*") == 0 &&
+ policy.database_names.count(database_name) == 0) {
+ continue;
+ }
+ }
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
}
- // 2.3. Not in any 'allow_policies_exclude', it's allowed.
- return true;
+ // When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
+ // 'allow_policies_exclude'.
+ if (policy_check_type::kAllow == check_type &&
+ policy_check_status::kAllowed == check_status) {
+ return access_control_result::kAllowed;
+ }
+ // In a 'policies' and in a 'policies_exclude' or not match.
+ if (policy_check_status::kPending == check_status ||
+ policy_check_status::kNotMatched == check_status) {
+ continue;
+ }
+ }
+ return access_control_result::kPending;
+}
+
+access_control_result check_ranger_database_table_policy_allowed(
+ const std::vector<matched_database_table_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name)
+{
+ // Check if it is denied by any DATABASE_TABLE policy.
+ auto check_res = do_check_ranger_database_table_policy(
+ policies, ac_type, user_name, policy_check_type::kDeny);
+ if (access_control_result::kDenied == check_res) {
+ return access_control_result::kDenied;
+ }
+
+ // Check if it is allowed by any DATABASE_TABLE policy.
+ check_res = do_check_ranger_database_table_policy(
+ policies, ac_type, user_name, policy_check_type::kAllow);
+ if (access_control_result::kAllowed == check_res) {
+ return access_control_result::kAllowed;
}
- // 3. Otherwise, it's not allowed.
- return false;
+ // The check that does not match any DATABASE_TABLE policy returns false.
+ return access_control_result::kDenied;
+}
+
+access_control_result
+do_check_ranger_database_table_policy(const std::vector<matched_database_table_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ const policy_check_type &check_type)
+{
+ for (const auto &policy : policies) {
+ auto check_status = policy.policies.policies_check(ac_type, user_name, check_type);
+ // When policy_check_type is 'kDeny' and in a 'deny_policies' and not in any
+ // 'deny_policies_exclude'.
+ if (policy_check_type::kDeny == check_type &&
+ policy_check_status::kDenied == check_status) {
+ return access_control_result::kDenied;
+ }
+ // When policy_check_type is 'kDeny' and in a 'allow_policies' and not in any
Review Comment:
```suggestion
// When policy_check_type is 'kAllow' and in a 'allow_policies' and not in any
```
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
Review Comment:
The added comments don't match the line below, the line just skip to update `matched_database_table_policies`.
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ // This table matches a policy.
+ matched_database_table_policy database_table_policy(
+ {database_name, table_name, policy.policies});
+ // This table matches the policy whose database is "*".
+ if (policy.database_names.count(database_name) == 0) {
+ database_table_policy.matched_database_name = "*";
+ }
+ // This table matches the policy whose database table is "*".
+ if (policy.table_names.count(table_name) == 0) {
Review Comment:
What would `policy.table_names` be in this case?
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -599,34 +586,50 @@ dsn::error_code ranger_resource_policy_manager::sync_policies_to_app_envs()
req->__set_app_name(app.app_name);
req->__set_keys(
{dsn::replication::replica_envs::REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES});
- bool is_policy_matched = false;
+ std::vector<matched_database_table_policy> matched_database_table_policies;
for (const auto &policy : table_policies->second) {
// If this table does not match any database, its Ranger policies will be cleaned up.
if (policy.database_names.count(database_name) == 0 &&
policy.database_names.count("*") == 0) {
continue;
}
+ // If this table does not match any database table, its Ranger policies will be cleaned
+ // up.
+ if (policy.table_names.count(table_name) == 0 && policy.table_names.count("*") == 0) {
+ continue;
+ }
+ // This table matches a policy.
+ matched_database_table_policy database_table_policy(
+ {database_name, table_name, policy.policies});
+ // This table matches the policy whose database is "*".
+ if (policy.database_names.count(database_name) == 0) {
Review Comment:
What would `policy.database_names` be in this case?
--
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