spring-security
spring-security copied to clipboard
#11510: Add isFullyAuthenticated to AuthenticatedTrustResolver
Spring Security 11510 - Implemented isFullyAuthenticated function which is composition of isAnonymous & isRememberMe
@karthikeyan-r, please also take a look at the ticket description to find the places to use this new method. Please change those places to use it.
Hi, @karthikeyan-r. Are you able to make the requested changes?
Apologies for the delay @jzheaux, I was travelling & couldnt check and respond to your message. I will work on these comment & push code back in a day.
Regarding changes in AuthenticatedAuthorizationManager#fullyAuthenticated
as mentioned in this issue,
FullyAuthenticatedAuthorizationStrategy
is using composition of isGranted
and isRememberMe
- not isAnonymous
. Please clarify on this.
private static final class FullyAuthenticatedAuthorizationStrategy extends AuthenticatedAuthorizationStrategy {
@Override
boolean isGranted(Authentication authentication) {
return super.isGranted(authentication) && !this.trustResolver.isRememberMe(authentication);
}
}
Now it's my turn to delay, @karthikeyan-r! Thanks for your question.
If you follow the method isGranted
to its implementation, you'll see that it is called isAnonymous
also.
I believe it is correct to change the implementation of FullyAuthenticatedAuthorizationStrategy
to do:
return this.trustResolver.isFullyAuthenticated(authentication);
Thanks for your suggestion @jzheaux, I have updated implementation
Thanks, @karthikeyan-r! It looks like there is a failing test. Can you check that out, please?
@jzheaux resolved that test failure. Added null check for authentication to fix that.
@jzheaux any comments on this fix ?
Thanks for the updates, @karthikeyan-r! I'll take things from here. We won't be able to merge it just yet since we are in the RC phase of the next release, but I'll merge it right after that.
Thanks, @karthikeyan-r! This is now merged into main
with 5fcbb9f4ed792ca24836efaad37d5454a7dd881d and will appear in the 6.1.0
release.