Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Bill Farner 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114560
---


One more drive-by: in the test I don't believe you need captures.  EasyMock 
gives you access to call arguments (EasyMock.getCurrentArguments I believe) 
within the andAnswer.  This unfortunately requires you to cast, but I find it 
preferable as the resulting code is more self-contained.

- Bill Farner


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> 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
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Amol Deshmukh 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/
---

(Updated Jan. 14, 2016, 1:10 p.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.


Changes
---

Implemented the last round of feedback from wfarner. In ``HttpSecurityIT``:
1. bound the Shiro post auth filter using an annotation
2. replaced use of EasyMock captures with ``EasyMock.getCurrentArguments`` for 
implementing the pass-through filter behavior.


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 (updated)
-

  NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
  docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  
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

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114579
---

Ship it!


Master (a80260e) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 14, 2016, 9:10 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 9:10 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> 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
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Zameer Manji 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114580
---

Ship it!


Ship It!

- Zameer Manji


On Jan. 14, 2016, 1:10 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 1:10 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> 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
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Amol Deshmukh 


> On Jan. 14, 2016, 12:26 p.m., Bill Farner wrote:
> > On mobile and can't comment on the specific line, but you should qualify 
> > the `Filter` binding with a binding annotation.  This is best practice for 
> > any binding that is moderately generic.

Ack.


- Amol


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114557
---


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> 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
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Amol Deshmukh 


> On Jan. 14, 2016, 12:29 p.m., Bill Farner wrote:
> > One more drive-by: in the test I don't believe you need captures.  EasyMock 
> > gives you access to call arguments (EasyMock.getCurrentArguments I believe) 
> > within the andAnswer.  This unfortunately requires you to cast, but I find 
> > it preferable as the resulting code is more self-contained.

Thanks, I was not aware of ``EasyMock.getCurrentArguments``.


- Amol


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114560
---


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> 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
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Bill Farner 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114557
---


On mobile and can't comment on the specific line, but you should qualify the 
`Filter` binding with a binding annotation.  This is best practice for any 
binding that is moderately generic.

- Bill Farner


On Jan. 14, 2016, 11:32 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 11:32 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> 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
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-14 Thread Zameer Manji 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114589
---


Amol, can you please rebase so I can apply this change?

- Zameer Manji


On Jan. 14, 2016, 1:10 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 14, 2016, 1:10 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji.
> 
> 
> 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
> -
> 
>   NEWS acaff9eb2ab184b0ef750f8b8a00c20131997f6b 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114159
---

Ship it!


Master (952ef6d) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 13, 2016, 7:20 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 13, 2016, 7:20 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
> -
> 
>   NEWS ecb26d2af2c624042e7819acde004278520b3cae 
>   docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
>   
> src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
>  4b6a872737e5934394e9511af7b6bd96c6034e72 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Amol Deshmukh 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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` 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
> > 
> >
> > 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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java
>  ac92117e14c105bec74e49d8e80eb56008237abf 
> 
> Diff: https://reviews.apache.org/r/42046/diff/
> 
> 
> Testing
> ---
> 
> Unit tests:
> ```
> $

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Amol Deshmukh 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/
---

(Updated Jan. 13, 2016, 7:20 a.m.)


Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Bill Farner.


Changes
---

* Addressed wfarner's review comments


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 (updated)
-

  NEWS ecb26d2af2c624042e7819acde004278520b3cae 
  docs/security.md f9b60e9afa3280dc132067c5167f624c34ade66f 
  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  
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

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Amol Deshmukh 

---
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.


Changes
---

- Changed multiline comments to use // instead of /* ... */


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 (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
19c8a1fe06a24022da11f74d7c96b2942587 
  
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

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Amol Deshmukh 


> 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
> > 
> >
> > 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?
> 
> Amol Deshmukh wrote:
> 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.

I spoke too soon - the filter is bound inside ``ShiroWebModule`` which being a 
``PrivateModule`` doesn't allow access to the filter instance. So I think I'm 
hitting a wall with the alternate approach.


- 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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Amol Deshmukh 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114054
---

Ship it!


Master (d542bd1) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Joshua Cohen 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114060
---

Ship it!


Ship It!

- Joshua Cohen


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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Bill Farner 

---
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)


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)


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` 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)


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)


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)


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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-12 Thread Joshua Cohen 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review114009
---


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`.


src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 (lines 174 - 179)


Does this need to be here? Can this just be done in the downstream 
user-defined filter instead?



src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
(line 123)


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?



src/test/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityIT.java 
(lines 153 - 156)


Nit, just use `//` for multi-line comments.


- Joshua Cohen


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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-11 Thread Amol Deshmukh 

---
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.


Changes
---

- Refactored previous change to allow specifying the shiro post filter as a 
constructor arg for tests.
- Updated existing tests with assertions for the new shiro post filter behavior.


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 (updated)
-

  
src/main/java/org/apache/aurora/scheduler/http/api/security/HttpSecurityModule.java
 4b6a872737e5934394e9511af7b6bd96c6034e72 
  src/test/java/org/apache/aurora/scheduler/http/AbstractJettyTest.java 
19c8a1fe06a24022da11f74d7c96b2942587 
  
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

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-11 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review113898
---

Ship it!


Master (f064dc1) is green with this patch.
  ./build-support/jenkins/build.sh

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


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 
> 19c8a1fe06a24022da11f74d7c96b2942587 
>   
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-11 Thread Amol Deshmukh 


> On Jan. 10, 2016, 5:21 a.m., Bill Farner wrote:
> > -1, please add test coverage to mitigate likelihood of breaking this change.
> 
> Joshua Cohen wrote:
> +1 to the -1 ;). Please add tests!

Please let me know if you have suggestions/guidance for adding tests for this 
change. Given that this was a change to add generic hook to the module, I 
presumed existing tests would suffice (to prove no regression was introduced).


- Amol


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review113638
---


On Jan. 9, 2016, 12:55 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 9, 2016, 12:55 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 
> 
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-09 Thread Bill Farner 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review113638
---


-1, please add test coverage to mitigate likelihood of breaking this change.

- Bill Farner


On Jan. 8, 2016, 4:55 p.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 8, 2016, 4:55 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 
> 
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-09 Thread Maxim Khutornenko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review113637
---

Ship it!


Ship It!

- Maxim Khutornenko


On Jan. 9, 2016, 12:55 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 9, 2016, 12:55 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 
> 
> 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
> 
>

Re: Review Request 42046: Allow for plugging in cli-configurable filters that are invoked post shiro filters.

2016-01-08 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42046/#review113591
---


Master (b25ab87) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Jan. 9, 2016, 12:55 a.m., Amol Deshmukh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42046/
> ---
> 
> (Updated Jan. 9, 2016, 12:55 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 
> 
> 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
> 
>