acelyc111 commented on code in PR #1478:
URL:
https://github.com/apache/incubator-pegasus/pull/1478#discussion_r1201874661
##########
src/utils/enum_helper.h:
##########
@@ -126,4 +126,11 @@ class enum_helper_xxx
template <typename TEnum>
std::unique_ptr<enum_helper_xxx<TEnum>> enum_helper_xxx<TEnum>::_instance;
+// Used to print the enum
Review Comment:
Is it possible to use `enum_to_string()` ?
##########
src/runtime/ranger/ranger_resource_policy.h:
##########
@@ -56,8 +79,17 @@ struct acl_policies
deny_policies,
deny_policies_exclude);
- // Check whether the 'user_name' is allowed to access the resource by type
of 'ac_type'.
- bool allowed(const access_type &ac_type, const std::string &user_name)
const;
+ // Check if 'allow_policies' or 'deny_policies' allow or deny "user_name"
access to resource by
Review Comment:
```suggestion
// Check if 'allow_policies' or 'deny_policies' allow or deny
'user_name' to access the resource by
```
##########
src/runtime/ranger/ranger_resource_policy.h:
##########
@@ -68,8 +100,36 @@ struct ranger_resource_policy
std::unordered_set<std::string> table_names;
acl_policies policies;
- DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies)
+ DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies);
};
+// A policy data structure definition of the DATABASE_TABLE resource, which
will be set in
+// 'app_envs'
+struct matched_database_table_policy
+{
+ std::string matched_database_name;
+ std::string matched_table_name;
+ acl_policies policies;
+
+ DEFINE_JSON_SERIALIZATION(matched_database_name, matched_table_name,
policies);
+};
+
+// Returns true if 'policies' allows "user_name" to access "database_name" via
"rpc_code".
+// 'is_need_match_database' being true means that the 'policies' needs to be
matched to the database
+// first, false means not
+bool check_ranger_resource_policy_allowed(const
std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ bool is_need_match_database,
+ const std::string &database_name,
+ const std::string
&default_database_name);
+
+// Return true if "policies" allow "user_name" access, this is used for
DATABASE_TABLE resource
Review Comment:
```suggestion
// Return true if 'policies' allow 'user_name' to access, this is used for
DATABASE_TABLE resource.
```
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
Review Comment:
Add a check to ensure check_type equals to policy_check_type::kDeny now.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
+ 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>
&policies_exclude) const
+{
+ for (const auto &policy : policies) {
+ // 1.1. Not match a 'allow_policies/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;
+ // 1.2. A policies has been matched.
Review Comment:
```suggestion
// 1.2. Matches a policy.
```
##########
src/runtime/ranger/ranger_resource_policy.h:
##########
@@ -68,8 +100,36 @@ struct ranger_resource_policy
std::unordered_set<std::string> table_names;
acl_policies policies;
- DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies)
+ DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies);
};
+// A policy data structure definition of the DATABASE_TABLE resource, which
will be set in
+// 'app_envs'
+struct matched_database_table_policy
+{
+ std::string matched_database_name;
+ std::string matched_table_name;
+ acl_policies policies;
+
+ DEFINE_JSON_SERIALIZATION(matched_database_name, matched_table_name,
policies);
+};
+
+// Returns true if 'policies' allows "user_name" to access "database_name" via
"rpc_code".
+// 'is_need_match_database' being true means that the 'policies' needs to be
matched to the database
+// first, false means not
+bool check_ranger_resource_policy_allowed(const
std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ bool is_need_match_database,
Review Comment:
As I've ever mentioned, it would be better to use enum rather than boolean.
Further more, add some comments to explain when needs to match database, and
when doesn't need.
##########
src/runtime/ranger/ranger_resource_policy.h:
##########
@@ -68,8 +100,36 @@ struct ranger_resource_policy
std::unordered_set<std::string> table_names;
acl_policies policies;
- DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies)
+ DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies);
};
+// A policy data structure definition of the DATABASE_TABLE resource, which
will be set in
+// 'app_envs'
+struct matched_database_table_policy
+{
+ std::string matched_database_name;
+ std::string matched_table_name;
+ acl_policies policies;
+
+ DEFINE_JSON_SERIALIZATION(matched_database_name, matched_table_name,
policies);
+};
+
+// Returns true if 'policies' allows "user_name" to access "database_name" via
"rpc_code".
Review Comment:
Where is `rpc_code`?
##########
src/runtime/ranger/ranger_resource_policy.h:
##########
@@ -68,8 +100,36 @@ struct ranger_resource_policy
std::unordered_set<std::string> table_names;
acl_policies policies;
- DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies)
+ DEFINE_JSON_SERIALIZATION(name, database_names, table_names, policies);
};
+// A policy data structure definition of the DATABASE_TABLE resource, which
will be set in
+// 'app_envs'
+struct matched_database_table_policy
+{
+ std::string matched_database_name;
+ std::string matched_table_name;
+ acl_policies policies;
+
+ DEFINE_JSON_SERIALIZATION(matched_database_name, matched_table_name,
policies);
+};
+
+// Returns true if 'policies' allows "user_name" to access "database_name" via
"rpc_code".
Review Comment:
```suggestion
// Returns true if 'policies' allows 'user_name' to access 'database_name'
via 'rpc_code'.
```
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
+ 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>
&policies_exclude) const
+{
+ for (const auto &policy : policies) {
+ // 1.1. Not match a 'allow_policies/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;
+ // 1.2. A policies has been matched.
+ bool in_policies_exclude = false;
+ // 1.3. In 'allow_policies_exclude/deny_policies_exclude'.
Review Comment:
```suggestion
// 1.3. Matches an allow/deny_policy_exclude.
```
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
+ 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>
&policies_exclude) const
+{
+ for (const auto &policy : policies) {
+ // 1.1. Not match a 'allow_policies/deny_policies'.
Review Comment:
Why `1.1.` ? I didn't see a `2.x`, the prefix `1.` can be removed?
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
+ 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>
&policies_exclude) const
+{
+ for (const auto &policy : policies) {
+ // 1.1. Not match a 'allow_policies/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;
+ // 1.2. A policies has been matched.
+ bool in_policies_exclude = false;
+ // 1.3. In 'allow_policies_exclude/deny_policies_exclude'.
+ for (const auto &policy_exclude : policies_exclude) {
+ if (policy_exclude.match(ac_type, user_name)) {
+ in_policies_exclude = true;
Review Comment:
It's OK to return kPending here.
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
+ 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>
&policies_exclude) const
+{
+ for (const auto &policy : policies) {
+ // 1.1. Not match a 'allow_policies/deny_policies'.
Review Comment:
The other places are the same.
```suggestion
// 1.1. Doesn't match an allow_policy or a deny_policy.
```
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
+ 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>
&policies_exclude) const
Review Comment:
How about renaming to `exclude_policies` to make it as a noun ?
##########
src/runtime/ranger/ranger_resource_policy.cpp:
##########
@@ -27,44 +27,155 @@ 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);
+ }
+ 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>
&policies_exclude) const
+{
+ for (const auto &policy : policies) {
+ // 1.1. Not match a 'allow_policies/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;
+ // 1.2. A policies has been matched.
+ bool in_policies_exclude = false;
+ // 1.3. In 'allow_policies_exclude/deny_policies_exclude'.
+ for (const auto &policy_exclude : policies_exclude) {
+ if (policy_exclude.match(ac_type, user_name)) {
+ in_policies_exclude = true;
break;
}
}
- // 1.2. Not in any 'deny_policies_exclude', it's not allowed.
- if (!in_deny_policies_exclude) {
- return false;
+ if (in_policies_exclude) {
+ // 1.5. In any 'policies_exclude'.
+ return policy_check_status::kPending;
+ } else {
+ // 1.6. Not in any 'policies_exclude'.
+ if (check_type == policy_check_type::kAllow) {
+ return policy_check_status::kAllowed;
+ } else {
+ return policy_check_status::kDenied;
+ }
}
}
+ // 1.7. not 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)) {
+bool check_ranger_resource_policy_allowed(const
std::vector<ranger_resource_policy> &policies,
+ const access_type &ac_type,
+ const std::string &user_name,
+ bool is_need_match_database,
Review Comment:
```suggestion
bool need_match_database,
```
--
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]
