Authentication via LDAP and authorization via JDBC

Question

I have a custom AuthenticationProvider for getting authentication via LDAP and authorization via JDBC. It's running but I think it's not the best way.

My custom AuthenticationProvider:

public class ADAuthenticationJDBCAuthorizationProvider implements AuthenticationProvider {

    private final ActiveDirectoryLdapAuthenticationProvider adDelegate;

    @Autowired
    private UtilisateurService utilisateurService;

    public ADAuthenticationJDBCAuthorizationProvider(ActiveDirectoryLdapAuthenticationProvider adDelegate) {
        this.adDelegate = adDelegate;
    }

    @Override
    public Authentication authenticate(Authentication authentication) {
        final Authentication a = adDelegate.authenticate(authentication);    
        final String name = a.getName().toLowerCase();

        // Load additional authorities and create an Authentication object
        final Collection<GrantedAuthority> authorities = utilisateurService.loadRolesFromDatabase(name);
        final boolean isActive = utilisateurService.isActive(name);

        return new AbstractAuthenticationToken(authorities) {
            @Override
            public Object getCredentials() {
                return a.getCredentials();
            }

            @Override
            public Object getPrincipal() {
                return a.getPrincipal();
            }

            @Override
            public boolean isAuthenticated() {
                return isActive && a.isAuthenticated();
            }

            @Override
            public String getName() {
                return name;
            }

        };
    }

    @Override
    public boolean supports(Class<?> authentication) {
        return adDelegate.supports(authentication);
    }
}

My configuration file:

public class GlobalSecurityConfig extends GlobalAuthenticationConfigurerAdapter {

    @Value("${ldap.domain}")
    private String DOMAIN;

    @Value("${ldap.url}")
    private String URL;

    @Autowired
    private JDBCUserDetailsService userDetailsService;

    @Override
    public void init(AuthenticationManagerBuilder auth) throws Exception {        
auth.authenticationProvider(activeDirectoryLdapAuthenticationProvider()).userDetailsService(userDetailsService);
    }

    @Bean
    public AuthenticationProvider activeDirectoryLdapAuthenticationProvider() {
        ActiveDirectoryLdapAuthenticationProvider adAuthenticationProvider
                = new ActiveDirectoryLdapAuthenticationProvider(DOMAIN, URL);
        adAuthenticationProvider.setConvertSubErrorCodesToExceptions(true);
        adAuthenticationProvider.setUseAuthenticationRequestCredentials(true);

    ADAuthenticationJDBCAuthorizationProvider authenticationProvider = new ADAuthenticationJDBCAuthorizationProvider(adAuthenticationProvider);

        return authenticationProvider;
    }
}

The custom UserDetailsService:

public class JDBCUserDetailsService implements UserDetailsService {

    @Autowired
    private UtilisateurService utilisateurService;

    @Override
    public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
        final String name = username.toLowerCase();
        Collection<GrantedAuthority> authorities = utilisateurService.loadRolesFromDatabase(name);
        final boolean isActive = utilisateurService.isActive(name);

        return new User(username, "", isActive, true, true, true, authorities);
    }
}

Do you have any suggestion for a better solution?

Answer 1

By using a framework like Spring Security you have all you cicle already defined and your task is just to customize the parts you need that should be integrated with your application and need some logic for that.

First of all I would raise here a good practice that Spring developers don't follow. In documentation you always found code like:

@Autowired
private UtilisateurService utilisateurService;

That is good for Spring to show you a cool and quick short way to inject stuff in your class avoiding boilerplate code.

Said that, it is not good in production code.

You should, instead, write a proper construction method for a mandatory dependency:

private UtilisateurService utilisateurService;

@Autowired
public ADAuthenticationJDBCAuthorizationProvider(ActiveDirectoryLdapAuthenticationProvider adDelegate, UtilisateurService utilisateurService) {
    this.adDelegate = adDelegate;
    this.utilisateurService = utilisateurService;
}

Or a setter if the dependency is not mandatory:

private UtilisateurService utilisateurService;

@Autowired
public void setUtilizateurService(UtilisateurService utilisateurService) {
    this.utilisateurService = utilisateurService;
}

The reason to do so is to expose your dependencies to the user of your class, instead of hiding.

You should just hide your data in the classes, not the dependencies.

Showing up all dependencies make your code easy to use and test.

Another point I would like to rise is in your UserDetailsService implementation:

@Override
public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
    final String name = username.toLowerCase();
    Collection<GrantedAuthority> authorities = utilisateurService.loadRolesFromDatabase(name);
    final boolean isActive = utilisateurService.isActive(name);

    return new User(username, "", isActive, true, true, true, authorities);
}

I think you should avoid to instantiate here a UserDetails object.

Instead you should have a builder that initialize the object as needed for you:

public class User implements UserDetails {
    // ... code ...

    public static User from(String userName, boolean isActive, List<GrantedAuthority> authorizations) {
        User user = new User(userName, "", isActive, true, true, true, authorizations);
    }
}

...

@Override
public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException {
    final String name = username.toLowerCase();
    Collection<GrantedAuthority> authorities = utilisateurService.loadRolesFromDatabase(name);

    return User.from(username, utilisateurService.isActive(name), authorities);
}

In this way you can ensure to have just valid users, and you have a single point in your code where you handle what a valid user is.

Answer 2

You have to consider that using Spring is like playing with lego bricks.

Spring will force you into a corset of for most usecases "well-defined" interfaces and dependencies if you want to reuse already existing functionality. Your task is to chose the right elements out of the toolbox and use predefined implementations or provide custom implementations under Spring interface contracts where you need them. Finally you configure the implementations to use in the predefined dependency graph as Spring provides it.

All I mentioned you have done. Everything is adapted to the interfaces Spring forces you to use in a minimal way.

Following suggestions I have, but that are small concerns:

1. I could tell something about conventions e.g. you should name "DOMAIN" and "URL" to lowercase.

2. I could tell you about creating a real class out of AbstractAuthenticationToken.

3. I could tell you to extract some magic values to meaningful constants ( ... , true, true, true, ...)

One other thing makes it hard to determine the quality of the code: All code is formulated in a sequence, no iteration and no selection. So there is inherently no room for concerns against the control flow.

You feel that this code could be better? I share your feelings but I won't argue with it. You have gone the Spring way correctly: You comply the contracts.