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