> On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote: > > 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.
Ack. > On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java, > > line 82 > > <https://reviews.apache.org/r/42046/diff/3/?file=1194765#file1194765line82> > > > > 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. Ack. > On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java, > > line 138 > > <https://reviews.apache.org/r/42046/diff/3/?file=1194765#file1194765line138> > > > > Make this `Optional` to have a consistent API/internals, and change the > > `null` above to `Optional.empty()` Using Optional for parameter types offers no benefit since it still doesn't prevent the caller from invoking it with a null value. i.e. the null check cannot be avoided (albeit with different consequences) when the parameter type is Optional. FWIW, there is support for this rationale in Kevin Bourrillion's comment here: https://plus.google.com/+JordanLewiser/posts/ayfR8F56PGy > On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java, > > line 209 > > <https://reviews.apache.org/r/42046/diff/3/?file=1194765#file1194765line209> > > > > 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. Ack. > On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java, > > line 118 > > <https://reviews.apache.org/r/42046/diff/3/?file=1194765#file1194765line118> > > > > 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. Thanks for the guidance here. > On Jan. 13, 2016, 1:31 a.m., Bill Farner wrote: > > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java, > > line 115 > > <https://reviews.apache.org/r/42046/diff/3/?file=1194767#file1194767line115> > > > > 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. Yes, removing the ``bind(...)`` makes this possible. Thanks! - Amol ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42046/#review114103 ----------------------------------------------------------- On Jan. 12, 2016, 6:52 p.m., Amol Deshmukh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42046/ > ----------------------------------------------------------- > > (Updated Jan. 12, 2016, 6:52 p.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 > >
