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


The tests look great! Just a minor comment about Forbidden (sorry I sent you 
down that path). Should be good to commit after another iteration.


3rdparty/libprocess/src/authenticator.cpp (lines 53 - 54)
<https://reviews.apache.org/r/38094/#comment172593>

    Are these const?



3rdparty/libprocess/src/authenticator.cpp (lines 58 - 84)
<https://reviews.apache.org/r/38094/#comment172592>

    These should go at the bottom, under the implementation of 
BasicAuthenticatorProcess::authenticate.



3rdparty/libprocess/src/authenticator.cpp (line 104)
<https://reviews.apache.org/r/38094/#comment172599>

    Could we avoid the .get() here and just store the option?
    
    ```
    Option<string> authorization = request.headers.get("Authorization");
    
    if (authorization.isNone()) {
      return unauthorized;
    }
    
    vector<string> components = strings::split(authorization.get(), " ");
    ```



3rdparty/libprocess/src/authenticator.cpp (lines 121 - 123)
<https://reviews.apache.org/r/38094/#comment172596>

    Sorry, looking at this again, looks like this should also be 401 with a 
challenge?
    
    https://tools.ietf.org/html/rfc7235#section-3.1
    
    ```
       If the request included authentication credentials, then the 401
       response indicates that authorization has been refused for those
       credentials.
    ```
    
    Looks like 403 is the real "unauthorized", in the sense that if the request 
was authenticated but the user is not authorized to access to the resource, 
then 403 should be returned: https://tools.ietf.org/html/rfc7231#section-6.5.3
    
    So, `AuthenticationResult.forbidden` should be set only when no-one is 
allowed and therefore we'll never issue a challenge? Perhaps we should clarify 
this in the documentation of AuthenticationResult?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1389)
<https://reviews.apache.org/r/38094/#comment172601>

    One more newline here



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1390 - 1446)
<https://reviews.apache.org/r/38094/#comment172602>

    Very nice!


- Ben Mahler


On Dec. 14, 2015, 12:53 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38094/
> -----------------------------------------------------------
> 
> (Updated Dec. 14, 2015, 12:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3232
>     https://issues.apache.org/jira/browse/MESOS-3232
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/Makefile.am 82778bbd34cfcf54b4268e019d634eb64e506d55 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 5a32e9a38a0bec7aa3faef23b792f3bf3d659d4f 
>   3rdparty/libprocess/src/CMakeLists.txt 
> 681f0cfec57e152568da41698c8bdd52c05f65a6 
>   3rdparty/libprocess/src/authenticator.cpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 9fe27039416290296e43c6327c85721342d02cb9 
> 
> Diff: https://reviews.apache.org/r/38094/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Reply via email to