[mesos] 04/05: Refactored createSubject and authorizeLogAccess to common/authorization.

[ti...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

tillt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git

commit 00e0707e32735886f61ab4f9c03d4b06a295d1b5
Author: Till Toenshoff <to...@me.com>
AuthorDate: Tue Nov 20 14:46:11 2018 +0100

    Refactored createSubject and authorizeLogAccess to common/authorization.
    
    Moves 'createSubject' out of common/http into common/authorization.
    
    Removes duplicate 'authorizeLogAccess' out of master.cpp and slave.cpp.
    Introduces 'authorizeLogAccess' within common/authorization.
    
    Review: https://reviews.apache.org/r/69385/
---
 src/common/authorization.cpp | 49 ++++++++++++++++++++++++++++++++++++++++++++
 src/common/authorization.hpp | 18 ++++++++++++++++
 src/common/http.cpp          | 27 +-----------------------
 src/common/http.hpp          | 10 ---------
 src/master/http.cpp          |  1 +
 src/master/master.cpp        | 25 ++++------------------
 src/master/master.hpp        |  3 ---
 src/master/quota_handler.cpp |  2 ++
 src/slave/http.cpp           |  1 +
 src/slave/slave.cpp          | 29 +++++---------------------
 src/slave/slave.hpp          |  3 ---
 11 files changed, 81 insertions(+), 87 deletions(-)

diff --git a/src/common/authorization.cpp b/src/common/authorization.cpp
index 5064ad2..fa0c0e8 100644
--- a/src/common/authorization.cpp
+++ b/src/common/authorization.cpp
@@ -17,15 +17,22 @@
 #include "common/authorization.hpp"
 
 #include <algorithm>
+#include <string>
+
+#include <mesos/mesos.hpp>
 
 #include <process/collect.hpp>
 
 #include <stout/foreach.hpp>
+#include <stout/none.hpp>
 
+using std::string;
 using std::vector;
 
 using process::Future;
 
+using process::http::authentication::Principal;
+
 namespace mesos {
 namespace authorization {
 
@@ -37,5 +44,47 @@ Future<bool> collectAuthorizations(const vector<Future<bool>>& authorizations)
     });
 }
 
+
+const Option<Subject> createSubject(const Option<Principal>& principal)
+{
+  if (principal.isSome()) {
+    Subject subject;
+
+    if (principal->value.isSome()) {
+      subject.set_value(principal->value.get());
+    }
+
+    foreachpair (const string& key, const string& value, principal->claims) {
+      Label* claim = subject.mutable_claims()->mutable_labels()->Add();
+      claim->set_key(key);
+      claim->set_value(value);
+    }
+
+    return subject;
+  }
+
+  return None();
+}
+
+
+Future<bool> authorizeLogAccess(
+    const Option<Authorizer*>& authorizer,
+    const Option<Principal>& principal)
+{
+  if (authorizer.isNone()) {
+    return true;
+  }
+
+  Request request;
+  request.set_action(ACCESS_MESOS_LOG);
+
+  Option<Subject> subject = createSubject(principal);
+  if (subject.isSome()) {
+    request.mutable_subject()->CopyFrom(subject.get());
+  }
+
+  return authorizer.get()->authorized(request);
+}
+
 } // namespace authorization {
 } // namespace mesos {
diff --git a/src/common/authorization.hpp b/src/common/authorization.hpp
index 439996a..fb73f30 100644
--- a/src/common/authorization.hpp
+++ b/src/common/authorization.hpp
@@ -19,7 +19,14 @@
 
 #include <vector>
 
+#include <mesos/authentication/authenticator.hpp>
+
+#include <mesos/authorizer/authorizer.hpp>
+
 #include <process/future.hpp>
+#include <process/http.hpp>
+
+#include <stout/option.hpp>
 
 namespace mesos {
 namespace authorization {
@@ -29,6 +36,17 @@ namespace authorization {
 process::Future<bool> collectAuthorizations(
     const std::vector<process::Future<bool>>& authorizations);
 
+// Creates a `Subject` for authorization purposes when given an
+// authenticated `Principal`. This function accepts and returns an
+// `Option` to make call sites cleaner, since it is possible that
+// `principal` will be `NONE`.
+const Option<Subject> createSubject(
+    const Option<process::http::authentication::Principal>& principal);
+
+process::Future<bool> authorizeLogAccess(
+    const Option<Authorizer*>& authorizer,
+    const Option<process::http::authentication::Principal>& principal);
+
 } // namespace authorization {
 } // namespace mesos {
 
diff --git a/src/common/http.cpp b/src/common/http.cpp
index eeeed97..2be94b2 100644
--- a/src/common/http.cpp
+++ b/src/common/http.cpp
@@ -48,6 +48,7 @@
 
 #include <stout/os/permissions.hpp>
 
+#include "common/authorization.hpp"
 #include "common/http.hpp"
 
 #include "messages/messages.hpp"
@@ -876,32 +877,6 @@ static void json(JSON::StringWriter* writer, const Value::Text& text)
   writer->set(text.value());
 }
 
-namespace authorization {
-
-const Option<authorization::Subject> createSubject(
-    const Option<Principal>& principal)
-{
-  if (principal.isSome()) {
-    authorization::Subject subject;
-
-    if (principal->value.isSome()) {
-      subject.set_value(principal->value.get());
-    }
-
-    foreachpair (const string& key, const string& value, principal->claims) {
-      Label* claim = subject.mutable_claims()->mutable_labels()->Add();
-      claim->set_key(key);
-      claim->set_value(value);
-    }
-
-    return subject;
-  }
-
-  return None();
-}
-
-} // namespace authorization {
-
 const AuthorizationCallbacks createAuthorizationCallbacks(
     Authorizer* authorizer)
 {
diff --git a/src/common/http.hpp b/src/common/http.hpp
index 6ca54a6..ac9ed5e 100644
--- a/src/common/http.hpp
+++ b/src/common/http.hpp
@@ -173,16 +173,6 @@ void json(
 void json(JSON::ObjectWriter* writer, const Task& task);
 void json(JSON::ObjectWriter* writer, const TaskStatus& status);
 
-namespace authorization {
-
-// Creates a subject for authorization purposes when given an authenticated
-// principal. This function accepts and returns an `Option` to make call sites
-// cleaner, since it is possible that `principal` will be `NONE`.
-const Option<authorization::Subject> createSubject(
-    const Option<process::http::authentication::Principal>& principal);
-
-} // namespace authorization {
-
 const process::http::authorization::AuthorizationCallbacks
   createAuthorizationCallbacks(Authorizer* authorizer);
 
diff --git a/src/master/http.cpp b/src/master/http.cpp
index 75ab6ea..68ee2a6 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -68,6 +68,7 @@
 #include <stout/utils.hpp>
 #include <stout/uuid.hpp>
 
+#include "common/authorization.hpp"
 #include "common/http.hpp"
 #include "common/protobuf_utils.hpp"
 #include "common/resources_utils.hpp"
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 0b43dc7..b4b02d8 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -1074,10 +1074,11 @@ void Master::initialize()
   provide("app", path::join(flags.webui_dir, "app"));
   provide("assets", path::join(flags.webui_dir, "assets"));
 
-  const PID<Master> masterPid = self();
+  // TODO(tillt): Use generalized lambda capture once we adopt C++14.
+  Option<Authorizer*> _authorizer = authorizer;
 
-  auto authorize = [masterPid](const Option<Principal>& principal) {
-    return dispatch(masterPid, &Master::authorizeLogAccess, principal);
+  auto authorize = [_authorizer](const Option<Principal>& principal) {
+    return authorization::authorizeLogAccess(_authorizer, principal);
   };
 
   // Expose the log file for the webui. Fall back to 'log_dir' if
@@ -1413,24 +1414,6 @@ void Master::_exited(Framework* framework)
 }
 
 
-Future<bool> Master::authorizeLogAccess(const Option<Principal>& principal)
-{
-  if (authorizer.isNone()) {
-    return true;
-  }
-
-  authorization::Request request;
-  request.set_action(authorization::ACCESS_MESOS_LOG);
-
-  Option<authorization::Subject> subject = createSubject(principal);
-  if (subject.isSome()) {
-    request.mutable_subject()->CopyFrom(subject.get());
-  }
-
-  return authorizer.get()->authorized(request);
-}
-
-
 void Master::consume(MessageEvent&& event)
 {
   // There are three cases about the message's UPID with respect to
diff --git a/src/master/master.hpp b/src/master/master.hpp
index baa6668..3b3c1a4 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -1179,9 +1179,6 @@ private:
       const hashset<SlaveID>& toRemoveGone,
       const process::Future<bool>& registrarResult);
 
-  process::Future<bool> authorizeLogAccess(
-      const Option<process::http::authentication::Principal>& principal);
-
   std::vector<std::string> filterRoles(
       const process::Owned<ObjectApprovers>& approvers) const;
 
diff --git a/src/master/quota_handler.cpp b/src/master/quota_handler.cpp
index a4975fd..8d417a9 100644
--- a/src/master/quota_handler.cpp
+++ b/src/master/quota_handler.cpp
@@ -37,6 +37,8 @@
 #include <stout/strings.hpp>
 #include <stout/utils.hpp>
 
+#include "common/authorization.hpp"
+
 #include "logging/logging.hpp"
 
 #include "master/quota.hpp"
diff --git a/src/slave/http.cpp b/src/slave/http.cpp
index 816aed1..ba38926 100644
--- a/src/slave/http.cpp
+++ b/src/slave/http.cpp
@@ -57,6 +57,7 @@
 #include <stout/strings.hpp>
 #include <stout/unreachable.hpp>
 
+#include "common/authorization.hpp"
 #include "common/build.hpp"
 #include "common/http.hpp"
 #include "common/recordio.hpp"
diff --git a/src/slave/slave.cpp b/src/slave/slave.cpp
index 74f6fb9..858b786 100644
--- a/src/slave/slave.cpp
+++ b/src/slave/slave.cpp
@@ -82,6 +82,7 @@
 
 #include "authentication/cram_md5/authenticatee.hpp"
 
+#include "common/authorization.hpp"
 #include "common/build.hpp"
 #include "common/protobuf_utils.hpp"
 #include "common/resources_utils.hpp"
@@ -836,13 +837,11 @@ void Slave::initialize()
             });
         });
 
-  const PID<Slave> slavePid = self();
+  // TODO(tillt): Use generalized lambda capture once we adopt C++14.
+  Option<Authorizer*> _authorizer = authorizer;
 
-  auto authorize = [slavePid](const Option<Principal>& principal) {
-    return dispatch(
-        slavePid,
-        &Slave::authorizeLogAccess,
-        principal);
+  auto authorize = [_authorizer](const Option<Principal>& principal) {
+    return authorization::authorizeLogAccess(_authorizer, principal);
   };
 
   // Expose the log file for the webui. Fall back to 'log_dir' if
@@ -8478,24 +8477,6 @@ Future<bool> Slave::authorizeTask(
 }
 
 
-Future<bool> Slave::authorizeLogAccess(const Option<Principal>& principal)
-{
-  if (authorizer.isNone()) {
-    return true;
-  }
-
-  authorization::Request request;
-  request.set_action(authorization::ACCESS_MESOS_LOG);
-
-  Option<authorization::Subject> subject = createSubject(principal);
-  if (subject.isSome()) {
-    request.mutable_subject()->CopyFrom(subject.get());
-  }
-
-  return authorizer.get()->authorized(request);
-}
-
-
 Future<bool> Slave::authorizeSandboxAccess(
     const Option<Principal>& principal,
     const FrameworkID& frameworkId,
diff --git a/src/slave/slave.hpp b/src/slave/slave.hpp
index 0bd3401..edf7269 100644
--- a/src/slave/slave.hpp
+++ b/src/slave/slave.hpp
@@ -608,9 +608,6 @@ private:
       const TaskInfo& task,
       const FrameworkInfo& frameworkInfo);
 
-  process::Future<bool> authorizeLogAccess(
-      const Option<process::http::authentication::Principal>& principal);
-
   process::Future<bool> authorizeSandboxAccess(
       const Option<process::http::authentication::Principal>& principal,
       const FrameworkID& frameworkId,