You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2017/04/06 16:52:48 UTC

Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

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

Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.


Repository: geode


Description
-------

GEODE-2756: Do not put security-* properties in the env.


Diffs
-----

  geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
  geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
  geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 


Diff: https://reviews.apache.org/r/58238/diff/1/


Testing
-------

precheckin running


Thanks,

Jinmei Liao

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Jinmei Liao <ji...@pivotal.io>.

> On April 6, 2017, 11:07 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java
> > Line 56 (original), 53 (patched)
> > <https://reviews.apache.org/r/58238/diff/1/?file=1686145#file1686145line56>
> >
> >     This doesn't relate to your changes per se, but I am curious why ENV needs to be ThreadLocal while securityService does not.  Do you know how Spring manages the instances of this class?  I.e. is there a singleton instance, or one instance per session, or something else?
> 
> Jinmei Liao wrote:
>     I believe it's just a singleton instance in the web container. It's defined in the servlet xml:
>     
>       <mvc:interceptors>
>         <bean class="org.apache.geode.management.internal.web.controllers.support.LoginHandlerInterceptor"/>
>       </mvc:interceptors>
> 
> Jared Stewart wrote:
>     Hm, that seems possibly worrisome to me. Maybe you can clarify.  When someone logs in, I believe Shrio sets the subject on the (ThreadLocal) ThreadContext.  But since this is a thread in a web server, can't that thread later potentially server requests from a different user?  What stops this later user from getting the subject set on the thread earlier?

the later user will come in and this interceptor will always call a login() and hence sets the subject with the new user on this thread. every request is stateless, after request is done, a logout() will get called to clear the subject on the thread as well.


- Jinmei


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


On April 6, 2017, 9:12 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Jared Stewart <js...@pivotal.io>.

> On April 6, 2017, 11:07 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java
> > Line 56 (original), 53 (patched)
> > <https://reviews.apache.org/r/58238/diff/1/?file=1686145#file1686145line56>
> >
> >     This doesn't relate to your changes per se, but I am curious why ENV needs to be ThreadLocal while securityService does not.  Do you know how Spring manages the instances of this class?  I.e. is there a singleton instance, or one instance per session, or something else?
> 
> Jinmei Liao wrote:
>     I believe it's just a singleton instance in the web container. It's defined in the servlet xml:
>     
>       <mvc:interceptors>
>         <bean class="org.apache.geode.management.internal.web.controllers.support.LoginHandlerInterceptor"/>
>       </mvc:interceptors>

Hm, that seems possibly worrisome to me. Maybe you can clarify.  When someone logs in, I believe Shrio sets the subject on the (ThreadLocal) ThreadContext.  But since this is a thread in a web server, can't that thread later potentially server requests from a different user?  What stops this later user from getting the subject set on the thread earlier?


- Jared


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


On April 6, 2017, 9:12 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Jinmei Liao <ji...@pivotal.io>.

> On April 6, 2017, 11:07 p.m., Jared Stewart wrote:
> > geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java
> > Line 56 (original), 53 (patched)
> > <https://reviews.apache.org/r/58238/diff/1/?file=1686145#file1686145line56>
> >
> >     This doesn't relate to your changes per se, but I am curious why ENV needs to be ThreadLocal while securityService does not.  Do you know how Spring manages the instances of this class?  I.e. is there a singleton instance, or one instance per session, or something else?

I believe it's just a singleton instance in the web container. It's defined in the servlet xml:

  <mvc:interceptors>
    <bean class="org.apache.geode.management.internal.web.controllers.support.LoginHandlerInterceptor"/>
  </mvc:interceptors>


- Jinmei


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


On April 6, 2017, 9:12 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58238/#review171268
-----------------------------------------------------------




geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java
Line 56 (original), 53 (patched)
<https://reviews.apache.org/r/58238/#comment244165>

    This doesn't relate to your changes per se, but I am curious why ENV needs to be ThreadLocal while securityService does not.  Do you know how Spring manages the instances of this class?  I.e. is there a singleton instance, or one instance per session, or something else?


- Jared Stewart


On April 6, 2017, 9:12 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Patrick Rhomberg <pr...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58238/#review172976
-----------------------------------------------------------


Ship it!




Ship It!

- Patrick Rhomberg


On April 6, 2017, 9:12 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Jared Stewart <js...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58238/#review171269
-----------------------------------------------------------


Ship it!




Ship It!

- Jared Stewart


On April 6, 2017, 9:12 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58238/#review172978
-----------------------------------------------------------


Fix it, then Ship it!




Fix and Ship!


geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java
Lines 37 (patched)
<https://reviews.apache.org/r/58238/#comment246025>

    Recommend private on these vars.


- Kirk Lund


On April 6, 2017, 9:12 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 9:12 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58238/
-----------------------------------------------------------

(Updated April 6, 2017, 9:12 p.m.)


Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.


Repository: geode


Description
-------

GEODE-2756: Do not put security-* properties in the env.


Diffs
-----

  geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
  geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
  geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 


Diff: https://reviews.apache.org/r/58238/diff/1/


Testing (updated)
-------

precheckin successful


Thanks,

Jinmei Liao

Re: Review Request 58238: GEODE-2756: Do not put security-* properties in the env.

Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58238/#review171229
-----------------------------------------------------------


Ship it!




Ship It!

- Kirk Lund


On April 6, 2017, 4:52 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58238/
> -----------------------------------------------------------
> 
> (Updated April 6, 2017, 4:52 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Ken Howe, Kirk Lund, and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-2756: Do not put security-* properties in the env.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptor.java 9e051748f48b6d792724706535b120f1f4b0d1be 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorJUnitTest.java 63d410f6335d9eabbb2547551e32dab8631c7871 
>   geode-web/src/test/java/org/apache/geode/management/internal/web/controllers/support/LoginHandlerInterceptorRequestHeaderJUnitTest.java 118e3853750d7ea3ef9af6ac092664a4dabb0108 
> 
> 
> Diff: https://reviews.apache.org/r/58238/diff/1/
> 
> 
> Testing
> -------
> 
> precheckin running
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>