Re: Review Request 42027: Changes HTTP responses from Unauthorized (401) to Forbidden (403).

2016-01-11 Thread Benjamin Bannier

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

Ship it!


Sorry for the nit-pick, but the summary still needs `s/Changes/Changed/` before 
a merge :|

- Benjamin Bannier


On Jan. 11, 2016, 2:25 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42027/
> ---
> 
> (Updated Jan. 11, 2016, 2:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Joerg Schad, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4305
> https://issues.apache.org/jira/browse/MESOS-4305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is a common patter within Mesos to return an HTTP 401 (Unauthorized) 
> response whenever the request is invalid for whatever reason. However, 
> according to the [RFC-2617 Section 
> 1.2](https://tools.ietf.org/html/rfc2617#section-1.2):
> > The 401 (Unauthorized) response message is used by an origin server  to 
> > challenge the authorization of a user agent. This response MUST include a 
> > WWW-Authenticate header field containing at least one challenge applicable 
> > to the requested resource.
> 
> Meaning that despite the confusing name, the status code _401 Unauthorized_ 
> should be used only for authentication purposes. On the other hand, the 
> [RFC-2616 Section 
> 10.4.4](http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4) 
> states:
> > _(403 Forbidden is returned when)_ The server understood the request, but 
> > is refusing to fulfill it. Authorization will not help and the request 
> > SHOULD NOT be repeated. If the request method was not HEAD and the server 
> > wishes to make public why the request has not been fulfilled, it SHOULD 
> > describe the reason for the refusal in the entity. If the server does not 
> > wish to make this information available to the client, the status code 404 
> > (Not Found) can be used instead.
> 
> As such, _403 (Forbidden)_ seems to be a better return code when replying 
> inside endpoint handlers, while _401 (Unauthorized)_ should be left to the 
> HTTP Authenticators only.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md a928f1722dc67cd791d78ebbe4591f2e8f2e8f2a 
>   src/master/http.cpp bcafc7aff89659a68352f3876ce6042f8b34bd5d 
>   src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 
>   src/tests/master_quota_tests.cpp 776a168254af6fa8a5d87d4580b35d83f2d5909a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> f0cce190abc90f0fae84d6c3db20e8215c2d8132 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
>   src/tests/scheduler_http_api_tests.cpp 
> 4d23a5a8368e0ed126469fa4a90a889b339ad004 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/42027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>



Re: Review Request 42027: Changes HTTP responses from Unauthorized (401) to Forbidden (403).

2016-01-11 Thread Alexander Rojas


> On Jan. 11, 2016, 3:02 p.m., Benjamin Bannier wrote:
> > Looks mostly gtm. `HTTPTest.Auth` still uses `Unauthorized`; given that 
> > that part of that test seems to just check the behavior of its own mocks it 
> > probably makes sense to remove it completely.

Adressed your issues in follow up patches.


- Alexander


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


On Jan. 11, 2016, 2:25 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42027/
> ---
> 
> (Updated Jan. 11, 2016, 2:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Joerg Schad, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4305
> https://issues.apache.org/jira/browse/MESOS-4305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is a common patter within Mesos to return an HTTP 401 (Unauthorized) 
> response whenever the request is invalid for whatever reason. However, 
> according to the [RFC-2617 Section 
> 1.2](https://tools.ietf.org/html/rfc2617#section-1.2):
> > The 401 (Unauthorized) response message is used by an origin server  to 
> > challenge the authorization of a user agent. This response MUST include a 
> > WWW-Authenticate header field containing at least one challenge applicable 
> > to the requested resource.
> 
> Meaning that despite the confusing name, the status code _401 Unauthorized_ 
> should be used only for authentication purposes. On the other hand, the 
> [RFC-2616 Section 
> 10.4.4](http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4) 
> states:
> > _(403 Forbidden is returned when)_ The server understood the request, but 
> > is refusing to fulfill it. Authorization will not help and the request 
> > SHOULD NOT be repeated. If the request method was not HEAD and the server 
> > wishes to make public why the request has not been fulfilled, it SHOULD 
> > describe the reason for the refusal in the entity. If the server does not 
> > wish to make this information available to the client, the status code 404 
> > (Not Found) can be used instead.
> 
> As such, _403 (Forbidden)_ seems to be a better return code when replying 
> inside endpoint handlers, while _401 (Unauthorized)_ should be left to the 
> HTTP Authenticators only.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md a928f1722dc67cd791d78ebbe4591f2e8f2e8f2a 
>   src/master/http.cpp bcafc7aff89659a68352f3876ce6042f8b34bd5d 
>   src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 
>   src/tests/master_quota_tests.cpp 776a168254af6fa8a5d87d4580b35d83f2d5909a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> f0cce190abc90f0fae84d6c3db20e8215c2d8132 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
>   src/tests/scheduler_http_api_tests.cpp 
> 4d23a5a8368e0ed126469fa4a90a889b339ad004 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/42027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>



Re: Review Request 42027: Changes HTTP responses from Unauthorized (401) to Forbidden (403).

2016-01-11 Thread Benjamin Bannier

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


Looks mostly gtm. `HTTPTest.Auth` still uses `Unauthorized`; given that that 
part of that test seems to just check the behavior of its own mocks it probably 
makes sense to remove it completely.

- Benjamin Bannier


On Jan. 11, 2016, 2:25 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42027/
> ---
> 
> (Updated Jan. 11, 2016, 2:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Joerg Schad, Jan 
> Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4305
> https://issues.apache.org/jira/browse/MESOS-4305
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It is a common patter within Mesos to return an HTTP 401 (Unauthorized) 
> response whenever the request is invalid for whatever reason. However, 
> according to the [RFC-2617 Section 
> 1.2](https://tools.ietf.org/html/rfc2617#section-1.2):
> > The 401 (Unauthorized) response message is used by an origin server  to 
> > challenge the authorization of a user agent. This response MUST include a 
> > WWW-Authenticate header field containing at least one challenge applicable 
> > to the requested resource.
> 
> Meaning that despite the confusing name, the status code _401 Unauthorized_ 
> should be used only for authentication purposes. On the other hand, the 
> [RFC-2616 Section 
> 10.4.4](http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4) 
> states:
> > _(403 Forbidden is returned when)_ The server understood the request, but 
> > is refusing to fulfill it. Authorization will not help and the request 
> > SHOULD NOT be repeated. If the request method was not HEAD and the server 
> > wishes to make public why the request has not been fulfilled, it SHOULD 
> > describe the reason for the refusal in the entity. If the server does not 
> > wish to make this information available to the client, the status code 404 
> > (Not Found) can be used instead.
> 
> As such, _403 (Forbidden)_ seems to be a better return code when replying 
> inside endpoint handlers, while _401 (Unauthorized)_ should be left to the 
> HTTP Authenticators only.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md a928f1722dc67cd791d78ebbe4591f2e8f2e8f2a 
>   src/master/http.cpp bcafc7aff89659a68352f3876ce6042f8b34bd5d 
>   src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 
>   src/tests/master_quota_tests.cpp 776a168254af6fa8a5d87d4580b35d83f2d5909a 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> f0cce190abc90f0fae84d6c3db20e8215c2d8132 
>   src/tests/reservation_endpoints_tests.cpp 
> b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
>   src/tests/scheduler_http_api_tests.cpp 
> 4d23a5a8368e0ed126469fa4a90a889b339ad004 
>   src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 
> 
> Diff: https://reviews.apache.org/r/42027/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>



Re: Review Request 42027: Changes HTTP responses from Unauthorized (401) to Forbidden (403).

2016-01-11 Thread Alexander Rojas

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

(Updated Jan. 11, 2016, 2:25 p.m.)


Review request for mesos, Alexander Rukletsov, Greg Mann, Joerg Schad, Jan 
Schlicht, and Till Toenshoff.


Changes
---

Benjamin's review requested changes.


Summary (updated)
-

Changes HTTP responses from Unauthorized (401) to Forbidden (403).


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


Repository: mesos


Description (updated)
---

It is a common patter within Mesos to return an HTTP 401 (Unauthorized) 
response whenever the request is invalid for whatever reason. However, 
according to the [RFC-2617 Section 
1.2](https://tools.ietf.org/html/rfc2617#section-1.2):
> The 401 (Unauthorized) response message is used by an origin server  to 
> challenge the authorization of a user agent. This response MUST include a 
> WWW-Authenticate header field containing at least one challenge applicable to 
> the requested resource.

Meaning that despite the confusing name, the status code _401 Unauthorized_ 
should be used only for authentication purposes. On the other hand, the 
[RFC-2616 Section 
10.4.4](http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4) 
states:
> _(403 Forbidden is returned when)_ The server understood the request, but is 
> refusing to fulfill it. Authorization will not help and the request SHOULD 
> NOT be repeated. If the request method was not HEAD and the server wishes to 
> make public why the request has not been fulfilled, it SHOULD describe the 
> reason for the refusal in the entity. If the server does not wish to make 
> this information available to the client, the status code 404 (Not Found) can 
> be used instead.

As such, _403 (Forbidden)_ seems to be a better return code when replying 
inside endpoint handlers, while _401 (Unauthorized)_ should be left to the HTTP 
Authenticators only.


Diffs (updated)
-

  docs/authorization.md a928f1722dc67cd791d78ebbe4591f2e8f2e8f2a 
  src/master/http.cpp bcafc7aff89659a68352f3876ce6042f8b34bd5d 
  src/master/quota_handler.cpp 134a93b1d1b6e050aa8a5037ffbec2cc305b0694 
  src/tests/master_quota_tests.cpp 776a168254af6fa8a5d87d4580b35d83f2d5909a 
  src/tests/persistent_volume_endpoints_tests.cpp 
f0cce190abc90f0fae84d6c3db20e8215c2d8132 
  src/tests/reservation_endpoints_tests.cpp 
b8edd6fafedd4c2221a8d19c1ebc71254071a8c7 
  src/tests/scheduler_http_api_tests.cpp 
4d23a5a8368e0ed126469fa4a90a889b339ad004 
  src/tests/teardown_tests.cpp 97cc89ba168aefff8512f6d1a25c4f7ddf180bae 

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


Testing
---

make check


Thanks,

Alexander Rojas