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/05/17 13:03:10 UTC
[GitHub] [incubator-pegasus] WHBANG opened a new pull request, #1478: feat(Ranger): global resource removes LIST access control and creates a default database policy for the legacy table
WHBANG opened a new pull request, #1478:
URL: https://github.com/apache/incubator-pegasus/pull/1478
### 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?
1. The access control management of RPC `RPC_CM_LIST_APPS` is removed from the global level resource, which is managed by the database resource.
2. Provide a default database-level policy name for access control to legacy tables.
3. Modified one ut.
--
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 #1478: feat(Ranger): global resource removes LIST access control and create a default database policy for the legacy table
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on PR #1478:
URL: https://github.com/apache/incubator-pegasus/pull/1478#issuecomment-1592311892
implemented by other PRs
--
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 #1478: feat(Ranger): global resource removes LIST access control and create a default database policy for the legacy table
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
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: 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 #1478: feat(Ranger): global resource removes LIST access control and create a default database policy for the legacy table
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 commented on PR #1478:
URL: https://github.com/apache/incubator-pegasus/pull/1478#issuecomment-1556884914
Could you please draw the flow-process diagram in raw text? It's a part of the code to understand the backgroud, ref: https://github.com/apache/incubator-pegasus/blob/master/src/server/hotkey_collector.h#L61
--
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 closed pull request #1478: feat(Ranger): global resource removes LIST access control and create a default database policy for the legacy table
Posted by "acelyc111 (via GitHub)" <gi...@apache.org>.
acelyc111 closed pull request #1478: feat(Ranger): global resource removes LIST access control and create a default database policy for the legacy table
URL: https://github.com/apache/incubator-pegasus/pull/1478
--
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