You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by Henry Saputra <hs...@apache.org> on 2011/05/19 20:47:32 UTC
Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/
-----------------------------------------------------------
Review request for shindig.
Summary
-------
Update the AuthenticationServletFilter:
1. Make the auth realm configurable via property or override able protected method.
2. Sets the auth header from the right handler. Currently the code sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to only add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
Diffs
-----
trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125043
trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125043
Diff: https://reviews.apache.org/r/760/diff
Testing
-------
Update unit test for null st.
Thanks,
Henry
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/#review692
-----------------------------------------------------------
trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java
<https://reviews.apache.org/r/760/#comment1389>
I think the @Override annotation should be added to methods thats being overridden.
I just created a method with protected scope that could be overridden if this class is extended.
This method is not defined in the InjectedFilter class.
- Henry
On 2011-05-19 18:47:32, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/760/
> -----------------------------------------------------------
>
> (Updated 2011-05-19 18:47:32)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> Update the AuthenticationServletFilter:
> 1. Make the auth realm configurable via property or override able protected method.
> 2. Sets the auth header from the right handler. Currently the code sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to only add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
>
>
> Diffs
> -----
>
> trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125043
> trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125043
>
> Diff: https://reviews.apache.org/r/760/diff
>
>
> Testing
> -------
>
> Update unit test for null st.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/
-----------------------------------------------------------
(Updated 2011-05-21 10:15:56.409455)
Review request for shindig.
Changes
-------
Only sets auth header when return status 401.
Summary
-------
Update the AuthenticationServletFilter:
1. Make the auth realm configurable via property or override able protected method.
2. Sets the auth header from the right handler. Currently the code just sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to also add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
Diffs (updated)
-----
trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125364
trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125364
Diff: https://reviews.apache.org/r/760/diff
Testing
-------
Update unit test for null st.
Thanks,
Henry
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Henry Saputra <hs...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/
-----------------------------------------------------------
(Updated 2011-05-21 10:03:03.945137)
Review request for shindig.
Changes
-------
Change the fix to sets auth header back for any handler that return null security token and also when throwing InvalidAuthentictionException. This is to make sre if any auth handler could sets auth header, then it has change to do it just in case the next one return null auth header for null security token.
Summary (updated)
-------
Update the AuthenticationServletFilter:
1. Make the auth realm configurable via property or override able protected method.
2. Sets the auth header from the right handler. Currently the code just sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to also add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
Diffs (updated)
-----
trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125364
trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125364
Diff: https://reviews.apache.org/r/760/diff
Testing
-------
Update unit test for null st.
Thanks,
Henry
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Henry Saputra <hs...@apache.org>.
> On 2011-05-20 01:37:25, Ryan Baxter wrote:
> > trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java, line 146
> > <https://reviews.apache.org/r/760/diff/1/?file=19450#file19450line146>
> >
> > Ah I see, so what's the point of passing in the request object then? Would classes which extend this class need that object for something? (Not really familiar with what this code does, I was just taking a look at your changes.)
Thanks for the review.
I was thinking if a class extending it needs request to get information to build the realm such as hostname, some request parameters, etc. The idea was to make developers life easier when they have to extend default functionality. In this case they dont have to copy-paste the logic in the doFilter method.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/#review693
-----------------------------------------------------------
On 2011-05-19 18:47:32, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/760/
> -----------------------------------------------------------
>
> (Updated 2011-05-19 18:47:32)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> Update the AuthenticationServletFilter:
> 1. Make the auth realm configurable via property or override able protected method.
> 2. Sets the auth header from the right handler. Currently the code sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to only add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
>
>
> Diffs
> -----
>
> trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125043
> trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125043
>
> Diff: https://reviews.apache.org/r/760/diff
>
>
> Testing
> -------
>
> Update unit test for null st.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/#review693
-----------------------------------------------------------
trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java
<https://reviews.apache.org/r/760/#comment1390>
Ah I see, so what's the point of passing in the request object then? Would classes which extend this class need that object for something? (Not really familiar with what this code does, I was just taking a look at your changes.)
- Ryan
On 2011-05-19 18:47:32, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/760/
> -----------------------------------------------------------
>
> (Updated 2011-05-19 18:47:32)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> Update the AuthenticationServletFilter:
> 1. Make the auth realm configurable via property or override able protected method.
> 2. Sets the auth header from the right handler. Currently the code sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to only add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
>
>
> Diffs
> -----
>
> trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125043
> trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125043
>
> Diff: https://reviews.apache.org/r/760/diff
>
>
> Testing
> -------
>
> Update unit test for null st.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/#review698
-----------------------------------------------------------
trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java
<https://reviews.apache.org/r/760/#comment1400>
Ok sounds good LGTM
- Ryan
On 2011-05-19 18:47:32, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/760/
> -----------------------------------------------------------
>
> (Updated 2011-05-19 18:47:32)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> Update the AuthenticationServletFilter:
> 1. Make the auth realm configurable via property or override able protected method.
> 2. Sets the auth header from the right handler. Currently the code sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to only add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
>
>
> Diffs
> -----
>
> trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125043
> trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125043
>
> Diff: https://reviews.apache.org/r/760/diff
>
>
> Testing
> -------
>
> Update unit test for null st.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Henry Saputra <hs...@apache.org>.
> On 2011-05-20 00:32:42, Ryan Baxter wrote:
> > trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java, line 146
> > <https://reviews.apache.org/r/760/diff/1/?file=19450#file19450line146>
> >
> > Should we add the @Override notation here to indicate your overriding the method? It wasn't clear to me until I read your comment.
I think the @Override annotation should be added to methods thats being overriden. Here I just create method with protected scope so it could be overridden if this class is extended.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/#review690
-----------------------------------------------------------
On 2011-05-21 10:03:03, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/760/
> -----------------------------------------------------------
>
> (Updated 2011-05-21 10:03:03)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> Update the AuthenticationServletFilter:
> 1. Make the auth realm configurable via property or override able protected method.
> 2. Sets the auth header from the right handler. Currently the code just sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to also add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
>
>
> Diffs
> -----
>
> trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125364
> trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125364
>
> Diff: https://reviews.apache.org/r/760/diff
>
>
> Testing
> -------
>
> Update unit test for null st.
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: Update AuthenticationServletFilter to make the realm
configurable and set auth header with the right Auth handler
Posted by Ryan Baxter <rb...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/760/#review690
-----------------------------------------------------------
trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java
<https://reviews.apache.org/r/760/#comment1387>
Should we add the @Override notation here to indicate your overriding the method? It wasn't clear to me until I read your comment.
- Ryan
On 2011-05-19 18:47:32, Henry Saputra wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/760/
> -----------------------------------------------------------
>
> (Updated 2011-05-19 18:47:32)
>
>
> Review request for shindig.
>
>
> Summary
> -------
>
> Update the AuthenticationServletFilter:
> 1. Make the auth realm configurable via property or override able protected method.
> 2. Sets the auth header from the right handler. Currently the code sets the response's WWW-Authenticate header whenever an auth handler return null st. So if the next handler return a security token, the response contains WWW-Authenticate header from previous handler. This CR change the logic to only add WWW-Authenticate header if token is not set or InvalidAuthenticationException is thrown.
>
>
> Diffs
> -----
>
> trunk/java/common/src/main/java/org/apache/shindig/auth/AuthenticationServletFilter.java 1125043
> trunk/java/common/src/test/java/org/apache/shindig/auth/AuthenticationServletFilterTest.java 1125043
>
> Diff: https://reviews.apache.org/r/760/diff
>
>
> Testing
> -------
>
> Update unit test for null st.
>
>
> Thanks,
>
> Henry
>
>