> On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote: > > It would've been nice if, by default, we did not wire up the post-filter at > > all, and only asserted its counters when it has been wired in, but from a > > quick glance at the way the tests are set up, it doesn't seem to be > > possible to conditionally configure the `HttpSecurityModule`.
I could set this up as a ``Parameterized`` test and parameterize the filter inclusion. However, given that the filter is pass through and does not add any behavior at all, it seems like overkill. > On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote: > > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java, > > line 126 > > <https://reviews.apache.org/r/42046/diff/2/?file=1192830#file1192830line126> > > > > Do we need to use a `StatsProvider` for this? Can we just expose call > > counts from this class directly? Are there even threading concerns that > > require atomicity? I debated about the alternative - that would require making the injector protected and getting the filter instance from the injector in ``HttpSecurityIT``. I don't have a strong preference -- I can update the review with that change. > On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote: > > src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java, > > lines 156-159 > > <https://reviews.apache.org/r/42046/diff/2/?file=1192830#file1192830line156> > > > > Nit, just use `//` for multi-line comments. Ack. > On Jan. 12, 2016, 5:41 p.m., Joshua Cohen wrote: > > src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java, > > lines 174-179 > > <https://reviews.apache.org/r/42046/diff/2/?file=1192828#file1192828line174> > > > > Does this need to be here? Can this just be done in the downstream > > user-defined filter instead? Unfortunately, this appears to be the only reasonable place to override the binding since ``ShiroWebModule`` is a ``PrivateModule``. The ``bindSessionManager`` method is offered as an extension point specifically for this purpose per javadocs on ``ShiroWebModule``: https://shiro.apache.org/static/1.2.3/apidocs/src-html/org/apache/shiro/guice/web/ShiroWebModule.html#line.190 (static links to shiro v1.2.4 source are broken at the moment, but there is no change wrt this method in shiro 1.2.4) - Amol ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42046/#review114009 ----------------------------------------------------------- On Jan. 12, 2016, 1:15 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, 1:15 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 > >
