Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On March 13, 2016, 11:50 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44621/
> ---
> 
> (Updated March 13, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With enabling http authentication for http endpoints we should also add tests 
> to check
> that http request to those endpoints return "401 Unauthorized" if queried 
> without or with
> bad credentials.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 5a421d4e8740b5105518283943087869025cbfa6 
>   src/tests/master_tests.cpp e9ddd360fd87aed091f12a35f92df3f4e9c4190b 
>   src/tests/repair_tests.cpp cb38bb1b6717c3eb003323278cf8f6902394be7c 
>   src/tests/role_tests.cpp 2e23926143a2c09c6e1fee961a4ede6f7b3a275e 
> 
> Diff: https://reviews.apache.org/r/44621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Adam B]

> On March 13, 2016, 12:26 a.m., Adam B wrote:
> > src/tests/master_tests.cpp, line 4265
> > 
> >
> > There is no GET request allowed on /weights (yet), so it's interesting 
> > to me that this part of the test passes. I would expect it to return 405 
> > MethodNotAllowed. Or I guess authentication happens before we even get to 
> > the handler to check the action?
> 
> Joerg Schad wrote:
> I removed the test for now (especially as there is no http::put yet). 
> Will add that with a follow up patch.

Authn/z tests for PUT to the /weights endpoint are being added separately in 
https://reviews.apache.org/r/41790
See DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
And there's no longer a need for `process::http::put()` since 
`process::http::request()` has been made public.


- Adam


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


On March 13, 2016, 11:50 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44621/
> ---
> 
> (Updated March 13, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With enabling http authentication for http endpoints we should also add tests 
> to check
> that http request to those endpoints return "401 Unauthorized" if queried 
> without or with
> bad credentials.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 5a421d4e8740b5105518283943087869025cbfa6 
>   src/tests/master_tests.cpp e9ddd360fd87aed091f12a35f92df3f4e9c4190b 
>   src/tests/repair_tests.cpp cb38bb1b6717c3eb003323278cf8f6902394be7c 
>   src/tests/role_tests.cpp 2e23926143a2c09c6e1fee961a4ede6f7b3a275e 
> 
> Diff: https://reviews.apache.org/r/44621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Joerg Schad]

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

(Updated March 13, 2016, 6:50 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed review.


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


Repository: mesos


Description
---

With enabling http authentication for http endpoints we should also add tests 
to check
that http request to those endpoints return "401 Unauthorized" if queried 
without or with
bad credentials.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
5a421d4e8740b5105518283943087869025cbfa6 
  src/tests/master_tests.cpp e9ddd360fd87aed091f12a35f92df3f4e9c4190b 
  src/tests/repair_tests.cpp cb38bb1b6717c3eb003323278cf8f6902394be7c 
  src/tests/role_tests.cpp 2e23926143a2c09c6e1fee961a4ede6f7b3a275e 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Joerg Schad]

> On March 13, 2016, 8:26 a.m., Adam B wrote:
> > src/tests/master_maintenance_tests.cpp, line 1786
> > 
> >
> > You can't use `badAuthnHeaders` here, because of the content-type?

Yes.


> On March 13, 2016, 8:26 a.m., Adam B wrote:
> > src/tests/master_tests.cpp, line 4265
> > 
> >
> > There is no GET request allowed on /weights (yet), so it's interesting 
> > to me that this part of the test passes. I would expect it to return 405 
> > MethodNotAllowed. Or I guess authentication happens before we even get to 
> > the handler to check the action?

I removed the test for now (especially as there is no http::put yet). Will add 
that with a follow up patch.


- Joerg


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


On March 11, 2016, 7:49 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44621/
> ---
> 
> (Updated March 11, 2016, 7:49 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With enabling http authentication for http endpoints we should also add tests 
> to check
> that http request to those endpoints return "401 Unauthorized" if queried 
> without or with
> bad credentials.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 3c7024cfbd7e5bef75f092eace8d0e8ca423 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
> 
> Diff: https://reviews.apache.org/r/44621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Adam B]

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



Looks good. Just some minor style points to clean up, and then we can ship it


src/tests/master_maintenance_tests.cpp (line 1749)


s/POST's/POSTs/ - it's plural, not possessive



src/tests/master_maintenance_tests.cpp (line 1750)


Sorry, but we don't like abbrevs in variable names. How about 
"unauthenticatedHeaders" or "anonymousHeaders"?



src/tests/master_maintenance_tests.cpp (line 1757)


Seems weird that this one doesn't get a pre-definition comment.



src/tests/master_maintenance_tests.cpp (line 1761)


s/POST's/POSTs/



src/tests/master_maintenance_tests.cpp (line 1762)


No abbreviations, so "badAuthenticationHeaders"?



src/tests/master_maintenance_tests.cpp (lines 1767 - 1768)


Comment: // Post the maintenance schedule without authentication.



src/tests/master_maintenance_tests.cpp (lines 1768 - 1781)


Could you please be consistent about testing POST first or GET first?



src/tests/master_maintenance_tests.cpp (line 1786)


You can't use `badAuthnHeaders` here, because of the content-type?



src/tests/master_maintenance_tests.cpp (line 1821)


s/up/down/



src/tests/master_tests.cpp (line 4195)


"Test get requests on various endpoints without..."



src/tests/master_tests.cpp (line 4265)


There is no GET request allowed on /weights (yet), so it's interesting to 
me that this part of the test passes. I would expect it to return 405 
MethodNotAllowed. Or I guess authentication happens before we even get to the 
handler to check the action?



src/tests/repair_tests.cpp (line 225)


Any reason you only test get, but not post?



src/tests/repair_tests.cpp (line 226)


s/EndpointBadAuthentication/ObserveEndpointBadAuthentication/ since 
`HealthTest` sounds too generic to just apply to the observe endpoint.


- Adam B


On March 11, 2016, 11:49 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44621/
> ---
> 
> (Updated March 11, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With enabling http authentication for http endpoints we should also add tests 
> to check
> that http request to those endpoints return "401 Unauthorized" if queried 
> without or with
> bad credentials.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 3c7024cfbd7e5bef75f092eace8d0e8ca423 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
> 
> Diff: https://reviews.apache.org/r/44621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Joerg Schad]

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

(Updated March 11, 2016, 7:49 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed review.


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


Repository: mesos


Description
---

With enabling http authentication for http endpoints we should also add tests 
to check
that http request to those endpoints return "401 Unauthorized" if queried 
without or with
bad credentials.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
3c7024cfbd7e5bef75f092eace8d0e8ca423 
  src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
  src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
  src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Greg Mann]

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


Fix it, then Ship it!





src/tests/master_maintenance_tests.cpp (lines 1770 - 1772)


One line?



src/tests/master_maintenance_tests.cpp (lines 1770 - 1772)


One line?



src/tests/master_tests.cpp (line 4195)


Looks like there are no POST requests here?



src/tests/master_tests.cpp (lines 4197 - 4199)


s/observe in the/observe endpoints in the/

Or, you could just say "we have similar checks for other endpoints in their 
respective..." ?



src/tests/master_tests.cpp (lines 4212 - 4214)


One line? Here and below in this file



src/tests/role_tests.cpp (line 657)


Since this file contains other tests which have nothing to do with HTTP 
endpoints, perhaps this name should be a bit more descriptive? Something like 
`RolesEndpointBadAuthentication`?


- Greg Mann


On March 10, 2016, 2:41 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44621/
> ---
> 
> (Updated March 10, 2016, 2:41 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With enabling http authentication for http endpoints we should also add tests 
> to check
> that http request to those endpoints return "401 Unauthorized" if queried 
> without or with
> bad credentials.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
> 
> Diff: https://reviews.apache.org/r/44621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Joerg Schad]

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

(Updated March 10, 2016, 2:41 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


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


Repository: mesos


Description
---

With enabling http authentication for http endpoints we should also add tests 
to check
that http request to those endpoints return "401 Unauthorized" if queried 
without or with
bad credentials.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
3faa8136cf57276295553910319480028f433e4c 
  src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
  src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
  src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Alexander Rojas]

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


Ship it!




Ship It!

- Alexander Rojas


On March 10, 2016, 3:41 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44621/
> ---
> 
> (Updated March 10, 2016, 3:41 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4844
> https://issues.apache.org/jira/browse/MESOS-4844
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With enabling http authentication for http endpoints we should also add tests 
> to check
> that http request to those endpoints return "401 Unauthorized" if queried 
> without or with
> bad credentials.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> 3faa8136cf57276295553910319480028f433e4c 
>   src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
>   src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
>   src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 
> 
> Diff: https://reviews.apache.org/r/44621/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44621: Added tests for http endpoints with bad authentication.

[Joerg Schad]

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

(Updated March 10, 2016, 2:40 p.m.)


Review request for mesos, Adam B and Alexander Rojas.


Changes
---

Addressed review.


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


Repository: mesos


Description (updated)
---

With enabling http authentication for http endpoints we should also add tests 
to check
that http request to those endpoints return "401 Unauthorized" if queried 
without or with
bad credentials.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
3faa8136cf57276295553910319480028f433e4c 
  src/tests/master_tests.cpp 2f4d820e223a48700ce1ac3a91b7256cc836c268 
  src/tests/repair_tests.cpp bb104562659e135492f9857e5b452c8a0a9e97da 
  src/tests/role_tests.cpp fc3a72894631279460ee7971a4627d73c3d8c351 

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


Testing
---

make check


Thanks,

Joerg Schad