Even if we weren't talking about IoC, why would you want to impose a concrete class on API end-users? There's nothing in the implementation that requires an EnumSet be used (just a Set - or even a Collection actually), so I recommend having the getter/setter use a Set instead. The internal attribute could be initialized via an EnumSet however, i.e.
private Set<RoleSource> roleSources = EnumSet.of(RoleSource.ROLES_FROM_CROWD_ROLES); Cheers, Les On Sat, Oct 16, 2010 at 1:02 PM, Alan D. Cabrera <[email protected]> wrote: > Normally I would agree but I was thinking that since this is an enum set it > might be better to return an EnumSet<>. IoC environments, e.g. Spring, > should still be able to inject since it has the same interface Set. > > Not married to the idea and happy to change it to Set<>. Please advise. > > > Regards, > Alan > > > On Oct 16, 2010, at 11:07 AM, Les Hazlewood wrote: > >> Cool stuff! >> >> One request though: that the public getter/setter for 'roleSources' >> does not expose or accept a concrete collection class and instead just >> assumes a java.util.Set - this way it can be more-easily configured >> in IoC environments. >> >> My .02, >> >> Les >> >> On Sat, Oct 16, 2010 at 10:24 AM, <[email protected]> wrote: >>> Author: adc >>> Date: Sat Oct 16 17:24:16 2010 >>> New Revision: 1023335 >>> >>> URL: http://svn.apache.org/viewvc?rev=1023335&view=rev >>> Log: >>> SHIRO-18 better constructors >>> >>> Modified: >>> >>> shiro/sandbox/crowd/src/main/java/org/apache/shiro/realm/crowd/CrowdRealm.java >>> >>> shiro/sandbox/crowd/src/test/java/org/apache/shiro/realm/crowd/CrowdRealmTest.java >>> >>> Modified: >>> shiro/sandbox/crowd/src/main/java/org/apache/shiro/realm/crowd/CrowdRealm.java >>> URL: >>> http://svn.apache.org/viewvc/shiro/sandbox/crowd/src/main/java/org/apache/shiro/realm/crowd/CrowdRealm.java?rev=1023335&r1=1023334&r2=1023335&view=diff >>> ============================================================================== >>> --- >>> shiro/sandbox/crowd/src/main/java/org/apache/shiro/realm/crowd/CrowdRealm.java >>> (original) >>> +++ >>> shiro/sandbox/crowd/src/main/java/org/apache/shiro/realm/crowd/CrowdRealm.java >>> Sat Oct 16 17:24:16 2010 >>> @@ -59,17 +59,35 @@ import org.apache.shiro.subject.Principa >>> public class CrowdRealm extends AuthorizingRealm { >>> >>> private static final Logger LOG = >>> LoggerFactory.getLogger(CrowdRealm.class); >>> - private SecurityServerClient crowdClient = >>> SecurityServerClientFactory.getSecurityServerClient(); >>> + private final SecurityServerClient crowdClient; >>> private EnumSet<RoleSource> roleSources = >>> EnumSet.of(RoleSource.ROLES_FROM_CROWD_ROLES); >>> >>> /** >>> - * Override the default mechanism of obtaining a Crowd client from a >>> - * <code>SecurityServerClientFactory</code>. >>> + * Initialize the Shiro Crowd realm with an instance of >>> + * {...@link SecurityServerClient} generated by the factory >>> + * {...@link SecurityServerClientFactory}. The method >>> + * {...@link SecurityServerClient#authenticate} is called by this >>> + * constructor. >>> * >>> - * @param crowdClient the crowd client to be used by the realm. >>> - * @see SecurityServerClientFactory >>> + * @throws InvalidAuthorizationTokenException >>> + * if the credentials in the >>> <code>crowd.properties</code> are not correct >>> + * @throws RemoteException if the client cannot reach the Crowd server >>> */ >>> - public void setCrowdClient(SecurityServerClient crowdClient) { >>> + public CrowdRealm() throws InvalidAuthorizationTokenException, >>> RemoteException { >>> + crowdClient = >>> SecurityServerClientFactory.getSecurityServerClient(); >>> + crowdClient.authenticate(); >>> + } >>> + >>> + /** >>> + * Initialize the Shiro Crowd realm with an instance of >>> + * {...@link SecurityServerClient}. The method {...@link >>> SecurityServerClient#authenticate} >>> + * is assumed to be called by the creator of this realm. >>> + * >>> + * @param crowdClient an instance of {...@link SecurityServerClient} >>> to be used when communicating with the Crowd server >>> + */ >>> + public CrowdRealm(SecurityServerClient crowdClient) { >>> + if (crowdClient == null) throw new IllegalArgumentException("Crowd >>> client cannot be null"); >>> + >>> this.crowdClient = crowdClient; >>> } >>> >>> >>> Modified: >>> shiro/sandbox/crowd/src/test/java/org/apache/shiro/realm/crowd/CrowdRealmTest.java >>> URL: >>> http://svn.apache.org/viewvc/shiro/sandbox/crowd/src/test/java/org/apache/shiro/realm/crowd/CrowdRealmTest.java?rev=1023335&r1=1023334&r2=1023335&view=diff >>> ============================================================================== >>> --- >>> shiro/sandbox/crowd/src/test/java/org/apache/shiro/realm/crowd/CrowdRealmTest.java >>> (original) >>> +++ >>> shiro/sandbox/crowd/src/test/java/org/apache/shiro/realm/crowd/CrowdRealmTest.java >>> Sat Oct 16 17:24:16 2010 >>> @@ -22,7 +22,6 @@ import java.util.Arrays; >>> import java.util.EnumSet; >>> >>> import >>> com.atlassian.crowd.integration.service.soap.client.SecurityServerClient; >>> -import >>> com.atlassian.crowd.integration.service.soap.client.SecurityServerClientFactory; >>> import org.junit.Test; >>> import static org.easymock.EasyMock.*; >>> import static org.junit.Assert.*; >>> @@ -41,14 +40,12 @@ public class CrowdRealmTest { >>> @Test >>> public void testAuthentication() throws Exception { >>> >>> - CrowdRealm realm = new CrowdRealm(); >>> - realm.setName("NutHouse"); >>> - >>> SecurityServerClient client = >>> createStrictMock(SecurityServerClient.class); >>> expect(client.authenticatePrincipalSimple("yoko", >>> "barbie")).andReturn("UNUSED"); >>> replay(client); >>> >>> - realm.setCrowdClient(client); >>> + CrowdRealm realm = new CrowdRealm(client); >>> + realm.setName("NutHouse"); >>> >>> AuthenticationInfo authenticationInfo = >>> realm.doGetAuthenticationInfo(new UsernamePasswordToken("yoko", "barbie")); >>> >>> @@ -69,15 +66,13 @@ public class CrowdRealmTest { >>> @Test >>> public void testDefaultRoles() throws Exception { >>> >>> - CrowdRealm realm = new CrowdRealm(); >>> - realm.setName("NutHouse"); >>> - >>> SecurityServerClient client = >>> createStrictMock(SecurityServerClient.class); >>> expect(client.authenticatePrincipalSimple("yoko", >>> "barbie")).andReturn("UNUSED"); >>> expect(client.findRoleMemberships("yoko")).andReturn(new >>> String[]{"big_sister", "table_setter", "dog_walker"}); >>> replay(client); >>> >>> - realm.setCrowdClient(client); >>> + CrowdRealm realm = new CrowdRealm(client); >>> + realm.setName("NutHouse"); >>> >>> AuthenticationInfo authenticationInfo = >>> realm.doGetAuthenticationInfo(new UsernamePasswordToken("yoko", "barbie")); >>> AuthorizationInfo authorizationInfo = >>> realm.doGetAuthorizationInfo(authenticationInfo.getPrincipals()); >>> @@ -92,16 +87,14 @@ public class CrowdRealmTest { >>> @Test >>> public void testRoleMemberships() throws Exception { >>> >>> - CrowdRealm realm = new CrowdRealm(); >>> - realm.setName("NutHouse"); >>> - >>> realm.setRoleSources(EnumSet.of(RoleSource.ROLES_FROM_CROWD_ROLES)); >>> - >>> SecurityServerClient client = >>> createStrictMock(SecurityServerClient.class); >>> expect(client.authenticatePrincipalSimple("yoko", >>> "barbie")).andReturn("UNUSED"); >>> expect(client.findRoleMemberships("yoko")).andReturn(new >>> String[]{"big_sister", "table_setter", "dog_walker"}); >>> replay(client); >>> >>> - realm.setCrowdClient(client); >>> + CrowdRealm realm = new CrowdRealm(client); >>> + realm.setName("NutHouse"); >>> + >>> realm.setRoleSources(EnumSet.of(RoleSource.ROLES_FROM_CROWD_ROLES)); >>> >>> AuthenticationInfo authenticationInfo = >>> realm.doGetAuthenticationInfo(new UsernamePasswordToken("yoko", "barbie")); >>> AuthorizationInfo authorizationInfo = >>> realm.doGetAuthorizationInfo(authenticationInfo.getPrincipals()); >>> @@ -117,16 +110,14 @@ public class CrowdRealmTest { >>> @Test >>> public void testGroupMemberships() throws Exception { >>> >>> - CrowdRealm realm = new CrowdRealm(); >>> - realm.setName("NutHouse"); >>> - >>> realm.setRoleSources(EnumSet.of(RoleSource.ROLES_FROM_CROWD_GROUPS)); >>> - >>> SecurityServerClient client = >>> createStrictMock(SecurityServerClient.class); >>> expect(client.authenticatePrincipalSimple("yoko", >>> "barbie")).andReturn("UNUSED"); >>> expect(client.findGroupMemberships("yoko")).andReturn(new >>> String[]{"girls", "naughty"}); >>> replay(client); >>> >>> - realm.setCrowdClient(client); >>> + CrowdRealm realm = new CrowdRealm(client); >>> + realm.setName("NutHouse"); >>> + >>> realm.setRoleSources(EnumSet.of(RoleSource.ROLES_FROM_CROWD_GROUPS)); >>> >>> AuthenticationInfo authenticationInfo = >>> realm.doGetAuthenticationInfo(new UsernamePasswordToken("yoko", "barbie")); >>> AuthorizationInfo authorizationInfo = >>> realm.doGetAuthorizationInfo(authenticationInfo.getPrincipals()); >>> @@ -140,17 +131,15 @@ public class CrowdRealmTest { >>> @Test >>> public void testAll() throws Exception { >>> >>> - CrowdRealm realm = new CrowdRealm(); >>> - realm.setName("NutHouse"); >>> - >>> realm.setRoleSources(EnumSet.of(RoleSource.ROLES_FROM_CROWD_GROUPS, >>> RoleSource.ROLES_FROM_CROWD_ROLES)); >>> - >>> SecurityServerClient client = >>> createStrictMock(SecurityServerClient.class); >>> expect(client.authenticatePrincipalSimple("yoko", >>> "barbie")).andReturn("UNUSED"); >>> expect(client.findRoleMemberships("yoko")).andReturn(new >>> String[]{"big_sister", "table_setter", "dog_walker"}); >>> expect(client.findGroupMemberships("yoko")).andReturn(new >>> String[]{"girls", "naughty"}); >>> replay(client); >>> >>> - realm.setCrowdClient(client); >>> + CrowdRealm realm = new CrowdRealm(client); >>> + realm.setName("NutHouse"); >>> + >>> realm.setRoleSources(EnumSet.of(RoleSource.ROLES_FROM_CROWD_GROUPS, >>> RoleSource.ROLES_FROM_CROWD_ROLES)); >>> >>> AuthenticationInfo authenticationInfo = >>> realm.doGetAuthenticationInfo(new UsernamePasswordToken("yoko", "barbie")); >>> AuthorizationInfo authorizationInfo = >>> realm.doGetAuthorizationInfo(authenticationInfo.getPrincipals()); >>> @@ -164,16 +153,13 @@ public class CrowdRealmTest { >>> assertTrue(authorizationInfo.getRoles().contains("naughty")); >>> } >>> >>> + �...@test >>> public void testIntegration() throws Exception { >>> + >>> CrowdRealm realm = new CrowdRealm(); >>> realm.setName("NutHouse"); >>> realm.setRoleSources(EnumSet.of(RoleSource.ROLES_FROM_CROWD_GROUPS, >>> RoleSource.ROLES_FROM_CROWD_ROLES)); >>> >>> - SecurityServerClient client = >>> SecurityServerClientFactory.getSecurityServerClient(); >>> - client.authenticate(); >>> - >>> - realm.setCrowdClient(client); >>> - >>> AuthenticationInfo authenticationInfo = >>> realm.doGetAuthenticationInfo(new UsernamePasswordToken("yoko", "barbie")); >>> >>> assertNotNull(authenticationInfo); >>> >>> > >
