> On April 26, 2016, 4:01 p.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 362
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361588#file1361588line362>
> >
> >     You assume that `slave(XYZ)/flags` will only receive `GET` requests, 
> > but I think it would make more sense to pass the method of the `Request` 
> > down into `authorizeEndpoint` so we do not need make this decision here (in 
> > some places it is directly tied to the handler, but the way the ACLs are 
> > designed you mix in concerns of the `Authorizer`).

That's a good point! The current routing in libprocess doesn't dispatch based 
on the request method. The functions that are routed to have to take care of 
that and unfortunately most of them don't check with what method they've been 
called. E.g. if '/flags' is requested with a POST, it will return the same as 
if requested with a GET -- I'd expect it to drop the POST request as 
unsupported.
So my assumption is here that while `/flags` may receive other requests than 
`GET`, it will always act like it's a `GET` request and therefore the authz 
should behave like it's a `GET`. That's a "not my department" approach to this 
issue. Your suggestion is "if it's a `POST` authorize it like a `POST`". I 
think we can go with that.


> On April 26, 2016, 4:01 p.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 787
> > <https://reviews.apache.org/r/46203/diff/15/?file=1361588#file1361588line787>
> >
> >     I really like that we use a dedicated `enum` in the switch below, but 
> > dislike how users of this function need to manually translate from a 
> > `std::string` to a `Method`.
> >     
> >     What about passing some `std::string` like stored inside the `Request` 
> > here (feel free to add a `TODO` to `Request` to strongly type `method`), 
> > and then doing the conversion in here? An unknown method would result in 
> > e.g., a `Failure`.

+1 for making `Request::method` strongly typed. To implement it right now I 
would remove the `Method` enum and compare the `Request::method` string 
directly with string of supported methods. I'd rather `switch` over the values 
of an enum, but the code will stay simple and readable by the string-string 
comparison.


- Jan


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


On April 26, 2016, 2:51 p.m., Jan Schlicht wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> -----------------------------------------------------------
> 
> (Updated April 26, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
>     https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   docs/configuration.md 318275fc5f935e6992ed4e8048cc4b42cc5d2cab 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Reply via email to