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