You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jie Yu <yu...@gmail.com> on 2017/02/01 00:15:13 UTC
Re: 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/#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>.
> 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
>
>