Improve warning message

[quaff]

We should assume that every UserDetailsService is wired with configured UserDetailsAuthenticationProvider.

Continuation of commit 7ddc005

[Kehrlann]

Hey @quaff , thanks for the pull request.

I'm trying to understand the underlying issue. For reference, the reasons for existing warnings have been documented in the issue gh-14663 . We want to help users realize why their UserDetailsService is not being used in case they provider, say, a @Component AuthenticationProvider.

---

The point of this PR is "there should be as many UserDetailsServer beans as there are AbstractUserDetailsAuthenticationProviderbeans, because each service should be used by one AuthProvider".

The case when there are multiple UserDetailsService beans is an edge case, so this leaves us with the case one service + one provider. So I imagine the issue is:

• There is one UserDetailsService bean
• It is injected into a single AbstractUserDetailsAuthenticationProvider bean
• That AbstractUserDetailsAuthenticationProvider is used for global authentication
• There is a warning, but there should not be, because the setup works as intended

Is this correct?

---

In any case, the conditional "number of UserDetailsService == number of AbstractUserDetailsAuthProvider" is too broad. You'd need to validate that these beans actually depend on each other. That's a heavy lift for a simple log message.

[quaff]

Is this correct?

Exactly.

In any case, the conditional "number of UserDetailsService == number of AbstractUserDetailsAuthProvider" is too broad. You'd need to validate that these beans actually depend on each other.

I tried, but AbstractUserDetailsAuthenticationProvider doesn't expose getUserDetailsService().

That's a heavy lift for a simple log message.

The warning message make me nervous.

[Kehrlann]

I tried, but AbstractUserDetailsAuthenticationProvider doesn't expose getUserDetailsService()

That's by design, I think. One workaround could be to obtain the bean dependencies' from the context's BeanFactory and compare them. But again, that's a heavy lift.

The warning message make me nervous.

I see. One workaround could be to provide EITHER AbstractUserDetailsAuthenticationProvider OR UserDetailsService as a bean, but not both. Is there a specific reason you provide both as beans?

[quaff]

I see. One workaround could be to provide EITHER AbstractUserDetailsAuthenticationProvider OR UserDetailsService as a bean, but not both. Is there a specific reason you provide both as beans?

UserDetailsService is required by most of real applications.
customized DaoAuthenticationProvider is defined because it required by customized AuthenticationManager.

	@Bean
	CustomAuthenticationManager authenticationManager(List<AuthenticationProvider> providers) {
		return new CustomAuthenticationManager(providers);
	}

	@Bean
	CustomAuthenticationProvider daoAuthenticationProvider(ObjectProvider<UserDetailsService> userDetailsService,
			ObjectProvider<PasswordEncoder> passwordEncoder,
			ObjectProvider<UserDetailsPasswordService> userDetailsPasswordService) {
		UserDetailsService uds = userDetailsService.getIfAvailable(() -> username -> {
			throw new UsernameNotFoundException(username);
		});
		CustomDaoAuthenticationProvider provider = new CustomDaoAuthenticationProvider();
		provider.setUserDetailsService(uds);
		passwordEncoder.ifAvailable(provider::setPasswordEncoder);
		userDetailsPasswordService.ifAvailable(provider::setUserDetailsPasswordService);
		return provider;
	}

[Kehrlann]

I don't know about the specifics of your app, but everything you're doing here should be automatically handled by InitializeUserDetailsBeanManagerConfigurer - if you have beans for UserDetailsService + PasswordEncoder + UserDetailsPasswordService, then it should all get picked up and wired into the DaoAuthenticationProvider.

See

spring-security/config/src/main/java/org/springframework/security/config/annotation/authentication/configuration/InitializeUserDetailsBeanManagerConfigurer.java

Lines 92 to 107 in 3117fef

	UserDetailsService userDetailsService = userDetailsServices.get(0).getBean();
	String userDetailsServiceBeanName = userDetailsServices.get(0).getName();
	PasswordEncoder passwordEncoder = getBeanOrNull(PasswordEncoder.class);
	UserDetailsPasswordService passwordManager = getBeanOrNull(UserDetailsPasswordService.class);
	CompromisedPasswordChecker passwordChecker = getBeanOrNull(CompromisedPasswordChecker.class);
	DaoAuthenticationProvider provider;
	if (passwordEncoder != null) {
	provider = new DaoAuthenticationProvider(passwordEncoder);
	}
	else {
	provider = new DaoAuthenticationProvider();
	}
	provider.setUserDetailsService(userDetailsService);
	if (passwordManager != null) {
	provider.setUserDetailsPasswordService(passwordManager);
	}

Does that work for you?

[quaff]

No, I have customized CustomDaoAuthenticationProvider extends DaoAuthenticationProvider and CustomAuthenticationManager extends ProviderManager.

[Kehrlann]

I'll try and take a stab at this using the ApplicationContext, a bit unsure how it will turn out. I'll report back here.

---

Depending on what you do with your UserDetailsService, you can separate the interfaces so that there is no UserDetailsService bean per-se, but rather:

// This UserService class does NOT implement UserDetailsService
class UserService {

	private UserDetails loadUser(String username) throws UsernameNotFoundException {
		// TODO
	}

	public UserDetailsService getUserDetailsService() {
		return this::loadUser;
	}

	// + your own user operations, not necessarily matching UserDetailsManager

	// You can also leverage an existing UserDetailsManager if you want
	private UserDetailsManager userDetailsManager;

	public void newUser(String username, String password, LocalDate birthDate) {
		userDetailsManager.createUser(new CustomUser(...));
	}
}

// and therefore you can decouple the interfaces ; one for business needs, one for Spring Security

@Bean
public UserService userService() {
	return new UserService();
}

@Bean
public UserDetailsService userDetailsService(UserService userService) {
	return userService.getUserDetailsService();
}

It's a workaround, not great, but it will make the warnings go away.

---

On the general theme of Global authentication, where these Initialize...ManagerConfigurer classes kick in, we would like to simplify the model. It's very non-obvious what's happening. This is why we put the warnings in the first place.

Hopefully, all these workarounds will eventually go away.

[quaff]

How about

List<BeanWithName<AbstractUserDetailsAuthenticationProvider>> userDetailsAuthenticationProviders = getBeansWithName(
		AbstractUserDetailsAuthenticationProvider.class);
List<BeanWithName<AuthenticationManager>> authenticationManagers = getBeansWithName(AuthenticationManager.class);
boolean shouldWarn = userDetailsAuthenticationProviders.size() == 1
		&& userDetailsAuthenticationProviders.getFirst().getBean().getClass().getName().startsWith("org.springframework.security.")
		&& authenticationManagers.size() == 1
		&& authenticationManagers.getFirst().getBean().getClass().getName().startsWith("org.springframework.security.");

[Kehrlann]

Why do you suggesting finding AuthenticationManager beans?
The current check is UserDetailsService + AuthenticationProvider, which is the interaction we're looking for.

While decent, the size() == 1 check is still an approximation. I'd rather look at the actual bean graph. If we are going to do this, I'm thinking:

String userDetailsServiceBeanName = userDetailsServices.get(0).getName();
if (auth.isConfigured()) {
	if (isUserDetailsServiceUnused(userDetailsServiceBeanName)) {
		this.logger.warn("Global AuthenticationManager configured with an AuthenticationProvider bean. "
				+ "UserDetailsService beans will not be used for username/password login. "
				+ "Consider removing the AuthenticationProvider bean. "
				+ "Alternatively, consider using the UserDetailsService in a manually instantiated "
				+ "DaoAuthenticationProvider.");
	}
	return;
}

// with:

private boolean isUserDetailsServiceUnused(String userDetailsServiceBeanName) {
	var authProviderBeans = getBeansWithName(AbstractUserDetailsAuthenticationProvider.class);
	if (authProviderBeans.isEmpty()) {
		// this never happens
		return true;
	}

	if (InitializeUserDetailsBeanManagerConfigurer.this.context instanceof ConfigurableApplicationContext ctx) {
		var dependencies = ctx.getBeanFactory().getDependenciesForBean(authProviderBeans.get(0).getName());
		return !Arrays.asList(dependencies).contains(userDetailsServiceBeanName);
	}
	return true;
}

But, as I mentioned earlier, this is a lot of code.

[Kehrlann]

Hey @quaff - I've circled back with the team. We feel it's a bit too much complexity for a simple "UX" log line, so we will not be making code changes here. We value your feedback and we want to improve the logger message. Feel free to update your PR with an improved warning message, and we'll happily take it in. Something like:

Global AuthenticationManager configured with an AuthenticationProvider bean. UserDetailsService beans will not be used by Spring Security for automatically configuring username/password login. Consider removing the AuthenticationProvider bean. Alternatively, consider using the UserDetailsService in a manually instantiated DaoAuthenticationProvider. If the current configuration is intentional, to turn off this warning, increase the logging level to ERROR with logging.level.org.springframework.security.config.annotation.authentication.configuration.InitializeUserDetailsBeanManagerConfigurer=ERROR

In addition to improving the error message, if you feel strongly about this log message, please open an issue. It will allow us to see how the community reacts to the issue.

---

For future reference, the easiest workaround is to increase the logger level:

logging.level.org.springframework.security.config.annotation.authentication.configuration.InitializeUserDetailsBeanManagerConfigurer=ERROR

Caveat: This logger is not part of our public API, and might change in the future.

[quaff]

@Kehrlann I revised message from your suggestion, since not every application uses Spring Boot.

[Kehrlann]

Good point, thanks!
@jzheaux could you please merge this?