mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 47509: Fixed authorization::Request initializings.
Date Tue, 24 May 2016 15:10:36 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47509/#review134586
-----------------------------------------------------------



LGTM!

I had to mechanically update the patch for changes in `master`, the patch is:

```
>From ee43fc0b1c8e0b6b5a46c54f8262a1c8c9c864a4 Mon Sep 17 00:00:00 2001
From: Benjamin Bannier <benjamin.bannier@mesosphere.io>
Date: Tue, 24 May 2016 17:04:13 +0200
Subject: [PATCH] Updated patch of rb47509.

---
 src/authorizer/local/authorizer.cpp |  3 +++
 src/common/http.cpp                 |  4 +++-
 src/master/http.cpp                 | 12 ++++++++---
 src/master/master.cpp               | 40 ++++++++++++++++++++++++++-----------
 src/master/quota_handler.cpp        | 20 ++++++++++++++-----
 src/master/weights_handler.cpp      |  4 +++-
 src/slave/http.cpp                  |  4 +++-
 src/tests/authorization_tests.cpp   | 12 ++++++++++-
 8 files changed, 75 insertions(+), 24 deletions(-)

diff --git a/src/authorizer/local/authorizer.cpp b/src/authorizer/local/authorizer.cpp
index dc53bc4..39eab7f 100644
--- a/src/authorizer/local/authorizer.cpp
+++ b/src/authorizer/local/authorizer.cpp
@@ -493,6 +493,9 @@ LocalAuthorizer::~LocalAuthorizer()
 process::Future<bool> LocalAuthorizer::authorized(
   const authorization::Request& request)
 {
+  CHECK(request.has_subject());
+  CHECK(request.has_object());
+
   typedef Future<bool> (LocalAuthorizerProcess::*F)(
       const authorization::Request&);
 
diff --git a/src/common/http.cpp b/src/common/http.cpp
index ad6a4b4..dcbbc60 100644
--- a/src/common/http.cpp
+++ b/src/common/http.cpp
@@ -599,8 +599,10 @@ const AuthorizationCallbacks createAuthorizationCallbacks(
         authorization::Request authRequest;
         authRequest.set_action(mesos::authorization::GET_ENDPOINT_WITH_PATH);
 
+        authorization::Subject* subject = authRequest.mutable_subject();
+
         if (principal.isSome()) {
-          authRequest.mutable_subject()->set_value(principal.get());
+          subject->set_value(principal.get());
         }
 
         const string path = httpRequest.url.path;
diff --git a/src/master/http.cpp b/src/master/http.cpp
index b36b439..b881612 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -1956,12 +1956,16 @@ Future<Response> Master::Http::teardown(
   authorization::Request teardown;
   teardown.set_action(authorization::TEARDOWN_FRAMEWORK_WITH_PRINCIPAL);
 
+  authorization::Subject* subject = teardown.mutable_subject();
+
   if (principal.isSome()) {
-    teardown.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
+  authorization::Object* object = teardown.mutable_object();
+
   if (framework->info.has_principal()) {
-    teardown.mutable_object()->set_value(framework->info.principal());
+    object->set_value(framework->info.principal());
   }
 
   return master->authorizer.get()->authorized(teardown)
@@ -2790,8 +2794,10 @@ Future<bool> Master::Http::authorizeEndpoint(
     return Failure("Unexpected request method '" + method + "'");
   }
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   request.mutable_object()->set_value(endpoint);
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 0005a29..d215b76 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -1961,8 +1961,10 @@ Future<bool> Master::authorizeFramework(
   authorization::Request request;
   request.set_action(authorization::REGISTER_FRAMEWORK_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (frameworkInfo.has_principal()) {
-    request.mutable_subject()->set_value(frameworkInfo.principal());
+    subject->set_value(frameworkInfo.principal());
   }
 
   request.mutable_object()->set_value(frameworkInfo.role());
@@ -3035,8 +3037,10 @@ Future<bool> Master::authorizeTask(
   authorization::Request request;
   request.set_action(authorization::RUN_TASK_WITH_USER);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (framework->info.has_principal()) {
-    request.mutable_subject()->set_value(framework->info.principal());
+    subject->set_value(framework->info.principal());
   }
 
   request.mutable_object()->set_value(user);
@@ -3056,8 +3060,10 @@ Future<bool> Master::authorizeReserveResources(
   authorization::Request request;
   request.set_action(authorization::RESERVE_RESOURCES_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   // The operation will be authorized if the principal is allowed to make
@@ -3066,10 +3072,11 @@ Future<bool> Master::authorizeReserveResources(
   hashset<string> roles;
   list<Future<bool>> authorizations;
   foreach (const Resource& resource, reserve.resources()) {
+    authorization::Object* object = request.mutable_object();
     if (!roles.contains(resource.role())) {
       roles.insert(resource.role());
 
-      request.mutable_object()->set_value(resource.role());
+      object->set_value(resource.role());
       authorizations.push_back(authorizer.get()->authorized(request));
     }
   }
@@ -3112,20 +3119,23 @@ Future<bool> Master::authorizeUnreserveResources(
   authorization::Request request;
   request.set_action(authorization::UNRESERVE_RESOURCES_WITH_PRINCIPAL);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   list<Future<bool>> authorizations;
   foreach (const Resource& resource, unreserve.resources()) {
+    authorization::Object* object = request.mutable_object();
+
     // NOTE: Since validation of this operation is performed after
     // authorization, we must check here that this resource is
     // dynamically reserved. If it isn't, the error will be caught
     // during validation.
     if (Resources::isDynamicallyReserved(resource) &&
         resource.reservation().has_principal()) {
-      request.mutable_object()->set_value(
-          resource.reservation().principal());
+      object->set_value(resource.reservation().principal());
 
       authorizations.push_back(authorizer.get()->authorized(request));
     }
@@ -3165,8 +3175,10 @@ Future<bool> Master::authorizeCreateVolume(
   authorization::Request request;
   request.set_action(authorization::CREATE_VOLUME_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   // The operation will be authorized if the principal is allowed to create
@@ -3175,10 +3187,11 @@ Future<bool> Master::authorizeCreateVolume(
   hashset<string> roles;
   list<Future<bool>> authorizations;
   foreach (const Resource& volume, create.volumes()) {
+    authorization::Object* object = request.mutable_object();
     if (!roles.contains(volume.role())) {
       roles.insert(volume.role());
 
-      request.mutable_object()->set_value(volume.role());
+      object->set_value(volume.role());
       authorizations.push_back(authorizer.get()->authorized(request));
     }
   }
@@ -3217,18 +3230,21 @@ Future<bool> Master::authorizeDestroyVolume(
   authorization::Request request;
   request.set_action(authorization::DESTROY_VOLUME_WITH_PRINCIPAL);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   list<Future<bool>> authorizations;
   foreach (const Resource& volume, destroy.volumes()) {
+    authorization::Object* object = request.mutable_object();
+
     // NOTE: Since validation of this operation may be performed after
     // authorization, we must check here that this resource is a persistent
     // volume. If it isn't, the error will be caught during validation.
     if (Resources::isPersistentVolume(volume)) {
-      request.mutable_object()->set_value(
-          volume.disk().persistence().principal());
+      object->set_value(volume.disk().persistence().principal());
 
       authorizations.push_back(authorizer.get()->authorized(request));
     }
diff --git a/src/master/quota_handler.cpp b/src/master/quota_handler.cpp
index 04639ef..f11aa9d 100644
--- a/src/master/quota_handler.cpp
+++ b/src/master/quota_handler.cpp
@@ -514,8 +514,10 @@ Future<bool> Master::QuotaHandler::authorizeGetQuota(
   authorization::Request request;
   request.set_action(authorization::GET_QUOTA_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   request.mutable_object()->set_value(role);
@@ -539,8 +541,10 @@ Future<bool> Master::QuotaHandler::authorizeUpdateQuota(
   authorization::Request request;
   request.set_action(authorization::UPDATE_QUOTA_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   request.mutable_object()->set_value(role);
@@ -566,8 +570,10 @@ Future<bool> Master::QuotaHandler::authorizeSetQuota(
   authorization::Request request;
   request.set_action(authorization::SET_QUOTA_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   request.mutable_object()->set_value(role);
@@ -594,12 +600,16 @@ Future<bool> Master::QuotaHandler::authorizeRemoveQuota(
   authorization::Request request;
   request.set_action(authorization::DESTROY_QUOTA_WITH_PRINCIPAL);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (requestPrincipal.isSome()) {
-    request.mutable_subject()->set_value(requestPrincipal.get());
+    subject->set_value(requestPrincipal.get());
   }
 
+  authorization::Object* object = request.mutable_object();
+
   if (quotaPrincipal.isSome()) {
-    request.mutable_object()->set_value(quotaPrincipal.get());
+    object->set_value(quotaPrincipal.get());
   }
 
   return master->authorizer.get()->authorized(request);
diff --git a/src/master/weights_handler.cpp b/src/master/weights_handler.cpp
index 4bc060f..9bc1ea3 100644
--- a/src/master/weights_handler.cpp
+++ b/src/master/weights_handler.cpp
@@ -229,8 +229,10 @@ Future<bool> Master::WeightsHandler::authorize(
   authorization::Request request;
   request.set_action(authorization::UPDATE_WEIGHTS_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   list<Future<bool>> authorizations;
diff --git a/src/slave/http.cpp b/src/slave/http.cpp
index dd1f509..18acc67 100644
--- a/src/slave/http.cpp
+++ b/src/slave/http.cpp
@@ -911,8 +911,10 @@ Future<bool> Slave::Http::authorizeEndpoint(
     return Failure("Unexpected request method '" + method + "'");
   }
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-    request.mutable_subject()->set_value(principal.get());
+    subject->set_value(principal.get());
   }
 
   request.mutable_object()->set_value(endpoint);
diff --git a/src/tests/authorization_tests.cpp b/src/tests/authorization_tests.cpp
index 54bfb46..69dbe74 100644
--- a/src/tests/authorization_tests.cpp
+++ b/src/tests/authorization_tests.cpp
@@ -595,7 +595,7 @@ TYPED_TEST(AuthorizationTest, Reserve)
     authorization::Request request;
     request.set_action(authorization::RESERVE_RESOURCES_WITH_ROLE);
     request.mutable_subject()->set_value("foo");
-    request.mutable_subject()->set_value("bar");
+    request.mutable_object()->set_value("bar");
     AWAIT_EXPECT_TRUE(authorizer.get()->authorized(request));
   }
 
@@ -636,6 +636,7 @@ TYPED_TEST(AuthorizationTest, Reserve)
     authorization::Request request;
     request.set_action(authorization::RESERVE_RESOURCES_WITH_ROLE);
     request.mutable_subject()->set_value("baz");
+    request.mutable_object();
     AWAIT_EXPECT_FALSE(authorizer.get()->authorized(request));
   }
 
@@ -1001,6 +1002,7 @@ TYPED_TEST(AuthorizationTest, UpdateQuota)
     authorization::Request request;
     request.set_action(authorization::UPDATE_QUOTA_WITH_ROLE);
     request.mutable_subject()->set_value("foo");
+    request.mutable_object();
     AWAIT_EXPECT_TRUE(authorizer.get()->authorized(request));
   }
 
@@ -1035,6 +1037,7 @@ TYPED_TEST(AuthorizationTest, UpdateQuota)
     authorization::Request request;
     request.set_action(authorization::UPDATE_QUOTA_WITH_ROLE);
     request.mutable_subject()->set_value("bar");
+    request.mutable_object();
     AWAIT_EXPECT_FALSE(authorizer.get()->authorized(request));
   }
 
@@ -1043,6 +1046,7 @@ TYPED_TEST(AuthorizationTest, UpdateQuota)
     authorization::Request request;
     request.set_action(authorization::UPDATE_QUOTA_WITH_ROLE);
     request.mutable_object()->set_value("test");
+    request.mutable_subject();
     AWAIT_EXPECT_TRUE(authorizer.get()->authorized(request));
   }
 
@@ -1053,6 +1057,7 @@ TYPED_TEST(AuthorizationTest, UpdateQuota)
     authorization::Request request;
     request.set_action(authorization::UPDATE_QUOTA_WITH_ROLE);
     request.mutable_subject()->set_value("jeff");
+    request.mutable_object();
     AWAIT_EXPECT_FALSE(authorizer.get()->authorized(request));
   }
 }
@@ -1153,6 +1158,7 @@ TYPED_TEST(AuthorizationTest, SetQuota)
     authorization::Request request;
     request.set_action(authorization::SET_QUOTA_WITH_ROLE);
     request.mutable_subject()->set_value("foo");
+    request.mutable_object();
     AWAIT_EXPECT_TRUE(authorizer.get()->authorized(request));
   }
 
@@ -1187,12 +1193,14 @@ TYPED_TEST(AuthorizationTest, SetQuota)
     authorization::Request request;
     request.set_action(authorization::SET_QUOTA_WITH_ROLE);
     request.mutable_subject()->set_value("bar");
+    request.mutable_object();
     AWAIT_EXPECT_FALSE(authorizer.get()->authorized(request));
   }
 
   // Anyone can set quotas for role "test", so request 6 will pass.
   {
     authorization::Request request;
+    request.mutable_subject();
     request.set_action(authorization::SET_QUOTA_WITH_ROLE);
     request.mutable_object()->set_value("test");
     AWAIT_EXPECT_TRUE(authorizer.get()->authorized(request));
@@ -1205,6 +1213,7 @@ TYPED_TEST(AuthorizationTest, SetQuota)
     authorization::Request request;
     request.set_action(authorization::SET_QUOTA_WITH_ROLE);
     request.mutable_subject()->set_value("jeff");
+    request.mutable_object();
     AWAIT_EXPECT_FALSE(authorizer.get()->authorized(request));
   }
 }
@@ -1290,6 +1299,7 @@ TYPED_TEST(AuthorizationTest, RemoveQuota)
     authorization::Request request;
     request.set_action(authorization::DESTROY_QUOTA_WITH_PRINCIPAL);
     request.mutable_subject()->set_value("ops");
+    request.mutable_object();
     AWAIT_EXPECT_TRUE(authorizer.get()->authorized(request));
   }
 
-- 
2.8.3

```


src/authorizer/local/authorizer.cpp (line 422)
<https://reviews.apache.org/r/47509/#comment199429>

    This looks like a really useful pattern. Mind adding a `TODO` that we should generate
such a check automatically with proto reflection, implemented e.g., with with tools similar
to the ones used here:
    
      https://github.com/google/protobuf/blob/master/src/google/protobuf/text_format.cc#L1568


- Benjamin Bannier


On May 18, 2016, 5:10 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47509/
> -----------------------------------------------------------
> 
> (Updated May 18, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Alexander Rojas, and Vinod Kone.
> 
> 
> Bugs: MESOS-5405
>     https://issues.apache.org/jira/browse/MESOS-5405
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Fixes a number of a authorization::Request initializings that were
> missing required fields. Also adds a CHECK to the LocalAuthorizer
> helping us to catch and prevent these problems in the future.
> 
> 
> Diffs
> -----
> 
>   src/authorizer/local/authorizer.cpp e95435327bb3b6f447e814b8657bce8084535346 
>   src/common/http.cpp ad6a4b44af3ec847a3a6c0839a88fba18cda5011 
>   src/master/http.cpp 6cefcc8b6ddcfe4098521e838bfdc95936fe7476 
>   src/master/master.cpp 35b428b0f7dee5954514d8860cfc498271ccf267 
>   src/master/quota_handler.cpp 6590519d8440f352f5bf00fda805414a5aad725c 
>   src/master/weights_handler.cpp 4bc060fdb015df6658194eef92fe11b14aa15c79 
>   src/slave/http.cpp 0fb9b81c4a87250e454e3380c61cf9037454810b 
>   src/tests/authorization_tests.cpp f50ac69c32d0551a63391d97f31559ff4f414011 
> 
> Diff: https://reviews.apache.org/r/47509/diff/
> 
> 
> Testing
> -------
> 
> make check (OSX)
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message