Skip to content

Remove deprecated Authentication#getAuthenticatedBy #91104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 25, 2022
Merged

Remove deprecated Authentication#getAuthenticatedBy #91104

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b11b37d
Remove deprecated Authentication#getAuthenticatedBy
ywangd Oct 25, 2022
811d14b
tweak
ywangd Oct 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...main/java/org/elasticsearch/xpack/core/security/action/saml/SamlAuthenticateResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public SamlAuthenticateResponse(StreamInput in) throws IOException {

public SamlAuthenticateResponse(Authentication authentication, String tokenString, String refreshToken, TimeValue expiresIn) {
this.principal = authentication.getEffectiveSubject().getUser().principal();
this.realm = authentication.getAuthenticatedBy().getName();
this.realm = authentication.getAuthenticatingSubject().getRealm().getName();
this.tokenString = tokenString;
this.refreshToken = refreshToken;
this.expiresIn = expiresIn;
Expand Down
26 changes: 9 additions & 17 deletions ...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Authentication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,6 @@ public boolean isRunAs() {
return authenticatingSubject != effectiveSubject;
}

/**
* Use {@code getAuthenticatingSubject().getRealm()} instead.
*/
@Deprecated
public RealmRef getAuthenticatedBy() {
return authenticatingSubject.getRealm();
}

/**
* The use case for this method is largely trying to tell whether there is a run-as user
* and can be replaced by {@code isRunAs}
Expand Down Expand Up @@ -367,7 +359,7 @@ public boolean isAssignedToDomain() {
}

public boolean isAuthenticatedWithServiceAccount() {
return ServiceAccountSettings.REALM_TYPE.equals(getAuthenticatedBy().getType());
return ServiceAccountSettings.REALM_TYPE.equals(getAuthenticatingSubject().getRealm().getType());
}

/**
Expand Down Expand Up @@ -568,12 +560,12 @@ public void toXContentFragment(XContentBuilder builder) throws IOException {
builder.field(User.Fields.METADATA.getPreferredName(), user.metadata());
builder.field(User.Fields.ENABLED.getPreferredName(), user.enabled());
builder.startObject(User.Fields.AUTHENTICATION_REALM.getPreferredName());
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatedBy().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatedBy().getType());
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatingSubject().getRealm().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatingSubject().getRealm().getType());
// domain name is generally ambiguous, because it can change during the lifetime of the authentication,
// but it is good enough for display purposes (including auditing)
if (getAuthenticatedBy().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatedBy().getDomain().name());
if (getAuthenticatingSubject().getRealm().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatingSubject().getRealm().getDomain().name());
}
builder.endObject();
builder.startObject(User.Fields.LOOKUP_REALM.getPreferredName());
Expand All @@ -584,10 +576,10 @@ public void toXContentFragment(XContentBuilder builder) throws IOException {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getLookedUpBy().getDomain().name());
}
} else {
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatedBy().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatedBy().getType());
if (getAuthenticatedBy().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatedBy().getDomain().name());
builder.field(User.Fields.REALM_NAME.getPreferredName(), getAuthenticatingSubject().getRealm().getName());
builder.field(User.Fields.REALM_TYPE.getPreferredName(), getAuthenticatingSubject().getRealm().getType());
if (getAuthenticatingSubject().getRealm().getDomain() != null) {
builder.field(User.Fields.REALM_DOMAIN.getPreferredName(), getAuthenticatingSubject().getRealm().getDomain().name());
}
}
builder.endObject();
Expand Down
2 changes: 1 addition & 1 deletion ...n/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,7 +224,7 @@ public void testIsServiceAccount() {
authentication = AuthenticationTestHelper.builder().serviceAccount().build();
} else {
authentication = randomValueOtherThanMany(
authc -> "_service_account".equals(authc.getAuthenticatedBy().getName()),
authc -> "_service_account".equals(authc.getAuthenticatingSubject().getRealm().getName()),
() -> AuthenticationTestHelper.builder().build()
);
}
Expand Down
4 changes: 2 additions & 2 deletions ...usterTest/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,8 +745,8 @@ public void testOperationsOnReservedUsers() throws Exception {
Collections.singletonMap("Authorization", basicAuthHeaderValue(username, getReservedPassword()))
).execute(AuthenticateAction.INSTANCE, AuthenticateRequest.INSTANCE).get();
assertThat(authenticateResponse.authentication().getEffectiveSubject().getUser().principal(), is(username));
assertThat(authenticateResponse.authentication().getAuthenticatedBy().getName(), equalTo("reserved"));
assertThat(authenticateResponse.authentication().getAuthenticatedBy().getType(), equalTo("reserved"));
assertThat(authenticateResponse.authentication().getAuthenticatingSubject().getRealm().getName(), equalTo("reserved"));
assertThat(authenticateResponse.authentication().getAuthenticatingSubject().getRealm().getType(), equalTo("reserved"));
assertNull(authenticateResponse.authentication().getLookedUpBy());
}

Expand Down
6 changes: 3 additions & 3 deletions ...java/org/elasticsearch/xpack/security/action/oidc/TransportOpenIdConnectLogoutAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ protected void doExecute(Task task, OpenIdConnectLogoutRequest request, ActionLi

private OpenIdConnectLogoutResponse buildResponse(Authentication authentication, Map<String, Object> tokenMetadata) {
final String idTokenHint = (String) getFromMetadata(tokenMetadata, "id_token_hint");
final Realm realm = this.realms.realm(authentication.getAuthenticatedBy().getName());
final Realm realm = this.realms.realm(authentication.getAuthenticatingSubject().getRealm().getName());
final JWT idToken;
try {
idToken = JWTParser.parse(idTokenHint);
Expand All @@ -108,11 +108,11 @@ private void validateAuthenticationAndMetadata(Authentication authentication, Ma
throw new ElasticsearchSecurityException("No active user");
}

final Authentication.RealmRef ref = authentication.getAuthenticatedBy();
final Authentication.RealmRef ref = authentication.getAuthenticatingSubject().getRealm();
if (ref == null || Strings.isNullOrEmpty(ref.getName())) {
throw new ElasticsearchSecurityException("Authentication {} has no authenticating realm", authentication);
}
final Realm realm = this.realms.realm(authentication.getAuthenticatedBy().getName());
final Realm realm = this.realms.realm(authentication.getAuthenticatingSubject().getRealm().getName());
if (realm == null) {
throw new ElasticsearchSecurityException("Authenticating realm {} does not exist", ref.getName());
}
Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/org/elasticsearch/xpack/security/action/saml/TransportSamlLogoutAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ private String getMetadataString(Map<String, Object> metadata, String key) {
}

private SamlRealm findRealm(Authentication authentication) {
final Authentication.RealmRef ref = authentication.getAuthenticatedBy();
final Authentication.RealmRef ref = authentication.getAuthenticatingSubject().getRealm();
if (ref == null || Strings.isNullOrEmpty(ref.getName())) {
throw SamlUtils.samlException("Authentication {} has no authenticating realm", authentication);
}
Expand Down
10 changes: 5 additions & 5 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ public void authenticationSuccess(String requestId, Authentication authenticatio
)
) == false) {
// this is redundant information maintained for bwc purposes
final String authnRealm = authentication.getAuthenticatedBy().getName();
final String authnRealm = authentication.getAuthenticatingSubject().getRealm().getName();
new LogEntryBuilder().with(EVENT_TYPE_FIELD_NAME, REST_ORIGIN_FIELD_VALUE)
.with(EVENT_ACTION_FIELD_NAME, "authentication_success")
.with(REALM_FIELD_NAME, authnRealm)
Expand Down Expand Up @@ -1531,10 +1531,10 @@ LogEntryBuilder withRestUriAndMethod(RestRequest request) {

LogEntryBuilder withRunAsSubject(Authentication authentication) {
logEntry.with(PRINCIPAL_FIELD_NAME, authentication.getAuthenticatingSubject().getUser().principal())
.with(PRINCIPAL_REALM_FIELD_NAME, authentication.getAuthenticatedBy().getName())
.with(PRINCIPAL_REALM_FIELD_NAME, authentication.getAuthenticatingSubject().getRealm().getName())
.with(PRINCIPAL_RUN_AS_FIELD_NAME, authentication.getEffectiveSubject().getUser().principal());
if (authentication.getAuthenticatedBy().getDomain() != null) {
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, authentication.getAuthenticatedBy().getDomain().name());
if (authentication.getAuthenticatingSubject().getRealm().getDomain() != null) {
logEntry.with(PRINCIPAL_DOMAIN_FIELD_NAME, authentication.getAuthenticatingSubject().getRealm().getDomain().name());
}
if (authentication.getLookedUpBy() != null) {
logEntry.with(PRINCIPAL_RUN_AS_REALM_FIELD_NAME, authentication.getLookedUpBy().getName());
Expand Down Expand Up @@ -1625,7 +1625,7 @@ LogEntryBuilder withAuthentication(Authentication authentication) {
// No domain information is needed here since API key itself does not work across realms
}
} else {
final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatedBy();
final Authentication.RealmRef authenticatedBy = authentication.getAuthenticatingSubject().getRealm();
if (authentication.isRunAs()) {
final Authentication.RealmRef lookedUpBy = authentication.getLookedUpBy();
logEntry.with(PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName())
Expand Down
33 changes: 18 additions & 15 deletions ...ck/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1513,17 +1513,20 @@ private static Optional<ElasticsearchSecurityException> checkClientCanRefresh(
clientAuthentication.getEffectiveSubject().getUser().principal()
);
return Optional.of(invalidGrantException("tokens must be refreshed by the creating client"));
} else if (clientAuthentication.getAuthenticatedBy().getName().equals(refreshToken.getAssociatedRealm()) == false) {
logger.warn(
"[{}] created the refresh token while authenticated by [{}] but is now authenticated by [{}]",
refreshToken.getAssociatedUser(),
refreshToken.getAssociatedRealm(),
clientAuthentication.getAuthenticatedBy().getName()
);
return Optional.of(invalidGrantException("tokens must be refreshed by the creating client"));
} else {
return Optional.empty();
}
} else if (clientAuthentication.getAuthenticatingSubject()
.getRealm()
.getName()
.equals(refreshToken.getAssociatedRealm()) == false) {
Comment on lines +1516 to +1519
Copy link
Member Author

@ywangd ywangd Oct 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is inconsistent, i.e. it first checks effective user but then checks authenticating realm. But it is an existing bug (the token was created by capturing effective user + authenticating realm). So this is kept as is. Also it does not really matter much in practice because we now capture full authentication object for new tokens and these logics are not really used much.

logger.warn(
"[{}] created the refresh token while authenticated by [{}] but is now authenticated by [{}]",
refreshToken.getAssociatedUser(),
refreshToken.getAssociatedRealm(),
clientAuthentication.getAuthenticatingSubject().getRealm().getName()
);
return Optional.of(invalidGrantException("tokens must be refreshed by the creating client"));
} else {
return Optional.empty();
}
}
}

Expand Down Expand Up @@ -1795,9 +1798,9 @@ static BytesReference createTokenDocument(
builder.field("authentication", originatingClientAuth.maybeRewriteForOlderVersion(userToken.getVersion()).encode());
} else {
builder.field("user", originatingClientAuth.getEffectiveSubject().getUser().principal())
.field("realm", originatingClientAuth.getAuthenticatedBy().getName());
if (originatingClientAuth.getAuthenticatedBy().getDomain() != null) {
builder.field("realm_domain", originatingClientAuth.getAuthenticatedBy().getDomain());
.field("realm", originatingClientAuth.getAuthenticatingSubject().getRealm().getName());
if (originatingClientAuth.getAuthenticatingSubject().getRealm().getDomain() != null) {
builder.field("realm_domain", originatingClientAuth.getAuthenticatingSubject().getRealm().getDomain());
}
}
builder.endObject().endObject();
Expand Down Expand Up @@ -2546,7 +2549,7 @@ static final class RefreshTokenStatus {
this.invalidated = invalidated;
// not used, filled-in for consistency's sake
this.associatedUser = associatedAuthentication.getEffectiveSubject().getUser().principal();
this.associatedRealm = associatedAuthentication.getAuthenticatedBy().getName();
this.associatedRealm = associatedAuthentication.getAuthenticatingSubject().getRealm().getName();
this.associatedAuthentication = associatedAuthentication;
this.refreshed = refreshed;
this.refreshInstant = refreshInstant;
Expand Down
2 changes: 1 addition & 1 deletion ...ck/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/pki/PkiRealm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ private void buildUser(X509AuthenticationToken token, String principal, ActionLi
"pki_delegated_by_user",
token.getDelegateeAuthentication().getEffectiveSubject().getUser().principal(),
"pki_delegated_by_realm",
token.getDelegateeAuthentication().getAuthenticatedBy().getName()
token.getDelegateeAuthentication().getAuthenticatingSubject().getRealm().getName()
);
} else {
metadata = Map.of("pki_dn", token.dn());
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,7 +866,7 @@ private static boolean checkChangePasswordAction(Authentication authentication)
if (isRunAs) {
realmType = authentication.getLookedUpBy().getType();
} else {
realmType = authentication.getAuthenticatedBy().getType();
realmType = authentication.getAuthenticatingSubject().getRealm().getType();
}

assert realmType != null;
Expand Down
2 changes: 1 addition & 1 deletion .../plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityContextTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ public void testExecuteAfterRewritingAuthentication() throws IOException {
securityContext.executeAfterRewritingAuthentication(originalCtx -> {
Authentication authentication = securityContext.getAuthentication();
assertEquals(original.getEffectiveSubject().getUser(), authentication.getEffectiveSubject().getUser());
assertEquals(original.getAuthenticatedBy(), authentication.getAuthenticatedBy());
assertEquals(original.getAuthenticatingSubject().getRealm(), authentication.getAuthenticatingSubject().getRealm());
assertEquals(original.getLookedUpBy(), authentication.getLookedUpBy());
assertEquals(VersionUtils.getPreviousVersion(), authentication.getEffectiveSubject().getVersion());
assertEquals(original.getAuthenticationType(), securityContext.getAuthentication().getAuthenticationType());
Expand Down
4 changes: 2 additions & 2 deletions ...t/java/org/elasticsearch/xpack/security/action/user/TransportAuthenticateActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ public void onFailure(Exception e) {
if (auth.isRunAs()) {
assertThat(auth.getAuthenticatingSubject().getUser(), sameInstance(authentication.getAuthenticatingSubject().getUser()));
}
assertThat(auth.getAuthenticatedBy(), sameInstance(auth.getAuthenticatedBy()));
assertThat(auth.getAuthenticatingSubject().getRealm(), sameInstance(auth.getAuthenticatingSubject().getRealm()));
assertThat(auth.getLookedUpBy(), sameInstance(auth.getLookedUpBy()));
assertThat(auth.getEffectiveSubject().getVersion(), sameInstance(auth.getEffectiveSubject().getVersion()));
assertThat(auth.getAuthenticationType(), sameInstance(auth.getAuthenticationType()));
Expand Down Expand Up @@ -196,7 +196,7 @@ public void onFailure(Exception e) {
final Authentication auth = responseRef.get().authentication();
final User authUser = auth.getEffectiveSubject().getUser();
assertThat(authUser.roles(), emptyArray());
assertThat(auth.getAuthenticatedBy(), sameInstance(auth.getAuthenticatedBy()));
assertThat(auth.getAuthenticatingSubject().getRealm(), sameInstance(auth.getAuthenticatingSubject().getRealm()));
assertThat(auth.getLookedUpBy(), sameInstance(auth.getLookedUpBy()));
assertThat(auth.getEffectiveSubject().getVersion(), sameInstance(auth.getEffectiveSubject().getVersion()));
assertThat(auth.getAuthenticationType(), sameInstance(auth.getAuthenticationType()));
Expand Down
4 changes: 2 additions & 2 deletions .../src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2454,7 +2454,7 @@ public void testAuthenticationSuccessRest() throws Exception {
auditTrail.authenticationSuccess(requestId, authentication, request);
checkedFields.put(LoggingAuditTrail.EVENT_TYPE_FIELD_NAME, LoggingAuditTrail.REST_ORIGIN_FIELD_VALUE)
.put(LoggingAuditTrail.EVENT_ACTION_FIELD_NAME, "authentication_success")
.put(LoggingAuditTrail.REALM_FIELD_NAME, authentication.getAuthenticatedBy().getName())
.put(LoggingAuditTrail.REALM_FIELD_NAME, authentication.getAuthenticatingSubject().getRealm().getName())
.put(LoggingAuditTrail.ORIGIN_TYPE_FIELD_NAME, LoggingAuditTrail.REST_ORIGIN_FIELD_VALUE)
.put(LoggingAuditTrail.ORIGIN_ADDRESS_FIELD_NAME, NetworkAddress.format(address))
.put(LoggingAuditTrail.REQUEST_METHOD_FIELD_NAME, request.method().toString())
Expand Down Expand Up @@ -2893,7 +2893,7 @@ private static void authentication(Authentication authentication, MapBuilder<Str
checkedFields.put(LoggingAuditTrail.PRINCIPAL_REALM_FIELD_NAME, creatorRealmName);
}
} else {
final RealmRef authenticatedBy = authentication.getAuthenticatedBy();
final RealmRef authenticatedBy = authentication.getAuthenticatingSubject().getRealm();
if (authentication.isRunAs()) {
final RealmRef lookedUpBy = authentication.getLookedUpBy();
checkedFields.put(LoggingAuditTrail.PRINCIPAL_REALM_FIELD_NAME, lookedUpBy.getName())
Expand Down
Loading