----------------------------------------------------------- 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 > >
