From 462e4b8003c68e954115e9335f3a86b3a16f0d6d Mon Sep 17 00:00:00 2001 From: jon-wei Date: Sun, 17 Sep 2017 22:13:26 -0700 Subject: [PATCH 1/3] Single auth check for authorized resource filtering --- .../overlord/http/OverlordResource.java | 105 +++++++++--------- .../supervisor/SupervisorResource.java | 105 ++++++++++-------- .../io/druid/server/ClientInfoResource.java | 16 ++- .../druid/server/http/MetadataResource.java | 16 ++- .../server/security/AuthorizationUtils.java | 67 +++++++---- 5 files changed, 179 insertions(+), 130 deletions(-) diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java b/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java index d8ef4bc87019..d7c27835ccd8 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java @@ -365,24 +365,22 @@ public Collection apply(TaskRunner taskRunner) // A bit roundabout, but works as a way of figuring out what tasks haven't been handed // off to the runner yet: final List allActiveTasks = taskStorageQueryAdapter.getActiveTasks(); - final List activeTasks; - Function raGenerator = new Function() - { - @Override - public ResourceAction apply(Task input) - { - return new ResourceAction( - new Resource(input.getDataSource(), ResourceType.DATASOURCE), - Action.READ - ); - } + final List activeTasks = Lists.newArrayList(); + Function> raGenerator = task -> { + return Lists.newArrayList( + new ResourceAction( + new Resource(task.getDataSource(), ResourceType.DATASOURCE), + Action.READ + ) + ); }; - activeTasks = AuthorizationUtils.filterAuthorizedResources( + AuthorizationUtils.filterAuthorizedResources( req, allActiveTasks, raGenerator, - authorizerMapper + authorizerMapper, + activeTasks ); final Set runnersKnownTasks = Sets.newHashSet( @@ -464,34 +462,32 @@ public Collection apply(TaskRunner taskRunner) @Produces(MediaType.APPLICATION_JSON) public Response getCompleteTasks(@Context final HttpServletRequest req) { - final List recentlyFinishedTasks; - Function raGenerator = new Function() - { - @Override - public ResourceAction apply(TaskStatus input) - { - final String taskId = input.getId(); - final Optional optionalTask = taskStorageQueryAdapter.getTask(taskId); - if (!optionalTask.isPresent()) { - throw new WebApplicationException( - Response.serverError().entity( - StringUtils.format("No task information found for task with id: [%s]", taskId) - ).build() - ); - } - - return new ResourceAction( - new Resource(optionalTask.get().getDataSource(), ResourceType.DATASOURCE), - Action.READ + final List recentlyFinishedTasks = Lists.newArrayList(); + Function> raGenerator = taskStatus -> { + final String taskId = taskStatus.getId(); + final Optional optionalTask = taskStorageQueryAdapter.getTask(taskId); + if (!optionalTask.isPresent()) { + throw new WebApplicationException( + Response.serverError().entity( + StringUtils.format("No task information found for task with id: [%s]", taskId) + ).build() ); } + + return Lists.newArrayList( + new ResourceAction( + new Resource(optionalTask.get().getDataSource(), ResourceType.DATASOURCE), + Action.READ + ) + ); }; - recentlyFinishedTasks = AuthorizationUtils.filterAuthorizedResources( + AuthorizationUtils.filterAuthorizedResources( req, taskStorageQueryAdapter.getRecentlyFinishedTaskStatuses(), raGenerator, - authorizerMapper + authorizerMapper, + recentlyFinishedTasks ); final List completeTasks = Lists.transform( @@ -648,34 +644,35 @@ private Collection securedTaskRunnerWorkItem( HttpServletRequest req ) { - Function raGenerator = new Function() - { - @Override - public ResourceAction apply(TaskRunnerWorkItem input) - { - final String taskId = input.getTaskId(); - final Optional optionalTask = taskStorageQueryAdapter.getTask(taskId); - if (!optionalTask.isPresent()) { - throw new WebApplicationException( - Response.serverError().entity( - StringUtils.format("No task information found for task with id: [%s]", taskId) - ).build() - ); - } - - return new ResourceAction( - new Resource(optionalTask.get().getDataSource(), ResourceType.DATASOURCE), - Action.READ + List taskRunnerWorkItems = Lists.newArrayList(); + Function> raGenerator = taskRunnerWorkItem -> { + final String taskId = taskRunnerWorkItem.getTaskId(); + final Optional optionalTask = taskStorageQueryAdapter.getTask(taskId); + if (!optionalTask.isPresent()) { + throw new WebApplicationException( + Response.serverError().entity( + StringUtils.format("No task information found for task with id: [%s]", taskId) + ).build() ); } + + return Lists.newArrayList( + new ResourceAction( + new Resource(optionalTask.get().getDataSource(), ResourceType.DATASOURCE), + Action.READ + ) + ); }; - return AuthorizationUtils.filterAuthorizedResources( + AuthorizationUtils.filterAuthorizedResources( req, collectionToFilter, raGenerator, - authorizerMapper + authorizerMapper, + taskRunnerWorkItems ); + + return taskRunnerWorkItems; } static class TaskResponseObject diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java index ec3aad39bf06..9adc9d8fe290 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -38,6 +38,7 @@ import io.druid.server.security.AuthorizerMapper; import io.druid.server.security.AuthorizationUtils; import io.druid.server.security.ForbiddenException; +import io.druid.server.security.ResourceAction; import javax.servlet.http.HttpServletRequest; import javax.ws.rs.Consumes; @@ -49,6 +50,7 @@ import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; +import java.util.Collection; import java.util.List; import java.util.Map; import java.util.Set; @@ -119,36 +121,12 @@ public Response specGetAll(@Context final HttpServletRequest req) @Override public Response apply(final SupervisorManager manager) { - final Set supervisorIds; - supervisorIds = Sets.newHashSet(); - for (String supervisorId : manager.getSupervisorIds()) { - Optional supervisorSpecOptional = manager.getSupervisorSpec(supervisorId); - if (supervisorSpecOptional.isPresent()) { - Access accessResult = AuthorizationUtils.authorizeAllResourceActions( - req, - Iterables.transform( - supervisorSpecOptional.get().getDataSources(), - AuthorizationUtils.DATASOURCE_WRITE_RA_GENERATOR - ), - authorizerMapper - ); - - if (accessResult.isAllowed()) { - supervisorIds.add(supervisorId); - } - } - } - - // If there were no supervisorIds, go ahead and authorize the request. - if (manager.getSupervisorIds().size() == 0) { - AuthorizationUtils.authorizeAllResourceActions( - req, - Lists.newArrayList(), - authorizerMapper - ); - } - - return Response.ok(supervisorIds).build(); + Set authorizedSupervisorIds = filterAuthorizedSupervisorIds( + req, + manager, + manager.getSupervisorIds() + ); + return Response.ok(authorizedSupervisorIds).build(); } } ); @@ -239,27 +217,22 @@ public Response specGetAllHistory(@Context final HttpServletRequest req) @Override public Response apply(final SupervisorManager manager) { - final Map> supervisorHistory; - supervisorHistory = Maps.filterKeys( - manager.getSupervisorHistory(), + final Map> supervisorHistory = manager.getSupervisorHistory(); + + final Set authorizedSupervisorIds = filterAuthorizedSupervisorIds( + req, + manager, + supervisorHistory.keySet() + ); + + final Map> authorizedSupervisorHistory = Maps.filterKeys( + supervisorHistory, new Predicate() { @Override public boolean apply(String id) { - Optional supervisorSpecOptional = manager.getSupervisorSpec(id); - if (!supervisorSpecOptional.isPresent()) { - return false; - } - Access accessResult = AuthorizationUtils.authorizeAllResourceActions( - req, - Iterables.transform( - supervisorSpecOptional.get().getDataSources(), - AuthorizationUtils.DATASOURCE_WRITE_RA_GENERATOR - ), - authorizerMapper - ); - return accessResult.isAllowed(); + return authorizedSupervisorIds.contains(id); } } ); @@ -337,4 +310,44 @@ private Response asLeaderWithSupervisorManager(Function filterAuthorizedSupervisorIds( + final HttpServletRequest req, + SupervisorManager manager, + Collection supervisorIds + ) + { + final Set authorizedSupervisorIds = Sets.newHashSet(); + + // If there were no supervisorIds, go ahead and authorize the request. + if (supervisorIds.size() == 0) { + AuthorizationUtils.authorizeAllResourceActions( + req, + Lists.newArrayList(), + authorizerMapper + ); + } else { + Function> raGenerator = supervisorId -> { + Optional supervisorSpecOptional = manager.getSupervisorSpec(supervisorId); + if (supervisorSpecOptional.isPresent()) { + return Iterables.transform( + supervisorSpecOptional.get().getDataSources(), + AuthorizationUtils.DATASOURCE_WRITE_RA_GENERATOR + ); + } else { + return null; + } + }; + + AuthorizationUtils.filterAuthorizedResources( + req, + supervisorIds, + raGenerator, + authorizerMapper, + authorizedSupervisorIds + ); + } + + return authorizedSupervisorIds; + } } diff --git a/server/src/main/java/io/druid/server/ClientInfoResource.java b/server/src/main/java/io/druid/server/ClientInfoResource.java index ff46da0be0de..490957040255 100644 --- a/server/src/main/java/io/druid/server/ClientInfoResource.java +++ b/server/src/main/java/io/druid/server/ClientInfoResource.java @@ -19,6 +19,7 @@ package io.druid.server; +import com.google.common.base.Function; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; @@ -43,6 +44,7 @@ import io.druid.server.security.AuthConfig; import io.druid.server.security.AuthorizerMapper; import io.druid.server.security.AuthorizationUtils; +import io.druid.server.security.ResourceAction; import io.druid.timeline.DataSegment; import io.druid.timeline.TimelineLookup; import io.druid.timeline.TimelineObjectHolder; @@ -119,12 +121,20 @@ private Map> getSegmentsForDatasources() @Produces(MediaType.APPLICATION_JSON) public Iterable getDataSources(@Context final HttpServletRequest request) { - return AuthorizationUtils.filterAuthorizedResources( + Set authorizedDatasources = Sets.newHashSet(); + Function> raGenerator = datasourceName -> { + return Lists.newArrayList(AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(datasourceName)); + }; + + AuthorizationUtils.filterAuthorizedResources( request, getSegmentsForDatasources().keySet(), - AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR, - authorizerMapper + raGenerator, + authorizerMapper, + authorizedDatasources ); + + return authorizedDatasources; } @GET diff --git a/server/src/main/java/io/druid/server/http/MetadataResource.java b/server/src/main/java/io/druid/server/http/MetadataResource.java index fc1fadd11ac1..3104dad7be62 100644 --- a/server/src/main/java/io/druid/server/http/MetadataResource.java +++ b/server/src/main/java/io/druid/server/http/MetadataResource.java @@ -23,6 +23,7 @@ import com.google.common.base.Predicate; import com.google.common.collect.Collections2; import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.inject.Inject; import com.sun.jersey.spi.container.ResourceFilters; @@ -33,6 +34,7 @@ import io.druid.server.security.AuthConfig; import io.druid.server.security.AuthorizerMapper; import io.druid.server.security.AuthorizationUtils; +import io.druid.server.security.ResourceAction; import io.druid.timeline.DataSegment; import org.joda.time.Interval; @@ -102,15 +104,19 @@ public String apply(DruidDataSource input) ); } - List datasourceNamesList = AuthorizationUtils.filterAuthorizedResources( + final Set dataSourceNamesPostAuth = Sets.newTreeSet(); + Function> raGenerator = datasourceName -> { + return Lists.newArrayList(AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(datasourceName)); + }; + + AuthorizationUtils.filterAuthorizedResources( req, dataSourceNamesPreAuth, - AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR, - authorizerMapper + raGenerator, + authorizerMapper, + dataSourceNamesPostAuth ); - final Set dataSourceNamesPostAuth = Sets.newTreeSet(datasourceNamesList); - // Cannot do both includeDisabled and full, let includeDisabled take priority // Always use dataSourceNamesPostAuth to determine the set of returned dataSources if (full != null && includeDisabled == null) { diff --git a/server/src/main/java/io/druid/server/security/AuthorizationUtils.java b/server/src/main/java/io/druid/server/security/AuthorizationUtils.java index 5c8fb2e41361..5cafe6eb19f8 100644 --- a/server/src/main/java/io/druid/server/security/AuthorizationUtils.java +++ b/server/src/main/java/io/druid/server/security/AuthorizationUtils.java @@ -26,9 +26,7 @@ import io.druid.java.util.common.ISE; import javax.servlet.http.HttpServletRequest; -import java.util.ArrayList; import java.util.Collection; -import java.util.List; import java.util.Map; import java.util.Set; @@ -153,25 +151,40 @@ public static Access authorizeAllResourceActions( } /** - * Filter a list of resource-actions using the request's authorization fields, returning a new list of - * resource-actions that were authorized. + * Filter a collection of resources by applying the resourceActionGenerator to each resource, adding + * the filtered resources to a caller provided Collection, filteredResources. + * + * The resourceActionGenerator returns an Iterable for each resource. + * + * If every resource-action in the iterable is authorized, the resource will be added to filteredResources. + * + * If there is an authorization failure for one of the resource-actions, the resource will not be + * added to filteredResources.. + * + * If the resourceActionGenerator returns null for a resource, that resource will not be added to filteredResources. * * This function will set the DRUID_AUTHORIZATION_CHECKED attribute in the request. * * If this attribute is already set when this function is called, an exception is thrown. * * @param request HTTP request to be authorized - * @param resources List of resources to be processed into resource-actions - * @param resourceActionGenerator Function that creates a resource-action from a resource - * @return A list containing the resource-actions from the resourceParser that were successfully authorized. + * @param resources resources to be processed into resource-actions + * @param resourceActionGenerator Function that creates an iterable of resource-actions from a resource + * @param authorizerMapper authorizer mapper + * @param filteredResources caller provided collection, resources that pass filtering will be added to this */ - public static List filterAuthorizedResources( + public static void filterAuthorizedResources( final HttpServletRequest request, final Collection resources, - final Function resourceActionGenerator, - final AuthorizerMapper authorizerMapper + final Function> resourceActionGenerator, + final AuthorizerMapper authorizerMapper, + final Collection filteredResources ) { + if (request.getAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED) != null) { + throw new ISE("Request already had authorization check."); + } + final AuthenticationResult authenticationResult = (AuthenticationResult) request.getAttribute( AuthConfig.DRUID_AUTHENTICATION_RESULT ); @@ -185,18 +198,27 @@ public static List filterAuthorizedResources( } final Map resultCache = Maps.newHashMap(); - List filteredResources = new ArrayList<>(); for (ResType resource : resources) { - final ResourceAction resourceAction = resourceActionGenerator.apply(resource); - Access access = resultCache.computeIfAbsent( - resourceAction, - ra -> authorizer.authorize( - authenticationResult, - ra.getResource(), - ra.getAction() - ) - ); - if (access.isAllowed()) { + final Iterable resourceActions = resourceActionGenerator.apply(resource); + if (resourceActions == null) { + continue; + } + boolean authorized = true; + for (ResourceAction resourceAction : resourceActions) { + Access access = resultCache.computeIfAbsent( + resourceAction, + ra -> authorizer.authorize( + authenticationResult, + ra.getResource(), + ra.getAction() + ) + ); + if (!access.isAllowed()) { + authorized = false; + break; + } + } + if (authorized) { filteredResources.add(resource); } } @@ -204,9 +226,10 @@ public static List filterAuthorizedResources( // We're filtering, so having access to none of the objects isn't an authorization failure (in terms of whether // to send an error response or not.) request.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); - return filteredResources; } + + /** * Function for the common pattern of generating a resource-action for reading from a datasource, using the * datasource name. From f350bb337cb7f21a91e810b0c99da1d7d09cbfce Mon Sep 17 00:00:00 2001 From: jon-wei Date: Mon, 18 Sep 2017 13:17:49 -0700 Subject: [PATCH 2/3] PR comment --- .../supervisor/SupervisorResource.java | 46 ++++++++----------- 1 file changed, 18 insertions(+), 28 deletions(-) diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java index 9adc9d8fe290..2cbc49c51f6a 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -25,7 +25,6 @@ import com.google.common.base.Predicate; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; -import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.inject.Inject; @@ -319,34 +318,25 @@ private Set filterAuthorizedSupervisorIds( { final Set authorizedSupervisorIds = Sets.newHashSet(); - // If there were no supervisorIds, go ahead and authorize the request. - if (supervisorIds.size() == 0) { - AuthorizationUtils.authorizeAllResourceActions( - req, - Lists.newArrayList(), - authorizerMapper - ); - } else { - Function> raGenerator = supervisorId -> { - Optional supervisorSpecOptional = manager.getSupervisorSpec(supervisorId); - if (supervisorSpecOptional.isPresent()) { - return Iterables.transform( - supervisorSpecOptional.get().getDataSources(), - AuthorizationUtils.DATASOURCE_WRITE_RA_GENERATOR - ); - } else { - return null; - } - }; + Function> raGenerator = supervisorId -> { + Optional supervisorSpecOptional = manager.getSupervisorSpec(supervisorId); + if (supervisorSpecOptional.isPresent()) { + return Iterables.transform( + supervisorSpecOptional.get().getDataSources(), + AuthorizationUtils.DATASOURCE_WRITE_RA_GENERATOR + ); + } else { + return null; + } + }; - AuthorizationUtils.filterAuthorizedResources( - req, - supervisorIds, - raGenerator, - authorizerMapper, - authorizedSupervisorIds - ); - } + AuthorizationUtils.filterAuthorizedResources( + req, + supervisorIds, + raGenerator, + authorizerMapper, + authorizedSupervisorIds + ); return authorizedSupervisorIds; } From fd1625e08063dc4d81091abddf3a88e4b6deaabd Mon Sep 17 00:00:00 2001 From: jon-wei Date: Mon, 18 Sep 2017 13:54:25 -0700 Subject: [PATCH 3/3] PR comments --- .../overlord/http/OverlordResource.java | 44 ++++++------ .../supervisor/SupervisorResource.java | 17 ++--- .../io/druid/server/ClientInfoResource.java | 8 +-- .../druid/server/http/MetadataResource.java | 14 ++-- .../server/security/AuthorizationUtils.java | 70 ++++++++++--------- 5 files changed, 74 insertions(+), 79 deletions(-) diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java b/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java index d7c27835ccd8..d87faa910720 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/http/OverlordResource.java @@ -365,7 +365,6 @@ public Collection apply(TaskRunner taskRunner) // A bit roundabout, but works as a way of figuring out what tasks haven't been handed // off to the runner yet: final List allActiveTasks = taskStorageQueryAdapter.getActiveTasks(); - final List activeTasks = Lists.newArrayList(); Function> raGenerator = task -> { return Lists.newArrayList( new ResourceAction( @@ -375,12 +374,13 @@ public Collection apply(TaskRunner taskRunner) ); }; - AuthorizationUtils.filterAuthorizedResources( - req, - allActiveTasks, - raGenerator, - authorizerMapper, - activeTasks + final List activeTasks = Lists.newArrayList( + AuthorizationUtils.filterAuthorizedResources( + req, + allActiveTasks, + raGenerator, + authorizerMapper + ) ); final Set runnersKnownTasks = Sets.newHashSet( @@ -462,7 +462,6 @@ public Collection apply(TaskRunner taskRunner) @Produces(MediaType.APPLICATION_JSON) public Response getCompleteTasks(@Context final HttpServletRequest req) { - final List recentlyFinishedTasks = Lists.newArrayList(); Function> raGenerator = taskStatus -> { final String taskId = taskStatus.getId(); final Optional optionalTask = taskStorageQueryAdapter.getTask(taskId); @@ -482,12 +481,13 @@ public Response getCompleteTasks(@Context final HttpServletRequest req) ); }; - AuthorizationUtils.filterAuthorizedResources( - req, - taskStorageQueryAdapter.getRecentlyFinishedTaskStatuses(), - raGenerator, - authorizerMapper, - recentlyFinishedTasks + final List recentlyFinishedTasks = Lists.newArrayList( + AuthorizationUtils.filterAuthorizedResources( + req, + taskStorageQueryAdapter.getRecentlyFinishedTaskStatuses(), + raGenerator, + authorizerMapper + ) ); final List completeTasks = Lists.transform( @@ -644,7 +644,6 @@ private Collection securedTaskRunnerWorkItem( HttpServletRequest req ) { - List taskRunnerWorkItems = Lists.newArrayList(); Function> raGenerator = taskRunnerWorkItem -> { final String taskId = taskRunnerWorkItem.getTaskId(); final Optional optionalTask = taskStorageQueryAdapter.getTask(taskId); @@ -664,15 +663,14 @@ private Collection securedTaskRunnerWorkItem( ); }; - AuthorizationUtils.filterAuthorizedResources( - req, - collectionToFilter, - raGenerator, - authorizerMapper, - taskRunnerWorkItems + return Lists.newArrayList( + AuthorizationUtils.filterAuthorizedResources( + req, + collectionToFilter, + raGenerator, + authorizerMapper + ) ); - - return taskRunnerWorkItems; } static class TaskResponseObject diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java index 2cbc49c51f6a..c23e028e6f10 100644 --- a/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java +++ b/indexing-service/src/main/java/io/druid/indexing/overlord/supervisor/SupervisorResource.java @@ -316,8 +316,6 @@ private Set filterAuthorizedSupervisorIds( Collection supervisorIds ) { - final Set authorizedSupervisorIds = Sets.newHashSet(); - Function> raGenerator = supervisorId -> { Optional supervisorSpecOptional = manager.getSupervisorSpec(supervisorId); if (supervisorSpecOptional.isPresent()) { @@ -330,14 +328,13 @@ private Set filterAuthorizedSupervisorIds( } }; - AuthorizationUtils.filterAuthorizedResources( - req, - supervisorIds, - raGenerator, - authorizerMapper, - authorizedSupervisorIds + return Sets.newHashSet( + AuthorizationUtils.filterAuthorizedResources( + req, + supervisorIds, + raGenerator, + authorizerMapper + ) ); - - return authorizedSupervisorIds; } } diff --git a/server/src/main/java/io/druid/server/ClientInfoResource.java b/server/src/main/java/io/druid/server/ClientInfoResource.java index 490957040255..20ecaee63066 100644 --- a/server/src/main/java/io/druid/server/ClientInfoResource.java +++ b/server/src/main/java/io/druid/server/ClientInfoResource.java @@ -121,20 +121,16 @@ private Map> getSegmentsForDatasources() @Produces(MediaType.APPLICATION_JSON) public Iterable getDataSources(@Context final HttpServletRequest request) { - Set authorizedDatasources = Sets.newHashSet(); Function> raGenerator = datasourceName -> { return Lists.newArrayList(AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(datasourceName)); }; - AuthorizationUtils.filterAuthorizedResources( + return AuthorizationUtils.filterAuthorizedResources( request, getSegmentsForDatasources().keySet(), raGenerator, - authorizerMapper, - authorizedDatasources + authorizerMapper ); - - return authorizedDatasources; } @GET diff --git a/server/src/main/java/io/druid/server/http/MetadataResource.java b/server/src/main/java/io/druid/server/http/MetadataResource.java index 3104dad7be62..5d1e6c74bdb0 100644 --- a/server/src/main/java/io/druid/server/http/MetadataResource.java +++ b/server/src/main/java/io/druid/server/http/MetadataResource.java @@ -109,12 +109,14 @@ public String apply(DruidDataSource input) return Lists.newArrayList(AuthorizationUtils.DATASOURCE_READ_RA_GENERATOR.apply(datasourceName)); }; - AuthorizationUtils.filterAuthorizedResources( - req, - dataSourceNamesPreAuth, - raGenerator, - authorizerMapper, - dataSourceNamesPostAuth + Iterables.addAll( + dataSourceNamesPostAuth, + AuthorizationUtils.filterAuthorizedResources( + req, + dataSourceNamesPreAuth, + raGenerator, + authorizerMapper + ) ); // Cannot do both includeDisabled and full, let includeDisabled take priority diff --git a/server/src/main/java/io/druid/server/security/AuthorizationUtils.java b/server/src/main/java/io/druid/server/security/AuthorizationUtils.java index 5cafe6eb19f8..b25da17a5a77 100644 --- a/server/src/main/java/io/druid/server/security/AuthorizationUtils.java +++ b/server/src/main/java/io/druid/server/security/AuthorizationUtils.java @@ -20,13 +20,13 @@ package io.druid.server.security; import com.google.common.base.Function; +import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; import io.druid.java.util.common.ISE; import javax.servlet.http.HttpServletRequest; -import java.util.Collection; import java.util.Map; import java.util.Set; @@ -151,17 +151,18 @@ public static Access authorizeAllResourceActions( } /** - * Filter a collection of resources by applying the resourceActionGenerator to each resource, adding - * the filtered resources to a caller provided Collection, filteredResources. + * Filter a collection of resources by applying the resourceActionGenerator to each resource, return an iterable + * containing the filtered resources. * * The resourceActionGenerator returns an Iterable for each resource. * - * If every resource-action in the iterable is authorized, the resource will be added to filteredResources. + * If every resource-action in the iterable is authorized, the resource will be added to the filtered resources. * * If there is an authorization failure for one of the resource-actions, the resource will not be - * added to filteredResources.. + * added to the returned filtered resources.. * - * If the resourceActionGenerator returns null for a resource, that resource will not be added to filteredResources. + * If the resourceActionGenerator returns null for a resource, that resource will not be added to the filtered + * resources. * * This function will set the DRUID_AUTHORIZATION_CHECKED attribute in the request. * @@ -171,14 +172,14 @@ public static Access authorizeAllResourceActions( * @param resources resources to be processed into resource-actions * @param resourceActionGenerator Function that creates an iterable of resource-actions from a resource * @param authorizerMapper authorizer mapper - * @param filteredResources caller provided collection, resources that pass filtering will be added to this + * @return Iterable containing resources that were authorized + * */ - public static void filterAuthorizedResources( + public static Iterable filterAuthorizedResources( final HttpServletRequest request, - final Collection resources, + final Iterable resources, final Function> resourceActionGenerator, - final AuthorizerMapper authorizerMapper, - final Collection filteredResources + final AuthorizerMapper authorizerMapper ) { if (request.getAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED) != null) { @@ -198,34 +199,35 @@ public static void filterAuthorizedResources( } final Map resultCache = Maps.newHashMap(); - for (ResType resource : resources) { - final Iterable resourceActions = resourceActionGenerator.apply(resource); - if (resourceActions == null) { - continue; - } - boolean authorized = true; - for (ResourceAction resourceAction : resourceActions) { - Access access = resultCache.computeIfAbsent( - resourceAction, - ra -> authorizer.authorize( - authenticationResult, - ra.getResource(), - ra.getAction() - ) - ); - if (!access.isAllowed()) { - authorized = false; - break; + final Iterable filteredResources = Iterables.filter( + resources, + resource -> { + final Iterable resourceActions = resourceActionGenerator.apply(resource); + if (resourceActions == null) { + return false; + } + for (ResourceAction resourceAction : resourceActions) { + Access access = resultCache.computeIfAbsent( + resourceAction, + ra -> authorizer.authorize( + authenticationResult, + ra.getResource(), + ra.getAction() + ) + ); + if (!access.isAllowed()) { + return false; + } + } + return true; } - } - if (authorized) { - filteredResources.add(resource); - } - } + ); // We're filtering, so having access to none of the objects isn't an authorization failure (in terms of whether // to send an error response or not.) request.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); + + return filteredResources; }