> On March 14, 2016, 9:38 a.m., Adam B wrote: > > src/authentication/http/basic_authenticator_factory.cpp, lines 63-64 > > <https://reviews.apache.org/r/44678/diff/3/?file=1296983#file1296983line63> > > > > Seems like you're changing the meaning of the parameters in > > `BasicAuthenticatorFactory::create(Parameters)`. Before, `Parameters` was > > just a convenient protobuf for username/password key-value pairs. Now, > > you're interpreting the keys as module parameter names. Are you actually > > introducing a new "authentication_realm" module parameter to the > > Authenticator module interface? > > Why not just use `BasicAuthenticatorFactory::create(const string realm, > > const Parameters& parameters)`?
One could certainly use the other overloads of `create` to specify a realm, but if we don't provide a way to include the realm in the `Parameters`, then we still need a default realm for this version of `create`, and I was trying to eliminate the default realm from this file. AFAICT, there isn't a well-defined interface for the HTTP authenticator modules, with respect to their input parameters. I didn't find mention of it in the [design doc](https://docs.google.com/document/d/1kM3_f7DSqXcE2MuERrLTGp_XMC6ss2wmpkNYDCY5rOM/edit#heading=h.bbxx6wwg3tpe), or in related JIRAs. I was working under the assumption that the structure of the input parameters for any given HTTP authN module is defined arbitrarily by the module's implementation, but perhaps that's not the case? - Greg ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44678/#review123389 ----------------------------------------------------------- On March 14, 2016, 6:01 p.m., Greg Mann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44678/ > ----------------------------------------------------------- > > (Updated March 14, 2016, 6:01 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. > > > Diffs > ----- > > include/mesos/authentication/http/basic_authenticator_factory.hpp > c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 > src/authentication/http/basic_authenticator_factory.cpp > 62f851685db3b42c52bbcb7cff3e4f4703004ed7 > src/master/master.cpp d0380db3b90a9166607445f8dd50cc63d547228e > src/tests/http_authentication_tests.cpp > cf2bb762272fa38e04e5c26aef2858300bbd0459 > > Diff: https://reviews.apache.org/r/44678/diff/ > > > Testing > ------- > > `make check` was used to test on both OSX and CentOS 7. > > > Thanks, > > Greg Mann > >
