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