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

Clarify WebInvocationPrivilegeEvaluator JavaDoc #16529

Open
jzheaux opened this issue Feb 3, 2025 · 0 comments · May be fixed by #16548
Open

Clarify WebInvocationPrivilegeEvaluator JavaDoc #16529

jzheaux opened this issue Feb 3, 2025 · 0 comments · May be fixed by #16548
Labels
in: docs An issue in Documentation or samples type: bug A general bug
Milestone

Comments

@jzheaux
Copy link
Contributor

jzheaux commented Feb 3, 2025

When someone passes null for the method to WebInvocationPrivilegeEvaluator, they are saying that the HTTP method doesn't matter to them for matching purposes. However, it's non-trivial to make an authorization decision in any circumstance where the relevant request matcher does require a method.

Imagine the following arrangement:

.requestMatchers(HttpMethod.GET, "/path").hasAuthority("USER")
.requestMatchers(HttpMethod.POST, "/path").permitAll()

When WebInvocationPrivilegeEvaluator#isAllowed("/path", authentication) is called, which matcher's authorization rules should it use?

This happens in the more generic case as well:

.requestMatchers(HttpMethod.GET, "/path").hasAuthority("USER")
.anyRequest().denyAll()

Or, in other words: "if GET /path, then require USER authority; if POST | PUT | DELETE /path, then deny". Here, also, there's no way to know which the user intends, unless they also specify a method.

As such, I believe it's reasonable to require passing a method if you want method-specific request matchers to be considered, which is what the method currently does.

We should update the JavaDoc to be clearer about this, something like:

* Note that this will only match authorization rules that don't require a certain {@code HttpMethod}

Both isAllowed methods should contain this clarification.

@jzheaux jzheaux added in: docs An issue in Documentation or samples type: bug A general bug labels Feb 3, 2025
@jzheaux jzheaux added this to the 6.3.7 milestone Feb 3, 2025
ngocnhan-tran1996 added a commit to ngocnhan-tran1996/spring-security that referenced this issue Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: docs An issue in Documentation or samples type: bug A general bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant