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



src/tests/authorization_tests.cpp (line 360)
<https://reviews.apache.org/r/37110/#comment149772>

    not sure what the "rules" are in tests, but shouldn't you use an `Owned` or 
`unique_ptr` instead of the "naked" ptr?



src/tests/authorization_tests.cpp (lines 376 - 377)
<https://reviews.apache.org/r/37110/#comment149773>

    what happens if the request comes from `foo` and `quz`? do I still get to 
reserve the resource?
    Should `quz` get it? shouldn't it?
    
    Can you please add a few comments in the methods above (and correspondingly 
in the tests?)
    
    When it comes to security / ACLs, always best to have enough info for the 
guys who will have to solve issues; that's usually never done with the luxury 
of time, when it comes to security :)



src/tests/authorization_tests.cpp (line 397)
<https://reviews.apache.org/r/37110/#comment149774>

    the comment here is wrong



src/tests/authorization_tests.cpp (lines 407 - 410)
<https://reviews.apache.org/r/37110/#comment149776>

    I'll be honest with you: this looks to me upside down :)
    
    I'd have: ANY principal can unreserve NONE's resources.
    In fact, what would happen if one did:
    ```
      acl4->mutable_principals()->set_type(mesos::ACL::Entity::ANY);
      acl4->mutable_reserver_principals()->set_type(mesos::ACL::Entity::NONE);
    ```
    
    And what happens if I do:
    ```
      acl4->mutable_principals()->set_type(mesos::ACL::Entity::NONE);
      acl4->mutable_reserver_principals()->set_type(mesos::ACL::Entity::NONE);
    ```



src/tests/authorization_tests.cpp (lines 427 - 430)
<https://reviews.apache.org/r/37110/#comment149777>

    isn't this identical to the case above?
    if not, care to add a comment as to what differs?



src/tests/authorization_tests.cpp (line 436)
<https://reviews.apache.org/r/37110/#comment149778>

    can we add a test for `ops` trying to unreserve multiple principals' 
resources? "foo", "bar" and "quz"'s
    (is this even allowed? if not, let's make sure it fails)


- Marco Massenzio


On Aug. 5, 2015, 9:58 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37110/
> -----------------------------------------------------------
> 
> (Updated Aug. 5, 2015, 9:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Bugs: MESOS-3062
>     https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f 
>   src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc 
>   src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 
>   src/tests/mesos.hpp 20418d4fbd2f4ae35ee0c707472cbf37125883b0 
> 
> Diff: https://reviews.apache.org/r/37110/diff/
> 
> 
> Testing
> -------
> 
> Added tests to `src/tests/authorization_tests.cpp` + `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Reply via email to