----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53541/#review157028 -----------------------------------------------------------
src/authorizer/local/authorizer.cpp (line 502) <https://reviews.apache.org/r/53541/#comment227411> Fits on one line. src/authorizer/local/authorizer.cpp (line 511) <https://reviews.apache.org/r/53541/#comment227412> The `const` is redundant here, mind cleaning it up here and above in a case? src/authorizer/local/authorizer.cpp (lines 479 - 480) <https://reviews.apache.org/r/53541/#comment227434> You could remove one level of nesting here, e.g., if (object->command_info != nullptr && object->command_info->has_user()) { ... src/authorizer/local/authorizer.cpp (line 508) <https://reviews.apache.org/r/53541/#comment227429> This can go now, right? src/authorizer/local/authorizer.cpp (line 545) <https://reviews.apache.org/r/53541/#comment227438> Mind adding a small comment here explicitly mentioning the approval policy? src/authorizer/local/authorizer.cpp (lines 554 - 555) <https://reviews.apache.org/r/53541/#comment227430> Move this down to the first use. src/authorizer/local/authorizer.cpp (line 569) <https://reviews.apache.org/r/53541/#comment227461> I think this behaves incorrectly for some cases when `permissive==true` and the presence of an explicit deny rule only for `userApprover_`. We might here fallback to `permissive` in `parentApprover_` before ever examing the rule in `userApprover_`. Instead I think you should create the subapprovers with `permissive=false` and handle permissive mode here explicitly. - Benjamin Bannier On Nov. 28, 2016, 5:42 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53541/ > ----------------------------------------------------------- > > (Updated Nov. 28, 2016, 5:42 p.m.) > > > Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff. > > > Bugs: MESOS-6474 > https://issues.apache.org/jira/browse/MESOS-6474 > > > Repository: mesos > > > Description > ------- > > Creates new authorization action for all the API's related to > nested containers. This patch does not add the code necesary to > call use those actions, this is done in a latter patch. > > > Diffs > ----- > > include/mesos/authorizer/acls.proto > e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b > include/mesos/authorizer/authorizer.hpp > 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2 > include/mesos/authorizer/authorizer.proto > 0696a629ac2d2b9950e20708f0c3666b58ff7ca0 > src/authorizer/local/authorizer.cpp > 77e05dd2475d6e7511e7c7eeea578ec31ff3d198 > src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474 > > Diff: https://reviews.apache.org/r/53541/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
