createEvaluationContext should defer lookup of Authentication

[evgeniycheban]

createEvaluationContext should defer lookup of Authentication

• Added createEvaluationContext method that accepts Supplier<Authentication>
• Refactored classes that use EvaluationContext to use lazy initialization of Authentication

Closes gh-9667

[jzheaux]

Thanks, @evgeniycheban! I've left some feedback inline. Also, will you please rebase this against 5.8.x as I imagine that's were this change will land?

[jzheaux]

The protected reference to Authentication obviously causes a bit of a problem. Given that there is getAuthentication() I wonder if it could be changed to private (possibly needs to wait until 6.0).

@rwinch, what do you think? I don't think it's still necessary for this property to be protected -- it only remains that way for backward compatibility. Can we introduce a breaking change in 5.8 that makes this field private so that it can be changed to a Supplier<Authentication>? Applications subclassing SecurityExpressionRoot would need to change this.authentication to getAuthentication().

[rwinch]

I'm not certain of this, but I think that changing the member variable to private will break SpEL expressions that use #authentication

[jzheaux]

Hmm, okay. I wondered if that was the case since I can access principal in a SpEL expression without there being a principal member variable. I'll do some experimenting and see.

[jzheaux]

I ran this test against SecurityExpressionRoot with protected Authentication authentication, private Authentication authentication, and private Supplier<Authentication> authentication:

@Test
public void authenticationIsCorrectlyResolved() {
	Authentication authentication = new TestingAuthenticationToken("user", "password");
	ExpressionParser parser = new SpelExpressionParser();
	EvaluationContext context = new StandardEvaluationContext(new SecurityExpressionRoot(authentication) {});
	Expression root = parser.parseExpression("authentication.name == 'user'");
	assertThat(root.getValue(context)).isEqualTo(true);
	Expression variable = parser.parseExpression("#authentication.name == 'user'");
	assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() -> variable.getValue(context));
}

And it passed for all three setups because the Expression uses the method resolver to resolve authentication. In no case could authentication be accessed as #authentication. Hypothetically, if we do need this, I think that we can do something like context.setVariable("authentication", authentication) during setup.

To remove noise, I ran a test against two custom objects unrelated to Security, other than to emulate the change from a protected field of Type A to a private field of Type B. In both classes, the public getter method remains the same:

@Test
public void valueFieldIsNotDirectlyReferenced() {
	ExpressionParser parser = new SpelExpressionParser();
	Expression method = parser.parseExpression("value.length() == 2");
	EvaluationContext privateField = new StandardEvaluationContext(new WithPrivateField());
	EvaluationContext protectedField = new StandardEvaluationContext(new WithProtectedField());
	assertThat(method.getValue(privateField)).isEqualTo(true);
	assertThat(method.getValue(protectedField)).isEqualTo(true);
	Expression field = parser.parseExpression("#value.length() == 2");
	assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() -> field.getValue(privateField));
	assertThatExceptionOfType(SpelEvaluationException.class).isThrownBy(() -> field.getValue(protectedField));
}

private static class WithPrivateField {
	private Integer value = 13;

	public String getValue() {
		return String.valueOf(this.value);
	}
}

private static class WithProtectedField {
	protected String value = "13";

	public String getValue() {
		return this.value;
	}
}

The test again passes.

Also note that if I remove the method getValue() from WithProtectedField, the test fails with "Property or field 'value' cannot be found on object of type 'org.springframework.security.access.expression.AbstractSecurityExpressionHandlerTests$WithProtectedField' - maybe not public or not valid?". Only if I also make the field public does it pass.

Since the property in SecurityExpressionRoot is not public, I don't think it has any bearing on how expressions are evaluated.

Let me know if you see other rocks we should try and uncover.

[evgeniycheban]

@jzheaux I've updated the PR and rebased this branch on top of 5.8.x, please take a look.
I've made changes to SecurityExpressionRoot.authentication now it’s private Supplier<Authentication> that can memorise the value.
Also I've added an implementation of new createEvaluationContext method for DefaultHttpSecurityExpressionHandler.

[jzheaux]

Looking good, @evgeniycheban. I added one cleanup mention.

Additionally, are you able to add an implementation for DefaultMessageSecurityExpressionHandler?

[jzheaux]

Thanks, @evgeniycheban! This is now merged into 5.8.x as well as main.