> On Dec. 4, 2019, 8:07 p.m., Benjamin Mahler wrote: > > src/master/authorization.cpp > > Lines 145-147 (patched) > > <https://reviews.apache.org/r/71863/diff/1/?file=2181776#file2181776line145> > > > > A broader question that relates to all of these reviews with Try > > values.. are we essentially changing the way validation works? Will > > validation no longer be catching cases (and creating error message for > > them) because we now catch them during authorization, and potentially send > > worse error messages than the validation code generates?
Of the two reviews with Try, the preceding patch does not change the way how authorization interacts with validation. Will check error messages there. This one does - but I will reconsider this change, as it does not address the underlying problem fully. ----- **The problem** is that currently combination of authorization + validation handles some invalid operations in a very *interesting* way. If the operation is "valid enough" to build a valid authz object, we build the object, authorize and, if authorized, validate. That produces appropriate validation/authorization errors. On the other hand, if we cannot even build a valid authz object, we request authorization for SOMETHING (usually, to perform the specified action on ANY object), authorize, and, if authorized, validate. This means that if permissions setup is restrictive enough, the caller will be **declined authorization** for a wrong reason instead of **getting a validation error**. I see two options to fix this systematically: - split ACCEPT validation into stateless and stateful part, and move the first one before authorization - if the synchronous authz makes it (and we manage to avoid having async fallback for non-compliant authorizers), to move the whole validation before ACCEPT authorization Will file a ticket and add this as TODO. > On Dec. 4, 2019, 8:07 p.m., Benjamin Mahler wrote: > > src/master/http.cpp > > Lines 1067 (patched) > > <https://reviews.apache.org/r/71863/diff/1/?file=2181777#file2181777line1067> > > > > `std::move(*actionObjects)` ? Checked once again - both ways are correct. `(*actionObjects)` in this context a non-const lvalue reference, hence `std::move(*actionObject)` is an rvalue reference to the vector. `std::move(actionObject)` is an rvalue reference, and then `T&& Try<T> get()&&` is used. Will use the first option everywhere, for consistency. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71863/#review218922 ----------------------------------------------------------- On Dec. 3, 2019, 5:54 p.m., Andrei Sekretenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71863/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2019, 5:54 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Mahler. > > > Bugs: MESOS-10023 and MESOS-10056 > https://issues.apache.org/jira/browse/MESOS-10023 > https://issues.apache.org/jira/browse/MESOS-10056 > > > Repository: mesos > > > Description > ------- > > Moved creating authorization Object out of `Master::authorize*Volume`. > > > Diffs > ----- > > src/master/authorization.hpp PRE-CREATION > src/master/authorization.cpp PRE-CREATION > src/master/http.cpp e03655863ea2d4e4464b3d14b359de3d7f059778 > src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665 > src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 > > > Diff: https://reviews.apache.org/r/71863/diff/1/ > > > Testing > ------- > > > Thanks, > > Andrei Sekretenko > >
