Fix bugs for unexpired API keys and id filtering (elastic#76208)

This PR fixed an old bug and a new bug introduced elastic#75335. Interestingly, the
two bugs somewhat cancelled each other in tests. In addition, the test setup
also contributed to the overall issue.

The old bug is about filtering out expired API keys, but the relationship was
wrong in the search query. The new bug is that _id field should be allowed in
the index level for the new API key search API. Because of the old bug, the
query always gets rewritten because the tests do not have any API keys that are
expired before the query time. The query rewriting effectively bypasses the _id
field check. Hence the new bug is not triggered.

Author: ywangd

x-pack/plugin/security/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/QueryApiKeyIT.java

@@ -279,9 +279,15 @@ private Tuple<String, String> createApiKey(String name,
             roleDescriptors == null ? org.elasticsearch.core.Map.of() : roleDescriptors, XContentType.JSON).utf8ToString();
         final String metadataString = XContentTestUtils.convertToXContent(
             metadata == null ? org.elasticsearch.core.Map.of() : metadata, XContentType.JSON).utf8ToString();
-        request.setJsonEntity("{\"name\":\"" + name
-            + "\", \"role_descriptors\":" + roleDescriptorsString
-            + ", \"metadata\":" + metadataString + "}");
+        if (randomBoolean()) {
+            request.setJsonEntity("{\"name\":\"" + name
+                + "\", \"role_descriptors\":" + roleDescriptorsString
+                + ", \"metadata\":" + metadataString + "}");
+        } else {
+            request.setJsonEntity("{\"name\":\"" + name
+                + "\", \"expiration\": \"10d\", \"role_descriptors\":" + roleDescriptorsString
+                + ", \"metadata\":" + metadataString + "}");
+        }
         request.setOptions(request.getOptions().toBuilder().addHeader(HttpHeaders.AUTHORIZATION, authHeader));
         final Response response = client().performRequest(request);
         assertOK(response);

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

@@ -0,0 +1,48 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0; you may not use this file except in compliance with the Elastic License
+ * 2.0.
+ */
+
+package org.elasticsearch.xpack.security.authc.apikey;
+
+import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.core.TimeValue;
+import org.elasticsearch.index.query.QueryBuilders;
+import org.elasticsearch.test.SecuritySingleNodeTestCase;
+import org.elasticsearch.xpack.core.XPackSettings;
+import org.elasticsearch.xpack.core.security.action.CreateApiKeyAction;
+import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
+import org.elasticsearch.xpack.core.security.action.apikey.QueryApiKeyAction;
+import org.elasticsearch.xpack.core.security.action.apikey.QueryApiKeyRequest;
+import org.elasticsearch.xpack.core.security.action.apikey.QueryApiKeyResponse;
+
+import static org.hamcrest.Matchers.equalTo;
+
+public class ApiKeySingleNodeTests extends SecuritySingleNodeTestCase {
+
+    @Override
+    protected Settings nodeSettings() {
+        Settings.Builder builder = Settings.builder().put(super.nodeSettings());
+        builder.put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true);
+        return builder.build();
+    }
+
+    public void testQueryWithExpiredKeys() throws InterruptedException {
+        final String id1 = client().execute(CreateApiKeyAction.INSTANCE,
+                new CreateApiKeyRequest("expired-shortly", null, TimeValue.timeValueMillis(1), null))
+            .actionGet()
+            .getId();
+        final String id2 = client().execute(CreateApiKeyAction.INSTANCE,
+                new CreateApiKeyRequest("long-lived", null, TimeValue.timeValueDays(1), null))
+            .actionGet()
+            .getId();
+        Thread.sleep(10); // just to be 100% sure that the 1st key is expired when we search for it
+
+        final QueryApiKeyRequest queryApiKeyRequest = new QueryApiKeyRequest(QueryBuilders.idsQuery().addIds(id1, id2));
+        final QueryApiKeyResponse queryApiKeyResponse = client().execute(QueryApiKeyAction.INSTANCE, queryApiKeyRequest).actionGet();
+        assertThat(queryApiKeyResponse.getApiKeyInfos().length, equalTo(1));
+        assertThat(queryApiKeyResponse.getApiKeyInfos()[0].getId(), equalTo(id2));
+    }
+}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

@@ -928,7 +928,7 @@ private void findApiKeys(final BoolQueryBuilder boolQuery, boolean filterOutInva
         }
         if (filterOutExpiredKeys) {
             final BoolQueryBuilder expiredQuery = QueryBuilders.boolQuery();
-            expiredQuery.should(QueryBuilders.rangeQuery("expiration_time").lte(Instant.now().toEpochMilli()));
+            expiredQuery.should(QueryBuilders.rangeQuery("expiration_time").gt(Instant.now().toEpochMilli()));
             expiredQuery.should(QueryBuilders.boolQuery().mustNot(QueryBuilders.existsQuery("expiration_time")));
             boolQuery.filter(expiredQuery);
         }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/ApiKeyBoolQueryBuilder.java

@@ -33,7 +33,7 @@ public class ApiKeyBoolQueryBuilder extends BoolQueryBuilder {
 
     // Field names allowed at the index level
     private static final Set<String> ALLOWED_EXACT_INDEX_FIELD_NAMES =
-        org.elasticsearch.core.Set.of("doc_type", "name", "api_key_invalidated", "creation_time", "expiration_time");
+        org.elasticsearch.core.Set.of("_id", "doc_type", "name", "api_key_invalidated", "creation_time", "expiration_time");
 
     private ApiKeyBoolQueryBuilder() {}