----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42046/#review114103 -----------------------------------------------------------
It sounds like you ran into an issue that necessitated this patch. Is there anything you can add to `docs/security.md` so others don't get stuck down the same dead end? Also, please add a NEWS entry summarizing the new field so we make sure to highlight it in the next release. src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java (line 82) <https://reviews.apache.org/r/42046/#comment174914> How about `shiro_after_auth_filter`? While more verbose, it avoids the term overload of `post` with HTTP POST request (which is what i actually thought this was for at first). If you do rename, please apply it throughout the patch. src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java (line 118) <https://reviews.apache.org/r/42046/#comment174900> Drop the `maybe`, the `Optional` type is sufficient to indicate the possibly-absent value. Also, you should generally prefer to accept `Key` instead of `Class` for APIs relating to identifiers for guice bindings. So in this case, use `Key<? extends Filter>` everywhere except the `Arg` as that's more general and has parity with the `ShiroWebModule` API. Additionally, you should remove the line `bind(postFilter).in(Singleton.class);`, and require that the filter be externally bound. This gives the caller greater flexibility when it comes to defining the binding. src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java (line 138) <https://reviews.apache.org/r/42046/#comment174902> Make this `Optional` to have a consistent API/internals, and change the `null` above to `Optional.empty()` src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java (line 209) <https://reviews.apache.org/r/42046/#comment174899> Usually we try to minimize the scope of suppressions to prevent unexpectedly covering real warnings. Consider extracting a function that covers only the line requiring the suppression. This obviates the need for the comment `// For xyz` and prevents issues later on. src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java (line 112) <https://reviews.apache.org/r/42046/#comment174909> Can you approach this with a mock? Seems like it would be more direct and less brittle way to verify calls. With the suggestion above to accept `Key` and avoid the internal `bind()`, this should become much more doable than with the current API. - Bill Farner On Jan. 12, 2016, 10:52 a.m., Amol Deshmukh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42046/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2016, 10:52 a.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner. > > > Bugs: AURORA-1576 > https://issues.apache.org/jira/browse/AURORA-1576 > > > Repository: aurora > > > Description > ------- > > Allow for plugging in cli-configurable filters that are invoked post shiro > filters. > > > Diffs > ----- > > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java > 4b6a872737e5934394e9511af7b6bd96c6034e72 > src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java > 19c8a1fe06a333324022da11f74d7c96b2942587 > > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java > ac92117e14c105bec74e49d8e80eb56008237abf > > Diff: https://reviews.apache.org/r/42046/diff/ > > > Testing > ------- > > Unit tests: > ``` > $ ./gradlew clean test > ... > BUILD SUCCESSFUL > > Total time: 3 mins 24.616 secs > ``` > > End to end tests: > ``` > $ ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > ... > > *** OK (All tests passed) *** > > aurora-scheduler-kerberos stop/waiting > aurora-scheduler start/running, process 15362 > + RETCODE=0 > + restore_netrc > + mv /home/vagrant/.netrc.bak /home/vagrant/.netrc > + true > Connection to 127.0.0.1 closed. > > ``` > > > Thanks, > > Amol Deshmukh > >
