mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Schlicht <...@mesosphere.io>
Subject Re: Review Request 46203: Added authorization of the '/flags' endpoint.
Date Tue, 26 Apr 2016 14:42:35 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message