empiredan commented on code in PR #1378:
URL:
https://github.com/apache/incubator-pegasus/pull/1378#discussion_r1130723123
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -36,6 +64,15 @@ void register_rpc_access_type(access_type ac_type,
ac_type_of_rpc.emplace(code, ac_type);
}
}
+
+// Used to map access_type matched resources policies json string.
+std::map<std::string, access_type> access_type_maping({{"READ",
access_type::kRead},
Review Comment:
```suggestion
const std::map<std::string, access_type> kAccessTypeMaping({{"READ",
access_type::kRead},
```
##########
src/runtime/ranger/access_type.h:
##########
@@ -0,0 +1,52 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+
+namespace dsn {
+namespace ranger {
+
+// ACL type defined in Range service for RPC matching policy
+enum class access_type : uint8_t
+{
+ kInvalid = 0,
+ kRead = 1,
+ kWrite = 1 << 1,
+ kCreate = 1 << 2,
+ kDrop = 1 << 3,
+ kList = 1 << 4,
+ kMetadata = 1 << 5,
+ kControl = 1 << 6
+};
+
+using act = std::underlying_type<access_type>::type;
+
+access_type operator|(access_type lhs, access_type rhs);
+
+access_type operator&(access_type lhs, access_type rhs);
+
+access_type &operator|=(access_type &lhs, access_type rhs);
+
+static access_type access_type_min = access_type::kInvalid;
+static access_type access_type_max =
Review Comment:
```suggestion
const access_type kAccessTypeMax =
```
##########
src/runtime/ranger/access_type.h:
##########
@@ -0,0 +1,52 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <cstdint>
+
+namespace dsn {
+namespace ranger {
+
+// ACL type defined in Range service for RPC matching policy
+enum class access_type : uint8_t
+{
+ kInvalid = 0,
+ kRead = 1,
+ kWrite = 1 << 1,
+ kCreate = 1 << 2,
+ kDrop = 1 << 3,
+ kList = 1 << 4,
+ kMetadata = 1 << 5,
+ kControl = 1 << 6
+};
+
+using act = std::underlying_type<access_type>::type;
+
+access_type operator|(access_type lhs, access_type rhs);
+
+access_type operator&(access_type lhs, access_type rhs);
+
+access_type &operator|=(access_type &lhs, access_type rhs);
+
+static access_type access_type_min = access_type::kInvalid;
Review Comment:
```suggestion
const access_type kAccessTypeMin = access_type::kInvalid;
```
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -97,5 +134,30 @@
ranger_resource_policy_manager::ranger_resource_policy_manager(
"RPC_CM_RENAME_APP"},
_ac_type_of_database_rpcs);
}
+
+void ranger_resource_policy_manager::parse_policies_from_json(const
rapidjson::Value &data,
+
std::vector<policy_item> &policies)
+{
+ CHECK(policies.empty(), "Ranger policy list should not be empty.");
+ RETURN_VOID_IF_NOT_ARRAY(data);
+ for (const auto &item : data.GetArray()) {
+ CONTINUE_IF_MISSING_MEMBER(item, "accesses");
+ policy_item pi;
+ for (const auto &access : item["accesses"].GetArray()) {
+ CONTINUE_IF_MISSING_MEMBER(access, "isAllowed");
+ CONTINUE_IF_MISSING_MEMBER(access, "type");
+ if (access["isAllowed"].GetBool()) {
+ std::string type = access["type"].GetString();
+ std::transform(type.begin(), type.end(), type.begin(),
toupper);
+ pi.access_types |= access_type_maping[type];
Review Comment:
Is it ensured that `type` must be in `access_type_maping` ?
```suggestion
pi.access_types |= kAccessTypeMaping.at(type);
```
##########
src/runtime/test/ranger_resource_policy_manager_test.cpp:
##########
@@ -0,0 +1,195 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include <gtest/gtest.h>
+
+#include "runtime/ranger/ranger_resource_policy.h"
+#include "runtime/ranger/ranger_resource_policy_manager.h"
+
+namespace dsn {
+namespace ranger {
+
+TEST(ranger_resource_policy_manager_test, parse_policies_from_json_for_test)
+{
+ std::string data = R"(
+ [{
+ "accesses": [{
+ "type": "create",
+ "isAllowed": true
+ }, {
+ "type": "drop",
+ "isAllowed": true
+ }, {
+ "type": "control",
+ "isAllowed": true
+ }, {
+ "type": "metadata",
+ "isAllowed": true
+ }, {
+ "type": "list",
+ "isAllowed": true
+ }],
+ "users": ["user1", "user2"],
+ "groups": [],
+ "roles": [],
+ "conditions": [],
+ "delegateAdmin": true
+ }, {
+ "accesses": [{
+ "type": "read",
+ "isAllowed": true
+ }, {
+ "type": "write",
+ "isAllowed": true
+ }],
+ "users": ["user2"],
+ "groups": [],
+ "roles": [],
+ "conditions": [],
+ "delegateAdmin": true
+ }]
+ )";
+
+ std::vector<policy_item> fake_policies;
+
+ rapidjson::Document fake_doc;
+ fake_doc.Parse(data.c_str());
+ ranger_resource_policy_manager::parse_policies_from_json(fake_doc,
fake_policies);
+
+ EXPECT_EQ(2, fake_policies.size());
+
+ ASSERT_EQ(access_type::kCreate | access_type::kDrop | access_type::kList |
+ access_type::kMetadata | access_type::kControl,
+ fake_policies[0].access_types);
+
+ ASSERT_EQ(access_type::kRead | access_type::kWrite,
fake_policies[1].access_types);
+
+ struct test_case
+ {
+ policy_item item;
+ access_type ac_type;
+ std::string user_name;
+ bool expected_result;
+ } tests[] = {{fake_policies[0], access_type::kRead, "", false},
+ {fake_policies[0], access_type::kRead, "user", false},
+ {fake_policies[0], access_type::kRead, "user1", false},
+ {fake_policies[0], access_type::kWrite, "user1", false},
+ {fake_policies[0], access_type::kCreate, "user1", true},
+ {fake_policies[0], access_type::kDrop, "user1", true},
+ {fake_policies[0], access_type::kList, "user1", true},
+ {fake_policies[0], access_type::kMetadata, "user1", true},
+ {fake_policies[0], access_type::kControl, "user1", true},
+ {fake_policies[0], access_type::kRead, "user2", false},
+ {fake_policies[0], access_type::kWrite, "user2", false},
+ {fake_policies[0], access_type::kCreate, "user2", true},
+ {fake_policies[0], access_type::kDrop, "user2", true},
+ {fake_policies[0], access_type::kList, "user2", true},
+ {fake_policies[0], access_type::kMetadata, "user2", true},
+ {fake_policies[0], access_type::kControl, "user2", true},
+ {fake_policies[1], access_type::kRead, "user1", false},
+ {fake_policies[1], access_type::kWrite, "user1", false},
+ {fake_policies[1], access_type::kCreate, "user1", false},
+ {fake_policies[1], access_type::kDrop, "user1", false},
+ {fake_policies[1], access_type::kList, "user1", false},
+ {fake_policies[1], access_type::kMetadata, "user1", false},
+ {fake_policies[1], access_type::kControl, "user1", false},
+ {fake_policies[1], access_type::kRead, "user2", true},
+ {fake_policies[1], access_type::kWrite, "user2", true},
+ {fake_policies[1], access_type::kCreate, "user2", false},
+ {fake_policies[1], access_type::kDrop, "user2", false},
+ {fake_policies[1], access_type::kList, "user2", false},
+ {fake_policies[1], access_type::kMetadata, "user2", false},
+ {fake_policies[1], access_type::kControl, "user2", false}};
+ for (const auto &test : tests) {
+ auto actual_result = test.item.match(test.ac_type, test.user_name);
+ EXPECT_EQ(test.expected_result, actual_result);
+ }
+}
+
+TEST(ranger_resource_policy_manager_test,
ranger_resource_policy_serialized_test)
+{
Review Comment:
Add some comments or encapsulate logics into some functions ? I think both
will make the main logic clearer.
##########
src/runtime/ranger/ranger_resource_policy_manager.h:
##########
@@ -32,8 +32,28 @@ namespace replication {
class meta_service;
}
+enum class resource_type
+{
+ kGlobal = 0,
+ kdatabase,
Review Comment:
```suggestion
kDatabase,
```
##########
src/runtime/ranger/ranger_resource_policy_manager.h:
##########
@@ -44,6 +64,11 @@ class ranger_resource_policy_manager
~ranger_resource_policy_manager() = default;
+private:
+ // Parse Ranger ACL policies from 'data' in JSON format into 'policies'.
+ static void parse_policies_from_json(const rapidjson::Value &data,
Review Comment:
Could this be changed as a function defined in anonymous in cpp file ?
##########
src/runtime/ranger/ranger_resource_policy_manager.cpp:
##########
@@ -97,5 +134,30 @@
ranger_resource_policy_manager::ranger_resource_policy_manager(
"RPC_CM_RENAME_APP"},
_ac_type_of_database_rpcs);
}
+
+void ranger_resource_policy_manager::parse_policies_from_json(const
rapidjson::Value &data,
+
std::vector<policy_item> &policies)
+{
+ CHECK(policies.empty(), "Ranger policy list should not be empty.");
+ RETURN_VOID_IF_NOT_ARRAY(data);
Review Comment:
Is `data` allowed be non-array ?
--
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]
