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