Allen Gilliland wrote: > > > Elias Torres wrote: >> Dave, >> >> I've committed all of my changes including a base auto-provision feature. >> >> http://svn.apache.org/viewvc?view=rev&revision=438595 > > this didn't go through the normal proposal process which is fine in this > case, but i'd like to raise a couple minor concerns which i think are > worth addressing ...
We were going to work on the proposal [1] but I guess David wanted this done much quicker because of the RC release. I sent several emails on the issue but I didn't hear any objections so I committed the code, on the contrary, we have many interested on the new features. [1] http://rollerweblogger.org/wiki/Wiki.jsp?page=Proposal_SingleSignOnOption > > 1. I think it would be more fitting to move the contents of > AutoProvisioningHelper a little bit. I think the check for if > auto-provisioning is enabled should lie in the code segment that was > added to RollerSession and the retrieval of the AutoProvision instance > should come from RollerContext, i.e. ... > > // try one time to auto-provision, only happens if user==null > // which means installation has SSO-enabled in security.xml > if(user == null && > RollerConfig.getBooleanProperty("users.sso.autoProvision.enabled")) { > > // provisioning enabled, get provisioner and execute > AutoProvision provisioner = RollerContext.getAutoProvision(); > boolean userProvisioned = provisioner.execute(); > if(userProvisioned) { > // try lookup again real quick > user = umgr.getUserByUserName(principal.getName()); > } > } Much cleaner than what I did. I've checked it in. http://svn.apache.org/viewvc?view=rev&revision=438727 > > > 2. The way getUserDetailsFromAuthentication() method works in > CustomUserRegistry seems limiting since to support a new type of > UserDetails would require modifying that class. I'm not sure exactly > how the appropriate implementation of UserDetails is chosen and > instantiated, presumably by Acegi somehow, but I think that we need to > force the use of a common interface like the RollerUserDetails interface > by all integrating providers. i.e. maybe when we cast the principal > that we get from Acegi we need to force the use of RollerUserPrincipals? > > Object oPrincipal = authentication.getPrincipal(); > if (!(oPrincipal instanceof RollerUserDetails)) { > log.warn("Unsupported Principal type in Authentication. Skipping > auto-registration."); > return null; > } > > can we do both of those? > > -- Allen > The code you pasted is not the code I committed. The code I committed is as follows: Object oPrincipal = authentication.getPrincipal(); ... if (!(oPrincipal instanceof UserDetails)) { // ##1 log.warn("Unsupported Principal..."); return null; } UserDetails userDetails = (UserDetails) oPrincipal; ... if(userDetails instanceof RollerUserDetails) { // ##2 RollerUserDetails rollerDetails = (RollerUserDetails) userDetails; ... } else if(userDetails instanceof LdapUserDetails) { // ##3 LdapUserDetails ldapDetails = (LdapUserDetails) userDetails; ... } ##1 UserDetails is the base interface in Acegi Security to provide user information. All UserDetailService implementation must return an object of that interface. If not, we throw an unsupported warning and return null, not likely to happen. Then (in ...) I'd get the basic user information that applies to all UserDetail implementations and covers 80% of the information needed to register a new User. ##2 Here's where we support extra information. Let's say that someone wanted to store specific information like timezone, locale in a custom directory (not supported by Acegi Security) for Roller, then they would have to implement their own UserDetailService and return something of base type UserDetail and that it extends RollerUserDetails. RollerUserDetails has all of the necessary required information for UserData. ##3 It's the case for when they have an LDAP directory and they want to get email and name from LDAP but have still the option to customize the attributes. I wouldn't want to force RollerUserDetails always because then either us or our users would have to reimplement more from Acegi Security. The way it is doesn't require any code for most cases, but if you want your own custom implementation I don't think this class needs to be changed anyways. At least that's what I had in mind. -Elias -Elias
