Grails 7: grails-spring-security-acl #44
Conversation
...in/src/main/groovy/grails/plugin/springsecurity/acl/ProxyAwareParameterNameDiscoverer.groovy
Outdated
Show resolved
Hide resolved
functional-test-app/build.gradle
Outdated
| classpath "gradle.plugin.com.energizedwork.webdriver-binaries:webdriver-binaries-gradle-plugin:1.4" | ||
| classpath "com.bertramlabs.plugins:asset-pipeline-gradle:5.0.1" | ||
| classpath "org.grails.plugins:hibernate5:$gormVersion" | ||
| classpath "gradle.plugin.com.github.erdi.webdriver-binaries:webdriver-binaries-gradle-plugin:2.7" |
There was a problem hiding this comment.
Converting to ContainerGebSpec removes the requirement for this. Going forward, the webdriver binaries are abandoned so I would switch to the newer way. You can also remove the withTest, GebConfig, web driver config, and any Geb headless properties in the CI. The geb plugin has an updated readme that shows a single gradle line to support ContainerGebSpec.
| cacheManagerName = 'spring-security-acl-cache-' + UUID.randomUUID() | ||
| } | ||
| ehcacheAclCache(EhCacheFactoryBean) { | ||
| aclCacheManager(JCacheCacheManager) |
There was a problem hiding this comment.
It makes sense to use the spring cache, but we probably should document this change since it has different defaults.
There was a problem hiding this comment.
I opened an issue (#46) to update the docs for Grails 7.
bkoehm
force-pushed
the
bkoehm.5.0.x
branch
4 times, most recently
from
e58b742
to
cece6f8
Compare
| @@ -212,8 +211,6 @@ class SpringSecurityAclGrailsPlugin extends Plugin { | |||
|
|
|||
| private configureExpressionBeans = { conf -> | |||
|
|
|||
| parameterNameDiscoverer(ProxyAwareParameterNameDiscoverer) | |||
There was a problem hiding this comment.
@jamesfredley @jdaugherty @codeconsole This PR has been approved but before I merge it there's one thing that I want to ask about because I am not sure who this is going to affect. Once I got the tests running, it was clear there was an issue with this custom ProxyAwareParameterNameDiscoverer. It just plain wasn't working: parameters weren't being discovered. I will tag you in another line comment where this is being tested in the tests, which will make this more clear with an actual example of where parameter name discovery is needed. It has to do with @PreAuthorize and other similar annotations.
I am not sure why this behavior has changed between Spring (or Java) versions, but I think it has to do with the javac -parameters option. The Spring StandardReflectionParameterNameDiscoverer documentation alludes to this compiler parameter being required. I don't think this compiler parameter is on by default. I am not sure if that has changed with recent versions of Java, or perhaps it's a change in Spring behavior.
I don't think we want people to have to use compiler flags to get the old custom ProxyAwareParameterNameDiscoverer to work. The alternative is to use the Spring @P annotation with Spring Security's DefaultSecurityParameterNameDiscoverer . @P is documented here in the Spring Security docs. This is the route I decided to take. I removed use of ProxyAwareParameterNameDiscoverer, deleted the class file, and modified the tests to use the @P annotation. (The default DefaultSecurityParameterNameDiscoverer is in effect instead.)
Example from the docs:
@PreAuthorize("hasPermission(#c, 'write')")
public void updateContact(@P("c") Contact contact);
Issue #46 has been opened to update the docs.
If we don't want to go down this route, then I guess we would have to update the docs to tell people they have to use the -parameters compiler option and I can restore ProxyAwareParameterNameDiscoverer.
There was a problem hiding this comment.
Pinging @matrei for his thoughts too.
| @@ -17,13 +18,13 @@ class ReportService { | |||
|
|
|||
| @PreAuthorize('hasPermission(#report, admin)') | |||
| @Transactional | |||
| void addPermission(Report report, String username, int permission) { | |||
| void addPermission(@P("report") Report report, String username, int permission) { | |||
There was a problem hiding this comment.
@jamesfredley @jdaugherty @codeconsole As mentioned in the other comment, here are the test changes where the @P annotation was added to get parameter name discovery working again.
This was required for grails/grails-spring-security-ui#160