----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39520/#review111166 -----------------------------------------------------------
I would like to see one more test, where both authn and authz are disabled and quota set request succeeds. src/tests/master_quota_tests.cpp (lines 994 - 996) <https://reviews.apache.org/r/39520/#comment171194> It's not the fixture which is set up to expect certain credentials, it's the master which is set up in the fixture in a way that only a certain credentials will be authenticated. Let's be explicit about it. How about: The master is configured so that only requests from `DEFAULT_CREDENTIAL` are authenticated. src/tests/master_quota_tests.cpp (lines 1010 - 1011) <https://reviews.apache.org/r/39520/#comment171195> How about a one-liner: // The absense of credentials leads to authentication failure as well. src/tests/master_quota_tests.cpp (line 1031) <https://reviews.apache.org/r/39520/#comment171196> It's not the operator, but a rather specific principal. How about // `DEFAULT_CREDENTIAL.principal()` can request quotas for `ROLE1` (mind backticks). src/tests/master_quota_tests.cpp (line 1055) <https://reviews.apache.org/r/39520/#comment171197> quota set request; backtick `ROLE1` s/ that can be authorized././ Or you can kill the comment entirely. src/tests/master_quota_tests.cpp (line 1091) <https://reviews.apache.org/r/39520/#comment171198> Ditto as in the previous test. src/tests/master_quota_tests.cpp (lines 1097 - 1101) <https://reviews.apache.org/r/39520/#comment171199> Let's refactor for readability: // Disable authentication by not providing credentials. master::Flags masterFlags = CreateMasterFlags(); masterFlags.acls = acls; masterFlags.credentials = None(); src/tests/master_quota_tests.cpp (line 1118) <https://reviews.apache.org/r/39520/#comment171200> See my comment in the previous test. src/tests/master_quota_tests.cpp (line 1127) <https://reviews.apache.org/r/39520/#comment171205> Let's drag reader's attention that no credentials are provided. src/tests/master_quota_tests.cpp (line 1136) <https://reviews.apache.org/r/39520/#comment171201> You can kill this balnk line src/tests/master_quota_tests.cpp (line 1147) <https://reviews.apache.org/r/39520/#comment171202> backtick `ROLE1`, here and everywhere, please. src/tests/master_quota_tests.cpp (line 1161) <https://reviews.apache.org/r/39520/#comment171203> Let's kill this comment. - Alexander Rukletsov On Dec. 18, 2015, 9:12 a.m., Jan Schlicht wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39520/ > ----------------------------------------------------------- > > (Updated Dec. 18, 2015, 9:12 a.m.) > > > Review request for mesos, Alexander Rukletsov and Till Toenshoff. > > > Bugs: MESOS-4082 > https://issues.apache.org/jira/browse/MESOS-4082 > > > Repository: mesos > > > Description > ------- > > Quota: Added authentication, authorization tests. > > > Diffs > ----- > > src/tests/authorization_tests.cpp dcf348acc75b143e3a854bde17e6042dc90599d5 > src/tests/master_quota_tests.cpp 0473869783a714766ed26fff61d7f8c56342df74 > > Diff: https://reviews.apache.org/r/39520/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jan Schlicht > >
