----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44678/#review124151 -----------------------------------------------------------
Needs more const references, but otherwise in nearly-shippable shape. include/mesos/authentication/http/basic_authenticator_factory.hpp (line 41) <https://reviews.apache.org/r/44678/#comment186540> std::string& include/mesos/authentication/http/basic_authenticator_factory.hpp (line 45) <https://reviews.apache.org/r/44678/#comment186541> std::string& src/authentication/http/basic_authenticator_factory.cpp (line 44) <https://reviews.apache.org/r/44678/#comment186542> string& src/authentication/http/basic_authenticator_factory.cpp (line 68) <https://reviews.apache.org/r/44678/#comment186545> Looks like you're parsing it as a JSON::Value, not a JSON::Array, right? src/authentication/http/basic_authenticator_factory.cpp (line 75) <https://reviews.apache.org/r/44678/#comment186546> Could you elaborate on the error returned from protobuf::parse, to put it into scope? `Error("Unable to parse credentials for basic authenticator: " + credentials_.error());` src/authentication/http/basic_authenticator_factory.cpp (line 83) <https://reviews.apache.org/r/44678/#comment186547> s/module //, since this is not necessarily for a module. src/authentication/http/basic_authenticator_factory.cpp (lines 87 - 91) <https://reviews.apache.org/r/44678/#comment186544> Is it ok to specify a realm but no credentials? Does that just mean that nobody can authenticate? Is that still a valid authenticator? src/authentication/http/basic_authenticator_factory.cpp (line 96) <https://reviews.apache.org/r/44678/#comment186548> string& src/examples/test_http_authenticator_module.cpp (line 43) <https://reviews.apache.org/r/44678/#comment186549> "Failed to create basic HTTP authenticator: "? src/tests/http_authentication_tests.cpp (line 88) <https://reviews.apache.org/r/44678/#comment186550> s/Auth/Authenticator/ src/tests/http_authentication_tests.cpp (lines 89 - 90) <https://reviews.apache.org/r/44678/#comment186551> const Option<foo>& (const refs!) src/tests/http_authentication_tests.cpp (line 95) <https://reviews.apache.org/r/44678/#comment186557> Any reason you called it `entry` instead of `parameter`? src/tests/http_authentication_tests.cpp (line 100) <https://reviews.apache.org/r/44678/#comment186556> Why the second check? This is a test helper, and a test may want to test with an empty credential set. Why the unnecessary restriction? - Adam B On March 17, 2016, 5:20 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44678/ > ----------------------------------------------------------- > > (Updated March 17, 2016, 5:20 p.m.) > > > Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till > Toenshoff. > > > Bugs: MESOS-4850 > https://issues.apache.org/jira/browse/MESOS-4850 > > > Repository: mesos > > > Description > ------- > > Modified basic HTTP authenticator creator to accept realm. > > To accommodate different authentication realms for the master and agent, the > default basic HTTP authenticator needs to accept its authentication realm as > a parameter. This patch adds this parameter and modifies the HTTP > authentication tests to validate it appropriately. A new test was also added: > `HttpAuthenticationTest.BasicWithoutRealm`. > > > Diffs > ----- > > include/mesos/authentication/http/basic_authenticator_factory.hpp > c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 > src/authentication/http/basic_authenticator_factory.cpp > 62f851685db3b42c52bbcb7cff3e4f4703004ed7 > src/examples/test_http_authenticator_module.cpp > 459b7046bd76d3043d2484a2dd30c10d7deaedd4 > src/master/master.cpp e6290ea686ccf17813d6faeaf2f2012f79cf3b7f > src/tests/http_authentication_tests.cpp > cf2bb762272fa38e04e5c26aef2858300bbd0459 > > Diff: https://reviews.apache.org/r/44678/diff/ > > > Testing > ------- > > HTTP authentication tests were updated to pass the authentication realm to > the basic HTTP authenticator, and to adhere to the new credentials format in > the module parameters. A new test was also added: > `HttpAuthenticationTest.BasicWithoutRealm` > > `make check` was used to test on both OSX and CentOS 7. > > > Thanks, > > Greg Mann > >
