----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61925/#review185728 -----------------------------------------------------------
src/slave/http.cpp Lines 209-214 (original), 209-214 (patched) <https://reviews.apache.org/r/61925/#comment262042> Small nit: I think the following alternative would be a bit more readable: ``` if (authorizeTask_->accept(*task, framework_->info)) { writer->element(*task); } ``` Ditto in the rest of file. src/slave/http.cpp Lines 869-872 (original), 860-864 (patched) <https://reviews.apache.org/r/61925/#comment262080> I'd do: ``` if (acceptor->accept()) { return OK(_flags(), request.url.query.get("jsonp")); } return Forbidden(); ``` Ditto in the rest of the file. src/slave/http.cpp Lines 1951-1961 (original), 1868-1879 (patched) <https://reviews.apache.org/r/61925/#comment262089> Why do you prefer to reimplement part of `authorizeEndpoint()` here? If we do this, we might want to log the same thing that method logs: ``` LOG(INFO) << "Authorizing principal '" << (principal.isSome() ? stringify(principal.get()) : "ANY") << "' to " << method << " the '" << endpoint << "' endpoint"; ``` And we should also update `Http::containers()`. src/tests/slave_authorization_tests.cpp Line 1266 (original), 1266-1268 (patched) <https://reviews.apache.org/r/61925/#comment262091> do we need `containers.json` too? - Gastón Kleiman On Sept. 14, 2017, 1:50 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61925/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2017, 1:50 a.m.) > > > Review request for mesos, Anand Mazumdar, Alexander Rojas, and Gastón Kleiman. > > > Bugs: MESOS-7914 > https://issues.apache.org/jira/browse/MESOS-7914 > > > Repository: mesos > > > Description > ------- > > This patch updates the agent endpoint handlers to make use of > the `AuthorizationAcceptor` exclusively for authorization, > eliminating the need to explicitly create authorization > subjects and objects. > > Endpoint-related slave authorization tests are also updated to > accommodate this change. > > > Diffs > ----- > > src/slave/http.hpp 44a95dec4c9b8bb65d712c5538bbd7afffe2cf7b > src/slave/http.cpp 3ea7829df8c1c35d2fa3a44f19a60b7e261042ce > src/tests/slave_authorization_tests.cpp > 30eceae0920351cffc9c3393c6d08917c4041c1a > > > Diff: https://reviews.apache.org/r/61925/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Greg Mann > >
