From 912d380c2a73b6ebbaf02acc7e28b6a35eb1d94b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Kokosi=C5=84ski?= Date: Wed, 11 Jan 2023 14:37:03 +0100 Subject: [PATCH] Deprecate getting groups in Oauth2 authentication This feature is not working properly and it is not possible to fix it. The problem is widely understood impersonation: - view SECURITY DEFINER - sessionUser JDBC parameter - identity from view expression for column masking and row filtering Using Oauth2 is able to provide groups only during authentication. However it is unable to provide groups for any impersonated user. --- .../server/security/oauth2/OAuth2Config.java | 4 ++-- .../server/security/TestResourceSecurity.java | 2 +- .../server/security/oauth2/TestOAuth2Config.java | 2 +- docs/src/main/sphinx/security/oauth2.rst | 2 -- .../config.properties | 1 - .../config.properties | 1 - .../singlenode-oauth2-refresh/config.properties | 1 - .../singlenode-oauth2/config.properties | 1 - .../singlenode-oidc-refresh/config.properties | 1 - .../environment/singlenode-oidc/config.properties | 1 - .../jdbc/TestExternalAuthorizerOAuth2.java | 15 --------------- 11 files changed, 4 insertions(+), 27 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Config.java b/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Config.java index a960c8007852..c5a88f798194 100644 --- a/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Config.java +++ b/core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Config.java @@ -157,8 +157,8 @@ public Optional getGroupsField() return groupsField; } - @Config("http-server.authentication.oauth2.groups-field") - @ConfigDescription("Groups field in the claim") + @Config("deprecated.http-server.authentication.oauth2.groups-field") + @ConfigDescription("Groups field in the claim. This configuration is scheduled for removal.") public OAuth2Config setGroupsField(String groupsField) { this.groupsField = Optional.ofNullable(groupsField); diff --git a/core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java b/core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java index e0bf402fb69d..ef1d9ba4e77a 100644 --- a/core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java +++ b/core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java @@ -783,7 +783,7 @@ public void testOAuth2Groups(Optional> groups) .put("web-ui.enabled", "true") .put("http-server.authentication.type", "oauth2") .putAll(getOAuth2Properties(tokenServer)) - .put("http-server.authentication.oauth2.groups-field", GROUPS_CLAIM) + .put("deprecated.http-server.authentication.oauth2.groups-field", GROUPS_CLAIM) .buildOrThrow()) .setAdditionalModule(oauth2Module(tokenServer)) .build()) { diff --git a/core/trino-main/src/test/java/io/trino/server/security/oauth2/TestOAuth2Config.java b/core/trino-main/src/test/java/io/trino/server/security/oauth2/TestOAuth2Config.java index b434b28bc367..a9cfdfba4452 100644 --- a/core/trino-main/src/test/java/io/trino/server/security/oauth2/TestOAuth2Config.java +++ b/core/trino-main/src/test/java/io/trino/server/security/oauth2/TestOAuth2Config.java @@ -64,7 +64,7 @@ public void testExplicitPropertyMappings() .put("http-server.authentication.oauth2.client-secret", "consumer-secret") .put("http-server.authentication.oauth2.scopes", "email,offline") .put("http-server.authentication.oauth2.principal-field", "some-field") - .put("http-server.authentication.oauth2.groups-field", "groups") + .put("deprecated.http-server.authentication.oauth2.groups-field", "groups") .put("http-server.authentication.oauth2.additional-audiences", "test-aud1,test-aud2") .put("http-server.authentication.oauth2.challenge-timeout", "90s") .put("http-server.authentication.oauth2.max-clock-skew", "15s") diff --git a/docs/src/main/sphinx/security/oauth2.rst b/docs/src/main/sphinx/security/oauth2.rst index ad55cbc9a633..caff7fb8c87b 100644 --- a/docs/src/main/sphinx/security/oauth2.rst +++ b/docs/src/main/sphinx/security/oauth2.rst @@ -148,8 +148,6 @@ The following configuration properties are available: for more information. * - ``http-server.authentication.oauth2.principal-field`` - The field of the access token used for the Trino user principal. Defaults to ``sub``. Other commonly used fields include ``sAMAccountName``, ``name``, ``upn``, and ``email``. - * - ``http-server.authentication.oauth2.groups-field`` - - Array-based field in the access token used to list group information for a user. * - ``http-server.authentication.oauth2.oidc.discovery`` - Enable reading the `OIDC provider metadata `_. Default is ``true``. diff --git a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-http-proxy/config.properties b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-http-proxy/config.properties index 28c3b3c8be38..64e7a3f53d4d 100644 --- a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-http-proxy/config.properties +++ b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-http-proxy/config.properties @@ -20,7 +20,6 @@ http-server.authentication.oauth2.jwks-url=http://hydra:4444/.well-known/jwks.js http-server.authentication.oauth2.client-id=trinodb_client_id http-server.authentication.oauth2.client-secret=trinodb_client_secret http-server.authentication.oauth2.user-mapping.pattern=(.*)(@.*)? -http-server.authentication.oauth2.groups-field=groups http-server.authentication.oauth2.oidc.discovery=false oauth2-jwk.http-client.trust-store-path=/docker/presto-product-tests/conf/presto/etc/hydra.pem oauth2-jwk.http-client.http-proxy=proxy:8888 diff --git a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-https-proxy/config.properties b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-https-proxy/config.properties index 7fa9418f58f3..d3560c2305bc 100644 --- a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-https-proxy/config.properties +++ b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-https-proxy/config.properties @@ -20,7 +20,6 @@ http-server.authentication.oauth2.jwks-url=http://hydra:4444/.well-known/jwks.js http-server.authentication.oauth2.client-id=trinodb_client_id http-server.authentication.oauth2.client-secret=trinodb_client_secret http-server.authentication.oauth2.user-mapping.pattern=(.*)(@.*)? -http-server.authentication.oauth2.groups-field=groups http-server.authentication.oauth2.oidc.discovery=false oauth2-jwk.http-client.trust-store-path=/docker/presto-product-tests/conf/presto/etc/cert/truststore.jks oauth2-jwk.http-client.trust-store-password=123456 diff --git a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-refresh/config.properties b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-refresh/config.properties index b4e2cbab6fb1..6a62a169a8e9 100644 --- a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-refresh/config.properties +++ b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2-refresh/config.properties @@ -21,7 +21,6 @@ http-server.authentication.oauth2.jwks-url=http://hydra:4444/.well-known/jwks.js http-server.authentication.oauth2.client-id=trinodb_client_id http-server.authentication.oauth2.client-secret=trinodb_client_secret http-server.authentication.oauth2.user-mapping.pattern=(.*)(@.*)? -http-server.authentication.oauth2.groups-field=groups http-server.authentication.oauth2.refresh-tokens=true http-server.authentication.oauth2.refresh-tokens.issued-token.timeout=30s http-server.authentication.oauth2.oidc.discovery=false diff --git a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2/config.properties b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2/config.properties index 04dfaad7ac89..31c18d830d20 100644 --- a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2/config.properties +++ b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oauth2/config.properties @@ -20,7 +20,6 @@ http-server.authentication.oauth2.jwks-url=http://hydra:4444/.well-known/jwks.js http-server.authentication.oauth2.client-id=trinodb_client_id http-server.authentication.oauth2.client-secret=trinodb_client_secret http-server.authentication.oauth2.user-mapping.pattern=(.*)(@.*)? -http-server.authentication.oauth2.groups-field=groups http-server.authentication.oauth2.oidc.discovery=false oauth2-jwk.http-client.trust-store-path=/docker/presto-product-tests/conf/presto/etc/hydra.pem diff --git a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc-refresh/config.properties b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc-refresh/config.properties index b4703c0604e0..6a452392cce9 100644 --- a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc-refresh/config.properties +++ b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc-refresh/config.properties @@ -18,7 +18,6 @@ http-server.authentication.oauth2.scopes=openid,offline http-server.authentication.oauth2.client-id=trinodb_client_id http-server.authentication.oauth2.client-secret=trinodb_client_secret http-server.authentication.oauth2.user-mapping.pattern=(.*)(@.*)? -http-server.authentication.oauth2.groups-field=groups http-server.authentication.oauth2.refresh-tokens=true http-server.authentication.oauth2.refresh-tokens.issued-token.timeout=30s http-server.authentication.oauth2.oidc.use-userinfo-endpoint=false diff --git a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc/config.properties b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc/config.properties index 9b1323c999c1..0d3de0c078c2 100644 --- a/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc/config.properties +++ b/testing/trino-product-tests-launcher/src/main/resources/docker/presto-product-tests/conf/environment/singlenode-oidc/config.properties @@ -17,7 +17,6 @@ http-server.authentication.oauth2.issuer=http://hydra:4444/ http-server.authentication.oauth2.client-id=trinodb_client_id http-server.authentication.oauth2.client-secret=trinodb_client_secret http-server.authentication.oauth2.user-mapping.pattern=(.*)(@.*)? -http-server.authentication.oauth2.groups-field=groups http-server.authentication.oauth2.oidc.use-userinfo-endpoint=false oauth2-jwk.http-client.trust-store-path=/docker/presto-product-tests/conf/presto/etc/hydra.pem diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/jdbc/TestExternalAuthorizerOAuth2.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/jdbc/TestExternalAuthorizerOAuth2.java index 7bd7d31575e8..82eda57b7444 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/jdbc/TestExternalAuthorizerOAuth2.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/jdbc/TestExternalAuthorizerOAuth2.java @@ -13,7 +13,6 @@ */ package io.trino.tests.product.jdbc; -import com.google.common.collect.ImmutableList; import com.google.inject.Inject; import com.google.inject.name.Named; import io.trino.jdbc.TestingRedirectHandlerInjector; @@ -39,10 +38,8 @@ import java.sql.DriverManager; import java.sql.PreparedStatement; import java.sql.ResultSet; -import java.sql.SQLException; import static com.google.common.base.Preconditions.checkState; -import static io.trino.tempto.assertions.QueryAssert.Row.row; import static io.trino.tempto.assertions.QueryAssert.assertThat; import static io.trino.tempto.query.QueryResult.forResultSet; import static io.trino.tests.product.TestGroups.OAUTH2; @@ -123,18 +120,6 @@ public void shouldAuthenticateAfterTokenExpires() } } - @Test(groups = {OAUTH2, PROFILE_SPECIFIC_TESTS}) - public void shouldReturnGroups() - throws SQLException - { - prepareHandler(); - try (Connection connection = DriverManager.getConnection(jdbcUrl); - PreparedStatement statement = connection.prepareStatement("SELECT array_sort(current_groups())"); - ResultSet rs = statement.executeQuery()) { - assertThat(forResultSet(rs)).containsOnly(row(ImmutableList.of("admin", "public"))); - } - } - private void prepareHandler() { TestingRedirectHandlerInjector.setRedirectHandler(uri -> {