Allow UserInfoInterceptor to use a principal to locate UserInfo

[morungos]

This is probably more an observation of an architectural issue than anything, but it's worth noting and discussing. I have a slightly unusual use-case, of multiple Active Directory LDAP providers as a back end, and it's proving extremely hard to integrate into a provider server, much harder than it ought to. This is partly because Spring Security uses an entirely different LDAP service that doesn't allow contexts to be exposed, but the core problem is that even though I get rich object descriptions back from LDAP, there's no way to access them and generate UserInfo objects.

The core issue is in UserInfoInterceptor, which only locates a UserInfo object by username, not by a returned Principal. There's an implicit acknowledgement of the issue in a special case to handle a second OIDC provider, but everything else is assumed to be a username-based lookup. Since Spring Security trivially provides a decent InetOrgPerson, effectively all that information is lost at the interceptor, which is then supposed to use a service to make an entire new context connection to search and read it all back.

I'd suggest that UserInfoService should try with a principal first, and its default behaviour ought to be the username fallback. The heavy use of instanceof in UserInfoInterceptor indicates the issue. As it is, it seems like I probably need to override the whole of UserInfoInterceptor, which feels invasive.

[praseodym]

UserInfoRepository has a getByUsername method, and its argument is the name of the Spring Security authentication token. It can be controlled by your overlay, for example by providing your own UserDetails object. Then you could e.g. prefix name per provider, and in the UserInfoRepository do a lookup based on this prefix.

I done a similar thing with our own UserInfoRepository that might get you started.

[morungos]

Wow, @praseodym, that''s a huge start, but I'm nervous. Spring gives me a InetOrgPerson that is a good UserDetails object already, and it has the name, and everything else I want to store in the UserInfoRepository. So looking at your UserInfoRepository it gets the Principal from the SecurityContextHolder and checks it matches the passed username. If it does, it does the mapping.

Does that really work? The logic is there in case it is different, but I have no fallback if they do differ, as I can't use a service to retrieve user information. I can only get that in the bind stage. So you use a repository to load missing users but I have no means to do that.

It just feels weird to actually have a principal in the interceptor and then reduce it to a username, and then attempt to use that username to locate the principal again. Sure, I could build a cache of all principals I get back from Active Directory and search in that, but it's a dirty solution to a work around the enforced information-loss in the interceptor.

[praseodym]

I'm not quite sure if the method is ever called without the principal being available, but for our code I assumed it could be. Because the Spring Security authentication object is persisted in the database (SavedUserAuthentication), and UserDetails is a part of that object, it might never happen though.

If you don't need to bind to LDAP with the username/password of the original user, you could also query LDAP in your repository (we do that as well).

Please note that the repository is used through UserInfoService in quite a few places, so don't focus on just the interceptor.

[morungos]

Yes, I'd query LDAP if I could, but Active Directory's main difference is that it requires all binding to be done with username/password of the original user. I have to totally rely on the Spring Security authentication object.

But if I can assume the first request is made in the interceptor thread and therefore has my original principal, I can probably use a trick like yours (although I'm tempted to put it in the repository, like the model LDAP implementation) and then rely on caching and hope for the best.

I suppose my question could be simply this: has anyone managed to adapt the provider to work with Active Directory? If not, I'm the first :-)

[praseodym]

Spring Security will erase the user's credentials when authentication is complete, so the authentication chain is the only chance. I'd recommend replacing/enriching the UserDetails object somewhere during authentication.

Note that you could also create a service account to bind to AD, so you won't need the user's credentials at all.

[httpants]

Your statement that Active Directory requires all binding to be done with username/password of the original user is not correct. We're using Active Directory as well and I also tried to get this all to work using the ActiveDirectoryLdapAuthenticationProvider but found the same issues you're having. The solution is to use the LdapAuthenticationProvider instead and use a service account to connect to Active Directory.

An extract of my user-context.xml shows the configuration...

<!-- LDAP -->
   <bean id="contextSource" class="org.springframework.ldap.core.support.LdapContextSource">
      <property name="url" value="${ldap.url}" />
      <property name="base" value="${ldap.base}" />
      <property name="userDn" value="${ldap.userDn}" />
      <property name="password" value="${ldap.password}" />
   </bean>

   <bean id="ldapUserSearch" class="org.springframework.security.ldap.search.FilterBasedLdapUserSearch">
      <constructor-arg value="" />
      <constructor-arg value="sAMAccountName={0}" />
      <constructor-arg ref="contextSource" />
      <property name="searchSubtree" value="true" />
   </bean>

   <bean id="ldapAuthProvider" class="org.springframework.security.ldap.authentication.LdapAuthenticationProvider">
      <constructor-arg>
         <bean class="org.springframework.security.ldap.authentication.BindAuthenticator">
            <constructor-arg ref="contextSource" />
            <property name="userSearch" ref="ldapUserSearch" />
         </bean>
      </constructor-arg>
      <constructor-arg ref="ldapAuthoritiesPopulator" />
   </bean>

   <bean id="ldapAuthoritiesPopulator" class="com.acme.ActiveDirectoryAuthoritiesPopulator">
      <property name="admins">
         <set>
            <value>xyz</value>
         </set>
      </property>
      <property name="memberOfRolesRegExp" value="${ldap.memberOfRolesRegExp}"/>
   </bean>

   <bean id="ldapUserInfoRepository" primary="true" class="com.acme.ActiveDirectoryUserInfoRepository">
      <property name="userSearch" ref="ldapUserSearch" />
      <property name="authoritiesPopulator" ref="ldapAuthoritiesPopulator" />
   </bean>

   <!-- end LDAP -->

ActiveDirectoryAuthoritiesPopulator

public class ActiveDirectoryAuthoritiesPopulator implements LdapAuthoritiesPopulator {

    private Set<String> admins = Collections.emptySet();
    private String memberOfRolesRegExp = "OU=Security Groups";
    private Pattern memberOfRolesPattern = Pattern.compile(memberOfRolesRegExp);

    private static final GrantedAuthority ROLE_USER = new SimpleGrantedAuthority("ROLE_USER");
    private static final GrantedAuthority ROLE_ADMIN = new SimpleGrantedAuthority("ROLE_ADMIN");

    public Set<String> getAdmins() {
        return admins;
    }

    public void setAdmins(Set<String> admins) {
        this.admins = admins;
    }

    public String getMemberOfRolesRegExp() {
        return memberOfRolesRegExp;
    }

    public void setMemberOfRolesRegExp(String memberOfRolesRegExp) {
        this.memberOfRolesRegExp = memberOfRolesRegExp;
        memberOfRolesPattern = Pattern.compile(memberOfRolesRegExp);
    }

    @Override
    public Collection<? extends GrantedAuthority> getGrantedAuthorities(DirContextOperations userData, String username) {

        List<GrantedAuthority> authorities = new ArrayList<GrantedAuthority>();
        authorities.add(ROLE_USER);
        if (admins.contains(username)) {
            authorities.add(ROLE_ADMIN);
        }

        try {
            Attributes attributes = userData.getAttributes();
            Attribute memberOf = attributes.get("memberOf");
            NamingEnumeration<?> roleEnum = memberOf.getAll();
            if (roleEnum != null) {
                Pattern cnPattern = Pattern.compile("CN=([^,]*)");
                while (roleEnum.hasMore()) {
                    String role = (String)roleEnum.next();
                    Matcher m = memberOfRolesPattern.matcher(role);
                    if (m.find()) {
                        // This group represents a ROLE
                        Matcher cnMatcher = cnPattern.matcher(role);
                        if (cnMatcher.find()) {
                            String roleName = "ROLE_" + cnMatcher.group(1).toUpperCase();
                            authorities.add(new SimpleGrantedAuthority(roleName));
                        }
                    }
                }
            }
        } catch (NamingException e) {
            e.printStackTrace();
        } finally {

        }

        return authorities;
    }

}

ActiveDirectoryUserInfoRepository

public class ActiveDirectoryUserInfoRepository implements UserInfoRepository, UserDetailsService {

    private LdapUserSearch userSearch;
    private MappedLdapAuthoritiesPopulator authoritiesPopulator;
    private LdapTemplate ldapTemplate;

    public LdapUserSearch getUserSearch() {
        return userSearch;
    }

    public void setUserSearch(LdapUserSearch ldapUserSearch) {
        this.userSearch = ldapUserSearch;
    }

    public LdapTemplate getLdapTemplate() {
        return ldapTemplate;
    }

    public void setLdapTemplate(LdapTemplate ldapTemplate) {
        this.ldapTemplate = ldapTemplate;
    }

    public MappedLdapAuthoritiesPopulator getAuthoritiesPopulator() {
        return authoritiesPopulator;
    }

    public void setAuthoritiesPopulator(
            MappedLdapAuthoritiesPopulator authoritiesPopulator) {
        this.authoritiesPopulator = authoritiesPopulator;
    }

    public UserInfo mapFromAttributes(DirContextOperations dirCtx, String username) throws NamingException {
        Attributes attr = dirCtx.getAttributes();

        if (attr.get("sAMAccountName") == null) {
            return null; // we can't go on if there's no UID to look up
        }

        UserInfoWithRoles ui = new UserInfoWithRoles();

        // save the sAMAccountName as the preferred username
        ui.setPreferredUsername(attr.get("sAMAccountName").get().toString());

        // for now we use the sAMAccountName as the subject as well (this should probably be different)
        ui.setSub(attr.get("sAMAccountName").get().toString());

        // add in the optional fields

        // email address
        if (attr.get("mail") != null) {
            ui.setEmail(attr.get("mail").get().toString());
            // if this domain also provisions email addresses, this should be set to true
            ui.setEmailVerified(false);
        }

        // phone number
        if (attr.get("telephoneNumber") != null) {
            ui.setPhoneNumber(attr.get("telephoneNumber").get().toString());
            // if this domain also provisions phone numbers, this should be set to true
            ui.setPhoneNumberVerified(false);
        }

        // name structure
        if (attr.get("displayName") != null) {
            ui.setName(attr.get("displayName").get().toString());
        }

        if (attr.get("givenName") != null) {
            ui.setGivenName(attr.get("givenName").get().toString());
        }

        if (attr.get("sn") != null) {
            ui.setFamilyName(attr.get("sn").get().toString());
        }

        if (attr.get("initials") != null) {
            ui.setMiddleName(attr.get("initials").get().toString());
        }

        Collection<? extends GrantedAuthority> authorities = authoritiesPopulator.getGrantedAuthorities(dirCtx, username);
        for (GrantedAuthority authority: authorities) {
            String roleName = authority.getAuthority();
            if (roleName.startsWith("ROLE_")) {
                roleName = roleName.substring(5);
            }
            ui.getRoles().add(roleName);
        }

        return ui;

    }

    // lookup result cache, key from username to userinfo
    private LoadingCache<String, UserInfo> cache;

    private CacheLoader<String, UserInfo> cacheLoader = new CacheLoader<String, UserInfo>() {
        @Override
        public UserInfo load(String username) throws Exception {
            DirContextOperations dirCtx = userSearch.searchForUser(username);
            return (UserInfo)mapFromAttributes(dirCtx, username);
        }

    };

    public ActiveDirectoryUserInfoRepository() {
        this.cache = CacheBuilder.newBuilder()
                    .maximumSize(100)
                    .expireAfterAccess(14, TimeUnit.DAYS)
                    .build(cacheLoader);
    }

    @Override
    public UserInfo getBySubject(String sub) {
        // TODO: right now the subject is the username, should probably change

        return getByUsername(sub);
    }

    @Override
    public UserInfo save(UserInfo userInfo) {
        // read-only repository, unimplemented
        return userInfo;
    }

    @Override
    public void remove(UserInfo userInfo) {
        // read-only repository, unimplemented

    }

    @Override
    public Collection<? extends UserInfo> getAll() {
        // return a copy of the currently cached users
        return cache.asMap().values();
    }

    @Override
    public UserInfo getByUsername(String username) {
        try {
            return cache.get(username);
        } catch (UncheckedExecutionException ue) {
            return null;
        } catch (ExecutionException e) {
            e.printStackTrace();
            return null;
        }
    }

    @Override
    public UserDetails loadUserByUsername(String username)
            throws UsernameNotFoundException {
        DirContextOperations dirCtx = userSearch.searchForUser(username);
        if (dirCtx == null) {
            throw new UsernameNotFoundException("Could not find user");
        }
        Collection<? extends GrantedAuthority> authorities = authoritiesPopulator.getGrantedAuthorities(dirCtx, username);
        return new User(username, "", authorities);
    }

}

[morungos]

@httpants Okay, I did overstate the case, and I know I could try to get manager accounts but, and I quote from the Spring Security docs:

Active Directory supports its own non-standard authentication options, and the normal usage pattern doesn't fit too cleanly with the standard LdapAuthenticationProvider. Typically authentication is performed using the domain username (in the form user@domain), rather than using an LDAP distinguished name. To make this easier, Spring Security 3.1 has an authentication provider which is customized for a typical Active Directory setup.

Note the "normal usage pattern". In practice, I work in an organization with four different LDAP providers, with no centralization and not much cooperation. Getting one account might be possible, but getting four different manager accounts for four different systems, in all cases deviating from the normal usage pattern of LDAP, would be tough. Plus, it's in a healthcare organization so I'd probably need four separate privacy assessments for these manager accounts. I really do not want to go there.

Please, not everyone has admin rights on these things. I can't even get permission to use virtual machines on my own darned laptop, let alone having root access to anything.

None of this is necessary if I use AD the way it's designed to be used. When a user signs on, the information about that user is nicely returned.

I maintain: if I need to set up manager accounts to use LDAP authentication to back the OIDC provider, then there is an architectural problem as the design is locked to a post-hoc lookup of user details from a username alone. That's basically my point.

I've almost completed an apparently working solution on the pattern of @praseodym's thoughts above: i.e., assuming that the first access will be in the thread with the principal properly set and containing all details, and using that to populate the repository for subsequent accesses. When it's done I'll document it here, but the provider is doing slightly different key things to my previous tests, so there are client-side issues I need to resolve to test fully.

[jricher]

I agree that the right layer to fix this is in the UserInfoRepository or UserInfoService. We've had one deployment based on user certificates that takes the information from the certificate on the first login and populates a database cache, which is then read by the repository. Apart from not being able to do user discovery before they log in the first time and being limited to the information that's included in the cert, this works quite well.

[httpants]

I wouldn't be comfortable making the assumption that the fist call to retrieve userinfo would be made in the SecurityContext of the actual user. I'm just thinking how that would work in a load balanced setup, or even if you reboot a server and someone has an access token which they use to call the introspection endpoint.

Also the spring documentation is a bit misleading in my opinion. The usage pattern for authenticating against ActiveDirectory doesn't have to be any different to any other standard ldap. In my experience of using ActiveDirectory over the years, users logon using their user name (without a domain suffix), having a service account to perform user lookup is a very standard approach. The spring ActiveDirectory authenticator is handy for a quick solution, but as you've seen, has it's own limitations when trying to get it to work with openid, especially around the lack of ability to perform a user search.

There's also other reasons you may need to do a user search, like if you want to use "remember me". If you want that (which we do), then you must have the ability to lookup a user in active directory, which means you need a service account.

I know it can be difficult to sometimes get service accounts setup. IT departments are too often setup to prevent IT delivery.

[morungos]

One of our several IT departments has told us they won't provide us with a service account, as they have demonstrated it isn't necessary through some model PHP code (!!). Since we'd need one for every department, that's a blocker.

The AD lookup is different. Basically, AD allows lookups of the form "x@domain" which effectively behave as if they are a unitary sequence of anonymous bind, search for user DN, re-bind. This is what our IT departments have told us they expect us to use. I don't think I'll be able to convince them to allow it because I want to use this OpenID Connect provider.

The first call to retrieve userinfo has to be made in the right context or you'd never be able to get a username. All I'm doing is getting more than a username at the same time, and then sticking that in the repository. Yes, I'm at risk of the interceptor doing something sneaky (like a thread switch) before it calls the service, but as I said initially: I think the interceptor is making the wrong call here, and should pass the full Principal to the service.

But if this really is the policy, maybe I need to look at other providers, as maybe Mitre is trying to do too much to accommodate this use case.

Oh, by the way: the code I've put together is at: https://github.com/morungos/openid-connect-server, although still very much a work in progress (especially with the repository replacement).

[jricher]

By doing the injection into the repository at authentication time, all you lose is the ability to do per-user discovery, which isn't necessary in a closed IT environment. Every other call to UserInfo service is done after the user has logged in at least once. We've used this same technique with other systems where we can't use a backend user lookup service (in particular, there's one that's certificate based -- we suck the values out of the cert on the way in and store them, then use the cert's DN as the "username" to look up the data in a custom user service).

You can always inject the current principal object into a custom service directly, but that's going to only work for the cases where the user is currently present. A very important place that the userinfo service is called, though, is the userinfo endpoint, and the principal in this case isn't going to be representing the user, since it's the client making the call via OAuth. This doesn't use the interceptor above, of course, but calls the service directly.

Alternatively, if you can implement your own service or repository, you can map the "username" string to whatever field you need to do the lookup in your system. We've had deployments do this as well, where the incoming identifier was not the username but instead a unique ID that we had to map to a custom field on the back end.

All said, we've found the current setup to be very flexible, but if it doesn't work for you I'd recommend looking at another project like OpenAM from ForgeRock.