Skip to content
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

Add with() method to apply Custom DSLs returning the builder #13204

Closed
bwgjoseph opened this issue May 21, 2023 · 5 comments · Fixed by #13432
Closed

Add with() method to apply Custom DSLs returning the builder #13204

bwgjoseph opened this issue May 21, 2023 · 5 comments · Fixed by #13432
Assignees
Labels
in: config An issue in spring-security-config status: duplicate A duplicate of another issue type: enhancement A general enhancement

Comments

@bwgjoseph
Copy link

Given that and() is stated for removal, does it mean that in the future, I cannot chain the HttpSecurity?

image

image

// not possible
@Bean
public SecurityFilterChain docsFilterChain(HttpSecurity http) throws Exception {
    return http
        .apply(CustomDSL.customDSL()) //.and() deprecated
        .build();
}

// good to go
@Bean
public SecurityFilterChain docsFilterChain(HttpSecurity http) throws Exception {
    http
        .apply(CustomDSL.customDSL());

    return http.build();
}
@bwgjoseph bwgjoseph added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels May 21, 2023
@jzheaux jzheaux added in: config An issue in spring-security-config and removed status: waiting-for-triage An issue we've not yet triaged labels May 24, 2023
@jzheaux
Copy link
Contributor

jzheaux commented May 24, 2023

Thanks for reaching out, @bwgjoseph, I think you make a good point that HttpSecurity#apply may not be suitable since it returns the given configurer instead of HttpSecurity. I'll reach out to the team for some thoughts and then get back to you.

@jzheaux jzheaux added the for: team-attention This ticket should be discussed as a team before proceeding label May 24, 2023
@marcusdacoregio marcusdacoregio self-assigned this May 31, 2023
@marcusdacoregio marcusdacoregio moved this from Planning to Prioritized in Spring Security Team Jun 5, 2023
@marcusdacoregio
Copy link
Contributor

Hi @bwgjoseph, thank you so much for the report.

After talking to @jzheaux, we decided that we want to add a new with(...) method that returns the HttpSecurity in that case. It would look something like this:

public <C extends SecurityConfigurerAdapter<O, B>> B with(C configurer) throws Exception {
	configurer.addObjectPostProcessor(this.objectPostProcessor);
	configurer.setBuilder((B) this);
	add(configurer);
	return (B) this;
}

Then you can do:

http
	.with(new CustomDsl().myMethod())
	.formLogin(Customizer.withDefaults())
	.httpBasic(Customizer.withDefaults());

I already prioritized this for 6.2. I'll update the issue's title to align better with the problem if you are okay with it.

As of now, you can create a new method inside your custom DSL that returns the builder:

static class CustomDsl extends AbstractHttpConfigurer<CustomDsl, HttpSecurity> {

    // ...

    public HttpSecurity build() {
        return getBuilder();
    }

}

// then

http
    .apply(new CustomDsl()
        .myMethod()
        .myOtherMethod()
        .build())
    .formLogin(...)
    // ...

@marcusdacoregio marcusdacoregio changed the title Chaining SecurityFilterChain build with CustomDsl Add with() method to apply Custom DSLs returning the builder Jun 5, 2023
@marcusdacoregio marcusdacoregio removed the for: team-attention This ticket should be discussed as a team before proceeding label Jun 5, 2023
@bwgjoseph
Copy link
Author

Thank you, this new way looks good.

I just tried what you suggested but encounter some error.

image

The method apply(C) in the type AbstractConfiguredSecurityBuilder<DefaultSecurityFilterChain,HttpSecurity> is not applicable for the arguments (HttpSecurity)
public class DummyDsl extends AbstractHttpConfigurer<DummyDsl, HttpSecurity> {
    @Override
    public void init(HttpSecurity http) throws Exception {
        http.formLogin(AbstractHttpConfigurer::disable);
    }

    public static DummyDsl dummyDsl() {
        return new DummyDsl();
    }

    public HttpSecurity build() {
        return getBuilder();
    }
}

@marcusdacoregio
Copy link
Contributor

What I meant is to replace .and() with that method

http.apply(new DummyDsl()).build()
  .formLogin(...)
  .build();

I realize that the name of the method might be somewhat confusing. You might call it different than build().

@bwgjoseph
Copy link
Author

Great, I think it works now.

Thanks for the great work 👏

@marcusdacoregio marcusdacoregio added the status: duplicate A duplicate of another issue label Jun 27, 2023
@marcusdacoregio marcusdacoregio moved this from Prioritized to In Progress in Spring Security Team Jun 29, 2023
marcusdacoregio added a commit to marcusdacoregio/spring-security that referenced this issue Jun 29, 2023
This method is intended to replace .apply() because it will not be possible to chain configurations when .and() gets removed

Closes spring-projectsgh-13204
@github-project-automation github-project-automation bot moved this from In Progress to Done in Spring Security Team Jun 29, 2023
marcusdacoregio added a commit that referenced this issue Jun 29, 2023
This method is intended to replace .apply() because it will not be possible to chain configurations when .and() gets removed

Closes gh-13204
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: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants