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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
