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 Sat, 12 Dec 2015 01:59:32 GMT

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


Thanks for the test, main question is why BasicAuthenticator is not a Process? Was that an
oversight, or were you intending for it to be pure-const after construction and therefore
thread-safe?


3rdparty/libprocess/include/process/authenticator.hpp (lines 82 - 97)
<https://reviews.apache.org/r/38094/#comment169840>

    This is going to be accessed concurrently, so shouldn't this be a Process? Especially
since you've marked 'authenticate' as non-const? (I think it could have been const)



3rdparty/libprocess/src/authenticator.cpp (lines 47 - 49)
<https://reviews.apache.org/r/38094/#comment169842>

    Renaming 'result' to 'unauthorized' will look a bit clearer in the returns below. What
about Forbidden here? If the credentials are bad, why do we re-issue the same challenge?



3rdparty/libprocess/src/authenticator.cpp (lines 57 - 59)
<https://reviews.apache.org/r/38094/#comment169843>

    Isn't the empty case caught below when you're parsing with the split?



3rdparty/libprocess/src/authenticator.cpp (lines 79 - 82)
<https://reviews.apache.org/r/38094/#comment169841>

    Rather than re-using the object by setting unauthorized to none here, let's just use a
different object here to keep it simple.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1387 - 1389)
<https://reviews.apache.org/r/38094/#comment169849>

    How about:
    
    // Tests the "Basic" authenticator.
    
    Not sure we need to say the rest?



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

    Why repeat Authentication here? s/AuthenticationBasic/Basic/ will do



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

    How about: s/testuser/user/
    
    Why did you need the 'principal' variable here? Looks like you don't use it, also we can
just hard code "user" and "password" to keep the test really simple.



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

    Why the temporary Owned variable here?



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

    Can you rebase? I did a s/TEST_AUTHENTICATION_REALM/"realm"/ in your previous patch



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

    How about: s/testpassword/password/



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

    What's a "pass"? ;)


- Ben Mahler


On Dec. 11, 2015, 2:01 p.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38094/
> -----------------------------------------------------------
> 
> (Updated Dec. 11, 2015, 2:01 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 2de75ca1c7e224c36b534c368e7379dc158aa5bb

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