Propagate Authorities From Previous Factors #17790
Conversation
jzheaux
changed the title
jzheaux
force-pushed
the
authentication-builder
branch
5 times, most recently
from
4f62b6b to
6eb00d0
Compare
This commit adds a new default method to Authentication for the purposes of creating a Builder based on the current authentication, allowing other authentications to be applied to it as a composite. It also adds Builders for each one of the authentication result classes.
This commit allows looking up the current authentication and applying it to the latest authentication. This is specifically handy when collecting authorities gained from each authentication factor.
This commit provides the SecurityContextHolderStrategy bean to ProviderManager instances that the HttpSecurity DSL constructs.
jzheaux
force-pushed
the
authentication-builder
branch
from
6eb00d0 to
b48b10a
Compare
There was a problem hiding this comment.
Thanks for the PR @jzheaux! I've provided feedback inline, but my main pieces of feedback are:
- The
AuthenticationManagershould not be accessing theSecurityContext. Instead, we should have the controller (e.g.Filter) that invokes theAuthenticationManagerperform the merging of the twoAuthenticationinstances. - I think that the builder APIs should function independently of MFA and should work for any properties on the
Authentication. Doing this would also allow deprecation of thesetAuthenticatedmethod. - I don't think we should have an
Authentication.apply(Authentication)method. Especially so if it is only applying the authorities and ignoring many other properties that are on theAuthenticationobject.
|
|
||
| @Override | ||
| public B apply(Authentication token) { | ||
| Assert.isTrue(token.isAuthenticated(), "cannot mutate an unauthenticated token"); |
There was a problem hiding this comment.
Why can't an unauthenticated user be mutated? I believe that the builders should be independent of the MFA feature. If we keep this assertion, it would be good to include the why in the error message.
| @Override | ||
| public B apply(Authentication token) { | ||
| Assert.isTrue(token.isAuthenticated(), "cannot mutate an unauthenticated token"); | ||
| Assert.notNull(token.getPrincipal(), "principal cannot be null"); |
There was a problem hiding this comment.
Why can't a principal be null? If we keep this assertion, it would be good to include the why in the error message.
| @@ -185,4 +187,36 @@ public String toString() { | |||
| return sb.toString(); | |||
| } | |||
|
|
|||
| protected abstract static class AbstractAuthenticationBuilder<A extends Authentication, B extends AbstractAuthenticationBuilder<A, B>> | |||
There was a problem hiding this comment.
I'd expect that this builder's properties aligns with the properties available on AbstractAuthentication.
| } | ||
|
|
||
| @Override | ||
| public B apply(Authentication token) { |
There was a problem hiding this comment.
Related to the comment about only having an authorities member, it is surprising that an Authentication is passed in here, but only the authorities are used from it. There are quite a few other properties that I think should be taken into account.
| return (B) this; | ||
| } | ||
|
|
||
| protected abstract A build(Collection<GrantedAuthority> authorities); |
There was a problem hiding this comment.
I think that I'd prefer to make authorities protected and not pass anything into this method. This gives us flexibility of adding additional properties later.
| * | ||
| * @since 7.0 | ||
| */ | ||
| public static final class Builder extends AbstractAuthenticationBuilder<@NonNull CasAuthenticationToken, Builder> { |
There was a problem hiding this comment.
There is no need to explicitly declare @NonNull as this is the default
| } | ||
|
|
||
| @Override | ||
| protected @NonNull CasAuthenticationToken build(Collection<GrantedAuthority> authorities) { |
There was a problem hiding this comment.
There is no need to explicitly declare @NonNull as this is the default
| if (!current.isAuthenticated()) { | ||
| return result; | ||
| } | ||
| return doAuthenticate(current).map((r) -> r.toBuilder().apply(current).build()); |
There was a problem hiding this comment.
Just as in the ProviderManager, the combining of these should not be done in the ReactiveAuthenticationManager.
| @@ -54,6 +57,9 @@ | |||
| */ | |||
| public interface Authentication extends Principal, Serializable { | |||
|
|
|||
| @Serial | |||
| long serialVersionUID = -3884394378624019849L; | |||
There was a problem hiding this comment.
Can you help me to understand why this is specified?
| * @since 7.0 | ||
| */ | ||
| public static final class Builder | ||
| extends AbstractAuthenticationBuilder<@NonNull Saml2AssertionAuthentication, @NonNull Builder> { |
There was a problem hiding this comment.
There is no need to explicitly declare @NonNull as this is the default
Applications can use
AuthenticationBuilderto apply existing authentications to new ones.For example, if the current logged in user is represented by:
And they provide a second set of authenticated credentials, represented by:
Then the first factor can be applied to the second factor as follows:
This draft PR adds a basic builder to each Authentication implementation that implements
Authentication.Builder. In order to simplify upgrades,toBuilderby default returns a no-op implementation ofAuthentication.Builderthat ultimately returns the same authentication unchanged.