----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44322/#review122991 -----------------------------------------------------------
include/mesos/authorizer/authorizer.hpp (line 43) <https://reviews.apache.org/r/44322/#comment185106> s/it does/it does/ include/mesos/authorizer/authorizer.proto (lines 27 - 28) <https://reviews.apache.org/r/44322/#comment185097> How about "If `value` is not set the subject is unknown." include/mesos/authorizer/authorizer.proto (line 37) <https://reviews.apache.org/r/44322/#comment185098> s/unix// You need to also add: "If `value` is not set the object is unknown". include/mesos/authorizer/authorizer.proto (line 57) <https://reviews.apache.org/r/44322/#comment185101> 2 blank lines. include/mesos/authorizer/authorizer.proto (line 58) <https://reviews.apache.org/r/44322/#comment185105> <`subject`, `action`, `object`> include/mesos/authorizer/authorizer.proto (line 59) <https://reviews.apache.org/r/44322/#comment185102> s/can/Can/ src/authorizer/local/authorizer.hpp (lines 51 - 55) <https://reviews.apache.org/r/44322/#comment185109> this comment should be in the review that introduced this overload. src/authorizer/local/authorizer.cpp (lines 83 - 88) <https://reviews.apache.org/r/44322/#comment185225> This method's signature is not intuitive. For example, what's a selector? and why objects need a selector but not subjects? I know the reason for the latter, but you understand what i'm getting at. One proposal: ``` struct GenericACL { required ACL::Entity subjects; required ACL::Entity objects; } Future<bool> authorized(const authorization::Request& request) { switch (request.action()) { case authorization::REGISTER_FRAMEWORK_WITH_ROLE: // Convert "RegisterFramework" ACL to a generic ACL. vector<GenericACL> acls; foreach (const ACL::RegisterFramework& acl, acls.register_frameworks()) { GenericACL acl_; acl_.subjects = acl.principals(); acl_.object = acl.roles(); acls.push_back(acl_); } return authorized(request, acls); ... ... ... } } Future<bool> authorized(const authorization::Request& request, const vector<GenericACL>& acls) { ... } ``` If there is a way to templatize the conversion to GenericACL that would be better. But even without that, the above code is easier to follow IMO. src/authorizer/local/authorizer.cpp (line 160) <https://reviews.apache.org/r/44322/#comment185120> no need for default. the local authorizer should be never called with an action it doesn't understand. src/master/master.cpp (line 2892) <https://reviews.apache.org/r/44322/#comment185227> s/however/However/ src/master/master.cpp (line 2939) <https://reviews.apache.org/r/44322/#comment185232> new line. src/master/master.cpp (line 3041) <https://reviews.apache.org/r/44322/#comment185231> new line. - Vinod Kone On March 10, 2016, 2:15 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44322/ > ----------------------------------------------------------- > > (Updated March 10, 2016, 2:15 p.m.) > > > Review request for mesos, Adam B, Joerg Schad, and Vinod Kone. > > > Bugs: MESOS-2950 > https://issues.apache.org/jira/browse/MESOS-2950 > > > Repository: mesos > > > Description > ------- > > Implements the [Generic Authorizer Interface v > 0.3.1](https://docs.google.com/document/d/1-XARWJFUq0r_TgRHz_472NvLZNjbqE4G8c2JL44OSMQ). > > It effectively separates the language used to define ACLs from the > language used to query it allowing for arbitrary identity server > backends. > > > Diffs > ----- > > include/mesos/authorizer/authorizer.hpp > 705482656788ac12e9d21e355b71fd2ede2be558 > include/mesos/authorizer/authorizer.proto PRE-CREATION > src/CMakeLists.txt e9f7c3a447aad79ab19d53879888a413a587408f > src/Makefile.am b24f0f58fa188c16770fe6a3c23ec06262cb0955 > src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 > src/authorizer/local/authorizer.hpp > c87a9915bae6bae7744bd57abd12e8d857181051 > src/authorizer/local/authorizer.cpp > 15c857de79cd7dd2c29b6b8cfb81204b919f1b28 > src/master/http.cpp 7304bfd5350d763d9ed1d5acdc285874b6d8f5df > src/master/master.cpp 249e82ffcef35aa8df3c5b9faef5b9b25d68facc > src/master/quota_handler.cpp a41c91f10bc0eedc754425b4de1b3e184c4ffb08 > src/master/weights_handler.cpp 9e4ab19fd760a56f1bbce915d1c7b63a0d1e5ed5 > src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 > src/tests/master_authorization_tests.cpp > 29c89fb11da792c3e71eb880a19657ea225b3cc8 > src/tests/mesos.hpp 9409da7ffe81ab4b1fc01213e27f1f639ba36581 > src/tests/mesos.cpp 577072136415191334c74b69a1c0b7eed8e77e8b > src/tests/reconciliation_tests.cpp 97112c4d64c75a16fdd7bbefd517a039fbf55b64 > > Diff: https://reviews.apache.org/r/44322/diff/ > > > Testing > ------- > > make -j4 check > > > Thanks, > > Alexander Rojas > >
