----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56619/#review165838 -----------------------------------------------------------
I personally find this patch way too big, so at the end I found myself just going through the changeset, perhaps breaking it into the _master_, _agent_ and _files_ components will help reviewers. What do you think? src/files/files.hpp (line 89) <https://reviews.apache.org/r/56619/#comment237673> Could you indent this more... or less... not sure how, but it is hard to tell wether `authorized` is part of the type declaration or the name of the parameter. src/files/files.cpp (line 188) <https://reviews.apache.org/r/56619/#comment237684> Same here, it is not clear that `authorizations` is not part of type. Although this is what clang format returns I think either: ```c++ hashmap< string, lambda::function<Future<bool>(const Option<AuthenticationContext>&)>> authorizations; ``` or ```c++ hashmap< string, lambda::function<Future<bool>(const Option<AuthenticationContext>&)> > authorizations; ``` are more readable. src/master/http.cpp (line 520) <https://reviews.apache.org/r/56619/#comment237685> Any readon why `validation::master::call::validate(...)` cannot take the whole context as parameter? src/master/http.cpp (lines 655 - 657) <https://reviews.apache.org/r/56619/#comment237686> Note to self: This may be a bug that violates the semantics of `Subject`, write a test. src/master/http.cpp (lines 795 - 797) <https://reviews.apache.org/r/56619/#comment237687> This appears here, there and everywhere, that I'm considering it may be a good idea to abstract it to something like: ``` static Option<string> AuthenticationContext::extractPrincipal(const Option<AuthenticationContext>& context); ``` That will make it easier if one day we need to modify how the extraction works. src/master/http.cpp (lines 1477 - 1479) <https://reviews.apache.org/r/56619/#comment237689> likewise, this looks like a prime candidate to be abstracted away. src/master/weights_handler.cpp (line 52) <https://reviews.apache.org/r/56619/#comment237692> Not sure if using `using` here is the way to go, we don't seem to use it for `http::Request`. This goes with my idea of just using `http::authentication::Context` as the name type. - Alexander Rojas On Feb. 14, 2017, 1:14 a.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56619/ > ----------------------------------------------------------- > > (Updated Feb. 14, 2017, 1:14 a.m.) > > > Review request for mesos, Adam B, Alexander Rojas, Jan Schlicht, Till > Toenshoff, and Vinod Kone. > > > Bugs: MESOS-7003 > https://issues.apache.org/jira/browse/MESOS-7003 > > > Repository: mesos > > > Description > ------- > > This patch updates the HTTP endpoint handlers in Mesos > to accept an `AuthenticationContext` instead of an > `Option<string>& principal`. > > > Diffs > ----- > > src/files/files.hpp 8cffc26fc7d674187e55663f23f1e10bed40229e > src/files/files.cpp 8327f8002fbfa3be77a4bbe4aa83a73d0f170f7a > src/master/http.cpp c5324abc0db82275fd65d3f7d361ad8ee9e017d1 > src/master/master.hpp 764adb18ddf09b62529c5c96f8e4dfaf7803483e > src/master/master.cpp dff320acd1a13995d51cc0c303edea0cdfaa336d > src/master/quota_handler.cpp 3ad28e4a9363a877d0610b529a6c17fb30ece37a > src/master/registrar.cpp d7134eea34102ab7b24d2f0131363bdd9005cfd3 > src/master/weights_handler.cpp da0b995e1866e7eb57b8ef885864c6c6d66405d6 > src/slave/http.cpp ccd489d700a605dc41037e00dbd6a7cb3b6edcc6 > src/slave/slave.hpp 3b0aea4e3e9a17501077beccbccaab4abbe11af2 > src/slave/slave.cpp ebba8e16bc9ec45781183e78cb5a3c351a5f65f5 > > Diff: https://reviews.apache.org/r/56619/diff/ > > > Testing > ------- > > Testing information can be found at the end of this review chain. > > > Thanks, > > Greg Mann > >
