search

grails / grails-spring-security-core

Grails Spring Security Core Plugin
Apache License 2.0
259 stars 224 forks source link

isFullyAuthenticated() vs IS_AUTHENTICATED_FULLY #574

Closed frnktrgr closed 5 years ago

frnktrgr commented 5 years ago

As discussed with Dean Del Ponte ...

I think there is an inconsistency in the grails-spring-security-core 3.3.x (should als be in 4.0.x, but not tested) code and/or at least in the documentation about isFullyAuthenticated().

Steps to Reproduce

In User guide 3.3.x Listing 69 Guarding the switch user url with controllerAnnotations.staticRules (and some other places in this document) the example given is [pattern: '/login/impersonate', access: ['ROLE_SWITCH_USER', 'isFullyAuthenticated()']]

Expected Behaviour

As explained in User guide 3.3.x:

Note that when specifying multiple roles, the user must have at least one of them, but when combining IS_AUTHENTICATED_FULLY, IS_AUTHENTICATED_REMEMBERED, or IS_AUTHENTICATED_ANONYMOUSLY (or their corresponding SpEL expressions) with one or more roles means the user must have one of the roles and satisty the IS_AUTHENTICATED rule.

Actual Behaviour

Only IS_AUTHENTICATED_FULLY is working as described (logical AND) and isFullyAuthenticated() results in logical OR!

Full Description

In checkAuthenticatedVoters only org.springframework.security.access.vote.AuthenticatedVoter is taken into account. But AuthenticatedVoter does not support SpEL expressions and the expression is therefore ignored: AuthenticatedVoter.java#L100. The expression isFullyAuthenticated() is further handled by grails.plugin.springsecurity.web.access.expression.WebExpressionVoter. But this voter is part of checkOtherVoters. So [pattern: '/login/impersonate', access: ['ROLE_SWITCH_USER', 'isFullyAuthenticated()']] is evaluated as ROLE_SWITCH_USER OR isFullyAuthenticated().

Possible Solutions

a) fix documentation to only support IS_AUTHENTICATED... variant b) change code (needs more investigation, at least for me ;-))

ddelponte commented 5 years ago

I will update the documentation to address this short term but will leave this issue open until the issue is addressed in the code. Thanks for this great writeup!

ddelponte commented 5 years ago

A PR for the documentation update is available at #578

sdelamo commented 5 years ago

I cherry picked the documentation commit also to 3.3.x branch. https://github.com/grails-plugins/grails-spring-security-core/commit/db21d461381abaada9dbb73df852ae9ff5931576

ddelponte commented 5 years ago

Closing. Related PR has been merged to the grails 4.x branch and commits have been cherry-picked to the 3.3.x branch.