Open Crystark opened 1 week ago
Thanks for reaching out @Crystark!
I can return null as a response to
getAuthority
which is allowed as per the documentation:
Please consider the entirety of the method's documentation:
If the
GrantedAuthority
can be represented as aString
and thatString
is sufficient in precision to be relied upon for an access control decision by anAccessDecisionManager
(or delegate), this method should return such aString
.If the
GrantedAuthority
cannot be expressed with sufficient precision as aString
,null
should be returned. Returningnull
will require anAccessDecisionManager
(or delegate) to specifically support theGrantedAuthority
implementation, so returningnull
should be avoided unless actually required.
Note: Unfortunately, this documentation doesn't mention AuthorizationManager
(or the reactive counterpart) so I think it would be worth updating this documentation.
The point the docs are making is that the thing consuming the GrantedAuthority
has to understand null
and/or your GrantedAuthority
implementation. The AuthorityReactiveAuthorizationManager
does not handle this, and so you would need to provide a custom implementation.
I expect
org.springframework.security.authorization.AuthorityReactiveAuthorizationManager#check
to not crash on a null authority and just filter it out.
I'm not really sure it is valuable or desirable to handle null
authorities by default, as this would likely hide a problem from the user. In your case, what is expected to handle the authority that is returning null
?
This is actually an indication that you may have reversed the role of ReactiveAuthorizationManager
and GrantedAuthority
by placing logic in the GrantedAuthority
itself, and in fact the logic in your custom authority should be extracted into a custom ReactiveAuthorizationManager
instead.
Having said all of that, I feel that instead of ignoring null
, it could be beneficial for AuthorityReactiveAuthorizationManager
to return a more informative error message using Assert.hasText()
. Would you be interested in submitting a PR for this?
Thanks for getting back to me so fast on this @sjohnr
In your case, what is expected to handle the authority that is returning null?
The authority returning null is used in my PermissionEvaluator
triggered on @PreAuthorize
:
class MyPermissionEvaluator : PermissionEvaluator {
override fun hasPermission(authentication: Authentication, targetDomainObject: Any?, permission: Any): Boolean {
throw NotImplementedError("Use hasPermission(targetId, targetType, permission) or hasAuthority(authority) instead")
}
override fun hasPermission(
authentication: Authentication,
targetId: Serializable?,
targetType: String,
permission: Any
): Boolean =
if (targetId == null) {
log.warn(
"Permission denied: unexpected null 'targetId' argument for target type '{}' and permission '{}'",
targetType,
permission
)
false
} else if (permission !is String) {
log.warn("Permission denied: 'permission' argument ({}) should be a String", permission.javaClass.name)
false
} else {
authentication.authorities
.filterIsInstance<MyAuthority>()
.any { it.hasPermission(permission, targetType, targetId.toString()) }
}
companion object {
val log: Logger = LoggerFactory.getLogger(MyPermissionEvaluator::class.java)
}
}
And this is how I use this:
@PreAuthorize("hasPermission(#someId, '$RESOURCE_TYPE', '$RESOURCE_SCOPE')")
This is working fine by the way.
I only got the error I mentioned when the null-returning GrantedAuthority was moved in the JWT token before the access
-returning one and because I'm using hasAuthority("access")
So I'm not directly using AuthorityReactiveAuthorizationManager
right ? Maybe I should ?
The reason I did everything like that is that I wanted to be able to extract the various permissions from my JWT token and make them available to my PermissionEvaluator
as well as be able to use hasAuthority("access")
. Maybe I'm mixing up things that should not be ?
Do you have any suggestions ? I'm already thinking I can just return a random string instead of null 😏 but that feels a bit hacky and I'd rather try and do this the right way. Well actually I would probably concatenate the scope and the permission separated by # but still doesn't really feel right.
Thanks
@Crystark thanks for your reply.
While reviewing your code, I am having a bit of difficulty understanding it and the intent of it, but it appears to be all about a resource name taken from a JWT and combined with a list of scopes. There’s a lot to unpack there and this isn’t really the place to work through your solution. I can tell you however that I’m fairly certain there are improvements to be made. If you would like to work through that, please open a stack overflow question and provide the link here and I’ll be happy to take a look.
In any case, I think returning null for the authority requires a custom AuthorizationManager
which you have not provided. I feel the best we can do here is throw a more informative exception (perhaps with Assert.hasText(...)
).
Describe the bug When my custom GrantedAuthority returns null on
getAuthority()
, I get the following exception that makes the application fail.To Reproduce I can't provide a complete working example but hopefully the following samples will help:
First here is a condensed version of my base security config so that I can work with my permissions provider. The key points are my asking of the
access
authority and that I can return null as a response togetAuthority
which is allowed as per the documentation:It seems that the order in which I process the permissions matter as if it finds the
access
authority it won't continue and I won't get the NPE. However if I have my nullable authority first it's going to fail.Expected behavior I expect
org.springframework.security.authorization.AuthorityReactiveAuthorizationManager#check
to not crash on a null authority and just filter it out.Code proposition