mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 44678: Modified basic HTTP authenticator creator to accept realm.
Date Fri, 11 Mar 2016 15:59:56 GMT


> On March 11, 2016, 2:52 p.m., Alexander Rojas wrote:
> > src/authentication/http/basic_authenticator_factory.cpp, lines 70-71
> > <https://reviews.apache.org/r/44678/diff/2/?file=1295691#file1295691line70>
> >
> >     Wouldn't it be better to just get a `Credentials` object?
> >     
> >     Or at least in the documentation add an example of how exactly a module protobuf
would look like. (see my comment's in the next patch)

My reasoning for doing it this way was to avoid having nested "credentials" keys. i.e., if
we parse directly to `Credentials` here we end up with parameters like:

{
  "key": "credentials",
  "value": {
    "credentials": [
      {
        "principal": "user",
        "secret": "password"
      }
    ]
  }
}

Though, the way I've implemented it, it doesn't exactly mirror the structure of the `--credentials`
flag due to the structure of the `Parameters` message. I think I still prefer the current
solution, however. I updated the Doxygen comments as you suggested :-)


- Greg


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44678/#review123144
-----------------------------------------------------------


On March 11, 2016, 9:39 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44678/
> -----------------------------------------------------------
> 
> (Updated March 11, 2016, 9:39 a.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 249e82ffcef35aa8df3c5b9faef5b9b25d68facc 
>   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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message