فهرست منبع

Query API Keys support for both `aggs` and `aggregations` keywords (#107054)

The Query API Key Information endpoint supports aggs since #104895.
But some lang clients actually use the `aggregations` keyword in requests,
as the preferred synonym to `aggs`.
This PR adds support for the `aggregations` request keyword as a synonym
for the existing `aggs` term.

Closes #106839
Albert Zaharovits 1 سال پیش
والد
کامیت
36bcb6b398

+ 6 - 0
docs/changelog/107054.yaml

@@ -0,0 +1,6 @@
+pr: 107054
+summary: Query API Keys support for both `aggs` and `aggregations` keywords
+area: Security
+type: enhancement
+issues:
+ - 106839

+ 1 - 1
docs/reference/rest-api/security/query-api-key.asciidoc

@@ -232,7 +232,7 @@ simply mentioning `metadata` (not followed by any dot and sub-field name).
 NOTE: You cannot query the role descriptors of an API key.
 ====
 
-`aggs`::
+`aggs` or `aggregations`::
 (Optional, object) Any <<search-aggregations,aggregations>> to run over the corpus of returned API keys.
 Aggregations and queries work together. Aggregations are computed only on the API keys that match the query.
 This supports only a subset of aggregation types, namely: <<search-aggregations-bucket-terms-aggregation,terms>>,

+ 4 - 4
x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/ApiKeyAggsIT.java

@@ -98,7 +98,7 @@ public class ApiKeyAggsIT extends SecurityInBasicRestTestCase {
         // other bucket
         assertAggs(API_KEY_USER_AUTH_HEADER, typedAggs, """
             {
-              "aggs": {
+              "aggregations": {
                 "only_user_keys": {
                   "filters": {
                     "other_bucket_key": "other_user_keys",
@@ -267,7 +267,7 @@ public class ApiKeyAggsIT extends SecurityInBasicRestTestCase {
                           "good-api-key-invalidated": { "term": {"invalidated": false}}
                         }
                       },
-                      "aggs": {
+                      "aggregations": {
                         "wrong-field": {
                           "filters": {
                             "filters": {
@@ -487,7 +487,7 @@ public class ApiKeyAggsIT extends SecurityInBasicRestTestCase {
                       { "usernames": { "terms": { "field": "username" } } }
                     ]
                   },
-                  "aggs": {
+                  "aggregations": {
                     "not_expired": {
                       "filter": {
                         "range": {
@@ -564,7 +564,7 @@ public class ApiKeyAggsIT extends SecurityInBasicRestTestCase {
             );
             request.setJsonEntity("""
                 {
-                  "aggs": {
+                  "aggregations": {
                     "all_.security_docs": {
                       "global": {},
                       "aggs": {

+ 19 - 9
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/action/apikey/RestQueryApiKeyAction.java

@@ -36,6 +36,8 @@ import static org.elasticsearch.index.query.AbstractQueryBuilder.parseTopLevelQu
 import static org.elasticsearch.rest.RestRequest.Method.GET;
 import static org.elasticsearch.rest.RestRequest.Method.POST;
 import static org.elasticsearch.search.aggregations.AggregatorFactories.parseAggregators;
+import static org.elasticsearch.search.builder.SearchSourceBuilder.AGGREGATIONS_FIELD;
+import static org.elasticsearch.search.builder.SearchSourceBuilder.AGGS_FIELD;
 import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;
 
 /**
@@ -47,19 +49,27 @@ public final class RestQueryApiKeyAction extends ApiKeyBaseRestHandler {
     @SuppressWarnings("unchecked")
     private static final ConstructingObjectParser<Payload, Void> PARSER = new ConstructingObjectParser<>(
         "query_api_key_request_payload",
-        a -> new Payload(
-            (QueryBuilder) a[0],
-            (AggregatorFactories.Builder) a[1],
-            (Integer) a[2],
-            (Integer) a[3],
-            (List<FieldSortBuilder>) a[4],
-            (SearchAfterBuilder) a[5]
-        )
+        a -> {
+            if (a[1] != null && a[2] != null) {
+                throw new IllegalArgumentException("Duplicate 'aggs' or 'aggregations' field");
+            } else {
+                return new Payload(
+                    (QueryBuilder) a[0],
+                    (AggregatorFactories.Builder) (a[1] != null ? a[1] : a[2]),
+                    (Integer) a[3],
+                    (Integer) a[4],
+                    (List<FieldSortBuilder>) a[5],
+                    (SearchAfterBuilder) a[6]
+                );
+            }
+        }
     );
 
     static {
         PARSER.declareObject(optionalConstructorArg(), (p, c) -> parseTopLevelQuery(p), new ParseField("query"));
-        PARSER.declareObject(optionalConstructorArg(), (p, c) -> parseAggregators(p), new ParseField("aggs"));
+        // only one of aggs or aggregations is allowed
+        PARSER.declareObject(optionalConstructorArg(), (p, c) -> parseAggregators(p), AGGREGATIONS_FIELD);
+        PARSER.declareObject(optionalConstructorArg(), (p, c) -> parseAggregators(p), AGGS_FIELD);
         PARSER.declareInt(optionalConstructorArg(), new ParseField("from"));
         PARSER.declareInt(optionalConstructorArg(), new ParseField("size"));
         PARSER.declareObjectArray(optionalConstructorArg(), (p, c) -> {

+ 66 - 0
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/action/apikey/RestQueryApiKeyActionTests.java

@@ -13,6 +13,7 @@ import org.elasticsearch.action.ActionRequest;
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.action.ActionType;
 import org.elasticsearch.client.internal.node.NodeClient;
+import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.env.Environment;
@@ -35,6 +36,7 @@ import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.rest.FakeRestRequest;
 import org.elasticsearch.threadpool.ThreadPool;
 import org.elasticsearch.xcontent.NamedXContentRegistry;
+import org.elasticsearch.xcontent.XContentParseException;
 import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.xcontent.XContentType;
 import org.elasticsearch.xpack.core.security.action.apikey.ApiKey;
@@ -48,6 +50,7 @@ import java.util.List;
 import java.util.Map;
 
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
@@ -145,6 +148,69 @@ public class RestQueryApiKeyActionTests extends ESTestCase {
         assertNotNull(responseSetOnce.get());
     }
 
+    public void testAggsAndAggregationsTogether() {
+        String agg1;
+        String agg2;
+        if (randomBoolean()) {
+            agg1 = "aggs";
+            agg2 = "aggregations";
+        } else {
+            agg1 = "aggregations";
+            agg2 = "aggs";
+        }
+        final String requestBody = Strings.format("""
+            {
+              "%s": {
+                "all_keys_by_type": {
+                  "composite": {
+                    "sources": [
+                      { "type": { "terms": { "field": "type" } } }
+                    ]
+                  }
+                }
+              },
+              "%s": {
+                "type_cardinality": {
+                  "cardinality": {
+                    "field": "type"
+                  }
+                }
+              }
+            }""", agg1, agg2);
+
+        final FakeRestRequest restRequest = new FakeRestRequest.Builder(xContentRegistry()).withContent(
+            new BytesArray(requestBody),
+            XContentType.JSON
+        ).build();
+        final SetOnce<RestResponse> responseSetOnce = new SetOnce<>();
+        final RestChannel restChannel = new AbstractRestChannel(restRequest, randomBoolean()) {
+            @Override
+            public void sendResponse(RestResponse restResponse) {
+                responseSetOnce.set(restResponse);
+            }
+        };
+        final var client = new NodeClient(Settings.EMPTY, threadPool) {
+            @SuppressWarnings("unchecked")
+            @Override
+            public <Request extends ActionRequest, Response extends ActionResponse> void doExecute(
+                ActionType<Response> action,
+                Request request,
+                ActionListener<Response> listener
+            ) {
+                fail("TEST failed, request parsing should've failed");
+                listener.onResponse((Response) QueryApiKeyResponse.EMPTY);
+            }
+        };
+        RestQueryApiKeyAction restQueryApiKeyAction = new RestQueryApiKeyAction(Settings.EMPTY, mockLicenseState);
+        XContentParseException ex = expectThrows(
+            XContentParseException.class,
+            () -> restQueryApiKeyAction.handleRequest(restRequest, restChannel, client)
+        );
+        assertThat(ex.getCause().getMessage(), containsString("Duplicate 'aggs' or 'aggregations' field"));
+        assertThat(ex.getMessage(), containsString("Failed to build [query_api_key_request_payload]"));
+        assertNull(responseSetOnce.get());
+    }
+
     public void testParsingSearchParameters() throws Exception {
         final String requestBody = """
             {