Allow configuring the ActiveDirectoryLdapAuthenticationProvider in AuthenticationManagerBuilder

[petrdvorak]

Currently, we are able to set standard LDAP provider via:

@Configuration
public class WebSecurityConfig extends WebSecurityConfigurerAdapter {

    @Override
    public void configure(AuthenticationManagerBuilder auth) throws Exception {
        final LdapAuthenticationProviderConfigurer<AuthenticationManagerBuilder> ldapAuthenticationBuilder
            = auth.ldapAuthentication();
        // ... proceed with additional configuration
    }

}

However, the LdapAuthenticationProviderConfigurer is hardcoded to create LdapAuthenticationProvider in the build method (here).

There is no way to setup the configurer to build the ActiveDirectoryLdapAuthenticationProvider, which uses a different internal logic on top of the same base AbstractLdapAuthenticationProvider class.

To be able to configure Active Directory the same way we currently can configure classic LDAP, we would like to see either of these options:

Option 1: Own configurer for Active Directory

... providing the following new method:

@Configuration
public class WebSecurityConfig extends WebSecurityConfigurerAdapter {

    @Override
    public void configure(AuthenticationManagerBuilder auth) throws Exception {
        final LdapAuthenticationProviderConfigurer<AuthenticationManagerBuilder> ldapAuthenticationBuilder
            = auth.activeDirectoryAuthentication();
        // ... proceed with additional configuration
    }

}

Option 2: Picking the right class from a registered bean

... instead of creating the class, the configurer could automatically detect a bean:

    @Bean
    @ConditionalOnProperty(name = "my.props.ldap.security.method", havingValue = "active-directory")
    public ActiveDirectoryLdapAuthenticationProvider activeDirectoryLdapAuthenticationProvider(LdapConfiguration configuration) {
        final String activeDirectoryDomain = configuration.getActiveDirectoryDomain();
        final String ldapUrl = configuration.getLdapUrl();
        final String ldapRoot = configuration.getLdapRoot();
        return new ActiveDirectoryLdapAuthenticationProvider(activeDirectoryDomain, ldapUrl, ldapRoot);
    }

Option 3: Consolidation of LDAP authentication providers

... so that we do not need to handle different providers.

Having ActiveDirectoryLdapAuthenticationProvider and LdapAuthenticationProvider that do not inherit from each other seems a bit unexpected. Maybe there could be a strategy pattern used instead to configure behavior of one LdapAuthenticationProvider class?

Option 4: Allow Builder to construct the abstract class instance

... and probably many more options framework could support it?

[jzheaux]

@petrdvorak, I agree that the hierarchy could likely use some cleanup, and I feel like Option 3 would be ideal.

With the introduction of AbstractLdapAuthenticationManagerFactory, I think we should revise Option 1 and Option 2 like so:

Option 1: Introduce an authentication manager factory for Active Directory
Option 2: Change AbstractLdapAuthenticationManagerFactory to detect and use Active Directory components

[Haarolean]

Previously, we'd just pass an authentication provider (either LdapAuthenticationProvider or ActiveDirectoryLdapAuthenticationProvider) upon creating an authentication manager:

AbstractLdapAuthenticationProvider authenticationProvider;
    if (!isActiveDirectory) {
      authenticationProvider = new LdapAuthenticationProvider(bindAuthenticator);
    } else {
      authenticationProvider = new ActiveDirectoryLdapAuthenticationProvider(activeDirectoryDomain, ldapUrl);
    }

    AuthenticationManager authManager = new ProviderManager(List.of(authenticationProvider));

Now, with AbstractLdapAuthenticationManagerFactory having #createAuthenticationManager method final and #getProvider private doesn't help, to put it lightly.

---

Option 2: Change AbstractLdapAuthenticationManagerFactory to detect and use Active Directory components

To my knowledge, that's not achievable without doing some network stuff, even if we have the host URL, as AD is basically an implementation of LDAP with some overhead.

Option 1: Introduce an authentication manager factory for Active Directory

This would probably be almost 100% copy-paste of the same factory, as it seems to be enough just to pass the AD authentication provider (which is configurable itself) into the authentication manager.

My suggestion is to allow setting an instance of an authentication provider within the aforementioned factory. If that works, I could raise a PR with the change.
@jzheaux what do you think?

[jzheaux]

@Haarolean your existing approach still works:

@Bean 
AuthenticationManager authenticationManager() {
    AbstractLdapAuthenticationProvider authenticationProvider;
    if (!isActiveDirectory) {
      authenticationProvider = new LdapAuthenticationProvider(bindAuthenticator);
    } else {
      authenticationProvider = new ActiveDirectoryLdapAuthenticationProvider(activeDirectoryDomain, ldapUrl);
    }

    return new ProviderManager(List.of(authenticationProvider));
}

So I'm not sure why you are looking for something different?

My suggestion is to allow setting an instance of an authentication provider within the aforementioned factory. If that works, I could raise a PR with the change.

I don't really see the point, since someone could do:

@Bean 
AuthenticationManager authenticationManager() {
    ActiveDirectoryLdapAuthenticationProvider activeDirectory = // ... construct
    return new ProviderManager(activeDirectory);
}

[jzheaux]

@Haarolean, thanks very much for raising the PR and pointing out some of the limitations with further simplifying ActiveDirectoryLdapAuthenticationProvider.

At this point, I think we should close this issue; what do you think?

[Haarolean]

@jzheaux

I don't really see the point, since someone could do

Yes, that's possible, but that neglects the convenience of using AbstractLdapAuthenticationManagerFactory in the first place, I believe.

Here are two examples, with and without AbstractLdapAuthenticationManagerFactory:

With the factory:

  @Bean
  ReactiveAuthenticationManager ldapAuthenticationManager(BaseLdapPathContextSource contextSource, ApplicationContext context,
                                                  @Nullable AccessControlService acs) {
    var factory = new LdapBindAuthenticationManagerFactory(contextSource);

    factory.setLdapAuthoritiesPopulator(new DefaultLdapAuthoritiesPopulator(contextSource, "null"));
    factory.setUserDetailsContextMapper(new UserDetailsMapper());
    factory.setUserSearchFilter("userSearchFilter");
    factory.setUserDnPatterns("uid={0},ou=people");
    factory.setUserSearchBase("userSearchBase");
    var authenticationManager = factory.createAuthenticationManager();
    return new ReactiveAuthenticationManagerAdapter(authenticationManager);
  }

Without one:

@Bean
  public ReactiveAuthenticationManager authenticationManager(BaseLdapPathContextSource contextSource,
                                                             LdapAuthoritiesPopulator ldapAuthoritiesPopulator,
                                                             @Nullable AccessControlService acs) {
    var bindAuthenticator = new BindAuthenticator(contextSource);
      bindAuthenticator.setUserDnPatterns(new String[] {props.getBase()});
    if (props.getUserFilterSearchFilter() != null) {
      LdapUserSearch userSearch =
          new FilterBasedLdapUserSearch("searchBase", "searchFilter", contextSource);
      bindAuthenticator.setUserSearch(userSearch);
    }
    AbstractLdapAuthenticationProvider authenticationProvider;
    if (!props.isActiveDirectory()) {
      authenticationProvider = new LdapAuthenticationProvider(bindAuthenticator, ldapAuthoritiesPopulator);
    } else {
      authenticationProvider = new ActiveDirectoryLdapAuthenticationProvider(props.getActiveDirectoryDomain(), props.getUrls());
      authenticationProvider.setUseAuthenticationRequestCredentials(true);
    }
    authenticationProvider.setUserDetailsContextMapper(new UserDetailsMapper());
    AuthenticationManager am = new ProviderManager(List.of(authenticationProvider));
    return new ReactiveAuthenticationManagerAdapter(am);
  }

As you can see, without using the factory we have to write a lot more code to create things like bindAuthenticator, authenticationProvider. Let me know what you think.

[jzheaux]

I think I see what you are saying now. I think some of this is a slight mismatch between AuthenticationManager and AuthenticationProvider that will be straightened out in a future release.

What if you did this instead:

@Bean
public ReactiveAuthenticationManager authenticationManager(BaseLdapPathContextSource contextSource) 
    if (props.isActiveDirectory()) {
        var provider = new ActiveDirectoryLdapAuthenticationProvider(props.getActiveDirectoryDomain(), props.getUrls());
        provider.setUseAuthenticationRequestCredentials(true);
        return new ReactiveAuthenticationManagerAdapter(new ProviderManager(provider));
    } else {
        var factory = new LdapBindAuthenticationManagerFactory(contextSource);
        factory.setLdapAuthoritiesPopulator(new DefaultLdapAuthoritiesPopulator(contextSource, "null"));
        factory.setUserDetailsContextMapper(new UserDetailsMapper());
        factory.setUserSearchFilter("userSearchFilter");
        factory.setUserDnPatterns("uid={0},ou=people");
        factory.setUserSearchBase("userSearchBase");
        return new ReactiveAuthenticationManagerAdapter(factory.createAuthenticationManager());
    }
}

[Haarolean]

@jzheaux thank you for the clarification. Looking forward to #11428.

Thanks for the snippet, yes, looks way better with current restraints :)