From 1998b7ef460ab0dc5804843160cdbdcfaacfd765 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Mon, 23 Jan 2017 15:44:40 +0100 Subject: [PATCH] Remove RequestContext from Security (elastic/elasticsearch#4710) RequestContext is a leftover from when we had no thread context. This commit removes the last place where it was used and uses the thread context instead. Original commit: elastic/x-pack-elasticsearch@50a2bff4005c9ab10fd731165e880f4c8729c3db --- .../xpack/security/Security.java | 5 +- .../authz/accesscontrol/OptOutQueryCache.java | 28 +++------ .../authz/accesscontrol/RequestContext.java | 61 ------------------- .../SecurityServerTransportInterceptor.java | 11 +--- 4 files changed, 13 insertions(+), 92 deletions(-) delete mode 100644 elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/RequestContext.java diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/Security.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/Security.java index bcd2c6ec949..04d05fb060a 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -176,6 +176,7 @@ public class Security implements ActionPlugin, IngestPlugin, NetworkPlugin { private final SetOnce ipFilter = new SetOnce<>(); private final SetOnce authcService = new SetOnce<>(); private final SetOnce securityContext = new SetOnce<>(); + private final SetOnce threadContext = new SetOnce<>(); public Security(Settings settings, Environment env, XPackLicenseState licenseState, SSLService sslService) throws IOException { this.settings = settings; @@ -246,7 +247,7 @@ public class Security implements ActionPlugin, IngestPlugin, NetworkPlugin { if (enabled == false) { return Collections.emptyList(); } - + threadContext.set(threadPool.getThreadContext()); List components = new ArrayList<>(); securityContext.set(new SecurityContext(settings, threadPool.getThreadContext(), cryptoService)); components.add(securityContext.get()); @@ -472,7 +473,7 @@ public class Security implements ActionPlugin, IngestPlugin, NetworkPlugin { * This impl. disabled the query cache if field level security is used for a particular request. If we wouldn't do * forcefully overwrite the query cache implementation then we leave the system vulnerable to leakages of data to * unauthorized users. */ - module.forceQueryCacheProvider(OptOutQueryCache::new); + module.forceQueryCacheProvider((settings, cache) -> new OptOutQueryCache(settings, cache, threadContext.get())); } } diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCache.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCache.java index 76d23cdbbdf..695a8c93222 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCache.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/OptOutQueryCache.java @@ -8,15 +8,15 @@ package org.elasticsearch.xpack.security.authz.accesscontrol; import org.apache.lucene.search.QueryCachingPolicy; import org.apache.lucene.search.Weight; import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.action.support.broadcast.BroadcastShardRequest; +import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.index.AbstractIndexComponent; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.cache.query.QueryCache; import org.elasticsearch.indices.IndicesQueryCache; -import org.elasticsearch.search.internal.ShardSearchRequest; import org.elasticsearch.xpack.security.authz.AuthorizationService; import java.util.HashSet; +import java.util.Objects; import java.util.Set; /** @@ -25,11 +25,15 @@ import java.util.Set; */ public final class OptOutQueryCache extends AbstractIndexComponent implements QueryCache { - final IndicesQueryCache indicesQueryCache; + private final IndicesQueryCache indicesQueryCache; + private final ThreadContext context; + private final String indexName; - public OptOutQueryCache(IndexSettings indexSettings, IndicesQueryCache indicesQueryCache) { + public OptOutQueryCache(IndexSettings indexSettings, IndicesQueryCache indicesQueryCache, ThreadContext context) { super(indexSettings); this.indicesQueryCache = indicesQueryCache; + this.context = Objects.requireNonNull(context, "threadContext must not be null"); + this.indexName = indexSettings.getIndex().getName(); } @Override @@ -45,27 +49,13 @@ public final class OptOutQueryCache extends AbstractIndexComponent implements Qu @Override public Weight doCache(Weight weight, QueryCachingPolicy policy) { - final RequestContext context = RequestContext.current(); - if (context == null) { - throw new IllegalStateException("opting out of the query cache. current request can't be found"); - } - IndicesAccessControl indicesAccessControl = context.getThreadContext().getTransient( + IndicesAccessControl indicesAccessControl = context.getTransient( AuthorizationService.INDICES_PERMISSIONS_KEY); if (indicesAccessControl == null) { logger.debug("opting out of the query cache. current request doesn't hold indices permissions"); return weight; } - // At this level only IndicesRequest - final String indexName; - if (context.getRequest() instanceof ShardSearchRequest) { - indexName = ((ShardSearchRequest) context.getRequest()).shardId().getIndexName(); - } else if (context.getRequest() instanceof BroadcastShardRequest) { - indexName = ((BroadcastShardRequest) context.getRequest()).shardId().getIndexName(); - } else { - return weight; - } - IndicesAccessControl.IndexAccessControl indexAccessControl = indicesAccessControl.getIndexPermissions(indexName); if (indexAccessControl != null && indexAccessControl.getFieldPermissions().hasFieldLevelSecurity()) { if (cachingIsSafe(weight, indexAccessControl)) { diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/RequestContext.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/RequestContext.java deleted file mode 100644 index e9e7deed4a4..00000000000 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/authz/accesscontrol/RequestContext.java +++ /dev/null @@ -1,61 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.security.authz.accesscontrol; - -import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.transport.TransportRequest; - -import java.util.Objects; - -/** - * A thread local based holder of the currnet {@link TransportRequest} instance. - */ -public final class RequestContext { - - // Need thread local to make the current transport request available to places in the code that - // don't have direct access to the current transport request - private static final ThreadLocal current = new ThreadLocal<>(); - - /** - * If set then this returns the current {@link RequestContext} with the current {@link TransportRequest}. - */ - public static RequestContext current() { - return current.get(); - } - - /** - * Invoked by the transport service to set the current transport request in the thread local - */ - public static void setCurrent(RequestContext value) { - current.set(value); - } - - /** - * Invoked by the transport service to remove the current request from the thread local - */ - public static void removeCurrent() { - current.remove(); - } - - private final ThreadContext threadContext; - private final TransportRequest request; - - public RequestContext(TransportRequest request, ThreadContext threadContext) { - this.request = Objects.requireNonNull(request); - this.threadContext = Objects.requireNonNull(threadContext); - } - - /** - * @return current {@link TransportRequest} - */ - public TransportRequest getRequest() { - return request; - } - - public ThreadContext getThreadContext() { - return threadContext; - } -} diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java index c4e2e2fe99f..37eb0e52727 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java @@ -33,7 +33,6 @@ import org.elasticsearch.xpack.security.SecurityContext; import org.elasticsearch.xpack.security.authc.AuthenticationService; import org.elasticsearch.xpack.security.authz.AuthorizationService; import org.elasticsearch.xpack.security.authz.AuthorizationUtils; -import org.elasticsearch.xpack.security.authz.accesscontrol.RequestContext; import org.elasticsearch.xpack.security.transport.netty4.SecurityNetty4Transport; import org.elasticsearch.xpack.security.user.KibanaUser; import org.elasticsearch.xpack.security.user.SystemUser; @@ -217,15 +216,7 @@ public class SecurityServerTransportInterceptor extends AbstractComponent implem @Override protected void doRun() throws Exception { - // FIXME we should remove the RequestContext completely since we have ThreadContext but cannot yet due to - // the query cache - RequestContext context = new RequestContext(request, threadContext); - RequestContext.setCurrent(context); - try { - handler.messageReceived(request, channel, task); - } finally { - RequestContext.removeCurrent(); - } + handler.messageReceived(request, channel, task); } }; }