Skip to content

Conversation

evgeniycheban
Copy link
Contributor

Simplify RequestMatcherDelegatingAuthorizationManager.Builder matcher registration

Closes gh-11624

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 27, 2023
@evgeniycheban evgeniycheban force-pushed the gh-11624 branch 4 times, most recently from dac9fd0 to 652b526 Compare April 28, 2023 00:25
Copy link
Contributor

@marcusdacoregio marcusdacoregio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @evgeniycheban, I have left feedback inline.

Can you please ensure that the @since tags are updated to 6.2 because we already have a release candidate for 6.1?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The requestMatchers method should keep the same behavior as AbstractRequestMatcherRegistry by creating a MvcRequestMatcher if possible and use AntPathRequestMatcher as a fallback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcusdacoregio, thanks for the review. I'm not sure if it would be a good idea to propagate an ApplicationContext to this builder, maybe we could create here AntPathRequestMatcher by default
and let the user register MvcRequestMatcher using requestMatchers(RequestMatcher... matchers).

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, can you make that behavior explicit in the javadoc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reflected this behavior in the javadoc and updated @since tags to 6.2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @evgeniycheban, I am concerned about having the ant matcher the default if Spring MVC is in the classpath. This deviates from the default that Spring Security adopted in 6.0. For now, what if requestMatchers only takes a RequestMatcher instance? We can think more (in another issue) about how to communicate the MVC nuance to the builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @marcusdacoregio, makes sense, let's stay only with requestMatchers(RequestMatcher... matchers) for now and think about supporting MvcRequestMatcher in the future releases. I will update the PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR.

@marcusdacoregio marcusdacoregio self-assigned this May 3, 2023
@marcusdacoregio marcusdacoregio added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 3, 2023
@jzheaux jzheaux added this to the 6.2.0-M1 milestone May 12, 2023
@marcusdacoregio
Copy link
Contributor

Thanks @evgeniycheban, this is now merged into main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Simplify RequestMatcherDelegatingAuthorizationManager.Builder matcher registration

4 participants