Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 11, 2015, 3:01 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Changes after bernd's review.


Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/http.hpp 
c9e38e5a382f29f87a2ed62602cae3d62f8e16cc 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/authentication_router.hpp 
5777deafd7420324627802a7ab9da9aaa2b46825 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

Diff: https://reviews.apache.org/r/38000/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Ben Mahler]

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

Ship it!


I'll make the changes below in order to avoid more round trips on reviewboard. 
The biggest change was related to simplifying the pipelining code between the 
http proxy and the process manager, hope you like it better now!

Take a look over the comments, let me know if you think we should follow up on 
anything:

```
commit 6bd9b65b9d93f154ff9187fb5e5136262ede043c
Author: Alexander Rojas 
Date:   Fri Dec 11 12:30:37 2015 -0800

Allow users of libprocess to perform authentication of HTTP requests.

This change integrates the previously added AuthenticationRouter into
libprocess in order to allow users to authenticate HTTP requests. Now,
when making a 'route', a security 'realm' can be specified. The user
of libprocess determines which Authenticator will be used for each
'realm'.

Review: https://reviews.apache.org/r/38000
```


3rdparty/libprocess/include/process/event.hpp (line 121)


const ref here



3rdparty/libprocess/include/process/process.hpp (lines 237 - 238)


One newline here :)



3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 53)


"... rathern than having the authentication router maintain the mapping"



3rdparty/libprocess/src/process.cpp (lines 238 - 256)


Hm.. it looks like the complexity here is coming from the fact that we 
currently use the router outside of the HttpProxy and we wanted to pipeline 
per-UPID.

We can do a big simplification here by using the router in the http proxy 
itself and punting on the per-UPID pipelining in favor of per-socket 
pipelining, since that was just a performance optimization (not worth the 
complexity cost after seeing how the code turned out).

We end up with the following in the proxy (comments omitted):

```
HttpProxy
{

Future authenticate(const Request& request)
{
  Future authentication =
authentication_router->authenticate(request);

  return authentications.add(
  [=]() { return authentication; });
}

Sequence authentications;
}
```

And we don't have to have any cleanup code. Then the process manager code 
looks like:

```
dispatch(proxy, ::authenticate, *request)
  .onAny([this, request, promise, receiver](
  const Future& authentication) {
  
```

Which looks a lot better! :)



3rdparty/libprocess/src/process.cpp (line 2485)


Should we s/authorized/authenticated/ here since that's really what this 
means?



3rdparty/libprocess/src/process.cpp (line 2498)


Double // here?



3rdparty/libprocess/src/process.cpp (line 3367)


We probably need a little comment here and in removeEndpoint that we don't 
need to wait for the operations to complete.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 51 - 54)


Why did you introduce these when all of the other tests are still using the 
http:: prefix? This makes it really inconsistent: for example, if I look at the 
mock method introduced to HttpProcess, it is the only one without the http 
qualifiers.



3rdparty/libprocess/src/tests/http_tests.cpp (line 76)


Let's just use "real" inline to keep it simple, no need to introduce a 
global :)



3rdparty/libprocess/src/tests/http_tests.cpp (lines 105 - 109)


At this point we would just wrap with the open paren, but this fits on one 
line when we just use "realm" instead of the global.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1219)


Why call this 'addAuthenticator' instead of 'setAuthenticator' to match the 
interface we're wrapping?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1227 - 1229)


We actually have to wait here, otherwise we can trigger the leak of a mock 
object if we exit after leaving tear down but before the unset takes place, 
leading to:

```
../../../3rdparty/libprocess/src/tests/http_tests.cpp:1334: ERROR: this 
mock object (used in test HttpAuthenticationTest.Pipelining) should be deleted 
but never is. Its address is @0x7feca1603300.
```

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/include/process/process.hpp, lines 65-86
> > 
> >
> > Why isn't this in the http header? It looks like firewall belongs there 
> > too since it's more aptly namespaced as 'http::Firewall'?

I will move the firewall functions in a different request.


> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1205-1223
> > 
> >
> > Can you instead just add another handler to the existing HttpProcess 
> > above, rather than introducing another Process here?
> > 
> > There is already a handler for 'auth', so we can add one called 
> > '/authenticated' and document why '/auth' is still there (looks like it is 
> > going to be obviated by this?).

About this one, there is one test that checks authentication which is handled 
through the auth method. That test (and its functionality) become obsolete with 
this patch, should I just removed?


> On Dec. 8, 2015, 8:52 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2400-2402
> > 
> >
> > Why are you dispatching here rather than calling directly? Note though 
> > that I'm not sure we should keep `schedule`.

Mostly because BenH has strongly discourage me from calling methods directly 
from processes. Moreover, once the cleaning of unused sequences is implemented, 
is imposible to guarantee that requests are being done from the same thread 
always, so we may have concurrency issues.


- Alexander


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


On Dec. 8, 2015, 3:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Dec. 8, 2015, 3:38 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/authentication_router.hpp 
> 5777deafd7420324627802a7ab9da9aaa2b46825 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

> On Dec. 9, 2015, 12:19 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1339-1343
> > 
> >
> > Hm.. it looks like you're using the principals here to check that the 
> > requests arrive to the endpoint in the right order?
> > 
> > How about avoiding the captures here and just specifying "principal1" 
> > and "principal2" in the expectation?
> > 
> > Also, you're returning "1" and "2" but that's not checked at all? Did 
> > you mean to check that the responses below are ordered correctly?

It is not enough to just set the principal in the expectation, since the 
expectation only says that the function will be called with the given 
parameters, but mentions nothing about the ordering.


> On Dec. 9, 2015, 12:19 a.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 1373-1375
> > 
> >
> > It's not clear why the sleep is here. If I had to guess, it looks like 
> > you wanted to ensure that the both authentication calls were made at this 
> > point? If so, we don't need a sleep for this.
> > 
> > Let's try to avoid the sleep here if possible.

When pipelining is not enforced, without the sleep the test is just flaky, 
since often the messages will be executed in the right order. I can remove it, 
but as I mentioned, the test passes from failing everytime to only sometimes.


- Alexander


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


On Dec. 8, 2015, 3:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Dec. 8, 2015, 3:38 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/authentication_router.hpp 
> 5777deafd7420324627802a7ab9da9aaa2b46825 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 10, 2015, 3:25 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Changes after ben's review.


Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/http.hpp 
3f33b46527e07ed73621e07e9c5d40594e32f6a9 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/authentication_router.hpp 
5777deafd7420324627802a7ab9da9aaa2b46825 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

Diff: https://reviews.apache.org/r/38000/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 8, 2015, 11:23 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Changes
---

Alex's review.


Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

Diff: https://reviews.apache.org/r/38000/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

> On Dec. 7, 2015, 11:51 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 3218-3220
> > 
> >
> > If this is an internal invariant, how about adding a `CHECK` here?

How would the check be liked? if you read the comment it either has a principal 
because otherwise authentication failed or there is no principal because 
authentication wasn't needed at all. I changed the comment though.


- Alexander


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


On Dec. 7, 2015, 3:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Dec. 7, 2015, 3:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 8, 2015, 3:38 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/authentication_router.hpp 
5777deafd7420324627802a7ab9da9aaa2b46825 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

Diff: https://reviews.apache.org/r/38000/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Ben Mahler]

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


This is starting to shape up, thanks Alexander. As we discussed offline, just 
wanted to surface the main bits of feedback so you can start addressing them. I 
will follow up with feedback for the tests, wasn't able to do a complete pass 
in one sitting.


3rdparty/libprocess/include/process/process.hpp (lines 65 - 86)


Why isn't this in the http header? It looks like firewall belongs there too 
since it's more aptly namespaced as 'http::Firewall'?



3rdparty/libprocess/include/process/process.hpp (lines 68 - 73)


How about the following to be a bit more succinct?

```
/**
 * Sets (or overwrites) the authenticator for the realm.
 * 
 * Every incoming HTTP request to an endpoint associated
 * with the realm will be authenticated with the given
 * authenticator.
 */
```



3rdparty/libprocess/include/process/process.hpp (lines 79 - 82)


Not sure we need to say "still", how about:

```
/**
 * Unsets the authenticator for the realm.
 * 
 * Any endpoint mapped to the realm will no
 * longer be authenticated.
 */
```



3rdparty/libprocess/include/process/process.hpp (lines 261 - 263)


I'm guessing this is about introducing a type to make this more explicit in 
the signature, not type safety:

```
TODO(arojas): Consider introducing an `authentication::Principal` type.
```

Similarly, we probably need a TODO on top of the `route()` declarations for 
an `authentication::Realm` type?



3rdparty/libprocess/include/process/process.hpp (lines 261 - 264)


Looks like this needs javadoc like the existing typedef, then you can 
remove the `/* principal */` that you've added here.



3rdparty/libprocess/include/process/process.hpp (lines 372 - 374)


There is a typo in here :)

Also, could you try to avoid the run on sentence? How about saying 
something about http request handlers are equivalent to authenticated http 
request handlers with a none principal, so we convert them.



3rdparty/libprocess/src/authentication_router.hpp (lines 52 - 56)


The second sentence here confused me (e.g. the current implementation of 
what?), so how about we just write the first sentence and let the reader figure 
out the implications? Note that the implications could change over time so it's 
possible for it to go out of sync and cause more confusion.



3rdparty/libprocess/src/process.cpp (lines 231 - 232)


We really need to help the reader understand this!

```
// Http pipelining requires that when we perform http
// authentication, we need to ensure that authentication
// results are processed in sequential order. Otherwise,
// we may send responses out-of-order!
// 
// The ordering needs to be respected within a Process, but
// not across Processes since dispatches are asynchronous.
//
// Note that this needs to be done explicitly here
// because the authentication router does not guarantee
// ordered completion of its Futures (it doesn't have the
// knowledge of UPIDs necessary to do it in a fine-grained
// manner).
hashmap authenticationPipelines; // Or,
// hashmap authenticationSequences;
```

Also, it would be great to manage this correctly and erase entries when 
they are not relevant any more. As we discussed:

* Store the last future from the sequence alongside here.
* Set up a callback on future completion, the callback should check for 
equality against the current last. If equal, the entry can be erased.

Anything I'm missing there?



3rdparty/libprocess/src/process.cpp (line 2399)


"within a Process' execution context"



3rdparty/libprocess/src/process.cpp (lines 2400 - 2402)


Why are you dispatching here rather than calling directly? Note though that 
I'm not sure we should keep `schedule`.



3rdparty/libprocess/src/process.cpp (line 2447)






3rdparty/libprocess/src/process.cpp (lines 2447 - 2455)


The first comment here should match the first comment from the other 
blocks, no? How about:

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Ben Mahler]

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


Second part of my earlier review, this time I went throug the test code.


3rdparty/libprocess/src/tests/http_tests.cpp (lines 1214 - 1216)


Looking at this, it's a little strange that realm comes after help.

Also, what's "mesos"? :)

In the interest of making this as readable as possible, how about we just 
use "realm" in the tests here? Why do anything less direct?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1226 - 1250)


After you re-use HttpProxy, the only thing that should be in this fixture 
is setting the authenticator during setup and unsetting during teardown.

How about naming this fixture 'HttpAuthenticationTest'? I can't quite tell 
why some names have http as the test case name, and some don't:

```
AuthenticationTest. HTTPAuthenticationNoAuthenticator
AuthenticationTest, AuthenticationNoCredentials
AuthenticationTest, AuthenticationSuccess
AuthenticationTest, DelayedAuthentication
```

How about the following, also note I tried not to repeat authentication 
since it's clear from the test fixture name:

```
HttpAuthenticationTest, Disabled
HttpAuthenticationTest, Unauthorized
HttpAuthenticationTest, Forbidden
HttpAuthenticationTest, Authenticated
HttpAuthenticationTest, Pipelining
```



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1253 - 1254)


An endpoint doesn't really request authentication, it just specifies a 
realm, no? I imagine that the wiring of authenticators to realms isn't tightly 
tied to the route() calls.

How about:

```
Ensures that when there is no authenticator for
a realm, requests are not authenticated (i.e. the
principal is None).
```



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1258 - 1259)


Ugh.

I suspected I was going to see something like this when I saw your test 
fixture. If we're going to put things in test fixtures I would suggest making 
them flexible and explicit, rather than implicit and static.

For example, rather than statically deciding to add an authenticator to the 
"mesos" realm (which let's just call "realm"!), we can have an explicit method 
for adding an authenticator:

```
addAuthenticator("realm", Owned(new MockAuthenticator());
```

When a test calls this, the test fixture will keep track of the set of 
realms that need to be unset, so that we can clean up in TearDown.

Sound good?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1270)


Have you looked at gtest.hpp in libprocess?

We have: AWAIT_EXPECT_RESPONSE_STATUS_EQ

We should update that assertion to check both the code and status, not sure 
why we manually check both all over the place.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1273)


No CHECKs in tests please :(

Also, you don't need to do this AFAICT, you can just do the following above 
to simplify this:

```
EXPECT_CALL((*process.get()), handler1(_, Option::none()));
```

Does that not work?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1281)


Let's avoid the "auth" abbreviation entirely, can you do a sweep? I also 
see it in the tests below.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1295 - 1297)


Why is this EXPECT_SOME_EQ instead of EXPECT_EQ? Aren't both of these 
options?



3rdparty/libprocess/src/tests/http_tests.cpp (line 1305)


In the interest of making this more readable:

s/"foo"/"principal"/



3rdparty/libprocess/src/tests/http_tests.cpp (line 1309)


Why is there an underscore here?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1310 - 1311)


Do you need to capture? It looks like you can just place the value in the 
expectation:

```
EXPECT_CALL((*process.get()), handler1(_, "principal"));
```

Not sure if it can implicitly construct the option here, might need to do 
that explicitly.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1316)


Can you just do "user:password" here? The comment seems sufficient to me

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Dec. 7, 2015, 3:14 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233


Repository: mesos


Description
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/event.hpp 
a03824c061c4a0eb865b163999a763635e56744c 
  3rdparty/libprocess/include/process/process.hpp 
81c094414d4d5ac5eb593df2a6d14aaacb19a826 
  3rdparty/libprocess/src/process.cpp e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
  3rdparty/libprocess/src/tests/http_tests.cpp 
2de75ca1c7e224c36b534c368e7379dc158aa5bb 

Diff: https://reviews.apache.org/r/38000/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rukletsov]

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



3rdparty/libprocess/include/process/process.hpp (lines 261 - 263)


I'm not sure how the comment is related to the typedef. Do you mean one of 
the parameter is `principal`? Mind explaining it then?



3rdparty/libprocess/include/process/process.hpp (line 264)


Given the second argument is optional (and assuming it's principal), I 
believe `AuthenticatedHttpRequestHandler` may also be responsible for requests 
that do not require authentication (e.g. no authenticator for the realm). If my 
understanding is correct, naming the handler with `Authenticated*` can be 
misleading.



3rdparty/libprocess/src/process.cpp (lines 3218 - 3220)


If this is an internal invariant, how about adding a `CHECK` here?



3rdparty/libprocess/src/tests/http_tests.cpp (lines 20 - 23)


Sort alphabetically



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1314 - 1315)


Blank line or swap.



3rdparty/libprocess/src/tests/http_tests.cpp (lines 1357 - 1358)


Blank line or swap.



3rdparty/libprocess/src/tests/http_tests.cpp (line 1373)


Let's definitely have a comment here why there is sleep. Also, why not 
`os::sleep()`?


- Alexander Rukletsov


On Dec. 7, 2015, 2:14 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38000/
> ---
> 
> (Updated Dec. 7, 2015, 2:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3233
> https://issues.apache.org/jira/browse/MESOS-3233
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds functions which allow libprocess users to register HTTP authenticators.
> Overloads `ProcesBase::route()` to allow for registering of authenticating 
> endpoints.
> Includes tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> a03824c061c4a0eb865b163999a763635e56744c 
>   3rdparty/libprocess/include/process/process.hpp 
> 81c094414d4d5ac5eb593df2a6d14aaacb19a826 
>   3rdparty/libprocess/src/process.cpp 
> e93709d3bb4ac588457bb9331fc05ec5ab539f6d 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 2de75ca1c7e224c36b534c368e7379dc158aa5bb 
> 
> Diff: https://reviews.apache.org/r/38000/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38000: Introduced support for user interaction with HTTP AuthenticationRouter.

[Alexander Rojas]

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

(Updated Nov. 18, 2015, 3:51 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, Bernd Mathiske, and Till 
Toenshoff.


Summary (updated)
-

Introduced support for user interaction with HTTP AuthenticationRouter.


Bugs: MESOS-3233
https://issues.apache.org/jira/browse/MESOS-3233


Repository: mesos


Description (updated)
---

Adds functions which allow libprocess users to register HTTP authenticators.
Overloads `ProcesBase::route()` to allow for registering of authenticating 
endpoints.
Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
8b086f296c80a43be2edaf496a04dadf0c64251a 
  3rdparty/libprocess/src/process.cpp f1ae455b509c854869a056c5f59de537f5a4eb81 
  3rdparty/libprocess/src/tests/http_tests.cpp 
5e70f1896a86104ac01dfe725eb4d7d1d25bee77 

Diff: https://reviews.apache.org/r/38000/diff/


Testing
---

make check


Thanks,

Alexander Rojas