mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 38094: Added implementation of Http Basic authentication scheme.
Date Tue, 29 Dec 2015 22:56:45 GMT

-----------------------------------------------------------
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
> 
>


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