empiredan commented on code in PR #1433:
URL:
https://github.com/apache/incubator-pegasus/pull/1433#discussion_r1166797639
##########
src/runtime/security/replica_access_controller.h:
##########
@@ -30,14 +32,39 @@ namespace security {
class replica_access_controller : public access_controller
{
public:
- explicit replica_access_controller(const std::string &name);
- bool allowed(message_ex *msg) override;
+ explicit replica_access_controller(const std::string &replica_name);
+
+ // Check whether replica can be accessed, this method is compatible with
ACL using
+ // '_allowed_users' and ACL using Ranger policy.
+ bool allowed(message_ex *msg, ranger::access_type req_type) override;
+
+ // Update '_allowed_users' when the
app_env(REPLICA_ACCESS_CONTROLLER_ALLOWED_USERS) of the
+ // table changes
void update_allowed_users(const std::string &users) override;
+ // Update '_ranger_policies' when the
app_env(REPLICA_ACCESS_CONTROLLER_RANGER_POLICIES) of the
+ // table changes
+ void update_ranger_policies(const std::string &policies) override;
+
+private:
+ // Security check to avoid allowed_users is not empty in special scenarios.
+ void check_allowed_users_valid();
+
private:
utils::rw_lock_nr _lock; // [
Review Comment:
```suggestion
mutable utils::rw_lock_nr _lock;
```
##########
src/replica/replica.cpp:
##########
@@ -599,5 +599,11 @@ error_code replica::store_app_info(app_info &info, const
std::string &path)
return err;
}
+bool replica::access_controller_allowed(message_ex *msg, ranger::access_type
req_type)
Review Comment:
Declared as `const` ?
##########
src/runtime/security/replica_access_controller.cpp:
##########
@@ -62,10 +88,37 @@ void replica_access_controller::update_allowed_users(const
std::string &users)
utils::split_args(users.c_str(), users_set, ',');
{
utils::auto_write_lock l(_lock);
- // This swap operation is in constant time
- _users.swap(users_set);
+ _allowed_users.swap(users_set);
_env_users = users;
+ check_allowed_users_valid();
+ }
+}
+
+void replica_access_controller::update_ranger_policies(const std::string
&policies)
+{
+ {
+ utils::auto_read_lock l(_lock);
+ if (_env_policies == policies) {
+ return;
+ }
+ }
+ ranger::acl_policies tmp_policies;
+ std::string tmp_policies_str = policies;
Review Comment:
```suggestion
auto tmp_policies_str = policies;
```
##########
src/runtime/security/access_controller.cpp:
##########
@@ -46,14 +46,6 @@ access_controller::access_controller()
access_controller::~access_controller() {}
-bool access_controller::pre_check(const std::string &user_name)
-{
- if (!FLAGS_enable_acl || _super_users.find(user_name) !=
_super_users.end()) {
- return true;
- }
- return false;
-}
-
bool access_controller::is_enable_ranger_acl() { return
FLAGS_enable_ranger_acl; }
Review Comment:
```suggestion
bool access_controller::is_enable_ranger_acl() const { return
FLAGS_enable_ranger_acl; }
```
##########
src/runtime/security/replica_access_controller.cpp:
##########
@@ -15,36 +15,61 @@
// specific language governing permissions and limitations
// under the License.
-#include "replica_access_controller.h"
+#include <utility>
+
+// Disable class-memaccess warning to facilitate compilation with gcc>7
+// https://github.com/Tencent/rapidjson/issues/1700
+#pragma GCC diagnostic push
+#if defined(__GNUC__) && __GNUC__ >= 8
+#pragma GCC diagnostic ignored "-Wclass-memaccess"
+#endif
+#include "common/json_helper.h"
+
+#pragma GCC diagnostic pop
+#include "replica_access_controller.h"
#include "runtime/rpc/network.h"
#include "runtime/rpc/rpc_message.h"
#include "utils/autoref_ptr.h"
+#include "utils/blob.h"
+#include "utils/flags.h"
#include "utils/fmt_logging.h"
#include "utils/strings.h"
namespace dsn {
namespace security {
-replica_access_controller::replica_access_controller(const std::string &name)
{ _name = name; }
+DSN_DECLARE_bool(enable_acl);
+DSN_DECLARE_bool(enable_ranger_acl);
+replica_access_controller::replica_access_controller(const std::string
&replica_name)
+{
+ _name = replica_name;
+}
-bool replica_access_controller::allowed(message_ex *msg)
+bool replica_access_controller::allowed(message_ex *msg, ranger::access_type
req_type)
Review Comment:
Could this function be declared as `const` ? Could use
`unordered_set::count` instead of `unordered_set::find`.
--
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]
