spring-security icon indicating copy to clipboard operation
spring-security copied to clipboard

#11510: Add isFullyAuthenticated to AuthenticatedTrustResolver

Open karthikeyan-r opened this issue 2 years ago • 8 comments

Spring Security 11510 - Implemented isFullyAuthenticated function which is composition of isAnonymous & isRememberMe

karthikeyan-r avatar Jul 30 '22 18:07 karthikeyan-r

@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.

jzheaux avatar Aug 16 '22 21:08 jzheaux

Hi, @karthikeyan-r. Are you able to make the requested changes?

jzheaux avatar Aug 30 '22 00:08 jzheaux

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.

karthikeyan-r avatar Sep 09 '22 19:09 karthikeyan-r

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);
		}

	}

karthikeyan-r avatar Sep 09 '22 21:09 karthikeyan-r

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);

jzheaux avatar Sep 20 '22 22:09 jzheaux

Thanks for your suggestion @jzheaux, I have updated implementation

karthikeyan-r avatar Sep 26 '22 20:09 karthikeyan-r

Thanks, @karthikeyan-r! It looks like there is a failing test. Can you check that out, please?

jzheaux avatar Oct 05 '22 16:10 jzheaux

@jzheaux resolved that test failure. Added null check for authentication to fix that.

karthikeyan-r avatar Oct 15 '22 19:10 karthikeyan-r

@jzheaux any comments on this fix ?

karthikeyan-r avatar Oct 25 '22 17:10 karthikeyan-r

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.

jzheaux avatar Oct 26 '22 22:10 jzheaux

Thanks, @karthikeyan-r! This is now merged into main with 5fcbb9f4ed792ca24836efaad37d5454a7dd881d and will appear in the 6.1.0 release.

jzheaux avatar Nov 29 '22 23:11 jzheaux