From b0c18d287a2335e3bb86f0fbd2029fcd6fa13b28 Mon Sep 17 00:00:00 2001 From: Andrew Cholewa Date: Mon, 19 Dec 2022 09:59:39 -0600 Subject: [PATCH] Reworks role based resource validation. (#1288) -- We fully generalize RoleBasedTableValidatorRequestMapper to apply to any ApiRequest, not just DataApiRequest, and in the process, rename it to `RoleBasedValidatorRequestMapper` since it no longer necessarily involves tables. -- RoleBasedTableValidatorRequestMapper becomes a subclass of `RoleBasedValidatorRequestMapper` specifically for TableApiRequests. The subclass delegates to its parent if the request contains a table (i.e. the user requests information about a specific table). Otherwise, if there is no table on the request (i.e. the user asks for information about all tables), the `RoleBasedTableValidatorRequestMapper` instead modifies the TableApiRequest to only expose those tables that pass validation. Co-authored-by: Andrew Cholewa --- CHANGELOG.md | 6 + .../RoleBasedTableValidatorRequestMapper.java | 64 ++----- .../RoleBasedValidatorRequestMapper.java | 70 +++++++ ...asedTableValidatorRequestMapperSpec.groovy | 174 ++++++++++++------ ...RoleBasedValidatorRequestMapperSpec.groovy | 163 ++++++++++++++++ 5 files changed, 374 insertions(+), 103 deletions(-) create mode 100644 fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapper.java create mode 100644 fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapperSpec.groovy diff --git a/CHANGELOG.md b/CHANGELOG.md index aaf17c6b57..b9bed33654 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ Current ### Added: ### Changed: +- [Reworked RoleBased Request Mapping for greater generality and yet more flexibility](https://github.com/yahoo/fili/pull/1288) + - `RoleBasedValidatorRequestMapper` has been added that applies to any ApiRequest, but otherwise behaves like the original + `RoleBasedTableValidatorRequestMapper`. + - The `RoleBasedTableValidatorRequestMapper` is now specific to the TablesApiRequest and handles requests for specific tables and + fullview requests. + - [Updated Groovy to 3.0](https://github.com/yahoo/fili/pull/1171) diff --git a/fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapper.java b/fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapper.java index d3bfc88dc5..1cc01fbdbd 100644 --- a/fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapper.java +++ b/fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapper.java @@ -3,34 +3,29 @@ package com.yahoo.bard.webservice.web.security; import com.yahoo.bard.webservice.data.config.ResourceDictionaries; -import com.yahoo.bard.webservice.web.ChainingRequestMapper; +import com.yahoo.bard.webservice.table.LogicalTable; import com.yahoo.bard.webservice.web.RequestMapper; import com.yahoo.bard.webservice.web.RequestValidationException; -import com.yahoo.bard.webservice.web.apirequest.DataApiRequest; +import com.yahoo.bard.webservice.web.apirequest.TablesApiRequest; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.LinkedHashSet; import java.util.Map; -import java.util.function.Function; import java.util.function.Predicate; +import java.util.stream.Collectors; import javax.validation.constraints.NotNull; import javax.ws.rs.container.ContainerRequestContext; -import javax.ws.rs.core.Response; import javax.ws.rs.core.SecurityContext; /** * A RequestMapper that validates table access for user based on roles that a user is associated with. - * - * @param Type of API Request this RequestMapper will work on */ -public class RoleBasedTableValidatorRequestMapper extends ChainingRequestMapper { - - public static String DEFAULT_SECURITY_MAPPER_NAME = "__default"; - - public static Predicate NO_OP_PREDICATE = (ignored -> true); +public class RoleBasedTableValidatorRequestMapper extends RoleBasedValidatorRequestMapper { - private final Map> securityRules; - - private final Function securityContextSelector; + private static final Logger LOG = LoggerFactory.getLogger(RoleBasedTableValidatorRequestMapper.class); /** * Constructor. @@ -41,42 +36,21 @@ public class RoleBasedTableValidatorRequestMapper exte public RoleBasedTableValidatorRequestMapper( Map> securityRules, ResourceDictionaries resourceDictionaries, - @NotNull RequestMapper next + @NotNull RequestMapper next ) { - this(securityRules, resourceDictionaries, next, r -> r.getTable().getName()); - } - - /** - * Constructor. - * @param securityRules A map of predicates for validating table access. - * @param resourceDictionaries The dictionaries to use for request mapping. - * @param securityContextSelector A function for selecting a security group name from the context. - * @param next The next request mapper to process this ApiRequest - */ - public RoleBasedTableValidatorRequestMapper( - Map> securityRules, - ResourceDictionaries resourceDictionaries, - @NotNull RequestMapper next, - Function securityContextSelector - ) { - super(resourceDictionaries, next); - this.securityRules = securityRules; - this.securityContextSelector = securityContextSelector; + super(securityRules, resourceDictionaries, next, r -> r.getTable().getName()); } @Override - public T internalApply(T request, ContainerRequestContext context) + public TablesApiRequest internalApply(TablesApiRequest request, ContainerRequestContext context) throws RequestValidationException { - SecurityContext securityContext = context.getSecurityContext(); - String securityTag = securityContextSelector.apply(request); - Predicate isAllowed = securityRules.containsKey(securityTag) ? - securityRules.get(securityTag) : - securityRules.getOrDefault(DEFAULT_SECURITY_MAPPER_NAME, NO_OP_PREDICATE); - - if (!isAllowed.test(securityContext)) { - throw new RequestValidationException(Response.Status.FORBIDDEN, "Permission Denied", - "Request cannot be completed as you do not have enough permission"); + if (request.getTable() == null) { + SecurityContext securityContext = context.getSecurityContext(); + LinkedHashSet exposedTables = request.getTables().stream() + .filter(table -> validate(table.getName(), securityContext)) + .collect(Collectors.toCollection(LinkedHashSet::new)); + return request.withTables(exposedTables); } - return request; + return super.internalApply(request, context); } } diff --git a/fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapper.java b/fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapper.java new file mode 100644 index 0000000000..ccdd553f06 --- /dev/null +++ b/fili-security/src/main/java/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapper.java @@ -0,0 +1,70 @@ +// Copyright 2018 Yahoo Inc. +// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms. +package com.yahoo.bard.webservice.web.security; + +import com.yahoo.bard.webservice.data.config.ResourceDictionaries; +import com.yahoo.bard.webservice.web.ChainingRequestMapper; +import com.yahoo.bard.webservice.web.RequestMapper; +import com.yahoo.bard.webservice.web.RequestValidationException; +import com.yahoo.bard.webservice.web.apirequest.ApiRequest; + +import java.util.Map; +import java.util.function.Function; +import java.util.function.Predicate; + +import javax.validation.constraints.NotNull; +import javax.ws.rs.container.ContainerRequestContext; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.SecurityContext; + +/** + * A RequestMapper that validates resource access for user based on roles that a user is associated with. + * + * @param Type of API Request this RequestMapper will work on + */ +public class RoleBasedValidatorRequestMapper extends ChainingRequestMapper { + + public static String DEFAULT_SECURITY_MAPPER_NAME = "__default"; + + public static Predicate NO_OP_PREDICATE = (ignored -> true); + + private final Map> securityRules; + + private final Function securityIdSelector; + + /** + * Constructor. + * @param securityRules A map of predicates for validating table access. + * @param resourceDictionaries The dictionaries to use for request mapping. + * @param securityIdSelector A function for selecting a security group name from the context. + * @param next The next request mapper to process this ApiRequest + */ + public RoleBasedValidatorRequestMapper( + Map> securityRules, + ResourceDictionaries resourceDictionaries, + @NotNull RequestMapper next, + Function securityIdSelector + ) { + super(resourceDictionaries, next); + this.securityRules = securityRules; + this.securityIdSelector = securityIdSelector; + } + + @Override + public T internalApply(T request, ContainerRequestContext context) + throws RequestValidationException { + if (!validate(securityIdSelector.apply(request), context.getSecurityContext())) { + throw new RequestValidationException(Response.Status.FORBIDDEN, "Permission Denied", + "Request cannot be completed as you do not have enough permission"); + } + return request; + } + + protected boolean validate(String securityTag, SecurityContext context) { + Predicate isAllowed = securityRules.containsKey(securityTag) ? + securityRules.get(securityTag) : + securityRules.getOrDefault(DEFAULT_SECURITY_MAPPER_NAME, NO_OP_PREDICATE); + return isAllowed.test(context); + + } +} diff --git a/fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapperSpec.groovy b/fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapperSpec.groovy index 76743f2591..ee31465c50 100644 --- a/fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapperSpec.groovy +++ b/fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedTableValidatorRequestMapperSpec.groovy @@ -3,15 +3,33 @@ package com.yahoo.bard.webservice.web.security import com.yahoo.bard.webservice.data.config.ResourceDictionaries +import com.yahoo.bard.webservice.data.time.DefaultTimeGrain +import com.yahoo.bard.webservice.data.time.GranularityParser; +import com.yahoo.bard.webservice.data.time.StandardGranularityParser; +import com.yahoo.bard.webservice.druid.model.builders.DruidFilterBuilder; import com.yahoo.bard.webservice.table.LogicalTable +import com.yahoo.bard.webservice.table.LogicalTableDictionary; +import com.yahoo.bard.webservice.table.TableGroup +import com.yahoo.bard.webservice.table.TableIdentifier; +import com.yahoo.bard.webservice.web.NoOpRequestMapper; import com.yahoo.bard.webservice.web.RequestMapper import com.yahoo.bard.webservice.web.RequestValidationException -import com.yahoo.bard.webservice.web.apirequest.DataApiRequest +import com.yahoo.bard.webservice.web.apirequest.TablesApiRequest +import com.yahoo.bard.webservice.web.apirequest.TablesApiRequest; +import com.yahoo.bard.webservice.web.apirequest.TablesApiRequestImpl; +import com.yahoo.bard.webservice.web.apirequest.generator.having.HavingGenerator; +import com.yahoo.bard.webservice.web.filters.ApiFilters +import com.yahoo.bard.webservice.web.util.BardConfigResources + +import org.joda.time.DateTimeZone import spock.lang.Shared import spock.lang.Specification import spock.lang.Unroll +import java.util.Collections +import java.util.LinkedHashSet + import javax.ws.rs.container.ContainerRequestContext import javax.ws.rs.core.SecurityContext import java.util.function.Predicate @@ -19,10 +37,10 @@ import java.util.function.Predicate class RoleBasedTableValidatorRequestMapperSpec extends Specification { @Shared - RequestMapper next = Mock(RequestMapper) + ResourceDictionaries dictionaries = new ResourceDictionaries() @Shared - ResourceDictionaries dictionaries = Mock(ResourceDictionaries) + RequestMapper next = new NoOpRequestMapper(dictionaries) @Shared SecurityContext securityContext1 = Mock(SecurityContext) @@ -34,73 +52,113 @@ class RoleBasedTableValidatorRequestMapperSpec extends Specification { @Shared Predicate predicate + @Shared + Predicate predicate2 @Shared - DataApiRequest request1 + TablesApiRequest request1 @Shared - DataApiRequest request2 + TablesApiRequest request2 @Shared ContainerRequestContext containerRequestContext1 = Mock(ContainerRequestContext) @Shared ContainerRequestContext containerRequestContext2 = Mock(ContainerRequestContext) + @Shared + LogicalTable table1 + @Shared + LogicalTable table2 @Shared RoleBasedTableValidatorRequestMapper mapper - @Shared - RoleBasedTableValidatorRequestMapper reverseNameMapper + BardConfigResources getBardConfigResources() { + return new BardConfigResources() { + public LogicalTableDictionary getLogicalDictionary() { + return dictionaries.getLogicalDictionary() + } - def setupSpec() { - LogicalTable table1 = Mock(LogicalTable) { - getName() >> "TABLE1" - } + public ResourceDictionaries getResourceDictionaries() { + return dictionaries + } - request1 = Mock(DataApiRequest) { - getTable() >> table1 - } + public GranularityParser getGranularityParser() { + return new StandardGranularityParser() + } - LogicalTable table2 = Mock(LogicalTable) { - getName() >> "TABLE2" - } + public DruidFilterBuilder getFilterBuilder() { + return null + } - request2 = Mock(DataApiRequest) { - getTable() >> table2 + public HavingGenerator getHavingApiGenerator() { + return null + } + + public DateTimeZone getSystemTimeZone() { + return DateTimeZone.UTC + } } + } - predicate = {it.isUserInRole("GRANT_TABLE1")} as Predicate - securityRules.put("TABLE1", predicate) + TablesApiRequest testingTablesApiRequest() { + return new TablesApiRequestImpl( + "TABLE1", + DefaultTimeGrain.DAY.toString(), + "json", + "", + "", + getBardConfigResources() + ) + } - predicate = {it.isUserInRole("GRANT_TABLE2")} as Predicate - securityRules.put("TABLE2", predicate) + def setupSpec() { + securityRules.put( + RoleBasedValidatorRequestMapper.DEFAULT_SECURITY_MAPPER_NAME, + { + throw new IllegalStateException("Missing security rule. Security rules: " + securityRules) + } as Predicate + ) + TableGroup emptyTableGroup = new TableGroup( + new LinkedHashSet<>(), + Collections.emptySet(), + Collections.emptySet(), + new ApiFilters() + ) + table1 = new LogicalTable( + "TABLE1", + DefaultTimeGrain.DAY, + emptyTableGroup, + dictionaries.getMetricDictionary() + ) + table2 = new LogicalTable( + "TABLE2", + DefaultTimeGrain.DAY, + emptyTableGroup, + dictionaries.getMetricDictionary() + ) + dictionaries.getLogicalDictionary().put(new TableIdentifier(table1), table1) + dictionaries.getLogicalDictionary().put(new TableIdentifier(table2), table2) - predicate = {it.isUserInRole("GRANT_TABLE2")} as Predicate - securityRules.put("2ELBAT", predicate) + request1 = testingTablesApiRequest().withTable(table1) + request2 = testingTablesApiRequest().withTable(table2) - predicate = { r -> false} - securityRules.put(RoleBasedTableValidatorRequestMapper.DEFAULT_SECURITY_MAPPER_NAME, predicate); + predicate = {it.isUserInRole("GRANT_TABLE1")} as Predicate + securityRules.put("TABLE1", predicate) securityContext1.isUserInRole("GRANT_TABLE1") >> true containerRequestContext1.getSecurityContext() >> securityContext1 securityContext2.isUserInRole("GRANT_TABLE2") >> true containerRequestContext2.getSecurityContext() >> securityContext2 + predicate2 = {it.isUserInRole("GRANT_TABLE2")} + securityRules.put("TABLE2", predicate2) mapper = new RoleBasedTableValidatorRequestMapper( securityRules, dictionaries, next ) - - reverseNameMapper = new RoleBasedTableValidatorRequestMapper( - securityRules, - dictionaries, - next, - (r) -> r.getTable().getName().reverse() - ) - } - @Unroll def "check if tables matching user role are alone queryable"() { expect: mapper.internalApply(request, context) == response @@ -112,40 +170,40 @@ class RoleBasedTableValidatorRequestMapperSpec extends Specification { } @Unroll - def "check if tables whose name in reverse matches user role are queryable"() { - expect: - reverseNameMapper.internalApply(request, context) == response - - where: - request | context | response - request2 | containerRequestContext2 | request2 - } - - @Unroll - def "check if tables whose name is not in reverse matches user role are queryable"() { + def "check if exception is thrown if user's role does not match the security rules for the logical table"() { when: - reverseNameMapper.internalApply(request, context) + mapper.internalApply(request, context) then: thrown exception where: request | context | exception - request1 | containerRequestContext1 | RequestValidationException - + request1 | containerRequestContext2 | RequestValidationException + request2 | containerRequestContext1 | RequestValidationException } @Unroll - def "check if exception is thrown if user's role does not match the security rules for the logical table"() { + def "A user with permissions #permissions can see tables #tables when fullview is requested"() { + given: + SecurityContext securityContext = Stub(SecurityContext) + permissions.forEach({securityContext.isUserInRole(it) >> true}) + ContainerRequestContext requestContext = Stub(ContainerRequestContext) { + getSecurityContext() >> securityContext + } + and: + TablesApiRequest request = new TablesApiRequestImpl(null, null, "json", "", "", getBardConfigResources()) + when: - mapper.internalApply(request, context) + TablesApiRequest mappedRequest = mapper.apply(request, requestContext) then: - thrown exception - - where: - request | context | exception - request1 | containerRequestContext2 | RequestValidationException - request2 | containerRequestContext1 | RequestValidationException + mappedRequest.getTables() == tables + where: + permissions | tables + ["GRANT_TABLE1"] | [table1] as Set + ["GRANT_TABLE2"] | [table2] as LinkedHashSet + ["GRANT_TABLE1", "GRANT_TABLE2"] | [table1, table2] as LinkedHashSet + [] | [] as LinkedHashSet } } diff --git a/fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapperSpec.groovy b/fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapperSpec.groovy new file mode 100644 index 0000000000..bf96cc3855 --- /dev/null +++ b/fili-security/src/test/groovy/com/yahoo/bard/webservice/web/security/RoleBasedValidatorRequestMapperSpec.groovy @@ -0,0 +1,163 @@ +// Copyright 2018 Yahoo Inc. +// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms. +package com.yahoo.bard.webservice.web.security + +import static com.yahoo.bard.webservice.data.time.DefaultTimeGrain.DAY + +import com.yahoo.bard.webservice.data.config.ResourceDictionaries +import com.yahoo.bard.webservice.table.LogicalTable +import com.yahoo.bard.webservice.table.TableGroup; +import com.yahoo.bard.webservice.web.NoOpRequestMapper; +import com.yahoo.bard.webservice.web.RequestMapper +import com.yahoo.bard.webservice.web.RequestValidationException +import com.yahoo.bard.webservice.web.apirequest.DataApiRequest +import com.yahoo.bard.webservice.web.apirequest.utils.TestingDataApiRequestImpl +import com.yahoo.bard.webservice.web.filters.ApiFilters; + +import spock.lang.Shared +import spock.lang.Specification +import spock.lang.Unroll + +import java.util.Collections; + +import javax.ws.rs.container.ContainerRequestContext +import javax.ws.rs.core.SecurityContext +import java.util.function.Predicate + +class RoleBasedValidatorRequestMapperSpec extends Specification { + + @Shared + ResourceDictionaries dictionaries = new ResourceDictionaries() + + @Shared + RequestMapper next = new NoOpRequestMapper(dictionaries) + + @Shared + SecurityContext securityContext1 = Mock(SecurityContext) + @Shared + SecurityContext securityContext2 = Mock(SecurityContext) + + @Shared + Map> securityRules = new HashMap<>() + + @Shared + Predicate predicate + + @Shared + DataApiRequest request1 + @Shared + DataApiRequest request2 + @Shared + ContainerRequestContext containerRequestContext1 = Mock(ContainerRequestContext) + @Shared + ContainerRequestContext containerRequestContext2 = Mock(ContainerRequestContext) + + @Shared + RoleBasedValidatorRequestMapper mapper + + @Shared + RoleBasedValidatorRequestMapper reverseNameMapper + + def setupSpec() { + TableGroup emptyTableGroup = new TableGroup( + new LinkedHashSet<>(), + Collections.emptySet(), + Collections.emptySet(), + new ApiFilters() + ) + LogicalTable table1 = new LogicalTable( + "TABLE1", + DAY, + emptyTableGroup, + dictionaries.getMetricDictionary() + ) + + request1 = TestingDataApiRequestImpl.buildStableDataApiRequestImpl().withTable(table1) + + LogicalTable table2 = new LogicalTable("TABLE2", DAY, emptyTableGroup, dictionaries.getMetricDictionary()) + + request2 = TestingDataApiRequestImpl.buildStableDataApiRequestImpl().withTable(table2) + + predicate = {it.isUserInRole("GRANT_TABLE1")} as Predicate + securityRules.put("TABLE1", predicate) + + predicate = {it.isUserInRole("GRANT_TABLE2")} as Predicate + securityRules.put("TABLE2", predicate) + + predicate = {it.isUserInRole("GRANT_TABLE2")} as Predicate + securityRules.put("2ELBAT", predicate) + + predicate = { r -> false} + securityRules.put(RoleBasedValidatorRequestMapper.DEFAULT_SECURITY_MAPPER_NAME, predicate); + + securityContext1.isUserInRole("GRANT_TABLE1") >> true + containerRequestContext1.getSecurityContext() >> securityContext1 + + securityContext2.isUserInRole("GRANT_TABLE2") >> true + containerRequestContext2.getSecurityContext() >> securityContext2 + + mapper = new RoleBasedValidatorRequestMapper( + securityRules, + dictionaries, + next, + r -> r.getTable().getName() + ) + + reverseNameMapper = new RoleBasedValidatorRequestMapper( + securityRules, + dictionaries, + next, + (r) -> r.getTable().getName().reverse() + ) + + } + + @Unroll + def "check if tables matching user role are alone queryable"() { + expect: + mapper.internalApply(request, context) == response + + where: + request | context | response + request1 | containerRequestContext1 | request1 + request2 | containerRequestContext2 | request2 + } + + @Unroll + def "check if tables whose name in reverse matches user role are queryable"() { + expect: + reverseNameMapper.internalApply(request, context) == response + + where: + request | context | response + request2 | containerRequestContext2 | request2 + } + + @Unroll + def "check if tables whose name is not in reverse matches user role are queryable"() { + when: + reverseNameMapper.internalApply(request, context) + + then: + thrown exception + + where: + request | context | exception + request1 | containerRequestContext1 | RequestValidationException + + } + + @Unroll + def "check if exception is thrown if user's role does not match the security rules for the logical table"() { + when: + mapper.internalApply(request, context) + + then: + thrown exception + + where: + request | context | exception + request1 | containerRequestContext2 | RequestValidationException + request2 | containerRequestContext1 | RequestValidationException + } +}