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/25 15:34:55 UTC
Review Request 53168: GEODE-2030: security support for SDG
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/
-----------------------------------------------------------
Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
Repository: geode
Description
-------
GEODE-2030: security support for SDG
* added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
* The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
Diffs
-----
geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/53168/diff/
Testing
-------
precheckin successful
Thanks,
Jinmei Liao
Re: Review Request 53168: GEODE-2030: security support for SDG
Posted by Kevin Duling <kd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/#review153800
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Duling
On Oct. 25, 2016, 8:34 a.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53168/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2016, 8:34 a.m.)
>
>
> Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2030: security support for SDG
>
> * added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
> * The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
> geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
> geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
> geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
> geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53168/diff/
>
>
> Testing
> -------
>
> precheckin successful
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53168: GEODE-2030: security support for SDG
Posted by Jinmei Liao <ji...@pivotal.io>.
> On Oct. 26, 2016, 9:39 p.m., Kirk Lund wrote:
> > geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java, line 21
> > <https://reviews.apache.org/r/53168/diff/2/?file=1546442#file1546442line21>
> >
> > This should be:
> >
> > import static org.assertj.core.api.Assertions.assertThatThrownBy;
> >
> > Don't use Java6Assertions.
Oh, yeah, thanks for catching this.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/#review153948
-----------------------------------------------------------
On Oct. 26, 2016, 9:12 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53168/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2016, 9:12 p.m.)
>
>
> Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2030: security support for SDG
>
> * added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
> * The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
> geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
> geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
> geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
> geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53168/diff/
>
>
> Testing
> -------
>
> precheckin successful
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53168: GEODE-2030: security support for SDG
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/#review153948
-----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java (line 21)
<https://reviews.apache.org/r/53168/#comment223441>
This should be:
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Don't use Java6Assertions.
- Kirk Lund
On Oct. 26, 2016, 9:12 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53168/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2016, 9:12 p.m.)
>
>
> Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2030: security support for SDG
>
> * added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
> * The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
> geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
> geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
> geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
> geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53168/diff/
>
>
> Testing
> -------
>
> precheckin successful
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53168: GEODE-2030: security support for SDG
Posted by Karen Miller <km...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/#review153964
-----------------------------------------------------------
You can add a new commit to this review with
rbt post --repository geode --tracking-branch=origin/develop --target-groups=geode -r 53168 newSHA1
- Karen Miller
On Oct. 26, 2016, 9:12 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53168/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2016, 9:12 p.m.)
>
>
> Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2030: security support for SDG
>
> * added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
> * The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
> geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
> geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
> geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
> geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53168/diff/
>
>
> Testing
> -------
>
> precheckin successful
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53168: GEODE-2030: security support for SDG
Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/
-----------------------------------------------------------
(Updated Oct. 26, 2016, 9:12 p.m.)
Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
Changes
-------
Can anyone tell me how to only update the review with my commit? I used rbt post --repository geode --tracking-branch=origin/develop --target-groups=geode -u, but it just uploads all the change set again.
Repository: geode
Description
-------
GEODE-2030: security support for SDG
* added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
* The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
Diffs (updated)
-----
geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/53168/diff/
Testing
-------
precheckin successful
Thanks,
Jinmei Liao
Re: Review Request 53168: GEODE-2030: security support for SDG
Posted by Jinmei Liao <ji...@pivotal.io>.
> On Oct. 26, 2016, 6:20 p.m., John Blum wrote:
> > geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java, line 454
> > <https://reviews.apache.org/r/53168/diff/1/?file=1545181#file1545181line454>
> >
> > Note, this assertion will only apply if assertions were enabled on startup of the Geode Server JVM Process (i.e. java ... -ea).
> >
> > That does not appear to be the case...
> > https://github.com/apache/incubator-geode/blob/rel/v1.0.0-incubating/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java#L1715-L1837
> >
> > Therefore, it might be better to use an Assert facility, similar to Spring's Assert class... http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/util/Assert.html
I think we don't want to have core depend on Spring libraries. I put in checks for null.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/#review153913
-----------------------------------------------------------
On Oct. 26, 2016, 9:12 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53168/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2016, 9:12 p.m.)
>
>
> Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2030: security support for SDG
>
> * added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
> * The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
> geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
> geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
> geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
> geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53168/diff/
>
>
> Testing
> -------
>
> precheckin successful
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 53168: GEODE-2030: security support for SDG
Posted by John Blum <jb...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53168/#review153913
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java (line 331)
<https://reviews.apache.org/r/53168/#comment223424>
+1
geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java (line 433)
<https://reviews.apache.org/r/53168/#comment223425>
Note, this assertion will only apply if assertions were enabled on startup of the Geode Server JVM Process (i.e. java ... -ea).
That does not appear to be the case...
https://github.com/apache/incubator-geode/blob/rel/v1.0.0-incubating/geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/LauncherLifecycleCommands.java#L1715-L1837
Therefore, it might be better to use an Assert facility, similar to Spring's Assert class... http://docs.spring.io/spring/docs/current/javadoc-api/org/springframework/util/Assert.html
geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java (line 448)
<https://reviews.apache.org/r/53168/#comment223427>
Could simplify to...
isIntegratedSecurity = (SecurityUtils.getSecurityManager() != null);
This would better guard against Apache Shiro's implementation of SecurityUtils.getSecurityManager() from changing what they return vs. throw.
I think this looks pretty good overall. This will definitely help with configuring and using Apache Geode's Integrated Security (framework) in different contexts (Spring, PCF, and even simple testing).
- John Blum
On Oct. 25, 2016, 3:34 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53168/
> -----------------------------------------------------------
>
> (Updated Oct. 25, 2016, 3:34 p.m.)
>
>
> Review request for geode, John Blum, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> GEODE-2030: security support for SDG
>
> * added cacheFactory.setSecurityManager and cacheFactory.setPostProcessor
> * The isIntegragtedSecurity flag is set by checking if the Shiro's securityManger is configured or not.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/org/apache/geode/cache/CacheFactory.java b62feac4b2a3ae15e940afb50f71abf61e5eee50
> geode-core/src/main/java/org/apache/geode/internal/cache/CacheConfig.java 91ae33304a1e82fa7c02c97edce2318d359220e2
> geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java d9d572c45ac559e611fe2dc86d1028d39d1e592c
> geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java 9f4697f6c9f06dbfd671e9e0d80dc52f447e5829
> geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java 41b08d510876f95733c96d103b0826726d9a09bb
> geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceTest.java ee76dfc095c3aeb43a04cee899ba4b434c1e552e
> geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/53168/diff/
>
>
> Testing
> -------
>
> precheckin successful
>
>
> Thanks,
>
> Jinmei Liao
>
>