Skip to content

Add authorize() DSL method that accepts HttpMethod #8350

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

Merged
merged 3 commits into from
Apr 22, 2020
Merged

Add authorize() DSL method that accepts HttpMethod #8350

merged 3 commits into from
Apr 22, 2020

Conversation

adamu
Copy link
Contributor

@adamu adamu commented Apr 8, 2020

Attempted an implementation for #8307, which adds an implementation of authorize() for AuthorizeRequestsDsl, so that we can write code like this:

http {
    authorizeRequests {
        authorize(GET, "/path", permitAll)
        authorize(PUT, "/path", denyAll)
    }
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 8, 2020
@eleftherias eleftherias self-assigned this Apr 8, 2020
@eleftherias eleftherias added in: config An issue in spring-security-config type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 8, 2020
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @adamu.

There are some nice changes here that are not directly related to the reported issue.
It would be best if those changes were in separate commits to give a clearer picture to someone viewing the commit history.

One example is extracting PATTERN_TYPE.

Another example is using named arguments. While this is necessary given the changes to PatternAuthorizationRule it may not be obvious to someone viewing the history of this file.

I can see this broken into 3 commits, example titles
"Extract pattern type in request matcher DSL"
"Use named arguments in Kotlin authorization rule"
"Add authorize() DSL method that accepts HttpMethod"

All commits can still be part of this PR.

I have also left a few comments inline.

@adamu
Copy link
Contributor Author

adamu commented Apr 9, 2020

@eleftherias I have split the PR into 3 commits, using your suggested titles.

I also added the extra test.

However, there is some behaviour I am unsure about. For AuthorizeRequestsByMvcConfigWithAndWithoutHttpMethod:

http {
    authorizeRequests {
        authorize("/no_specified_method", permitAll)
        authorize(HttpMethod.GET, "/specified_method", permitAll)
        authorize(HttpMethod.PUT, "/specified_method", denyAll)
    }
}

With this spec, even though /no_specified_method is mapped with a @RequestMapping, a PUT request to /no_specified_method will result in a 403 error.

Additionally, if we change the spec for /specified_method so that the PUT is permitted and GET is denied, the PUT request will be denied.

I think this is the behaviour of MvcMatchersAuthorizedUrl and is not directly related to the behaviour of this DSL, but I wanted to mention it.

@adamu adamu requested a review from eleftherias April 9, 2020 03:00
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @adamu.

The PUT behaviour that you mentioned occurs because the request does not include a CSRF token.
I have added an inline comment showing how to add the CSRF token.

One more note about the commits, please add Fixes: gh-8307 to the final commit.
This will associate the commit with the reported issue and also close the issue when the commit is merged.
See the contributing guidelines for more details

@adamu
Copy link
Contributor Author

adamu commented Apr 16, 2020

@eleftherias I have made the following changes:

  1. Added the HttpMethod for the antMatchers() too.
  2. Added a corresponding test for that.
  3. Added the servlet path test..

Travis failed, but it seems to be due to style problems in CONTRIBUTING.adoc, which I've not changed in this PR (← Resolved in 2eebfd3).

@adamu adamu requested a review from eleftherias April 16, 2020 16:24
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thanks @adamu! I have left some more comments inline.

@adamu adamu requested a review from eleftherias April 17, 2020 16:23
Copy link
Contributor

@eleftherias eleftherias left a comment

Choose a reason for hiding this comment

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

Thank you for all your work @adamu.
I have one more comment and then we should be ready to merge.

@adamu adamu requested a review from eleftherias April 22, 2020 02:27
@eleftherias eleftherias merged commit 0f29bee into spring-projects:master Apr 22, 2020
@eleftherias eleftherias added this to the 5.4.0.M1 milestone Apr 22, 2020
@eleftherias
Copy link
Contributor

Thanks again @adamu!
This is now merged into master.

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 type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants