> On Oct. 14, 2016, 5:21 p.m., Jared Stewart wrote:
> > Many usages of ServerStarter have to specify 
> > "org/apache/geode/management/internal/security/clientServer.json".  I think 
> > it would be convenient to have some static factory methods like 
> > ServerStarter.startWithSecurity() that specify a default set of Properties 
> > like SampleSecurityManager and 
> > "org/apache/geode/management/internal/security/clientServer.json". 
> > 
> > The same addition would be useful in CacheServerStartupRule.  It would be 
> > convenient to not have to pass in 
> > "org/apache/geode/management/internal/security/cacheServer.json".
> > 
> > RestSecurityService has authorize methods that show up as unused in 
> > Intellij.  I know from looking at the other changes that they are used by 
> > @PreAuthorized annotations in the CrudControllers, but it might be nice to 
> > add comments to these methods explaining where they're used so that people 
> > in the future don't think they can be deleted.
> 
> Jinmei Liao wrote:
>     The thing is, that json file is very test specific. Each test can use a 
> different json file to change the username/password it's using and 
> permissions granted to that user. We started the tests using this 
> SampleSecurityManager which unfortunatly requires the json file. I would hope 
> all our later tests would use SimpleSecurityManager instead to get rid this.

Right now it looked like every place using this was specifying that same json 
file.  Creating a static factory method like 
CacheServerStartupRule.withSampleSecurityManager(Properties properties) that 
would return a CacheServerStartupRule with the sample security manager and 
security json specified by default, but still allowing the user to override 
them if they want to seems like it would give the best of both worlds.


- Jared


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


On Oct. 14, 2016, 5:25 p.m., Jinmei Liao wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52889/
> -----------------------------------------------------------
> 
> (Updated Oct. 14, 2016, 5:25 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Kevin Duling, and Kirk Lund.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> * created ServerStarter and LocatorStarter in the rule package
> * refacterred LocatorServerConfigurationRule
> * refactor tests to use these rules
> 
> 
> Diffs
> -----
> 
>   
> geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityDUnitTest.java
>  59e00c8bd0a7f2759f579abf99a87fcd98b42a06 
>   
> geode-core/src/main/java/org/apache/geode/internal/security/IntegratedSecurityService.java
>  79b70f875f74c15eb041e9d1cbd5b1f8ceeda899 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/AccessControlMBeanJUnitTest.java
>  c22fff3655131cf908a54289b66faf84311c5c69 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanAuthenticationJUnitTest.java
>  388094840fc587e662d231783fd5c7c8faf7b485 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanAuthorizationJUnitTest.java
>  7bbfbcc9691000601cf6bf4dd0be7c7394cf3fcf 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerMBeanShiroJUnitTest.java
>  721a4312456e0f1eaf41a6c1f41016cfe56450e4 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerStartupRule.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/CliCommandsSecurityTest.java
>  84155a9d82292dc70c6412908a1b57a09c1aea98 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/DataCommandsSecurityTest.java
>  0084cb8f5471ce7ecab086c955a842766d6ed790 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/DiskStoreMXBeanSecurityJUnitTest.java
>  750ce2ac76ecd0860f5f678a22615697c9ea5009 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/GatewayReceiverMBeanSecurityTest.java
>  b64a6f7b1c63eb41f5823b4971d5041431748db9 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/GatewaySenderMBeanSecurityTest.java
>  9acf8dbd172cca27ba4da942b3d5c1bd0368ff83 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsPostProcessorTest.java
>  34fd5a994c5a69d64398de3cb867892a30827e2c 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshCommandsSecurityTest.java
>  def17920b198e88f358dfcf4025f4a3eee6fd0df 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/GfshShellConnectionRule.java
>  4d1bae904c4964ca2be713fd8450f2f5f6db2552 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/JMXConnectionConfiguration.java
>  4f57baa85e3b5cd084ffd33c61f7741d20b0fdc9 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/JavaRmiServerNameTest.java
>  c544e6fe0f8d3c1975775979d1354c4adc7cf87a 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/JsonAuthorizationCacheStartRule.java
>  136319c06d9bd1e46b452219bf2894cf969fc1f1 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/LockServiceMBeanAuthorizationJUnitTest.java
>  1377fb643d035ad911ee5ceb80b87dfd72855428 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MBeanSecurityJUnitTest.java
>  4beff0bd55d69601266a0a856c8dcb80ffd4e1f7 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MBeanServerConnectionRule.java
>  92430327e999af627e88956fc24aa1592e9a000d 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/ManagerMBeanAuthorizationJUnitTest.java
>  873b6491f6703378d87383d96dc35e299b19a3fa 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java
>  e5cbd15da9274d2a86be399f45c9239b2ae373be 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/ResourcePermissionTest.java
>  d6491ffde316401b215b712d33d60ced8a2b65cf 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/security/ShiroCacheStartRule.java
>  848c05c77e6b9082075b07c5c91e21eeadde1bc0 
>   
> geode-core/src/test/java/org/apache/geode/security/AbstractSecureServerDUnitTest.java
>  d2e44408317b0f815de1322a82c0e87f4892107f 
>   
> geode-core/src/test/java/org/apache/geode/security/ClusterConfigWithoutSecurityDUnitTest.java
>  3854bb1021eb5e206d465aa315cc151cce829d7b 
>   
> geode-core/src/test/java/org/apache/geode/security/IntegratedClientAuthDUnitTest.java
>  2aa633ca373d04ac2cd95cdfe2c4dd0ec3f3e6f5 
>   
> geode-core/src/test/java/org/apache/geode/security/NoShowValue1PostProcessorDUnitTest.java
>  d2a98875d96ff55f6b6eec11c8695041ce8cd309 
>   
> geode-core/src/test/java/org/apache/geode/security/PDXPostProcessorDUnitTest.java
>  cf0df1b66bfa25bec3d110fc398108a6fbca8d4e 
>   
> geode-core/src/test/java/org/apache/geode/security/PostProcessorDUnitTest.java
>  a7cdb0fbfed65a5af1249f15f698efb0454f064a 
>   
> geode-core/src/test/java/org/apache/geode/security/SecurityClusterConfigDUnitTest.java
>  54c02f787f923ff7305bdc2ede81044fac446944 
>   
> geode-core/src/test/java/org/apache/geode/security/SecurityWithoutClusterConfigDUnitTest.java
>  d5f868634e7b4f93a9a5d6da44ef0e2c0c2729ae 
>   
> geode-core/src/test/java/org/apache/geode/security/StartServerAuthorizationTest.java
>  953cdb746beb220b148555952c9d27a2389e3a01 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerConfigurationRule.java
>  7f52ce1d54dd71e9a1c53323d3388e3ebca82ecd 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarter.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarter.java 
> PRE-CREATION 
>   geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java 
> 2386af1ec47e8f75350304d597128ef8aa63a3b7 
>   
> geode-cq/src/test/java/org/apache/geode/security/CQPDXPostProcessorDUnitTest.java
>  12f08ec49c42bec6aaf90b676d09d53b0eee8fd9 
>   
> geode-cq/src/test/java/org/apache/geode/security/CQPostProcessorDunitTest.java
>  e2b555a416d1a1896898959f9207477237f2af36 
>   
> geode-lucene/src/test/java/org/apache/geode/cache/lucene/internal/configuration/LuceneClusterConfigurationDUnitTest.java
>  30532b944cf6d171a26cd7800acfd30e1d151f67 
>   
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/CommonCrudController.java
>  35d8fb4629a3b706cb30c5b90e1a1d59ca0aa556 
>   
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/controllers/PdxBasedCrudController.java
>  3003b3d673c0ee5f075b93e204a65660ff3e568e 
>   
> geode-web-api/src/main/java/org/apache/geode/rest/internal/web/security/RestSecurityService.java
>  3d09f096a500e2533bb3e7e4ba330bffac209c35 
> 
> Diff: https://reviews.apache.org/r/52889/diff/
> 
> 
> Testing
> -------
> 
> precheckin successful
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Reply via email to