Remove the deprecated Authentication#getSourceRealm method #92222
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
ywangd
added
>refactoring
:Security/Authentication
|
Pinging @elastic/es-security (Team:Security) |
ywangd
commented
Dec 8, 2022
Comment on lines
73
to
88
| * | ||
| * The authentication is consisted of two {@link Subject}s | ||
| * <ul> | ||
| * <li>{@link #authenticatingSubject}</li> performs the authentication, i.e. it provides a credential.</li> | ||
| * <li>{@link #effectiveSubject} The subject that {@link #authenticatingSubject} impersonates ({@link #isRunAs()})</li> | ||
| * </ul> | ||
| * If {@link #isRunAs()} is {@code false}, the two {@link Subject}s will be the same object. | ||
| * | ||
| * Authentication also has a {@link #type} that tells which mechanism the {@link #authenticatingSubject} | ||
| * uses to perform the authentication. | ||
| * | ||
| * The Authentication's version is its {@link Subject}'s version, i.e. {@code getEffectiveSubject().getVersion()}. | ||
| * It is guaranteed that the versions are identical for the two Subjects. Hence {@code getAuthenticatingSubject().getVersion()} | ||
| * will give out the same result. But using {@code getEffectiveSubject()} is more idiomatic since most callers | ||
| * of this class should just need to know about the {@link #effectiveSubject}. That is, often times, the caller | ||
| * begins with {@code authentication.getEffectiveSubject()} for interrogating an Authentication object. |
There was a problem hiding this comment.
Added some more javadoc as discussed in #91067 (comment)
ywangd
commented
Dec 8, 2022
Comment on lines
-231
to
-233
| if (isAssignedToDomain() && false == newAuthentication.isAssignedToDomain()) { | ||
| logger.info("Rewriting authentication [" + this + "] without domain"); | ||
| } |
There was a problem hiding this comment.
Relocated this logging inside the RealmRef itself which I think is a better location and also get rid of the usages of isAssignedToDomain
ywangd
commented
Dec 8, 2022
Comment on lines
-343
to
+346
| public @Nullable RealmDomain getDomain() { | ||
| return getSourceRealm().getDomain(); | ||
| @Nullable | ||
| RealmDomain getDomain() { |
There was a problem hiding this comment.
I changed this method (also isAssignedToDomain) to package private because:
- It is not really used in production code
- I am not sure whether we want them. Since we removed
getRealmmethod from Authentication, having agetDomainfeels going backwards.
There was a problem hiding this comment.
Agreed that it doesn't make sense to expose either of the domain methods beyond tests.
n1v0lg
approved these changes
Dec 8, 2022
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
Outdated
Show resolved
Hide resolved
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
Outdated
Show resolved
Hide resolved
...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
Outdated
Show resolved
Hide resolved
Comment on lines
-343
to
+346
| public @Nullable RealmDomain getDomain() { | ||
| return getSourceRealm().getDomain(); | ||
| @Nullable | ||
| RealmDomain getDomain() { |
There was a problem hiding this comment.
Agreed that it doesn't make sense to expose either of the domain methods beyond tests.
| @@ -40,30 +40,6 @@ | |||
|
|
|||
| public class AuthenticationTests extends ESTestCase { | |||
|
|
|||
| public void testWillGetLookedUpByWhenItExists() { | |||
There was a problem hiding this comment.
Optional: would cover isFailedRunAs here
There was a problem hiding this comment.
Good point! I adapted the tests for isFailedRunAs. Also added new assertions in ApiKeyServiceTests to ensure the behaviours of getCreatorRealmName and getCreatorRealmType do not change.
Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>
ywangd
added
the
auto-merge-without-approval
|
@elasticmachine update branch |
|
CI failure is unrelated and already tracked at #91800 |
…-get-source-realm
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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