Skip to content

WebSecurityConfiguration should autowire PermissionEvaluator #5272

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.

Sign up for GitHub

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

Jump to bottom
Closed
candrews opened this issue Apr 30, 2018 · 10 comments
Closed

WebSecurityConfiguration should autowire PermissionEvaluator #5272

candrews opened this issue Apr 30, 2018 · 10 comments
Assignees
@rwinch
Labels
in: config An issue in spring-security-config in: web An issue in web modules (web, webmvc) type: bug A general bug
Milestone
5.0.5

Comments

@candrews
Copy link
Contributor

This issue effectively reopens #4077.

GlobalMethodSecurityConfiguration autowires PermissionEvaluator from the context:
https://github.com/spring-projects/spring-security/blob/4.1.3.RELEASE/config/src/main/java/org/springframework/security/config/annotation/method/configuration/GlobalMethodSecurityConfiguration.java#L154

WebSecurityConfiguration should do the same thing.

Currently, it's surprising that when a PermissionEvaluator is set up, it just works (with no configuration other than declaring the PermissionEvaluator bean) when used from Java annotations but the same expression always returns denied (as that's what the default configuration does) when used from in a web context (such as in a JSP sec: expression).

Using thymeleaf spring security extras, you can conditionally show elements like this:
<div sec:authorize="hasPermission(#vars.study,'DELETE')">

The DefaultWebSecurityExpressionHandler that ends up being used has a type of org.springframework.security.access.expression.DenyAllPermissionEvaluator instead of the permission evaluator.

This behavior worked as expected with the commit accepted from my pull request, 3bf6bf1, but some polish was done that made it no longer work, 8a66d0c.

Thanks again in advance for looking into this issue.
@rwinch rwinch self-assigned this Apr 30, 2018
@rwinch rwinch added the status: waiting-for-triage An issue we've not yet triaged label Apr 30, 2018
@rwinch rwinch added this to the 5.0.5 milestone Apr 30, 2018
@rwinch rwinch added in: config An issue in spring-security-config in: web An issue in web modules (web, webmvc) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 30, 2018
@rwinch rwinch closed this as completed Apr 30, 2018
@rwinch
Copy link
Member

rwinch commented Apr 30, 2018

Thanks for the report. A fix is now pushed to master and 5.0.x

@candrews
Copy link
Contributor Author

Thank you for addressing the issue so quickly!

@rwinch
Copy link
Member

rwinch commented Apr 30, 2018

No problem. Thanks for the report! If you get a chance it would be nice to hear if the changes work for you. The SNAPSHOTs should be published within 30 minutes if you want to give it a try.

rwinch added a commit that referenced this issue Apr 30, 2018
@rwinch
DefaultWebSecurityExpressionHandler uses PermissionEvaluator Bean
eb067bc 
The default instance of DefaultWebSecurityExpressionHandler uses the
PermissionEvaluator Bean by default.

Fixes: gh-5272
rwinch added a commit that referenced this issue Apr 30, 2018
@rwinch
DefaultWebSecurityExpressionHandler uses PermissionEvaluator Bean
f7f6798 
The default instance of DefaultWebSecurityExpressionHandler uses the
PermissionEvaluator Bean by default.

Fixes: gh-5272
@candrews
Copy link
Contributor Author

I tested just now with spring security 5.0.5.BUILD-SNAPSHOT by means of adding

<dependency>
 <groupId>org.springframework.security</groupId>
 <artifactId>spring-security-bom</artifactId>
 <version>5.0.5.BUILD-SNAPSHOT</version>
 <scope>import</scope>
</dependency>

to my pom.xml. The version I got was: using https://repo.spring.io/snapshot/org/springframework/security/spring-security-bom/5.0.5.BUILD-SNAPSHOT/spring-security-bom-5.0.5.BUILD-20180430.173119-29.pom

Unfortunately, the bug was still reproducible - I'm getting a DenyAllPermissionEvaluator instead of the expected one.

I believe the reason why this is happening is because your code isn't in there... I checked the source jar and org.springframework.security.config.annotation.web.builders.WebSecurity doesn't have the changes from your commit. It also looks like the build failed (the commit has a red x on it).

@rwinch rwinch reopened this Apr 30, 2018
@rwinch
Copy link
Member

rwinch commented Apr 30, 2018

Thanks for the fast follow up. You are right that the build was failing. It should have published recently. Can you try again and force updates?

@candrews
Copy link
Contributor Author

I tested again just now, updating snapshots and the problem isn't resolved. It would appear that a new snapshot has yet to be published.

@rwinch rwinch changed the title Reopened: WebSecurityConfiguration should autowire PermissionEvaluator the same way GlobalMethodSecurityConfiguration does WebSecurityConfiguration should autowire PermissionEvaluator Apr 30, 2018
@rwinch
Copy link
Member

rwinch commented Apr 30, 2018 •
edited
Loading

@candrews Once again thank you for your fast response. I have a sample project that demonstrates this is working at https://github.com/rwinch/spring-security-sample/tree/gh-5272-permissionevaluator NOte that it is on branch gh-5272-permissionevaluator

Run the tests to see it fail:

./mvnw test
...
 [ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 2.669 s <<< FAILURE! - in demo.ApplicationTests
[ERROR] authenticatedWorks(demo.ApplicationTests)  Time elapsed: 0.244 s  <<< FAILURE!
java.lang.AssertionError: 
Response content
Expected: a string containing "Can Delete"
     but: was "<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
    <meta name="description" content="">
    <meta name="author" content="">
    <title>Demo</title>
</head>
<body>
Here

</body>
</html>"
	at demo.ApplicationTests.authenticatedWorks(ApplicationTests.java:31)

2018-04-30 16:19:33.023  INFO 7965 --- [       Thread-4] o.s.w.c.s.GenericWebApplicationContext   : Closing org.springframework.web.context.support.GenericWebApplicationContext@5b38c1ec: startup date [Mon Apr 30 16:19:30 CDT 2018]; root of context hierarchy
[INFO] 
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   ApplicationTests.authenticatedWorks:31 Response content
Expected: a string containing "Can Delete"
     but: was "<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width, initial-scale=1, shrink-to-fit=no">
    <meta name="description" content="">
    <meta name="author" content="">
    <title>Demo</title>
</head>
<body>
Here

</body>
</html>"

then run it again with the fix profile to use the latest 5.0.5.BUILD-SNAPSHOT

./mvnw test -U -Pfix
...
[INFO] BUILD SUCCESS

Is your sample working yet? If not, can you please provide a sample that is broken? Feel free to base it off the sample I provided.

@rwinch rwinch added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 30, 2018
@rwinch
Copy link
Member

rwinch commented May 7, 2018

I'm closing this due to no more feedback. In the tests and the external sample project I provided it works.

@rwinch rwinch closed this as completed May 7, 2018
@candrews
Copy link
Contributor Author

candrews commented May 9, 2018

I confirm - it works fine.

My apologies for my delayed reply.

@rwinch
Copy link
Member

rwinch commented May 9, 2018

@candrews Thanks for the follow up and thanks again for raising this issue!

@larsgrefer larsgrefer mentioned this issue Jun 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rwinch rwinch

Labels
in: config An issue in spring-security-config in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
None yet
Milestone
5.0.5
Development

No branches or pull requests
2 participants
@candrews @rwinch