Skip to content

Commit 44f5b4b

Browse files
ywangdywangd
committed
address feedback, simplify changes
1 parent c74cc95 commit 44f5b4b

File tree

14 files changed

+83
-153
lines changed

14 files changed

+83
-153
lines changed

‎server/src/main/java/org/elasticsearch/Version.java‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -452,19 +452,6 @@ public boolean isCompatible(Version version) {
452452
return compatible;
453453
}
454454

455-
/**
456-
* Get the released version that is before the given version
457-
*/
458-
public Version getPreviousVersion() {
459-
for (int i = DeclaredVersionsHolder.DECLARED_VERSIONS.size() - 1; i >= 0; i--) {
460-
final Version version = DeclaredVersionsHolder.DECLARED_VERSIONS.get(i);
461-
if (version.isRelease() && version.before(this)) {
462-
return version;
463-
}
464-
}
465-
throw new IllegalArgumentException("cannot find a released version before [" + this + "]");
466-
}
467-
468455
@SuppressForbidden(reason = "System.out.*")
469456
public static void main(String[] args) {
470457
final String versionOutput = String.format(

‎server/src/test/java/org/elasticsearch/VersionTests.java‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -388,13 +388,4 @@ public void testUnreleasedVersion() {
388388
VersionTests.assertUnknownVersion(VERSION_5_1_0_UNRELEASED);
389389
}
390390

391-
public void testPreviousVersion() {
392-
assertThat(Version.V_7_13_0.getPreviousVersion(), is(Version.V_7_12_1));
393-
assertThat(Version.V_7_12_1.getPreviousVersion(), is(Version.V_7_12_0));
394-
assertThat(Version.V_7_12_0.getPreviousVersion(), is(Version.V_7_11_2));
395-
assertThat(Version.V_7_11_2.getPreviousVersion(), is(Version.V_7_11_1));
396-
assertThat(Version.V_7_11_1.getPreviousVersion(), is(Version.V_7_11_0));
397-
final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, Version.V_6_0_0::getPreviousVersion);
398-
assertThat(e.getMessage(), containsString("cannot find a released version before [6.0.0]"));
399-
}
400391
}

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,6 @@
200200
import org.elasticsearch.xpack.security.authc.TokenService;
201201
import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore;
202202
import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm;
203-
import org.elasticsearch.xpack.security.authc.support.ApiKeyGenerator;
204203
import org.elasticsearch.xpack.security.authc.support.SecondaryAuthenticator;
205204
import org.elasticsearch.xpack.security.authc.support.mapper.NativeRoleMappingStore;
206205
import org.elasticsearch.xpack.security.authz.AuthorizationService;
@@ -532,8 +531,6 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
532531
dlsBitsetCache.get(), new DeprecationRoleDescriptorConsumer(clusterService, threadPool));
533532
securityIndex.get().addIndexStateListener(allRolesStore::onSecurityIndexStateChange);
534533

535-
components.add(new ApiKeyGenerator(apiKeyService, allRolesStore, clusterService, xContentRegistry));
536-
537534
// to keep things simple, just invalidate all cached entries on license change. this happens so rarely that the impact should be
538535
// minimal
539536
getLicenseState().addListener(allRolesStore::invalidateAll);
@@ -1248,7 +1245,7 @@ private static SystemIndexDescriptor getSecurityMainIndexDescriptor() {
12481245
.setIndexPattern(".security-[0-9]+")
12491246
.setPrimaryIndex(RestrictedIndicesNames.INTERNAL_SECURITY_MAIN_INDEX_7)
12501247
.setDescription("Contains Security configuration")
1251-
.setMappings(getIndexMappings(Version.V_7_13_0.getPreviousVersion()))
1248+
.setMappings(getIndexMappings(Version.V_7_12_0))
12521249
.setSettings(getIndexSettings())
12531250
.setAliasName(SECURITY_MAIN_ALIAS)
12541251
.setIndexFormat(INTERNAL_MAIN_INDEX_FORMAT)

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,14 +11,17 @@
1111
import org.elasticsearch.action.support.ActionFilters;
1212
import org.elasticsearch.action.support.HandledTransportAction;
1313
import org.elasticsearch.common.inject.Inject;
14+
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
1415
import org.elasticsearch.tasks.Task;
1516
import org.elasticsearch.transport.TransportService;
1617
import org.elasticsearch.xpack.core.security.SecurityContext;
1718
import org.elasticsearch.xpack.core.security.action.CreateApiKeyAction;
1819
import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
1920
import org.elasticsearch.xpack.core.security.action.CreateApiKeyResponse;
2021
import org.elasticsearch.xpack.core.security.authc.Authentication;
22+
import org.elasticsearch.xpack.security.authc.ApiKeyService;
2123
import org.elasticsearch.xpack.security.authc.support.ApiKeyGenerator;
24+
import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore;
2225

2326
/**
2427
* Implementation of the action needed to create an API key
@@ -29,10 +32,10 @@ public final class TransportCreateApiKeyAction extends HandledTransportAction<Cr
2932
private final SecurityContext securityContext;
3033

3134
@Inject
32-
public TransportCreateApiKeyAction(TransportService transportService, ActionFilters actionFilters,
33-
SecurityContext context, ApiKeyGenerator apiKeyGenerator) {
35+
public TransportCreateApiKeyAction(TransportService transportService, ActionFilters actionFilters, ApiKeyService apiKeyService,
36+
SecurityContext context, CompositeRolesStore rolesStore, NamedXContentRegistry xContentRegistry) {
3437
super(CreateApiKeyAction.NAME, transportService, actionFilters, CreateApiKeyRequest::new);
35-
this.generator = apiKeyGenerator;
38+
this.generator = new ApiKeyGenerator(apiKeyService, rolesStore, xContentRegistry);
3639
this.securityContext = context;
3740
}
3841

@@ -45,4 +48,5 @@ protected void doExecute(Task task, CreateApiKeyRequest request, ActionListener<
4548
generator.generateApiKey(authentication, request, listener);
4649
}
4750
}
51+
4852
}

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportGrantApiKeyAction.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.elasticsearch.action.support.HandledTransportAction;
1414
import org.elasticsearch.common.inject.Inject;
1515
import org.elasticsearch.common.util.concurrent.ThreadContext;
16+
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
1617
import org.elasticsearch.tasks.Task;
1718
import org.elasticsearch.threadpool.ThreadPool;
1819
import org.elasticsearch.transport.TransportRequest;
@@ -22,9 +23,11 @@
2223
import org.elasticsearch.xpack.core.security.action.GrantApiKeyRequest;
2324
import org.elasticsearch.xpack.core.security.authc.Authentication;
2425
import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken;
26+
import org.elasticsearch.xpack.security.authc.ApiKeyService;
2527
import org.elasticsearch.xpack.security.authc.AuthenticationService;
2628
import org.elasticsearch.xpack.security.authc.TokenService;
2729
import org.elasticsearch.xpack.security.authc.support.ApiKeyGenerator;
30+
import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore;
2831

2932
/**
3033
* Implementation of the action needed to create an API key on behalf of another user (using an OAuth style "grant")
@@ -38,10 +41,10 @@ public final class TransportGrantApiKeyAction extends HandledTransportAction<Gra
3841

3942
@Inject
4043
public TransportGrantApiKeyAction(TransportService transportService, ActionFilters actionFilters, ThreadPool threadPool,
41-
AuthenticationService authenticationService, TokenService tokenService,
42-
ApiKeyGenerator apiKeyGenerator) {
44+
ApiKeyService apiKeyService, AuthenticationService authenticationService, TokenService tokenService,
45+
CompositeRolesStore rolesStore, NamedXContentRegistry xContentRegistry) {
4346
this(transportService, actionFilters, threadPool.getThreadContext(),
44-
apiKeyGenerator, authenticationService, tokenService
47+
new ApiKeyGenerator(apiKeyService, rolesStore, xContentRegistry), authenticationService, tokenService
4548
);
4649
}
4750

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

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,7 @@ public class ApiKeyService {
141141

142142
private static final Logger logger = LogManager.getLogger(ApiKeyService.class);
143143
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(ApiKeyService.class);
144+
private static final Version METADATA_INTRODUCED = Version.V_7_13_0;
144145
public static final String API_KEY_ID_KEY = "_security_api_key_id";
145146
public static final String API_KEY_NAME_KEY = "_security_api_key_name";
146147
public static final String API_KEY_METADATA_KEY = "_security_api_key_metadata";
@@ -250,6 +251,13 @@ public void invalidateAll() {
250251
public void createApiKey(Authentication authentication, CreateApiKeyRequest request, Set<RoleDescriptor> userRoles,
251252
ActionListener<CreateApiKeyResponse> listener) {
252253
ensureEnabled();
254+
if (request.getMetadata() != null && false == request.getMetadata().isEmpty()) {
255+
if (securityIndex.getInstallableMappingVersion().before(METADATA_INTRODUCED)) {
256+
listener.onFailure(new IllegalArgumentException("API metadata requires all nodes to be at least ["
257+
+ FLATTENED_FIELD_TYPE_INTRODUCED + "]"));
258+
return;
259+
}
260+
}
253261
if (authentication == null) {
254262
listener.onFailure(new IllegalArgumentException("authentication must be provided"));
255263
} else {
@@ -337,13 +345,11 @@ XContentBuilder newDocument(SecureString apiKey, String name, Authentication aut
337345
builder.field("name", name)
338346
.field("version", version.id);
339347

340-
final Version smallestNonClientNodeVersion = clusterService.state().nodes().getSmallestNonClientNodeVersion();
341-
if (smallestNonClientNodeVersion.onOrAfter(FLATTENED_FIELD_TYPE_INTRODUCED)) {
348+
if (metadata != null && false == metadata.isEmpty()) {
349+
// The check in createApiKey method should prevent this assertion from tripping
350+
assert securityIndex.getInstallableMappingVersion().onOrAfter(METADATA_INTRODUCED)
351+
: "API metadata requires all nodes to be at least [" + FLATTENED_FIELD_TYPE_INTRODUCED + "]";
342352
builder.field("metadata_flattened", metadata);
343-
} else {
344-
// The assertion should not trip because ApiKeyGenerator should prevent it
345-
assert metadata == null || metadata.isEmpty()
346-
: "API key metadata requires all nodes to be at least [" + FLATTENED_FIELD_TYPE_INTRODUCED + "]";
347353
}
348354

349355
builder.startObject("creator")

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/ApiKeyGenerator.java‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@
99

1010
import org.elasticsearch.ElasticsearchException;
1111
import org.elasticsearch.ElasticsearchSecurityException;
12-
import org.elasticsearch.Version;
1312
import org.elasticsearch.action.ActionListener;
14-
import org.elasticsearch.cluster.service.ClusterService;
1513
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
1614
import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
1715
import org.elasticsearch.xpack.core.security.action.CreateApiKeyResponse;
@@ -24,23 +22,18 @@
2422
import java.util.Arrays;
2523
import java.util.HashSet;
2624

27-
import static org.elasticsearch.xpack.security.Security.FLATTENED_FIELD_TYPE_INTRODUCED;
28-
2925
/**
3026
* Utility class for generating API keys for a provided {@link Authentication}.
3127
*/
3228
public class ApiKeyGenerator {
3329

3430
private final ApiKeyService apiKeyService;
3531
private final CompositeRolesStore rolesStore;
36-
private final ClusterService clusterService;
3732
private final NamedXContentRegistry xContentRegistry;
3833

39-
public ApiKeyGenerator(ApiKeyService apiKeyService, CompositeRolesStore rolesStore, ClusterService clusterService,
40-
NamedXContentRegistry xContentRegistry) {
34+
public ApiKeyGenerator(ApiKeyService apiKeyService, CompositeRolesStore rolesStore, NamedXContentRegistry xContentRegistry) {
4135
this.apiKeyService = apiKeyService;
4236
this.rolesStore = rolesStore;
43-
this.clusterService = clusterService;
4437
this.xContentRegistry = xContentRegistry;
4538
}
4639

@@ -50,14 +43,6 @@ public void generateApiKey(Authentication authentication, CreateApiKeyRequest re
5043
return;
5144
}
5245
apiKeyService.ensureEnabled();
53-
if (request.getMetadata() != null && false == request.getMetadata().isEmpty()) {
54-
final Version smallestNonClientNodeVersion = clusterService.state().nodes().getSmallestNonClientNodeVersion();
55-
if (smallestNonClientNodeVersion.before(FLATTENED_FIELD_TYPE_INTRODUCED)) {
56-
listener.onFailure(new IllegalArgumentException("API metadata requires all nodes to be at least ["
57-
+ FLATTENED_FIELD_TYPE_INTRODUCED + "], got [" + smallestNonClientNodeVersion + "]"));
58-
return;
59-
}
60-
}
6146
if (Authentication.AuthenticationType.API_KEY == authentication.getAuthenticationType() && grantsAnyPrivileges(request)) {
6247
listener.onFailure(new IllegalArgumentException(
6348
"creating derived api keys requires an explicit role descriptor that is empty (has no privileges)"));

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,6 +421,12 @@ public void onFailure(Exception e) {
421421
}
422422
}
423423

424+
public Version getInstallableMappingVersion() {
425+
final State indexState = this.indexState; // use a local copy so all checks execute against the same state!
426+
final SystemIndexDescriptor descriptor = systemIndexDescriptor.getDescriptorCompatibleWith(indexState.minimumNodeVersion);
427+
return descriptor == null ? Version.V_EMPTY : descriptor.getMappingVersion();
428+
}
429+
424430
/**
425431
* Return true if the state moves from an unhealthy ("RED") index state to a healthy ("non-RED") state.
426432
*/

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailFilterTests.java‎

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,7 @@
1111
import org.elasticsearch.Version;
1212
import org.elasticsearch.action.support.IndicesOptions;
1313
import org.elasticsearch.client.Client;
14-
import org.elasticsearch.cluster.ClusterState;
1514
import org.elasticsearch.cluster.node.DiscoveryNode;
16-
import org.elasticsearch.cluster.node.DiscoveryNodes;
1715
import org.elasticsearch.cluster.service.ClusterService;
1816
import org.elasticsearch.common.Strings;
1917
import org.elasticsearch.common.settings.ClusterSettings;
@@ -96,13 +94,10 @@ public void init() throws Exception {
9694
arg0.updateLocalNodeInfo(localNode);
9795
return null;
9896
}).when(clusterService).addListener(Mockito.isA(LoggingAuditTrail.class));
99-
final ClusterState clusterState = mock(ClusterState.class);
100-
final DiscoveryNodes nodes = mock(DiscoveryNodes.class);
101-
when(nodes.getSmallestNonClientNodeVersion()).thenReturn(Version.CURRENT);
102-
when(clusterState.nodes()).thenReturn(nodes);
103-
when(clusterService.state()).thenReturn(clusterState);
97+
final SecurityIndexManager securityIndexManager = mock(SecurityIndexManager.class);
98+
when(securityIndexManager.getInstallableMappingVersion()).thenReturn(Version.CURRENT);
10499
apiKeyService = new ApiKeyService(settings, Clock.systemUTC(), mock(Client.class), new XPackLicenseState(settings, () -> 0),
105-
mock(SecurityIndexManager.class), clusterService,
100+
securityIndexManager, clusterService,
106101
mock(CacheInvalidatorRegistry.class), mock(ThreadPool.class));
107102
}
108103

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrailTests.java‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,7 @@
1515
import org.elasticsearch.action.support.IndicesOptions;
1616
import org.elasticsearch.action.support.WriteRequest;
1717
import org.elasticsearch.client.Client;
18-
import org.elasticsearch.cluster.ClusterState;
1918
import org.elasticsearch.cluster.node.DiscoveryNode;
20-
import org.elasticsearch.cluster.node.DiscoveryNodes;
2119
import org.elasticsearch.cluster.service.ClusterService;
2220
import org.elasticsearch.common.Strings;
2321
import org.elasticsearch.common.bytes.BytesArray;
@@ -262,6 +260,7 @@ public void init() throws Exception {
262260
when(localNode.getAddress()).thenReturn(buildNewFakeTransportAddress());
263261
Client client = mock(Client.class);
264262
SecurityIndexManager securityIndexManager = mock(SecurityIndexManager.class);
263+
when(securityIndexManager.getInstallableMappingVersion()).thenReturn(Version.CURRENT);
265264
clusterService = mock(ClusterService.class);
266265
when(clusterService.localNode()).thenReturn(localNode);
267266
Mockito.doAnswer((Answer) invocation -> {
@@ -278,11 +277,6 @@ public void init() throws Exception {
278277
LoggingAuditTrail.FILTER_POLICY_IGNORE_INDICES, LoggingAuditTrail.FILTER_POLICY_IGNORE_ACTIONS,
279278
Loggers.LOG_LEVEL_SETTING)));
280279
when(clusterService.getClusterSettings()).thenReturn(clusterSettings);
281-
final ClusterState clusterState = mock(ClusterState.class);
282-
final DiscoveryNodes nodes = mock(DiscoveryNodes.class);
283-
when(nodes.getSmallestNonClientNodeVersion()).thenReturn(Version.CURRENT);
284-
when(clusterState.nodes()).thenReturn(nodes);
285-
when(clusterService.state()).thenReturn(clusterState);
286280
commonFields = new LoggingAuditTrail.EntryCommonFields(settings, localNode).commonFields;
287281
threadContext = new ThreadContext(Settings.EMPTY);
288282
if (randomBoolean()) {

0 commit comments

Comments
 (0)