Promote usage of Subjects in Authentication class #88494
Conversation
This PR is a follow-up of elastic#86246 to further clean up the Authentication class by: * Promoting usage of the Subject class. The User field and a few other related fields are now removed from Authentication. Relevant methods have their implementation replaced by using Subjects and same behaviours are retained. * Removed the temporary internal RunAsUser class. It essence is about wire serialisation which is now merged into Authentication itself. * Simplify serialisation of regular User object. All the complexities of handling inner user is now completely within the Authentication class itself. * Cosnolidate assertions in different places into a single method that is called in constructors. Also removed a few assertions because there is no RunAsUser class anymore and a User object is just a simple user. Relates: elastic#86246 Relates: elastic#86544
ywangd
added
>refactoring
:Security/Authentication
|
Pinging @elastic/es-security (Team:Security) |
| // TODO(hub-cap) Clean this up after moving User over - This class can re-inherit its field AUTHENTICATION_KEY in AuthenticationField. | ||
| // That interface can be removed |
There was a problem hiding this comment.
I don't know what this comment means. But I am pretty sure it no longer applies.
There was a problem hiding this comment.
It's from when we split core and security into separate jars. The decision about which class went into each project (and which classes were split down the middle) was made almost entirely on pragmatic grounds rather than a principled design choice. In some cases we split things apart just to make it work (a day or so before feature freeze).
There was a problem hiding this comment.
Thanks for the context. So the removal is safe enough.
| final Map<String, Object> newMetadata = maybeRewriteMetadataForApiKeyRoleDescriptors(olderVersion, this); | ||
|
|
||
| final Authentication newAuthentication; | ||
| if (isRunAs()) { | ||
| // The lookup user for run-as currently doesn't have authentication metadata associated with them because | ||
| // lookupUser only returns the User object. The lookup user for authorization delegation does have | ||
| // authentication metadata, but the realm does not expose this difference between authenticatingUser and | ||
| // delegateUser so effectively this is handled together with the authenticatingSubject not effectiveSubject. | ||
| newAuthentication = new Authentication( | ||
| new Subject( | ||
| effectiveSubject.getUser(), | ||
| maybeRewriteRealmRef(olderVersion, effectiveSubject.getRealm()), | ||
| olderVersion, | ||
| effectiveSubject.getMetadata() | ||
| ), | ||
| new Subject( | ||
| authenticatingSubject.getUser(), | ||
| maybeRewriteRealmRef(olderVersion, authenticatingSubject.getRealm()), | ||
| olderVersion, | ||
| newMetadata | ||
| ), | ||
| type | ||
| ); | ||
| } else { | ||
| newAuthentication = new Authentication( | ||
| new Subject( | ||
| authenticatingSubject.getUser(), | ||
| maybeRewriteRealmRef(olderVersion, authenticatingSubject.getRealm()), | ||
| olderVersion, | ||
| newMetadata | ||
| ), | ||
| type | ||
| ); |
There was a problem hiding this comment.
This is the only place that is more bloated after refactoring. In all other places, the change is a simplification.
| ); | ||
| this.authenticatingUser = Objects.requireNonNull(authenticatingUser); | ||
| } | ||
| public static class AuthenticationSerializationHelper { |
There was a problem hiding this comment.
This class now only deals with regular User object. It does not need to stay inside the Authentication class anymore. But I kept it here to avoid more changes. Besides the usages by Authentication, it is only used by GetUserResponse which should ever just read/write regular User objects.
| this.user = AuthenticationSerializationHelper.readUserFrom(in); | ||
| this.authenticatedBy = new RealmRef(in); | ||
| // Read the user(s) | ||
| final User outerUser = AuthenticationSerializationHelper.doReadUserFrom(in); |
There was a problem hiding this comment.
The distinction between readUserFrom and doReadUserFrom isn't very clear from the names. Do we really need both? If so, can the names reflect their differences in some way?
There was a problem hiding this comment.
Yeah the name doReadUserFrom is bad. I changed it to readUserWithoutTrailingBoolean. It is still not great. But I am out of ideas. Also added a comment to readUserFrom to explain the difference.
There was a problem hiding this comment.
I don't love it, but I suspect it needs a larger refactor to work around it.
We can leave it as is for now.
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
Outdated
Show resolved
Hide resolved
| } else { | ||
| this.lookedUpBy = null; | ||
| lookedUpBy = null; |
There was a problem hiding this comment.
Do we need an assert innerUser == null here? Should every inner user have a looked-up-by realm?
Ah, I see it down below. I think I'd prefer the two asserts are consistently placed - either on read from the stream or on construction of the Subject.
There was a problem hiding this comment.
Do we need an
assert innerUser == nullhere?
Unfortunately no. We use null lookedUpBy to indicate run-as failure (when the run-as user does not exists). In this case, we will have non-null innerUser but null lookedUpBy.
But the reverse is true, i.e. if innerUser is null, we should definitely have null lookedUpBy as well. That's why the other assertion is down below because it does not fit here.
That said, I pulled them together into a single statement and added an elaborative comment for it.
There was a problem hiding this comment.
Tweaked it a bit and made it simpler.
| @@ -205,15 +206,16 @@ public RealmRef getSourceRealm() { | |||
| * the lifetime of the original request. | |||
| */ | |||
| public Version getVersion() { | |||
| return version; | |||
| return effectiveSubject.getVersion(); | |||
There was a problem hiding this comment.
Should we deprecate this method? It doesn't feel like it makes sense to have a version on Authentication now
| public Map<String, Object> getMetadata() { | ||
| return metadata; | ||
| return authenticatingSubject.getMetadata(); |
There was a problem hiding this comment.
As above, should this be deprecated?
| @@ -243,34 +273,19 @@ public Authentication maybeRewriteForOlderVersion(Version olderVersion) { | |||
| */ | |||
| public Authentication runAs(User runAs, @Nullable RealmRef lookupRealmRef) { | |||
There was a problem hiding this comment.
Can we document the circumstances under which lookupRealmRef is null ?
There was a problem hiding this comment.
Added Javadoc for the arguments which covers this.
ywangd
added
the
auto-merge-without-approval
* upstream/master: (2974 commits) Reserved cluster state service (elastic#88527) Add transport action immutable state checks (elastic#88491) Remove suggest flag from index stats docs (elastic#85479) Polling cluster formation state for master-is-stable health indicator (elastic#88397) Add test execution guide in yamlRestTest asciidoc (elastic#88490) Add troubleshooting guide for corrupt repository (elastic#88391) [Transform] Finetune Schedule to be less noisy on retry and retry slower (elastic#88531) Updatable API keys - auto-update legacy RDs (elastic#88514) Fix typo in TransportForceMergeAction and TransportClearIndicesCacheA… (elastic#88064) Fixed NullPointerException on bulk request (elastic#88358) Avoid needless index metadata builders during reroute (elastic#88506) Set metadata on request in API key noop test (elastic#88507) Fix passing positional args to ES in Docker (elastic#88502) Improve description for task api detailed param (elastic#88493) Support cartesian shape with doc values (elastic#88487) Promote usage of Subjects in Authentication class (elastic#88494) Add CCx 2.0 feature flag (elastic#88451) Reword the watcher 'always' and 'never' condition docs (elastic#86105) Simplify azure discovery installation docs (elastic#88404) Breakup FIPS CI testing jobs ... # Conflicts: # server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java # x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java
This PR removes the deprecated Authentication#getMetadata method and replaces its usages with #getAuthenticatingSubject#getMetadata. Relates: elastic#88494
This PR removes the deprecated Authentication#getUser method and replaces its usages with #getEffectiveSubject#getUser. Relates: elastic#88494
This PR removes the deprecated Authentication#getMetadata method and replaces its usages with #getAuthenticatingSubject#getMetadata. Relates: #88494
This PR removes the deprecated Authentication#getUser method and replaces its usages with #getEffectiveSubject#getUser. Relates: #88494
This PR removes the deprecated Authentication#getAuthenticatedBy method and replaces its usages with #getAuthenticatingSubject#getRealm Relates: elastic#88494
This PR removes the deprecated Authentication#getAuthenticatedBy method and replaces its usages with #getAuthenticatingSubject#getRealm Relates: #88494
This PR removes the deprecated Authentication#getLookedUpBy method. Its usages are replaced by either isRunAs or getEffectiveSubject#getRealm or their combinations depending on the actual context. Relates: elastic#88494
This PR removes the deprecated Authentication#getLookedUpBy method. Its usages are replaced by either isRunAs or getEffectiveSubject#getRealm or their combination depending on the actual context. Relates: #88494
This PR removes the deprecated Authentication#getSourceRealm method. Its usage is mostly replaced by #getEffectiveSubject#getRealm except for ApiKeyService#getCreatorRealmName and ApiKeyService#getCreatorRealmType which has a special handling to return authenticatingSubject's realm when run-as lookup fails. This is to maintain BWC since these information is used in audit logs. Therefore, even it is technically incorrect, we should not break it without careful planning. Relates: #88494
This PR is a follow-up of #86246 to further clean up the Authentication
class by:
related fields are now removed from Authentication. Relevant methods
have their implementation replaced by using Subjects and same
behaviours are retained.
wire serialisation which is now merged into Authentication itself.
handling inner user is now completely within the Authentication class
itself.
is called in constructors. Also removed a few assertions because there
is no RunAsUser class anymore and a User object is just a simple user.
Relates: #86246
Relates: #86544
Resolves: #80117