Skip to content

Commit c3f90ef

Browse files
committed
Fix identification of action type in error message
Some actions that start with "indices:" are actually handled by cluster privileges in ES security (e.g. indices:admin/template/*) In elastic#60357 and elastic#66900 we added better context information for the error messages that are generated when an action is denied, but the generation of that message did not correctly classify actions between cluster and index level privileges. This change does 2 things: 1. It fixes the code that determines whether an action is handled by a cluster privilege or an index privilege 2. Includes the words "cluster" and "index" in the error message so that classification is clear to the reader The latter change is not directly related to the issue being resolved, but in the course of fixing the issue it became evident that the message lacked clarity because it did not tell the reader what type of privilege would be needed to resolve the access denied issue. Backport of: elastic#68260
1 parent 926eb91 commit c3f90ef

File tree

5 files changed

+159
-126
lines changed

5 files changed

+159
-126
lines changed

x-pack/plugin/ilm/qa/with-security/src/javaRestTest/java/org/elasticsearch/xpack/security/PermissionsIT.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,10 @@ public void testCanManageIndexWithNoPermissions() throws Exception {
144144
assertThat(indexExplain.get("failed_step"), equalTo("wait-for-shard-history-leases"));
145145
Map<String, String> stepInfo = (Map<String, String>) indexExplain.get("step_info");
146146
assertThat(stepInfo.get("type"), equalTo("security_exception"));
147-
assertThat(stepInfo.get("reason"), equalTo("action [indices:monitor/stats] is unauthorized for user [test_ilm]" +
148-
" on indices [not-ilm], this action is granted by the privileges [monitor,manage,all]"));
147+
assertThat(stepInfo.get("reason"), equalTo("action [indices:monitor/stats] is unauthorized" +
148+
" for user [test_ilm]" +
149+
" on indices [not-ilm]," +
150+
" this action is granted by the index privileges [monitor,manage,all]"));
149151
}
150152
});
151153
}

x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1257,14 +1257,14 @@ private Client getClientForRunAsUser() {
12571257
private void assertErrorMessage(final ElasticsearchSecurityException ese, String action, String userName, String apiKeyId) {
12581258
assertThat(ese, throwableWithMessage(
12591259
containsString("action [" + action + "] is unauthorized for API key id [" + apiKeyId + "] of user [" + userName + "]")));
1260-
assertThat(ese, throwableWithMessage(containsString(", this action is granted by the privileges [")));
1260+
assertThat(ese, throwableWithMessage(containsString(", this action is granted by the cluster privileges [")));
12611261
assertThat(ese, throwableWithMessage(containsString("manage_api_key,manage_security,all]")));
12621262
}
12631263

12641264
private void assertErrorMessage(final ElasticsearchSecurityException ese, String action, String userName) {
12651265
assertThat(ese, throwableWithMessage(
12661266
containsString("action [" + action + "] is unauthorized for user [" + userName + "]")));
1267-
assertThat(ese, throwableWithMessage(containsString(", this action is granted by the privileges [")));
1267+
assertThat(ese, throwableWithMessage(containsString(", this action is granted by the cluster privileges [")));
12681268
assertThat(ese, throwableWithMessage(containsString("manage_api_key,manage_security,all]")));
12691269
}
12701270
}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -637,15 +637,17 @@ private ElasticsearchSecurityException denialException(Authentication authentica
637637
message = message + " " + context;
638638
}
639639

640-
if (isIndexAction(action)) {
641-
final Collection<String> privileges = IndexPrivilege.findPrivilegesThatGrant(action);
640+
if (ClusterPrivilegeResolver.isClusterAction(action)) {
641+
final Collection<String> privileges = ClusterPrivilegeResolver.findPrivilegesThatGrant(action, request, authentication);
642642
if (privileges != null && privileges.size() > 0) {
643-
message = message + ", this action is granted by the privileges [" + collectionToCommaDelimitedString(privileges) + "]";
643+
message = message + ", this action is granted by the cluster privileges ["
644+
+ collectionToCommaDelimitedString(privileges) + "]";
644645
}
645-
} else if (ClusterPrivilegeResolver.isClusterAction(action)) {
646-
final Collection<String> privileges = ClusterPrivilegeResolver.findPrivilegesThatGrant(action, request, authentication);
646+
} else if (isIndexAction(action)) {
647+
final Collection<String> privileges = IndexPrivilege.findPrivilegesThatGrant(action);
647648
if (privileges != null && privileges.size() > 0) {
648-
message = message + ", this action is granted by the privileges [" + collectionToCommaDelimitedString(privileges) + "]";
649+
message = message + ", this action is granted by the index privileges ["
650+
+ collectionToCommaDelimitedString(privileges) + "]";
649651
}
650652
}
651653

0 commit comments

Comments
 (0)