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




include/mesos/authorizer/authorizer.hpp (line 43)
<https://reviews.apache.org/r/44322/#comment185106>

    s/it  does/it does/



include/mesos/authorizer/authorizer.proto (lines 27 - 28)
<https://reviews.apache.org/r/44322/#comment185097>

    How about "If `value` is not set the subject is unknown."



include/mesos/authorizer/authorizer.proto (line 37)
<https://reviews.apache.org/r/44322/#comment185098>

    s/unix//
    
    You need to also add: "If `value` is not set the object is unknown".



include/mesos/authorizer/authorizer.proto (line 57)
<https://reviews.apache.org/r/44322/#comment185101>

    2 blank lines.



include/mesos/authorizer/authorizer.proto (line 58)
<https://reviews.apache.org/r/44322/#comment185105>

    <`subject`, `action`, `object`>



include/mesos/authorizer/authorizer.proto (line 59)
<https://reviews.apache.org/r/44322/#comment185102>

    s/can/Can/



src/authorizer/local/authorizer.hpp (lines 51 - 55)
<https://reviews.apache.org/r/44322/#comment185109>

    this comment should be in the review that introduced this overload.



src/authorizer/local/authorizer.cpp (lines 83 - 88)
<https://reviews.apache.org/r/44322/#comment185225>

    This method's signature is not intuitive. For example, what's a selector? 
and why objects need a selector but not subjects? I know the reason for the 
latter, but you understand what i'm getting at. 
    
    One proposal:
    
    ```
    struct GenericACL {
      required ACL::Entity subjects;
      required ACL::Entity objects;
    }
    
    Future<bool> authorized(const authorization::Request& request)
    {
      switch (request.action()) {
        case authorization::REGISTER_FRAMEWORK_WITH_ROLE:
        
          // Convert "RegisterFramework" ACL to a generic ACL.
          vector<GenericACL> acls;
          foreach (const ACL::RegisterFramework& acl, 
acls.register_frameworks()) {
            GenericACL acl_;
            acl_.subjects = acl.principals();
            acl_.object = acl.roles();
            acls.push_back(acl_);
          }
    
          return authorized(request, acls);
    
        ...
        ...
        ...
      }
    }
    
    Future<bool> authorized(const authorization::Request& request, const 
vector<GenericACL>& acls)
    {
      ...
    }
      
    ```
    
    If there is a way to templatize the conversion to GenericACL that would be 
better. But even without that, the above code is easier to follow IMO.



src/authorizer/local/authorizer.cpp (line 160)
<https://reviews.apache.org/r/44322/#comment185120>

    no need for default. the local authorizer should be never called with an 
action it doesn't understand.



src/master/master.cpp (line 2892)
<https://reviews.apache.org/r/44322/#comment185227>

    s/however/However/



src/master/master.cpp (line 2939)
<https://reviews.apache.org/r/44322/#comment185232>

    new line.



src/master/master.cpp (line 3041)
<https://reviews.apache.org/r/44322/#comment185231>

    new line.


- Vinod Kone


On March 10, 2016, 2:15 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44322/
> -----------------------------------------------------------
> 
> (Updated March 10, 2016, 2:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-2950
>     https://issues.apache.org/jira/browse/MESOS-2950
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implements the [Generic Authorizer Interface v 
> 0.3.1](https://docs.google.com/document/d/1-XARWJFUq0r_TgRHz_472NvLZNjbqE4G8c2JL44OSMQ).
> 
> It effectively separates the language used to define ACLs from the
> language used to query it allowing for arbitrary identity server
> backends.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.hpp 
> 705482656788ac12e9d21e355b71fd2ede2be558 
>   include/mesos/authorizer/authorizer.proto PRE-CREATION 
>   src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f 
>   src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 
>   src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 
>   src/authorizer/local/authorizer.hpp 
> c87a9915bae6bae7744bd57abd12e8d857181051 
>   src/authorizer/local/authorizer.cpp 
> 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 
>   src/master/http.cpp 7304bfd5350d763d9ed1d5acdc285874b6d8f5df 
>   src/master/master.cpp 249e82ffcef35aa8df3c5b9faef5b9b25d68facc 
>   src/master/quota_handler.cpp a41c91f10bc0eedc754425b4de1b3e184c4ffb08 
>   src/master/weights_handler.cpp 9e4ab19fd760a56f1bbce915d1c7b63a0d1e5ed5 
>   src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 
>   src/tests/master_authorization_tests.cpp 
> 29c89fb11da792c3e71eb880a19657ea225b3cc8 
>   src/tests/mesos.hpp 9409da7ffe81ab4b1fc01213e27f1f639ba36581 
>   src/tests/mesos.cpp 577072136415191334c74b69a1c0b7eed8e77e8b 
>   src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 
> 
> Diff: https://reviews.apache.org/r/44322/diff/
> 
> 
> Testing
> -------
> 
> make -j4 check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to