Refactored authorization acceptors into a single class. Replaced different authorization-related Acceptor classes with one AuthorizationAcceptor class.
Removed the ObjectAcceptor parent class, since no inheritance features are provided by it. Review: https://reviews.apache.org/r/60716/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/9e208293 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/9e208293 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/9e208293 Branch: refs/heads/master Commit: 9e208293ba482d843e5c56a40d997ba18e764b58 Parents: 15656be Author: Quinn Leng <quinn.leng....@gmail.com> Authored: Thu Jul 13 17:44:03 2017 -0700 Committer: Greg Mann <gregorywm...@gmail.com> Committed: Thu Jul 13 20:41:31 2017 -0700 ---------------------------------------------------------------------- src/common/http.cpp | 90 ++++++++---------------------------------------- src/common/http.hpp | 68 +++++++++++++----------------------- src/master/http.cpp | 22 +++++++----- 3 files changed, 53 insertions(+), 127 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/9e208293/src/common/http.cpp ---------------------------------------------------------------------- diff --git a/src/common/http.cpp b/src/common/http.cpp index a9c2a4a..3825a13 100644 --- a/src/common/http.cpp +++ b/src/common/http.cpp @@ -1143,50 +1143,25 @@ void logRequest(const process::http::Request& request) } -Future<Owned<AuthorizeFrameworkInfoAcceptor>> - AuthorizeFrameworkInfoAcceptor::create( - const Option<Principal>& principal, - const Option<Authorizer*>& authorizer) -{ - if (authorizer.isNone()) { - return Owned<AuthorizeFrameworkInfoAcceptor>( - new AuthorizeFrameworkInfoAcceptor(Owned<ObjectApprover>( - new AcceptingObjectApprover()))); - } - - const Option<authorization::Subject> subject = - authorization::createSubject(principal); - - return authorizer.get()->getObjectApprover( - subject, - authorization::VIEW_FRAMEWORK) - .then([=](const Owned<ObjectApprover>& approver) { - return Owned<AuthorizeFrameworkInfoAcceptor>( - new AuthorizeFrameworkInfoAcceptor(approver)); - }); -} - - -Future<Owned<AuthorizeTaskAcceptor>> AuthorizeTaskAcceptor::create( +Future<Owned<AuthorizationAcceptor>> AuthorizationAcceptor::create( const Option<Principal>& principal, - const Option<Authorizer*>& authorizer) + const Option<Authorizer*>& authorizer, + const authorization::Action& action) { - if (authorizer.isNone()) { - return Owned<AuthorizeTaskAcceptor>( - new AuthorizeTaskAcceptor(Owned<ObjectApprover>( - new AcceptingObjectApprover()))); - } + if (authorizer.isNone()) { + return Owned<AuthorizationAcceptor>( + new AuthorizationAcceptor(Owned<ObjectApprover>( + new AcceptingObjectApprover()))); + } - const Option<authorization::Subject> subject = - authorization::createSubject(principal); + const Option<authorization::Subject> subject = + authorization::createSubject(principal); - return authorizer.get()->getObjectApprover( - subject, - authorization::VIEW_TASK) - .then([=](const Owned<ObjectApprover>& approver) { - return Owned<AuthorizeTaskAcceptor>( - new AuthorizeTaskAcceptor(approver)); - }); + return authorizer.get()->getObjectApprover(subject, action) + .then([=](const Owned<ObjectApprover>& approver) { + return Owned<AuthorizationAcceptor>( + new AuthorizationAcceptor(approver)); + }); } @@ -1211,41 +1186,6 @@ TaskIDAcceptor::TaskIDAcceptor(const Option<std::string>& _taskId) } -bool AuthorizeFrameworkInfoAcceptor::accept(const FrameworkInfo& frameworkInfo) -{ - ObjectApprover::Object object; - object.framework_info = &frameworkInfo; - - Try<bool> approved = objectApprover->approved(object); - if (approved.isError()) { - LOG(WARNING) << "Error during FrameworkInfo authorization: " - << approved.error(); - return false; - } - - return approved.get(); -} - - -bool AuthorizeTaskAcceptor::accept( - const Task& task, - const FrameworkInfo& frameworkInfo) -{ - ObjectApprover::Object object; - object.task = &task; - object.framework_info = &frameworkInfo; - - Try<bool> approved = objectApprover->approved(object); - - if (approved.isError()) { - LOG(WARNING) << "Error during Task authorization: " << approved.error(); - return false; - } - - return approved.get(); -} - - bool FrameworkIDAcceptor::accept(const FrameworkID& _frameworkId) { if (frameworkId.isSome()) { http://git-wip-us.apache.org/repos/asf/mesos/blob/9e208293/src/common/http.hpp ---------------------------------------------------------------------- diff --git a/src/common/http.hpp b/src/common/http.hpp index 93c9b2e..4822a23 100644 --- a/src/common/http.hpp +++ b/src/common/http.hpp @@ -161,21 +161,32 @@ public: }; -/** - * Determines which objects will be accepted when filtering results based on - * authorization or other criteria. - */ -class ObjectAcceptor +// Determines which objects will be accepted based on authorization. +class AuthorizationAcceptor { public: - virtual ~ObjectAcceptor() = default; -}; + static process::Future<process::Owned<AuthorizationAcceptor>> create( + const Option<process::http::authentication::Principal>& principal, + const Option<Authorizer*>& authorizer, + const authorization::Action& action); + template <typename... Args> + bool accept(Args&... args) + { + Try<bool> approved = + objectApprover->approved(ObjectApprover::Object(args...)); + if (approved.isError()) { + LOG(WARNING) << "Error during authorization: " << approved.error(); + return false; + } + + return approved.get(); + } -// Parent class for authorization-based acceptors. -class AuthorizationAcceptor : public ObjectAcceptor -{ protected: + // TODO(qleng): Currently, `Owned` is implemented with `shared_ptr` and allows + // copying. In the future, if `Owned` is implemented with `unique_ptr`, we + // will need to pass by rvalue reference here instead (see MESOS-5122). AuthorizationAcceptor(const process::Owned<ObjectApprover>& approver) : objectApprover(approver) {} @@ -183,46 +194,15 @@ protected: }; -class AuthorizeFrameworkInfoAcceptor : public AuthorizationAcceptor -{ -public: - static process::Future<process::Owned<AuthorizeFrameworkInfoAcceptor>> create( - const Option<process::http::authentication::Principal>& principal, - const Option<Authorizer*>& authorizer); - - bool accept(const FrameworkInfo& frameworkInfo); - -protected: - AuthorizeFrameworkInfoAcceptor(const process::Owned<ObjectApprover>& approver) - : AuthorizationAcceptor(approver) {} -}; - - -class AuthorizeTaskAcceptor : public AuthorizationAcceptor -{ -public: - static process::Future<process::Owned<AuthorizeTaskAcceptor>> create( - const Option<process::http::authentication::Principal>& principal, - const Option<Authorizer*>& authorizer); - - bool accept( - const Task& task, - const FrameworkInfo& frameworkInfo); - -protected: - AuthorizeTaskAcceptor(const process::Owned<ObjectApprover>& approver) - : AuthorizationAcceptor(approver) {} -}; - - /** * Filtering results based on framework ID. When no framework ID is specified * it will accept all inputs. */ -class FrameworkIDAcceptor : public ObjectAcceptor +class FrameworkIDAcceptor { public: FrameworkIDAcceptor(const Option<std::string>& _frameworkId); + bool accept(const FrameworkID& frameworkId); protected: @@ -234,7 +214,7 @@ protected: * Filtering results based on task ID. When no task ID is specified * it will accept all inputs. */ -class TaskIDAcceptor : public ObjectAcceptor +class TaskIDAcceptor { public: TaskIDAcceptor(const Option<std::string>& _taskId); http://git-wip-us.apache.org/repos/asf/mesos/blob/9e208293/src/master/http.cpp ---------------------------------------------------------------------- diff --git a/src/master/http.cpp b/src/master/http.cpp index 4ec275f..3ddb54b 100644 --- a/src/master/http.cpp +++ b/src/master/http.cpp @@ -3890,10 +3890,16 @@ Future<Response> Master::Http::tasks( Option<string> order = request.url.query.get("order"); string _order = order.isSome() && (order.get() == "asc") ? "asc" : "des"; - Future<Owned<AuthorizeFrameworkInfoAcceptor>> authorizeFrameworkInfo = - AuthorizeFrameworkInfoAcceptor::create(principal, master->authorizer); - Future<Owned<AuthorizeTaskAcceptor>> authorizeTask = - AuthorizeTaskAcceptor::create(principal, master->authorizer); + Future<Owned<AuthorizationAcceptor>> authorizeFrameworkInfo = + AuthorizationAcceptor::create( + principal, + master->authorizer, + authorization::VIEW_FRAMEWORK); + Future<Owned<AuthorizationAcceptor>> authorizeTask = + AuthorizationAcceptor::create( + principal, + master->authorizer, + authorization::VIEW_TASK); Future<Owned<FrameworkIDAcceptor>> selectFrameworkId = Owned<FrameworkIDAcceptor>( new FrameworkIDAcceptor(request.url.query.get("framework_id"))); @@ -3907,12 +3913,12 @@ Future<Response> Master::Http::tasks( selectTaskId) .then(defer( master->self(), - [=](const tuple<Owned<AuthorizeFrameworkInfoAcceptor>, - Owned<AuthorizeTaskAcceptor>, + [=](const tuple<Owned<AuthorizationAcceptor>, + Owned<AuthorizationAcceptor>, Owned<FrameworkIDAcceptor>, Owned<TaskIDAcceptor>>& acceptors)-> Future<Response> { - Owned<AuthorizeFrameworkInfoAcceptor> authorizeFrameworkInfo; - Owned<AuthorizeTaskAcceptor> authorizeTask; + Owned<AuthorizationAcceptor> authorizeFrameworkInfo; + Owned<AuthorizationAcceptor> authorizeTask; Owned<FrameworkIDAcceptor> selectFrameworkId; Owned<TaskIDAcceptor> selectTaskId; tie(authorizeFrameworkInfo,