> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, lines 407-410
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line407>
> >
> >     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);
> >     ```

Seems fixed in https://reviews.apache.org/r/39986


> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, lines 427-430
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line427>
> >
> >     isn't this identical to the case above?
> >     if not, care to add a comment as to what differs?

https://reviews.apache.org/r/39986


> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, line 436
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line436>
> >
> >     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)

https://reviews.apache.org/r/39986


> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, line 397
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line397>
> >
> >     the comment here is wrong

https://reviews.apache.org/r/39986


> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, lines 376-377
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line376>
> >
> >     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 :)

https://reviews.apache.org/r/39986


> On Aug. 11, 2015, 10:17 p.m., Marco Massenzio wrote:
> > src/tests/authorization_tests.cpp, line 360
> > <https://reviews.apache.org/r/37110/diff/1/?file=1032445#file1032445line360>
> >
> >     not sure what the "rules" are in tests, but shouldn't you use an 
> > `Owned` or `unique_ptr` instead of the "naked" ptr?

That's what you get from the protobuf factory.


- Till


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


On Sept. 12, 2015, 9:30 a.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37110/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2015, 9:30 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, and Till Toenshoff.
> 
> 
> 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