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

Make RequestMatcherDelegatingAuthorizationManager post-processable #15978

Conversation

codeconsole
Copy link
Contributor

@codeconsole codeconsole commented Oct 23, 2024

Fix extensibility issue since RequestMatcherDelegatingAuthorizationManager is final and does not expose any public methods other than what is available through AuthorizationManager. Fixes #15948

Allows the following:

http.authorizeHttpRequests((authorize) -> authorize
    .requestMatchers("/assets/**", "/logout").permitAll()
    .anyRequest().permitAll().withObjectPostProcessor(new ObjectPostProcessor<AuthorizationManager<HttpServletRequest>>() {
        @Override
         public <O extends AuthorizationManager<HttpServletRequest>> O postProcess(O object) {
            return (O) new WrappedRequestMatcherDelegatingAuthorizationManager(context, object);
        }
    })
);

which opens up the possibility to provide additional security checks such as Controller annotations by wrapping the current manager and using the outcome of is authorization check to be compared against other checks.

This works similar to what is already possible with the ObjectPostProcessor for AuthorizationFilter except unlike RequestMatcherDelegatingAuthorizationManager, AuthorizationFilter is not final and can be extended.

There is no benefit in post processing a final class that doesn't not expose any additional information that is not already provided by it's interface AuthorizationManager

Alternatively, you could just remove final from

public final class RequestMatcherDelegatingAuthorizationManager implements AuthorizationManager<HttpServletRequest> {

but either solution works.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 23, 2024
@codeconsole codeconsole force-pushed the fix-extensibility-AuthorizeHttpRequestsConfigurer branch from 7342218 to 6803412 Compare October 23, 2024 06:54
@jzheaux jzheaux self-assigned this Oct 23, 2024
@jzheaux jzheaux added in: config An issue in spring-security-config type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Oct 23, 2024
@jzheaux
Copy link
Contributor

jzheaux commented Oct 23, 2024

Thanks, @codeconsole! Will you please also add a unit test in AuthorizeHttpRequestsConfigurer that confirms the behavior works?

And when you push your update, it will help if you can change your commit to something more like this:

Make RequestMatcherDelegatingAuthorizationManager Post-Processable

Closes gh-15978

It improves readability in the git history to have a short title. The Closes line makes sure our GitHub integration creates tickets for the other supported branches when this is merged.

@jzheaux jzheaux changed the title AuthorizeHttpRequestsConfigurer - Allow postProcess() on interface AuthorizationManager<HttpServletRequest> instead of final RequestMatcherDelegatingAuthorizationManager class Make RequestMatcherDelegatingAuthorizationManager post-processable Oct 23, 2024
@jzheaux jzheaux added the status: waiting-for-feedback We need additional information before we can continue label Oct 23, 2024
@jzheaux jzheaux modified the milestones: 6.4.x, 6.2.x Oct 23, 2024
codeconsole added a commit to codeconsole/spring-security that referenced this pull request Oct 23, 2024
@codeconsole codeconsole force-pushed the fix-extensibility-AuthorizeHttpRequestsConfigurer branch from 6803412 to 641a480 Compare October 23, 2024 19:29
@codeconsole
Copy link
Contributor Author

@jzheaux Thanks for the feedback. I have changed the commit message and added the test.

I also published a snapshot locally and confirmed it works as expected with my application and that I am able to perform the behavior that I needed it to do.

This 1 little change allows me to delete 462 lines of code so I will be excited to see it merged. 😄

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 23, 2024
codeconsole added a commit to codeconsole/spring-security that referenced this pull request Oct 23, 2024
@codeconsole codeconsole force-pushed the fix-extensibility-AuthorizeHttpRequestsConfigurer branch from 8611d44 to 2fdf6a8 Compare October 23, 2024 19:58
jzheaux pushed a commit to codeconsole/spring-security that referenced this pull request Oct 23, 2024
@jzheaux jzheaux force-pushed the fix-extensibility-AuthorizeHttpRequestsConfigurer branch from 2fdf6a8 to bf6b163 Compare October 23, 2024 19:58
@jzheaux jzheaux changed the base branch from main to 6.2.x October 23, 2024 19:59
@jzheaux jzheaux modified the milestones: 6.2.x, 6.2.8 Oct 23, 2024
@codeconsole
Copy link
Contributor Author

@jzheaux The version of main I was based on was failing so I synced up and force pushed off of the latest main to make sure all the tests passed. I hope that didn't cause any issues?

@jzheaux jzheaux force-pushed the fix-extensibility-AuthorizeHttpRequestsConfigurer branch from bf6b163 to b3d0726 Compare October 23, 2024 20:33
@jzheaux
Copy link
Contributor

jzheaux commented Oct 23, 2024

It didn't cause any issues, @codeconsole, thanks for checking. I moved the PR to be based off of 6.2.x so that we can forward port from there. Also, I accidentally gave you the wrong ticket number to place in the commit, so I just fixed it myself rather than have you re-do it.

@jzheaux jzheaux merged commit 18dba34 into spring-projects:6.2.x Oct 23, 2024
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Oct 23, 2024

Thanks, @codeconsole! This is now merged into 6.2.x, 6.3.x, and main.

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: feedback-provided Feedback has been provided type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RequestMatcherDelegatingAuthorizationManager should be post-processable
3 participants