Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 12, 2016, 5:30 p.m., Alexander Rukletsov wrote:
> > src/tests/master_authorization_tests.cpp, line 1030
> > 
> >
> > Now this is not symmetrical to `SlaveAuthorizationTest`.

Yes, `MasterAuthorizationTest` was already taken.


- Jan


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


On May 12, 2016, 11:25 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 12, 2016, 11:25 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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




src/tests/master_authorization_tests.cpp (line 1030)


Now this is not symmetrical to `SlaveAuthorizationTest`.



src/master/http.cpp (line 861)


We should also update a help string saying that authz can be enabled. I'll 
take it in a separate patch.



src/master/http.cpp (lines 872 - 881)


This snippet relies on the fact, that the endpoint being hit is from the 
"delegated" actor. See https://issues.apache.org/jira/browse/MESOS-5369 for 
details. We should mention the ticket here and rephrase the TODO.


- Alexander Rukletsov


On May 12, 2016, 9:25 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 12, 2016, 9:25 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated May 12, 2016, 11:25 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Bannier.


Changes
---

Addressed Alex's issues.


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


Repository: mesos


Description
---

Access to the '/flags' endpoint is authorized using
the 'GET_ENDPOINT_WITH_PATH' action.


Diffs (updated)
-

  docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
  src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/tests/master_authorization_tests.cpp 
804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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


Fix it, then Ship it!





src/master/http.cpp (lines 869 - 878)


Could you please add a TODO here that this blob should be extracted in a 
separate function?



src/master/master.hpp (lines 1235 - 1239)


Maybe you can re-wrap it for readability.



src/master/master.hpp (lines 1239 - 1240)


Blank line.



src/tests/master_authorization_tests.cpp (lines 1020 - 1026)


Could you also please add an "integration" test that uses real authorizers? 
Similar to what you have done for the agent.


- Alexander Rukletsov


On May 10, 2016, 6:03 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 10, 2016, 6:03 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/http.cpp a0d67671c89b9794e721dc3ba012acd3bdc447e3 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated May 10, 2016, 6:07 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Bannier.


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


Repository: mesos


Description
---

Access to the '/flags' endpoint is authorized using
the 'GET_ENDPOINT_WITH_PATH' action.


Diffs (updated)
-

  docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
  src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/tests/master_authorization_tests.cpp 
804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 10, 2016, 5:54 p.m., Alexander Rukletsov wrote:
> > docs/configuration.md, line 465
> > 
> >
> > /monitor/statistics?

Ah, that isn't authorized yet! And is only available on agents! I'll remove it.


- Jan


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


On May 10, 2016, 12:12 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 10, 2016, 12:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 2846
> > 
> >
> > There's a lot of code duplication between here and 
> > `Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the 
> > two?
> 
> Jan Schlicht wrote:
> Indeed, that's quite some code duplication. But there are also some 
> slight differences in both functions. Had a discussion with @arojas about how 
> to handle that case. On one hand it's good to remove code duplication, on the 
> other hand the extracted function would look a bit more complicated to 
> support the different requirements of master and agent. We decided that in 
> this particular case it's okay to stick with the two similar functions.
> 
> Alexander Rukletsov wrote:
> If we split this function in two (per suggestion below), the copy-paste 
> wouldn't be so drastic: we already have a lot of functions that do mapping 
> principal->subject (it is unfortunate we have to repeat it again and again) 
> and a fucntion that extracts endpoint from an URL path. Maybe we can add a 
> helper in `Request` for this one.
> 
> Jan Schlicht wrote:
> I've changed `authorizeEndpoint` to no longer extract the endpoint. I've 
> also prepared some code that will add a function 
> `process::http::URL::extractEndpoint`, so that this won't need to be 
> duplicated. But as this will need some more effort (like added test cases), 
> I'd like to introduce that as a new RR. Is that enough to consider this issue 
> fixed or dropped?

Created [MESOS-5357](https://issues.apache.org/jira/browse/MESOS-5357) to track 
the needed libprocess changes.


- Jan


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


On May 10, 2016, 12:12 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 10, 2016, 12:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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




docs/configuration.md (line 465)


/monitor/statistics?


- Alexander Rukletsov


On May 10, 2016, 10:12 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 10, 2016, 10:12 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 2846
> > 
> >
> > There's a lot of code duplication between here and 
> > `Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the 
> > two?
> 
> Jan Schlicht wrote:
> Indeed, that's quite some code duplication. But there are also some 
> slight differences in both functions. Had a discussion with @arojas about how 
> to handle that case. On one hand it's good to remove code duplication, on the 
> other hand the extracted function would look a bit more complicated to 
> support the different requirements of master and agent. We decided that in 
> this particular case it's okay to stick with the two similar functions.
> 
> Alexander Rukletsov wrote:
> If we split this function in two (per suggestion below), the copy-paste 
> wouldn't be so drastic: we already have a lot of functions that do mapping 
> principal->subject (it is unfortunate we have to repeat it again and again) 
> and a fucntion that extracts endpoint from an URL path. Maybe we can add a 
> helper in `Request` for this one.

I've changed `authorizeEndpoint` to no longer extract the endpoint. I've also 
prepared some code that will add a function 
`process::http::URL::extractEndpoint`, so that this won't need to be 
duplicated. But as this will need some more effort (like added test cases), I'd 
like to introduce that as a new RR. Is that enough to consider this issue fixed 
or dropped?


- Jan


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


On May 10, 2016, 12:12 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 10, 2016, 12:12 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated May 10, 2016, 12:12 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Bannier.


Changes
---

Added explicit capture.


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


Repository: mesos


Description
---

Access to the '/flags' endpoint is authorized using
the 'GET_ENDPOINT_WITH_PATH' action.


Diffs (updated)
-

  docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
  src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/tests/master_authorization_tests.cpp 
804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated May 10, 2016, 11:57 a.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Bannier.


Changes
---

Addressed issues. (Removed endpoint extraction from authorizeEndpoint, added 
method validation)


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


Repository: mesos


Description
---

Access to the '/flags' endpoint is authorized using
the 'GET_ENDPOINT_WITH_PATH' action.


Diffs (updated)
-

  docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
  src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/tests/master_authorization_tests.cpp 
804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.
> 
> Jan Schlicht wrote:
> That's a good point! I'd would also aid in factoring out the 
> `authorizeEndpoint` function like you're suggesting above. Will work on that!
> 
> Jan Schlicht wrote:
> Hmm, but there's a reason why we pass an `http::Request` here: The first 
> version of `authorizeEndpoint` took an `endpoint` parameter. Because there's 
> no type for that or rather endpoints are a part of the string "path", this 
> parameter was passed as a `std::string`. That wasn't explicit enough, hence 
> we decided to extract the endpoint for the `Request.path` explicitly in this 
> function. The best solution would be to have an `Endpoint` type and 
> `authorizeEndpoint` uses that one. An `Endpoint` could be constructed from a 
> path using the same logic that we currenly apply in the function.
> 
> Jan Schlicht wrote:
> Regarding the `MethodNotAllowed`: IMO that's something that shouldn't be 
> returned by `authorizeEndpoint`, but needs to be validated by the routed 
> function before that. The current behavior (i.e. without authz) of for 
> example the "/flags" endpoint (and other endpoints as well) is not right: 
> Sending a POST request will create the same response as a GET request, 
> because the request method isn't validated. Method validation should become 
> part of _every_ endpoint, it doesn't even matter if that endpoint needs to be 
> authorized or not.

I've created (MESOS-5346)[https://issues.apache.org/jira/browse/MESOS-5346] for 
that, but will also add a method validation as part of this RR.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.
> 
> Jan Schlicht wrote:
> That's a good point! I'd would also aid in factoring out the 
> `authorizeEndpoint` function like you're suggesting above. Will work on that!
> 
> Jan Schlicht wrote:
> Hmm, but there's a reason why we pass an `http::Request` here: The first 
> version of `authorizeEndpoint` took an `endpoint` parameter. Because there's 
> no type for that or rather endpoints are a part of the string "path", this 
> parameter was passed as a `std::string`. That wasn't explicit enough, hence 
> we decided to extract the endpoint for the `Request.path` explicitly in this 
> function. The best solution would be to have an `Endpoint` type and 
> `authorizeEndpoint` uses that one. An `Endpoint` could be constructed from a 
> path using the same logic that we currenly apply in the function.

Regarding the `MethodNotAllowed`: IMO that's something that shouldn't be 
returned by `authorizeEndpoint`, but needs to be validated by the routed 
function before that. The current behavior (i.e. without authz) of for example 
the "/flags" endpoint (and other endpoints as well) is not right: Sending a 
POST request will create the same response as a GET request, because the 
request method isn't validated. Method validation should become part of _every_ 
endpoint, it doesn't even matter if that endpoint needs to be authorized or not.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.
> 
> Jan Schlicht wrote:
> That's a good point! I'd would also aid in factoring out the 
> `authorizeEndpoint` function like you're suggesting above. Will work on that!

Hmm, but there's a reason why we pass an `http::Request` here: The first 
version of `authorizeEndpoint` took an `endpoint` parameter. Because there's no 
type for that or rather endpoints are a part of the string "path", this 
parameter was passed as a `std::string`. That wasn't explicit enough, hence we 
decided to extract the endpoint for the `Request.path` explicitly in this 
function. The best solution would be to have an `Endpoint` type and 
`authorizeEndpoint` uses that one. An `Endpoint` could be constructed from a 
path using the same logic that we currenly apply in the function.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > 
> >
> > Could you make this capture list explicit (`[this, request]`)?
> 
> Jan Schlicht wrote:
> The style guide recommends to prefer default capture by value, that's why 
> I'm doing it here. The case here is different from the one in 
> `slave/http.cpp`. There we explicitly captured by value to be sure not to 
> capture `this`, because that would lead to problems there. We don't have this 
> problem here, hence the default capture by value.
> 
> Benjamin Bannier wrote:
> Note that `[this, request]` *does* capture by value. I am just asking you 
> to be explicit about what you capture instead of pulling in everything from 
> the current scope.
> 
> Jan Schlicht wrote:
> As does `[=]`, and that's preferred by the style guide.
> 
> Benjamin Bannier wrote:
> Hm, I read that part of the style guide differently, but well. I believe 
> we value explicit over implicit, but even in this file there seems to be no 
> preference either way, so feel free to drop this issue.

Seems that I'm the only one that read the style guide that way, see the 
discussion with @alexr below. I'll change it to explicit capture.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.

That's a good point! I'd would also aid in factoring out the 
`authorizeEndpoint` function like you're suggesting above. Will work on that!


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 879
> > 
> >
> > In r/46203/ you've cached flags and passed them into continuation. Here 
> > you call `master->flags` in the continuation. Why is the approach different 
> > to the one for the agent?
> 
> Jan Schlicht wrote:
> See [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293).
> tldr; We can do it for the master, but (currently) not for the agent.
> 
> Alexander Rukletsov wrote:
> But it does not prevent us from following the pattern we used in the 
> agent here, right?

No, it doesn't. But it's the pattern we used in the agent that's the outlier 
here. In other cases we are able to access `this` in the continuation and make 
use of it. IMO this is the pattern we should prefer: Having a non-static 
continuation, that can access `this` and its members and change the 
continuation in `Slave::Http` to that pattern as soon as MESOS-5293 is resolved.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 869
> > 
> >
> > Did you need to pass all ("=") the in-scope variables through to the 
> > lambda, or just `request`?
> 
> Jan Schlicht wrote:
> Yes, I only need to capture `request` here, but the way I understand the 
> C++ style guide, "default capture by value" ("=") is preferred over "explicit 
> capture by value".
> 
> Alexander Rukletsov wrote:
> I'm not sure this is what the styleguide says. I read "Prefer default 
> capture by value, explicit capture by value, then capture by reference." as 
> explicit and default captures both take precedence over captures by 
> reference. Anyway, if explicit capture is fine, this is the best use case for 
> it: a lambda taking one parameter, which is exactly your case. Please change 
> this.

Good to know. I'm actually a fan of explicit capture -- as long as the 
parameter list isn't too long. I've had a similar discussion with @bbannier 
about that above. There it's about the explicit capture of 2 parameters. I'll 
also change that there.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

> On May 9, 2016, 9:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.

So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you think 
it's right?

In retrospect, there was a mistake to move request parsing into this function. 
You know check that the request is correctly formed in `authorizeEndpoint()`, 
from where you can't return `Response`. I suggest to split this function in 
two: one for parsing and another for mapping principal to subject.


> On May 9, 2016, 9:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 879
> > 
> >
> > In r/46203/ you've cached flags and passed them into continuation. Here 
> > you call `master->flags` in the continuation. Why is the approach different 
> > to the one for the agent?
> 
> Jan Schlicht wrote:
> See [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293).
> tldr; We can do it for the master, but (currently) not for the agent.

But it does not prevent us from following the pattern we used in the agent 
here, right?


- Alexander


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


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:18 a.m., Alexander Rukletsov wrote:
> > Also, do you want to update configuration.md?

To add the "get_endpoints" action to the ACL example? Yep, makes sense to do 
that here, will add it.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

> On May 7, 2016, 10:24 a.m., Adam B wrote:
> > src/master/http.cpp, line 869
> > 
> >
> > Did you need to pass all ("=") the in-scope variables through to the 
> > lambda, or just `request`?
> 
> Jan Schlicht wrote:
> Yes, I only need to capture `request` here, but the way I understand the 
> C++ style guide, "default capture by value" ("=") is preferred over "explicit 
> capture by value".

I'm not sure this is what the styleguide says. I read "Prefer default capture 
by value, explicit capture by value, then capture by reference." as explicit 
and default captures both take precedence over captures by reference. Anyway, 
if explicit capture is fine, this is the best use case for it: a lambda taking 
one parameter, which is exactly your case. Please change this.


> On May 7, 2016, 10:24 a.m., Adam B wrote:
> > src/master/http.cpp, line 2846
> > 
> >
> > There's a lot of code duplication between here and 
> > `Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the 
> > two?
> 
> Jan Schlicht wrote:
> Indeed, that's quite some code duplication. But there are also some 
> slight differences in both functions. Had a discussion with @arojas about how 
> to handle that case. On one hand it's good to remove code duplication, on the 
> other hand the extracted function would look a bit more complicated to 
> support the different requirements of master and agent. We decided that in 
> this particular case it's okay to stick with the two similar functions.

If we split this function in two (per suggestion below), the copy-paste 
wouldn't be so drastic: we already have a lot of functions that do mapping 
principal->subject (it is unfortunate we have to repeat it again and again) and 
a fucntion that extracts endpoint from an URL path. Maybe we can add a helper 
in `Request` for this one.


- Alexander


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


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/tests/master_authorization_tests.cpp, line 1033
> > 
> >
> > Doesn't it compile without explicit casting to string?

It doesn't. See the discussion with Benjamin above.


> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )

Because `authorizeEndpoint` isn't supposed to create a `http::Response`. Its 
purpose is to answer the question whether a principal is authorized to do 
things and it's the caller's responsibility to create a HTTP response from that 
answer. But to create the answer, the function needs informations that it 
extracts from the HTTP request. Because these informations may be missing, we 
need to handle that case here, hence we fail the `Future`.


> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 879
> > 
> >
> > In r/46203/ you've cached flags and passed them into continuation. Here 
> > you call `master->flags` in the continuation. Why is the approach different 
> > to the one for the agent?

See [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293).
tldr; We can do it for the master, but (currently) not for the agent.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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



Also, do you want to update configuration.md?

- Alexander Rukletsov


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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



Looks good! One more round and we can commit this.

Do you want to add an integration test like you did in r/46203 with 
`AuthorizeFlagsEndpoint`?


src/master/http.cpp (line 879)


In r/46203/ you've cached flags and passed them into continuation. Here you 
call `master->flags` in the continuation. Why is the approach different to the 
one for the agent?



src/master/http.cpp (line 2872)


Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
actually : )



src/tests/master_authorization_tests.cpp (line 1033)


Doesn't it compile without explicit casting to string?



src/tests/master_authorization_tests.cpp (lines 1036 - 1037)


Please re-wrap to reduce jaggeddness.



src/tests/master_authorization_tests.cpp (lines 1100 - 1101)


Ditto.


- Alexander Rukletsov


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 869
> > 
> >
> > Did you need to pass all ("=") the in-scope variables through to the 
> > lambda, or just `request`?

Yes, I only need to capture `request` here, but the way I understand the C++ 
style guide, "default capture by value" ("=") is preferred over "explicit 
capture by value".


> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 2846
> > 
> >
> > There's a lot of code duplication between here and 
> > `Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the 
> > two?

Indeed, that's quite some code duplication. But there are also some slight 
differences in both functions. Had a discussion with @arojas about how to 
handle that case. On one hand it's good to remove code duplication, on the 
other hand the extracted function would look a bit more complicated to support 
the different requirements of master and agent. We decided that in this 
particular case it's okay to stick with the two similar functions.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Adam B]

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




src/master/http.cpp (line 869)


Did you need to pass all ("=") the in-scope variables through to the 
lambda, or just `request`?



src/master/http.cpp (line 2846)


There's a lot of code duplication between here and 
`Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the two?



src/tests/master_authorization_tests.cpp (line 1036)


s/form/forms/


- Adam B


On May 3, 2016, 5:42 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 5:42 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46784]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Benjamin Bannier]

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


Ship it!





src/master/http.cpp (line 2882)


I see this is being used also elsewhere, so not your issue, but this that 
downside that it becomes impossible to distinguish an absent principal from one 
called `ANY` when examing logs. @alexr What's the idea behind this?


- Benjamin Bannier


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

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

(Updated May 3, 2016, 2:42 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Bannier.


Changes
---

Parameterized the test fixture.


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


Repository: mesos


Description
---

Access to the '/flags' endpoint is authorized using
the 'GET_ENDPOINT_WITH_PATH' action.


Diffs (updated)
-

  src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
  src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
  src/tests/master_authorization_tests.cpp 
804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/tests/master_authorization_tests.cpp, line 1024
> > 
> >
> > This looks very similar to the `SlaveEndpointTest` suite ;) Should we 
> > just also make this parameterized from the start?
> 
> Jan Schlicht wrote:
> I would like to, but unfortunately a parameterized test in GTest needs at 
> least two values, at this point we have only one. It doesn't compile with a 
> single parameter.
> 
> Benjamin Bannier wrote:
> Hmm, it looks like this should work it you give it values of the right 
> type, i.e., pass `std::string` instead of a string literal to 
> `::testing::Values` when instantiating the test case with 
> `INSTANTIATE_TEST_CASE_P`.

Oh, right. Fixed.


- Jan


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


On April 29, 2016, 11:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated April 29, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46784]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 29, 2016, 9:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated April 29, 2016, 9:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Benjamin Bannier]

> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > 
> >
> > Could you make this capture list explicit (`[this, request]`)?
> 
> Jan Schlicht wrote:
> The style guide recommends to prefer default capture by value, that's why 
> I'm doing it here. The case here is different from the one in 
> `slave/http.cpp`. There we explicitly captured by value to be sure not to 
> capture `this`, because that would lead to problems there. We don't have this 
> problem here, hence the default capture by value.
> 
> Benjamin Bannier wrote:
> Note that `[this, request]` *does* capture by value. I am just asking you 
> to be explicit about what you capture instead of pulling in everything from 
> the current scope.
> 
> Jan Schlicht wrote:
> As does `[=]`, and that's preferred by the style guide.

Hm, I read that part of the style guide differently, but well. I believe we 
value explicit over implicit, but even in this file there seems to be no 
preference either way, so feel free to drop this issue.


- Benjamin


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


On April 29, 2016, 11:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated April 29, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Benjamin Bannier]

> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > 
> >
> > Could you make this capture list explicit (`[this, request]`)?
> 
> Jan Schlicht wrote:
> The style guide recommends to prefer default capture by value, that's why 
> I'm doing it here. The case here is different from the one in 
> `slave/http.cpp`. There we explicitly captured by value to be sure not to 
> capture `this`, because that would lead to problems there. We don't have this 
> problem here, hence the default capture by value.

Note that `[this, request]` *does* capture by value. I am just asking you to be 
explicit about what you capture instead of pulling in everything from the 
current scope.


- Benjamin


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


On April 29, 2016, 11:49 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated April 29, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > 
> >
> > Could you make this capture list explicit (`[this, request]`)?

The style guide recommends to prefer default capture by value, that's why I'm 
doing it here. The case here is different from the one in `slave/http.cpp`. 
There we explicitly captured by value to be sure not to capture `this`, because 
that would lead to problems there. We don't have this problem here, hence the 
default capture by value.


> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 874
> > 
> >
> > Not your pattern and a general problem here, but this does not return 
> > any useful response should we fail to get an answer from the `authorizer` :(

It would return a failed future. Don't know what libprocess does with that, by 
I would expect that it would create a "server error" response.


> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/tests/master_authorization_tests.cpp, line 1024
> > 
> >
> > This looks very similar to the `SlaveEndpointTest` suite ;) Should we 
> > just also make this parameterized from the start?

I would like to, but unfortunately a parameterized test in GTest needs at least 
two values, at this point we have only one. It doesn't compile with a single 
parameter.


- Jan


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


On April 28, 2016, 4:57 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated April 28, 2016, 4:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Benjamin Bannier]

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




src/master/http.cpp (line 868)


Could you make this capture list explicit (`[this, request]`)?



src/master/http.cpp (line 874)


Not your pattern and a general problem here, but this does not return any 
useful response should we fail to get an answer from the `authorizer` :(



src/tests/master_authorization_tests.cpp (line 1024)


This looks very similar to the `SlaveEndpointTest` suite ;) Should we just 
also make this parameterized from the start?



src/tests/master_authorization_tests.cpp (line 1027)


`s/agent/master/`



src/tests/master_authorization_tests.cpp (lines 1054 - 1057)


Once you have parameterized the fixture on the endpoint you should 
`s/Slave/Master/`. Before that it doesn't really fit.



src/tests/master_authorization_tests.cpp (line 1067)


`s/agent/master/`


- Benjamin Bannier


On April 28, 2016, 4:57 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated April 28, 2016, 4:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46784]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On April 28, 2016, 2:57 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated April 28, 2016, 2:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4f489fb9e5bc77b839cc4a44d7ec36af06f73e4a 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>