You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gilbert Song <so...@gmail.com> on 2017/01/31 09:10:19 UTC
Review Request 56116: Implemented new http::Headers abstraction for
WWW-Authenticate.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/
-----------------------------------------------------------
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
Repository: mesos
Description
-------
Implemented new http::Headers abstraction for WWW-Authenticate.
Diffs
-----
3rdparty/libprocess/include/process/http.hpp e8f53bfac1cf1c0758ef662e78754e9246a41eea
3rdparty/libprocess/src/http.cpp f4ae30b11f811fb6ade85bd6f0e1b028dcdee3b3
Diff: https://reviews.apache.org/r/56116/diff/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 56116: Implemented new http::Headers abstraction
for WWW-Authenticate.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/
-----------------------------------------------------------
(Updated Feb. 2, 2017, 12:39 a.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
Changes
-------
Added Headers::emtpy().
Bugs: MESOS-7051
https://issues.apache.org/jira/browse/MESOS-7051
Repository: mesos
Description
-------
This patch introduced a new abstraction for http::Headers and helper
methods for the http::Headers hashmap, as well as a new
http::WWWAuthenticate class for the new `Headers` abstraction.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 4b11e085c3abe8d656b0e358b1d3e7bdccae9177
3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820
Diff: https://reviews.apache.org/r/56116/diff/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 56116: Implemented new http::Headers abstraction
for WWW-Authenticate.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/
-----------------------------------------------------------
(Updated Feb. 1, 2017, 4:59 p.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
Bugs: MESOS-7051
https://issues.apache.org/jira/browse/MESOS-7051
Repository: mesos
Description
-------
This patch introduced a new abstraction for http::Headers and helper
methods for the http::Headers hashmap, as well as a new
http::WWWAuthenticate class for the new `Headers` abstraction.
Diffs
-----
3rdparty/libprocess/include/process/http.hpp 4b11e085c3abe8d656b0e358b1d3e7bdccae9177
3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820
Diff: https://reviews.apache.org/r/56116/diff/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 56116: Implemented new http::Headers abstraction
for WWW-Authenticate.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/
-----------------------------------------------------------
(Updated Feb. 1, 2017, 4:40 p.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
Repository: mesos
Description
-------
This patch introduced a new abstraction for http::Headers and helper
methods for the http::Headers hashmap, as well as a new
http::WWWAuthenticate class for the new `Headers` abstraction.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 4b11e085c3abe8d656b0e358b1d3e7bdccae9177
3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820
Diff: https://reviews.apache.org/r/56116/diff/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 56116: Implemented new http::Headers abstraction
for WWW-Authenticate.
Posted by Gilbert Song <so...@gmail.com>.
> On Jan. 31, 2017, 4:15 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/src/http.cpp, line 624
> > <https://reviews.apache.org/r/56116/diff/2/?file=1620494#file1620494line624>
> >
> > RFC says that realm is defined for all auth schemes, but didn't say that each auth challenge needs to have that.
This comment confuses me. The auth scheme is included in the challenge:
```
challenge = auth-scheme 1*SP 1#auth-param
```
and in https://tools.ietf.org/html/rfc2617#section-1.2 :
```
The realm directive (case-insensitive) is required for all
authentication schemes that issue a challenge.
```
I think it really depends on how we understand this sentence:
```
The authentication parameter realm is defined for all authentication schemes.
```
1. The defined `realm` can be used for all auth schemes in the challenge.
2. Each auth scheme should be provided with a `realm`.
I regard it as (2). Thoughts?
- Gilbert
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/#review163732
-----------------------------------------------------------
On Jan. 31, 2017, 3:28 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56116/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 3:28 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch introduced a new abstraction for http::Headers and helper
> methods for the http::Headers hashmap, as well as a new
> http::WWWAuthenticate class for the new `Headers` abstraction.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 4b11e085c3abe8d656b0e358b1d3e7bdccae9177
> 3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820
>
> Diff: https://reviews.apache.org/r/56116/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 56116: Implemented new http::Headers abstraction
for WWW-Authenticate.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/#review163732
-----------------------------------------------------------
3rdparty/libprocess/include/process/http.hpp (line 435)
<https://reviews.apache.org/r/56116/#comment235227>
Let's add a TODO here. According to RFC, an WWWAuthenticate header can contain multiple "challenges" each of which is in the format of auth-scheme + auth-params.
Here, we assume there is only one auth challenge
3rdparty/libprocess/include/process/http.hpp (lines 435 - 436)
<https://reviews.apache.org/r/56116/#comment235228>
I would use authScheme and authParam here. You can rename the field members to `authScheme_` and `authParam_`
3rdparty/libprocess/src/http.cpp (line 602)
<https://reviews.apache.org/r/56116/#comment235244>
Can we have unit test for this?
3rdparty/libprocess/src/http.cpp (line 612)
<https://reviews.apache.org/r/56116/#comment235245>
Why tokenize here, that means `,,` is valid? Can you add a unit test for this case?
3rdparty/libprocess/src/http.cpp (line 624)
<https://reviews.apache.org/r/56116/#comment235246>
RFC says that realm is defined for all auth schemes, but didn't say that each auth challenge needs to have that.
- Jie Yu
On Jan. 31, 2017, 11:28 p.m., Gilbert Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56116/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 11:28 p.m.)
>
>
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch introduced a new abstraction for http::Headers and helper
> methods for the http::Headers hashmap, as well as a new
> http::WWWAuthenticate class for the new `Headers` abstraction.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 4b11e085c3abe8d656b0e358b1d3e7bdccae9177
> 3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820
>
> Diff: https://reviews.apache.org/r/56116/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Gilbert Song
>
>
Re: Review Request 56116: Implemented new http::Headers abstraction
for WWW-Authenticate.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 3:28 p.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
Changes
-------
Added comments.
Repository: mesos
Description
-------
This patch introduced a new abstraction for http::Headers and helper
methods for the http::Headers hashmap, as well as a new
http::WWWAuthenticate class for the new `Headers` abstraction.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 4b11e085c3abe8d656b0e358b1d3e7bdccae9177
3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820
Diff: https://reviews.apache.org/r/56116/diff/
Testing
-------
make check
Thanks,
Gilbert Song
Re: Review Request 56116: Implemented new http::Headers abstraction
for WWW-Authenticate.
Posted by Gilbert Song <so...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56116/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 12:08 p.m.)
Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai Lin, and Timothy Chen.
Changes
-------
Added comments.
Repository: mesos
Description (updated)
-------
This patch introduced a new abstraction for http::Headers and helper
methods for the http::Headers hashmap, as well as a new
http::WWWAuthenticate class for the new `Headers` abstraction.
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 4b11e085c3abe8d656b0e358b1d3e7bdccae9177
3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820
Diff: https://reviews.apache.org/r/56116/diff/
Testing
-------
make check
Thanks,
Gilbert Song