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 2016/10/07 22:26:32 UTC
Review Request 52655: GEODE-1973: add more tests to cover
GMSAuthenticator and SimpleSecurityManager
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52655/
-----------------------------------------------------------
Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
Repository: geode
Description
-------
* Add a SimpleSecurityManager
* Add more tests to cover GMSAuthenticator for cached and cacheless locator
Diffs
-----
geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorWithCachelessLocatorDUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java PRE-CREATION
geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/52655/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 52655: GEODE-1973: add more tests to cover
GMSAuthenticator and SimpleSecurityManager
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52655/#review151995
-----------------------------------------------------------
Ship it!
Don't forget to add the copyright statements.
- Kevin Duling
On Oct. 10, 2016, 8:14 a.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52655/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2016, 8:14 a.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Add a SimpleSecurityManager
> * Add more tests to cover GMSAuthenticator for cached and cacheless locator
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorWithCachelessLocatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java PRE-CREATION
> geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52655: GEODE-1973: add more tests to cover
GMSAuthenticator and SimpleSecurityManager
Posted by Mark Bretl <mb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52655/#review152006
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java (line 1)
<https://reviews.apache.org/r/52655/#comment220683>
Missing license header
geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java (line 1)
<https://reviews.apache.org/r/52655/#comment220684>
Missing license header
- Mark Bretl
On Oct. 10, 2016, 8:14 a.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52655/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2016, 8:14 a.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Add a SimpleSecurityManager
> * Add more tests to cover GMSAuthenticator for cached and cacheless locator
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorWithCachelessLocatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java PRE-CREATION
> geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52655: GEODE-1973: add more tests to cover
GMSAuthenticator and SimpleSecurityManager
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52655/#review152011
-----------------------------------------------------------
Ship it!
Add license headers (already done I think?) and then Ship It!
- Kirk Lund
On Oct. 10, 2016, 3:14 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52655/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2016, 3:14 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Add a SimpleSecurityManager
> * Add more tests to cover GMSAuthenticator for cached and cacheless locator
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorWithCachelessLocatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java PRE-CREATION
> geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52655: GEODE-1973: add more tests to cover
GMSAuthenticator and SimpleSecurityManager
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52655/
-----------------------------------------------------------
(Updated Oct. 10, 2016, 3:14 p.m.)
Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
Repository: geode
Description
-------
* Add a SimpleSecurityManager
* Add more tests to cover GMSAuthenticator for cached and cacheless locator
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorWithCachelessLocatorDUnitTest.java PRE-CREATION
geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java PRE-CREATION
geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/52655/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 52655: GEODE-1973: add more tests to cover
GMSAuthenticator and SimpleSecurityManager
Posted by Jinmei Liao <ji...@pivotal.io>.
> On Oct. 7, 2016, 11:21 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java, line 9
> > <https://reviews.apache.org/r/52655/diff/1/?file=1527748#file1527748line9>
> >
> > This class in the external packages of src/main. If we're not exposing this purposely for Users then it should move to an internal package. If it's exposed, then we need fancy javadocs and the Docs team will need to write about it too.
> >
> > I recommend moving to internal package.
It's meant to be serving the same purpose as SampleSecurityManager, but a simpler implementation without involving json files and such. This makes playing with security a lot easier. Our tests can use this instead of SampleSecurityManager as well for easier code flow. I will add fancy javadocs and let doc team know about it.
> On Oct. 7, 2016, 11:21 p.m., Kirk Lund wrote:
> > geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java, line 55
> > <https://reviews.apache.org/r/52655/diff/1/?file=1527752#file1527752line55>
> >
> > I'd recommend changing this to INFO or just remove the line. FINE will create lots of output.
Sounds good.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52655/#review151900
-----------------------------------------------------------
On Oct. 7, 2016, 10:26 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52655/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2016, 10:26 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Add a SimpleSecurityManager
> * Add more tests to cover GMSAuthenticator for cached and cacheless locator
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorWithCachelessLocatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java PRE-CREATION
> geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 52655: GEODE-1973: add more tests to cover
GMSAuthenticator and SimpleSecurityManager
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52655/#review151900
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java (line 9)
<https://reviews.apache.org/r/52655/#comment220551>
This class in the external packages of src/main. If we're not exposing this purposely for Users then it should move to an internal package. If it's exposed, then we need fancy javadocs and the Docs team will need to write about it too.
I recommend moving to internal package.
geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java (line 55)
<https://reviews.apache.org/r/52655/#comment220552>
I'd recommend changing this to INFO or just remove the line. FINE will create lots of output.
- Kirk Lund
On Oct. 7, 2016, 10:26 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52655/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2016, 10:26 p.m.)
>
>
> Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> * Add a SimpleSecurityManager
> * Add more tests to cover GMSAuthenticator for cached and cacheless locator
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/security/templates/SimpleSecurityManager.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/PeerAuthenticatorWithCachelessLocatorDUnitTest.java PRE-CREATION
> geode-core/src/test/java/org/apache/geode/security/templates/SimpleSecurityManagerTest.java PRE-CREATION
> geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52655/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>