empiredan commented on code in PR #1360: URL: https://github.com/apache/incubator-pegasus/pull/1360#discussion_r1118383491
########## src/runtime/ranger/ranger_resource_policy.h: ########## @@ -0,0 +1,86 @@ +// 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 <map> +#include <string> +#include <unordered_set> +#include <vector> + +#include <rapidjson/document.h> + +#include "common/json_helper.h" +#include "utils/fmt_logging.h" + +namespace dsn { +namespace ranger { + +// ACL type defined in Range service for RPC matching policy +enum access_type Review Comment: Better to use enum classes instead of traditional enumerations which could lead to problems such as name clashes (see https://www.stroustrup.com/C++11FAQ.html#enum). You can define the class just like: ```c++ enum access_type : uint8_t ``` where the underlying type is `uint8_t`. Overload operator `&` and `|` where `std::underlying_type` can be used to extract `uint8_t`, see https://www.sandordargo.com/blog/2022/06/22/bitwise-enums for details. ########## src/runtime/ranger/ranger_resource_policy.h: ########## @@ -0,0 +1,86 @@ +// 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 <map> +#include <string> +#include <unordered_set> +#include <vector> + +#include <rapidjson/document.h> + +#include "common/json_helper.h" +#include "utils/fmt_logging.h" + +namespace dsn { +namespace ranger { + +// ACL type defined in Range service for RPC matching policy +enum access_type +{ + READ = 1, + WRITE = 1 << 1, + CREATE = 1 << 2, + DROP = 1 << 3, + LIST = 1 << 4, + METADATA = 1 << 5, + CONTROL = 1 << 6, +}; + +// Ranger policy data structure +struct policy_item +{ + int8_t access_types; + std::unordered_set<std::string> users; + + // Check if the 'acl_type' - 'user_name' pair is matched to the policy. + // Return true if it is matched, otherwise return false. + // TODO(wanghao): add benchmark test + bool match(const access_type &ac_type, const std::string &user_name) const; +}; + +// Data structure of policies with different priorities +struct acl_policies +{ + // policy priority: deny_policies_exclude > deny_policies > allow_policies_exclude > + // allow_policies + std::vector<policy_item> allow_policies; + std::vector<policy_item> allow_policies_exclude; + std::vector<policy_item> deny_policies; + std::vector<policy_item> 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; +}; + +// A policy data structure definition of ranger resources +struct ranger_resource_policy +{ +public: + ranger_resource_policy() = default; + ~ranger_resource_policy() = default; + +public: Review Comment: `public` could be omitted. ########## src/runtime/ranger/ranger_resource_policy.cpp: ########## @@ -0,0 +1,70 @@ +// 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 "ranger_resource_policy.h" + +namespace dsn { +namespace ranger { + +bool policy_item::match(const access_type &ac_type, const std::string &user_name) const +{ + return (access_types & ac_type) != 0 && users.count(user_name) != 0; Review Comment: Is `ac_type` allowed to just include single type or multiple types ? Are following examples matched ? ``` access_types = READ | DROP, ac_type = READ | CREATE access_types = WRITE, ac_type = READ | WRITE | CREATE ``` ########## src/runtime/ranger/ranger_resource_policy.h: ########## @@ -0,0 +1,86 @@ +// 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 <map> +#include <string> +#include <unordered_set> +#include <vector> + +#include <rapidjson/document.h> + +#include "common/json_helper.h" +#include "utils/fmt_logging.h" + +namespace dsn { +namespace ranger { + +// ACL type defined in Range service for RPC matching policy +enum access_type +{ + READ = 1, Review Comment: Prefer constant-style naming such as `kRead`, `kWrite`, `kCreate`, etc., see https://google.github.io/styleguide/cppguide.html#Enumerator_Names. ########## src/runtime/ranger/ranger_resource_policy.h: ########## @@ -0,0 +1,86 @@ +// 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 <map> +#include <string> +#include <unordered_set> +#include <vector> + +#include <rapidjson/document.h> + +#include "common/json_helper.h" +#include "utils/fmt_logging.h" + +namespace dsn { +namespace ranger { + +// ACL type defined in Range service for RPC matching policy +enum access_type +{ + READ = 1, + WRITE = 1 << 1, + CREATE = 1 << 2, + DROP = 1 << 3, + LIST = 1 << 4, + METADATA = 1 << 5, + CONTROL = 1 << 6, +}; + +// Ranger policy data structure +struct policy_item +{ + int8_t access_types; + std::unordered_set<std::string> users; + + // Check if the 'acl_type' - 'user_name' pair is matched to the policy. + // Return true if it is matched, otherwise return false. + // TODO(wanghao): add benchmark test + bool match(const access_type &ac_type, const std::string &user_name) const; +}; + +// Data structure of policies with different priorities +struct acl_policies +{ + // policy priority: deny_policies_exclude > deny_policies > allow_policies_exclude > + // allow_policies + std::vector<policy_item> allow_policies; + std::vector<policy_item> allow_policies_exclude; + std::vector<policy_item> deny_policies; + std::vector<policy_item> 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; +}; + +// A policy data structure definition of ranger resources +struct ranger_resource_policy +{ +public: + ranger_resource_policy() = default; Review Comment: Is it necessary to add both default constructor and destructor 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: [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]
