Skip to content

Form Login not possible when a single OAuth2 Provider is configured #6802

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
netmikey opened this issue Apr 20, 2019 · 27 comments · Fixed by #9827
Closed

Form Login not possible when a single OAuth2 Provider is configured #6802

netmikey opened this issue Apr 20, 2019 · 27 comments · Fixed by #9827
Assignees
Labels
in: config An issue in spring-security-config status: backported An issue that has been backported to maintenance branches type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug

Comments

@netmikey
Copy link

netmikey commented Apr 20, 2019

Summary

When using a Form Login, a single OAuth2 provider and the auto-generated login page, the auto-configured AuthenticationEntryPoint will redirect to the provider immediately, bypassing the login page and effectively preventing form login.

Actual Behavior

When trying to access a protected resource, spring security will immediately redirect to the OAuth2 provider's authentication page instead of the local login page.

Expected Behavior

When Form Login is configured, the login page should never be skipped.

Configuration

    @Override
    protected void configure(HttpSecurity http) throws Exception {
        http.authorizeRequests().anyRequest().authenticated()
            .and()
            .oauth2Login()
            .and()
            .oauth2Client()
            .and()
            .formLogin().permitAll();
    }
spring.security.oauth2.client.registration.facebook.client-id=some-id
spring.security.oauth2.client.registration.facebook.client-secret=some-secret

Version

5.1.4-RELEASE, not sure as of which version this happens.

Sample

I don't have a sample, but I found the exact location of the bug:

if (loginUrlToClientName.size() == 1) {
// Setup auto-redirect to provider login page
// when only 1 client is configured
this.updateAuthenticationDefaults();
this.updateAccessDefaults(http);
String providerLoginPage = loginUrlToClientName.keySet().iterator().next();
this.registerAuthenticationEntryPoint(http, this.getLoginEntryPoint(http, providerLoginPage));
} else {
super.init(http);
}

The condition should check whether Form Login is enabled and don't apply the shortcut if it is.

@jgrandja jgrandja added type: bug A general bug in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) OIDC labels Apr 30, 2019
@jgrandja jgrandja added this to the 5.2.0.M3 milestone Apr 30, 2019
@jgrandja
Copy link
Contributor

Thanks for the report @netmikey. Would you be interested in submitting a PR for this fix?

@rhamedy
Copy link
Contributor

rhamedy commented May 1, 2019

I could work on this if @netmikey didn't want to 👍

@jgrandja
Copy link
Contributor

jgrandja commented May 1, 2019

Thanks @rhamedy for the offer. Let's provide @netmikey a chance to respond. If it's not picked up by end of next week than feel free to take it on. Thanks!

@netmikey
Copy link
Author

netmikey commented May 1, 2019

Thanks, @jgrandja. Since I'm pretty busy those days, please go ahead @rhamedy 👍

@rhamedy
Copy link
Contributor

rhamedy commented May 1, 2019

Thanks both 🙂

@jgrandja anything I should know or keep in mind when working towards a fix in addition to the pointers that @netmikey included in the PR description?

@jgrandja
Copy link
Contributor

jgrandja commented May 2, 2019

@rhamedy Nothing that comes to mind at the moment. But feel free to ask any questions along the way. Thanks for taking this on.

@rhamedy
Copy link
Contributor

rhamedy commented May 3, 2019

Hi @jgrandja

I wanted to run these changes and questions by you before creating a pull request. For the actual issue pointed out in the description of the ticket, I did the following

if (loginUrlToClientName.size() == 1)

to following and verified it with a test 👍

if (loginUrlToClientName.size() == 1 
          && http.getConfigurer(FormLoginConfigurer.class) == null)

I could not find an alternative method to check if formLogin is configured 🤔

Secondly, I noticed that the same issue is there in the reactive side as well.
These might be the related issues 5339 & 5347.

Assuming that the Reactive side need fixing as well, I updated the

if (urlToText.size() == 1) {

to

if (urlToText.size() == 1 && formLogin == null) {

however I am not sure if formLogin == null is the right approach? 🤔 I added a test but, the test fails and complains that authenticationManager cannot be null when I have clearly set it as shown below

@Test
	public void defaultLoginPageWhenLoginFormEnabledAndSingleClientRegistrationThenShowDefault() {
		this.spring.register(OAuth2LoginWithSingleClientRegistrations.class,
				OAuth2LoginMockAuthenticationManagerConfigWithFormLoginEnabled.class).autowire();

		WebTestClient webTestClient = WebTestClientBuilder
				.bindToWebFilters(new GitHubWebFilter(), this.springSecurity)
				.build();

		WebDriver driver = WebTestClientHtmlUnitDriverBuilder
				.webTestClientSetup(webTestClient)
				.build();

		driver.get("http://localhost/");

		assertThat(driver.getCurrentUrl()).startsWith("http://localhost/login");
	}

	@Configuration
	static class OAuth2LoginMockAuthenticationManagerConfigWithFormLoginEnabled {
		ReactiveAuthenticationManager manager = mock(ReactiveAuthenticationManager.class);

		@Bean
		public SecurityWebFilterChain springSecurityFilter(ServerHttpSecurity http) {
			http
				.authorizeExchange()
					.anyExchange().authenticated()
					.and()
				.oauth2Login()
					.and()
				.formLogin()
					.authenticationManager(manager); // is still null in FormLoginSpec.configure line 2378
			return http.build();
		}
	}

The error snippet is as follow

Caused by: java.lang.IllegalArgumentException: authenticationManager cannot be null
	at org.springframework.util.Assert.notNull(Assert.java:198)
	at org.springframework.security.web.server.authentication.AuthenticationWebFilter.<init>(AuthenticationWebFilter.java:82)

I am new to Reactive so I feel like I am not configuring the mock correctly. Anyway, the test issue is not relevant unless we to update the Reactive side. If we have to update the Reactive side then could you please help with root cause of test failure?

@jgrandja
Copy link
Contributor

jgrandja commented May 3, 2019

@rhamedy I would like to avoid the http.getConfigurer(FormLoginConfigurer.class) conditional check, as I don't feel OAuth2LoginConfigurer should be aware of (or have reference) to FormLoginConfigurer. I'll investigate on my end and you continue as well and we'll figure out an alternative approach.

I would leave the reactive changes out for now. Let's figure out the apprach for the fix on servlet side first and go from there.

@rwinch rwinch removed the OIDC label May 3, 2019
@rhamedy
Copy link
Contributor

rhamedy commented May 4, 2019

Sounds good @jgrandja, I will look into other options 👍

@jgrandja
Copy link
Contributor

jgrandja commented May 7, 2019

@rhamedy If you look at the logic in OAuth2LoginConfigurer.initDefaultLoginFilter(), it obtains a reference to DefaultLoginPageGeneratingFilter which FormLoginConfigurer does the same. Maybe you can check DefaultLoginPageGeneratingFilter.isEnabled() which will return true if formLogin() is enabled. I think this should work. But I'm also wondering if there might be an ordering issue between FormLoginConfigurer and OAuth2LoginConfigurer when they get init(). Anyway, give this a try and see how it goes.

@jgrandja
Copy link
Contributor

jgrandja commented May 7, 2019

@netmikey As a temporary workaround, you can configure httpSecurity.oauth2Login().loginPage("/custom-login") to specify your custom login page and you won't have the same issue with the auto-redirect.

The auto-redirect only happens when there is one client configured and no custom loginPage() configured. Just as an FYI, the default login page is provided as a convenience for development or samples - it's not meant to be used in production applications since you can't customize.

@netmikey
Copy link
Author

netmikey commented May 8, 2019

I know, and I'm working on a custom login already.

The default form is very convenient for starting development though, and I found its behavior confusing, especially for someone starting on that subject.

@rhamedy
Copy link
Contributor

rhamedy commented May 9, 2019

hi @jgrandja last week I spent some quite some time debugging alternative options and a majority of my attempts resulted it either in no success or failing existing test. I added the following breaking test

@Test
public void oauth2LoginWithFormLoginPageThenRedirectDefaultLoginPage() throws Exception {
	loadConfig(OAuth2LoginConfigFormLoginPage.class);

	String requestUri = "/";
	this.request = new MockHttpServletRequest("GET", requestUri);
	this.request.setServletPath(requestUri);
	this.springSecurityFilterChain.doFilter(this.request, this.response, this.filterChain);

	assertThat(this.response.getRedirectedUrl()).matches("http://localhost/login");
}

@EnableWebSecurity
static class OAuth2LoginConfigFormLoginPage extends CommonWebSecurityConfigurerAdapter {
	@Override
	protected void configure(HttpSecurity http) throws Exception {
		http
			.oauth2Login()
			.clientRegistrationRepository(
					new InMemoryClientRegistrationRepository(GOOGLE_CLIENT_REGISTRATION))
			.and()
			.formLogin();
		super.configure(http);
	}
}

That above test breaks with no fix 👍 and then I proceeded with adding alternative fixes to http.getConfigurer(FormLoginConfigurer.class) which you rightly stated that is not the right approach. One of the alternatives is the following

Map<String, String> loginUrlToClientName = this.getLoginLinks();
DefaultLoginPageGeneratingFilter loginPageGeneratingFilter = http.getSharedObject(DefaultLoginPageGeneratingFilter.class);
if (loginUrlToClientName.size() == 1 && loginPageGeneratingFilter == null) {
...
}

With the above fix the newly added test above passes ✅ however, an existing test fails the assertions and the question to ask is it correct for DefaultLoginPageGeneratingFilter to be not null in case of this test when the test clearly does not set the form login and only declares oauth2 as shown in the config.

In your suggestions you mentioned making use of DefaultLoginPageGeneratingFilter.isEnabled() and the method is as follow (the following might explain why DefaultLoginPageGeneratingFilter is not null with oauth2Login and might not be null in case of openId too 😕).

public boolean isEnabled() {
	return formLoginEnabled || openIdEnabled || oauth2LoginEnabled;
}

Not sure if that is going to help since it's not just formLoginEnabled but, others too. Regardless, in the tests that I added above the value of isEnabled in OAuth2LoginConfigurer.initDefaultLoginFilter() is false. I tried adding a new public method just for isFormLoginEnabled and that returns false too.

I wanted to specifically check if the filter chain includes UsernamePasswordAuthenticationFilter which is added by FormLoginConfigurer and could not find a reference, not sure why.

This AbstractConfiguredSecurityBuilder.java class has a init method that initializes each configurer's init method and the init method for FormLoginConfigurer.java is different than OAuth2LoginConfigurer.java when it comes to what is being set and not. I added some debug lines in the AbstractConfiguredSecurityBuilder.init() to see whether the FormLoginConfigurer is one of the configurers for the test and it is there.

I might have to do a line by line debug to see where we could potentially add a fix and I am starting to feel like when inside OAuth2LoginConfigurer the only thing that we can do is check if FormLoginConfigurer is in http which is not good practice, any other fix we might have to apply it outside OAuth2LoginConfiguer either before or after (in after case, the oauth2 login configuer might overwrite the formlogin stuff).

Sorry for the detailed messages, it does not seem to be a straight curve and I am starting to feel that my limited knowledge of spring/security is catching up with me 😐

@jgrandja
Copy link
Contributor

@rhamedy

the question to ask is it correct for DefaultLoginPageGeneratingFilter to be not null

Take a look at WebSecurityConfigurerAdapter.getHttp() and you will notice that HttpSecurity.apply(new DefaultLoginPageConfigurer<>()) is called when defaults are enabled. So in common configurations, DefaultLoginPageConfigurer will be available so it can be configured by FormLoginConfigurer and/or OAuth2LoginConfigurer.

My original suggestion on checking DefaultLoginPageGeneratingFilter.isEnabled() will not work. The ordering of the configurers is dependent - formLogin() would need to be called before oauth2Login() in WebSecurityConfigurerAdapter.configure(), which obviously we cannot rely on. So this option is out.

This is a tricky one for sure. At the moment, I don't have another suggestion but will give this some further thought.

@rhamedy
Copy link
Contributor

rhamedy commented May 17, 2019

Make sense. I will give it some more thoughts in the coming days and will update here.

@jarpz
Copy link

jarpz commented Apr 22, 2021

Is there some way to customize loginPage when we use oauth2Login?

In the servlet configuration it is allowed by using this:

..extends WebSecurityConfigurerAdapter {

  @Override
  protected void configure(HttpSecurity http) throws Exception {
    http
        .cors().disable()
        .csrf().disable()
        .authorizeRequests()
        .antMatchers("/oauth2/**",
            "/login/**", "/actuator/**", "/", "/login.html").permitAll()
        .and()
        .authorizeRequests()
        .anyRequest().authenticated()
        .and()
        .oauth2Login()
        .loginPage("/login"); // This method does not exist on when we use: ServerHttpSecurity
  }

...

@Bean
  RouterFunction<ServerResponse> loginPage() {
    return RouterFunctions.route(GET("/login"), req -> {
      return ServerResponse.ok().render("login.html");
    });
  }

But using ServerHttpSecurity loginPage it is not present. Any suggestion on how to fix it with web flux?
Regards

@jgrandja jgrandja assigned sjohnr and unassigned jgrandja May 18, 2021
@sjohnr
Copy link
Member

sjohnr commented May 20, 2021

Hey everyone. I've read through the comments to catch up on this issue. Before looking to a fix, I wanted to see if I understood the issue correctly. So far, with a hello-world style app, I seem to be able to get the desired behavior to work, so I'm wondering if I'm missing anything. Here's what I have:

First, I ran auth-server from @jgrandja 's oauth2-protocol-patterns to stand in for okta, facebook, google, etc. Note: I've added an entry to /etc/hosts for auth-server:9000 to serve up from localhost:9000.

Second, I stood up a spring boot app with the following:

application.yml:

server:
  port: 8080

spring:
  security:
    oauth2:
      client:
        registration:
          login-client:
            provider: spring
            client-id: login-client
            client-secret: secret
            redirect-uri: "{baseUrl}/login/oauth2/code/{registrationId}"
            scope: openid
        provider:
          spring:
            issuer-uri: http://auth-server:9000
    user:
      password: "{noop}password"

SecurityConfiguration.java:

import org.springframework.boot.autoconfigure.security.SecurityProperties;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.security.config.annotation.web.builders.HttpSecurity;
import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity;
import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter;
import org.springframework.security.core.userdetails.User;
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.core.userdetails.UserDetailsService;
import org.springframework.security.provisioning.InMemoryUserDetailsManager;

@Configuration
@EnableWebSecurity
public class SecurityConfiguration extends WebSecurityConfigurerAdapter {

    @Override
    protected void configure(HttpSecurity http) throws Exception {
        http
            .authorizeRequests()
                .anyRequest().authenticated()
                .and()
            .oauth2Login()
                .loginPage("/login")
                .and()
            .oauth2Client()
                .and()
            .formLogin()
                .permitAll()
                .and();
    }

    @Bean
    public UserDetailsService userDetailsService(SecurityProperties securityProperties) {
        SecurityProperties.User user = securityProperties.getUser();
        UserDetails userDetails = User.withUsername(user.getName())
            .password(user.getPassword())
            .roles(user.getRoles().toArray(new String[0]))
            .build();

        return new InMemoryUserDetailsManager(userDetails);
    }
}

HomeController.java:

import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.RestController;

import java.util.Map;

@RestController
public class HomeController {
    @GetMapping("/")
    public Map<String, String> hello() {
        return Map.of("greeting", "Hello, World");
    }
}

You'll notice I had to work through an issue with the auto-configured UserDetailsService because the configurations do complete with each other a little bit. But it seems to work.

If I visit localhost:8080 it redirects to /login with the generated login form. It does not contain a link to the OAuth2 provider because I disabled it via .oauth2Login().loginPage("/login"). This seems to be the magic to disable OAuth2 redirects from any page, while retaining the form login out of the box.

If I visit http://localhost:8080/oauth2/authorization/login-client (for example, if a link on my fictitious web app took me there), I am redirected to http://auth-server:9000/login, where I log in successfully and am redirected back to http://localhost:8080/.

I have not tried anything with the oauth client yet.

If you all (@netmikey, @rhamedy, or @dcoraboeuf) have any thoughts on what I'm missing to reproduce your particular issue, let me know.

@jarpz, would you mind opening a separate issue (if one doesn't already exist) on that?

Update:

One additional note:

If I configure .oauth2Login() with a login page that is the same as the redirect-uri for the oauth2 client registration, then I can get default redirect behavior for my oauth provider, but retain ability to use form login.

    @Override
    protected void configure(HttpSecurity http) throws Exception {
        http
            .authorizeRequests()
                .anyRequest().authenticated()
                .and()
            .oauth2Login()
                .loginPage("/oauth2/authorization/login-client")
                .and()
            .oauth2Client()
                .and()
            .formLogin()
                .permitAll()
                .and();
    }

Does this achieve the desired behavior in the opening comment?

@jgrandja
Copy link
Contributor

@sjohnr To reproduce the issue, change your SecurityConfiguration:

From:

    protected void configure(HttpSecurity http) throws Exception {
        http
            .authorizeRequests()
                .anyRequest().authenticated()
                .and()
            .oauth2Login()
                .loginPage("/login")
                .and()
            .oauth2Client()
                .and()
            .formLogin()
                .permitAll()
                .and();
    }

To:

    protected void configure(HttpSecurity http) throws Exception {
        http
            .authorizeRequests()
                .anyRequest().authenticated()
                .and()
            .oauth2Login()
                .and()
            .oauth2Client()
                .and()
            .formLogin()
                .permitAll()
                .and();
    }

Removing .loginPage("/login") will enable the default login page to be rendered. However, because there is only 1 ClientRegistration registered it will auto-redirect to the provider instead. It should display the default login page because formLogin() is also configured.

Makes sense?

@jarpz
Copy link

jarpz commented May 31, 2021

Hey everyone. I've read through the comments to catch up on this issue. Before looking to a fix, I wanted to see if I understood the issue correctly. So far, with a hello-world style app, I seem to be able to get the desired behavior to work, so I'm wondering if I'm missing anything. Here's what I have:

The problem I reported is when we use WebFlux Security configuration, In that case, we are not allowed to "change" the default generated webpage for oauth2Client, like when you use "mvc" security adapter does. It seems both forms of build security expression are not equivalent. they don't have the same methods.

Regards

@jgrandja
Copy link
Contributor

@jarpz

Is there some way to customize loginPage when we use oauth2Login?

This is a question that would be better suited to Stack Overflow. We prefer to use GitHub issues only for bugs and enhancements.

But using ServerHttpSecurity loginPage it is not present. Any suggestion on how to fix it with web flux?

If a feature is missing in ServerHttpSecurity that exists in HttpSecurity then please log a new issue.

sjohnr pushed a commit to sjohnr/spring-security that referenced this issue Jun 15, 2022
@sjohnr sjohnr added the type: breaks-passivity A change that breaks passivity with the previous release label Jun 15, 2022
@sjohnr sjohnr added in: config An issue in spring-security-config status: backported An issue that has been backported to maintenance branches and removed in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) labels Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: config An issue in spring-security-config status: backported An issue that has been backported to maintenance branches type: breaks-passivity A change that breaks passivity with the previous release type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants