> On June 13, 2017, 7 p.m., Kirk Lund wrote: > > -1 I don't see why these changes are needed other than to wipe out what I > > did? For example, you've undone my introduction of RealmInitializer and > > ConfigInitializer. I introduced those so I can continue writing more > > mocking tests for SecurityService so that our unit tests don't have to be > > integration tests that actually interact with Shiro. We should be able to > > separate our code. Also, I really prefer using a "DisabledSecurityService" > > when security is disabled rather than using a "LegacySecurityService" -- > > when security is disabled, it isn't "legacy".
what about the consolidation between the EnabledSecurityService and CustomSecurityService? There are lots of duplicated code there. Also regarding DisabledSecurityService, as far the product is concerned, there is no DisabledSecurityService though, only test would use them, so I think it would be a good idea to lump them together. It does get rid of a log of duplicate code though. I do agree that the RealmInitializer would help for mocking. I will bring it back. - Jinmei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/60028/#review177781 ----------------------------------------------------------- On June 13, 2017, 4:49 p.m., Jinmei Liao wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/60028/ > ----------------------------------------------------------- > > (Updated June 13, 2017, 4:49 p.m.) > > > Review request for geode, Emily Yeh, Jared Stewart, Ken Howe, Kirk Lund, and > Patrick Rhomberg. > > > Repository: geode > > > Description > ------- > > * combine EnabledSecurityService and CustomSecurityService into > IntegratedSecurityService > * combine DisabledSecurityService and LegacySecurityService > * provide default impelementations of SecurityService > * consolidate SecurityService creation. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/distributed/internal/InternalDistributedSystem.java > 22edb6f06c7791929cc9a033ca1a1bfed5751a47 > > geode-core/src/main/java/org/apache/geode/internal/security/CallbackInstantiator.java > 3ff632d3857189513243959ee96da89da66d5a27 > > geode-core/src/main/java/org/apache/geode/internal/security/CustomSecurityService.java > 0ba1cb62468f15fda60a94dac9c781fe1793b28f > > geode-core/src/main/java/org/apache/geode/internal/security/DisabledSecurityService.java > b50569036db1acac927f08ee5c1771c72ede3c1d > > geode-core/src/main/java/org/apache/geode/internal/security/EnabledSecurityService.java > f0568c0ebc2b17c47b5059c60aa4f8bd500c5d6c > > geode-core/src/main/java/org/apache/geode/internal/security/LegacySecurityService.java > 44562537bc426e47a42d680bb410dc59bf9bd854 > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityService.java > a4041e198f1a9a4961915504c51256d0b03aa7c2 > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityServiceFactory.java > 02f34f15617f7bf4ad9ee7fa51f32be4db3c198a > > geode-core/src/main/java/org/apache/geode/internal/security/SecurityServiceType.java > 8ae76d22b628b3175db45116b80dfcfbe34aba1d > > geode-core/src/main/java/org/apache/geode/internal/security/shiro/ConfigInitializer.java > 60f014b9c33732a4ea134a654e666a9439b210bb > > geode-core/src/main/java/org/apache/geode/internal/security/shiro/CustomAuthRealm.java > 51449fdd5570494f3cf91325985a5eb9fc9f6d57 > > geode-core/src/main/java/org/apache/geode/internal/security/shiro/RealmInitializer.java > 978c4dd0ab92afde53972f7feb9d8f018d0bf662 > > geode-core/src/test/java/org/apache/geode/distributed/internal/membership/MembershipJUnitTest.java > a0c3cf3074051990cc50755131f8024db0b1faad > > geode-core/src/test/java/org/apache/geode/internal/security/DisabledSecurityServiceTest.java > cacbeed957c3b87d08c93db74e38e0134565f699 > > geode-core/src/test/java/org/apache/geode/internal/security/EnabledSecurityServiceTest.java > fca7eae9413cee98d351db5349fd950d3aa56180 > > geode-core/src/test/java/org/apache/geode/internal/security/FakePostProcessor.java > 70823443b5c4f776c86bb28ed49a73e690c5c872 > > geode-core/src/test/java/org/apache/geode/internal/security/FakeSecurityManager.java > ca4e6b7fb462bb4e3fefbc5db8a9503c6b13a865 > > geode-core/src/test/java/org/apache/geode/internal/security/IntegratedSecurityServiceConstructorTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/security/LegacySecurityServiceTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/security/SecurityServiceFactoryShiroIntegrationTest.java > 89070123978c22c4cfa8684fbb5b033dc9d83ffa > > geode-core/src/test/java/org/apache/geode/internal/security/SecurityServiceFactoryTest.java > f027a4367b38681f83dad2d4c4add67759633644 > > geode-core/src/test/java/org/apache/geode/internal/security/SecurityServiceTest.java > 44893520962331bcd41e972afa661538c28d4fb2 > > geode-core/src/test/java/org/apache/geode/internal/security/shiro/ConfigInitializerIntegrationTest.java > 857c0be8940b4acde2aa4992fac0122b687391c2 > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigWithSecurityDUnitTest.java > c551ca9104a85dcf54c0bebbc4178fce4114a416 > > geode-core/src/test/java/org/apache/geode/management/internal/security/SecurityServiceWithCustomRealmIntegrationTest.java > 01d6bb6488e76fb3cf652ad211e9f7e2fac51389 > > geode-core/src/test/java/org/apache/geode/management/internal/security/SecurityServiceWithShiroIniIntegrationTest.java > 1caedbcede239d6a96640381cc6941948637b442 > > geode-core/src/test/java/org/apache/geode/security/CacheFactoryWithSecurityObjectTest.java > cdb90f1b580edaf6a2762883d4159a45d69c4728 > > geode-core/src/test/java/org/apache/geode/security/ClusterConfigWithoutSecurityDUnitTest.java > e90bc0a69222998322e02fcfad1b6bad3c97f4d1 > > geode-core/src/test/java/org/apache/geode/security/SecurityManagerLifecycleDistributedTest.java > a9048b9219a494f61e3873ee3f2908da04bf6154 > geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/Server.java > 63f907cb007846626a9a66dc6b1ef28e0bb6db45 > > geode-pulse/src/test/java/org/apache/geode/tools/pulse/tests/rules/ServerRule.java > 767588d3717fccbb0b9c7dec7c5439e16d5381aa > > > Diff: https://reviews.apache.org/r/60028/diff/1/ > > > Testing > ------- > > precheckin successful > > > Thanks, > > Jinmei Liao > >
