chore(test): Increase Authorization Test Coverage by sungwy · Pull Request #3332 · apache/polaris

sungwy · 2025-12-24T20:19:04Z

This PR improves authorization test coverage for:

PolarisAuthorizerImpl based unit tests including missing listAssigneePrincipalRoles and listGrants for CatalogRole calls
OpaPolarisAuthorizer integration tests for endpoints that invoke authorization calls in the different handlers (PolarisAdminService, IcebergCatalogHandler, GenericTableCatalogHandler, PolicyCatalogHandler)

While exploring an alternative refactor of the PolarisAuthorizer API (similar to #3228) it became clear that broader coverage of these authorization paths was needed to reason about the behavioral impact of changing RBAC resolution in the authorization flow.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot

Pull request overview

This PR enhances authorization test coverage for the Polaris authorization system by adding comprehensive unit tests for PolarisAuthorizerImpl and integration tests for the OPA-based authorizer across multiple catalog handlers.

Key changes:

Added unit tests for listAssigneePrincipalRolesForCatalogRole and listGrantsForCatalogRole authorization paths
Created OPA integration tests covering Admin Service, Iceberg Catalog Handler, Generic Table Handler, and Policy Catalog Handler endpoints
Enhanced test infrastructure with shared helper methods for catalog/namespace creation and JSON serialization

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
runtime/service/src/test/java/org/apache/polaris/service/admin/PolarisAdminServiceAuthzTest.java	Adds authorization unit tests for catalog role grant listing operations with sufficient and insufficient privilege validation
extensions/auth/opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaPolicyCatalogHandlerIT.java	New integration test file covering OPA authorization for policy catalog endpoints (create, list, attach/detach, delete)
extensions/auth/opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIntegrationTestBase.java	Enhances base test infrastructure with JSON serialization helper, catalog creation utilities, and automatic cleanup mechanism
extensions/auth/opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIntegrationTest.java	Adds authorization integration tests for namespace, principal, principal role, catalog role, and catalog creation operations
extensions/auth/opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIcebergCatalogHandlerIT.java	New integration test file covering OPA authorization for Iceberg catalog operations (namespace listing, table registration/deletion)
extensions/auth/opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaGenericTableHandlerIT.java	New integration test file covering OPA authorization for generic table operations (list, create, drop)
extensions/auth/opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaAdminServiceIT.java	New integration test file covering OPA authorization for admin service operations (catalog roles, grants, principal role bindings)
extensions/auth/opa/tests/build.gradle.kts	Adds Iceberg API and core dependencies required for table metadata operations in integration tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.../src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIcebergCatalogHandlerIT.java

...pa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIntegrationTest.java

...ts/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaGenericTableHandlerIT.java

.../src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIcebergCatalogHandlerIT.java

...pa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIntegrationTest.java

...ests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIntegrationTestBase.java

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

...opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaAdminServiceIT.java

dimas-b

Thanks for increasing test coverage, @sungwy ! The PR LGTM in general... some minor comments below :)

dimas-b · 2025-12-24T22:02:38Z

...opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaAdminServiceIT.java

+        .header("Authorization", "Bearer " + rootToken)
+        .body(toJson(grantRequest))
+        .put(
+            "/api/management/v1/principal-roles/{pr}/catalog-roles/{cat}",


The success of this call under the OPA authorizer makes me think whether we may actually want to forbid it 🤔 Is it meaningful to assign catalog roles to principal roles when RBAC is external?

Yes — that’s actually one of the motivations for this PR. By explicitly encoding the current authorization outcomes in tests, we get a clear, executable snapshot of today’s behavior. Im hoping that'll help us:

refactor the authorization flow and RBAC resolution with confidence, and precisely see what behavior changes as a result

intentionally revisit (and potentially forbid) certain flows as RBAC resolution is removed from the Handlers - for example, whether operations like assigning catalog roles to principal roles should remain valid when authorization is fully externalized in OpaPolarisAuthorizer

Ah... makes sense 😅

dimas-b · 2025-12-24T22:05:22Z

...opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaAdminServiceIT.java

+    given()
+        .contentType(ContentType.JSON)
+        .header("Authorization", "Bearer " + rootToken)
+        .body(


successful and failed requests are identical in all aspects but the auth token, right? Would you mind refactoring these tests to make it explicit (at least share the payload maybe)?

That's a great suggestion - I'll adopt this in my next iteration

dimas-b · 2025-12-24T22:10:48Z

.../src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIcebergCatalogHandlerIT.java

+    rootToken = getRootToken();
+    catalogName = "opa-iceberg-" + UUID.randomUUID().toString().replace("-", "");
+    namespace = "ns_" + UUID.randomUUID().toString().replace("-", "");
+    Path tempDir = Files.createTempDirectory("opa-iceberg");


nit: maybe use @TempDir params injected by JUnit5 so that the test framework would own all temporary files?

...pa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIntegrationTest.java

...ests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIntegrationTestBase.java

dimas-b

LGTM... left some more nit-pickish comments, but they are not critical :)

dimas-b · 2025-12-25T16:49:50Z

.../src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaIcebergCatalogHandlerIT.java

+  @BeforeEach
+  void setupBaseCatalog(@TempDir Path tempDir) throws Exception {
+    rootToken = getRootToken();
+    catalogName = "opa-iceberg-" + UUID.randomUUID().toString().replace("-", "");


nit: why bother replacing - in the UUID when we have those chars in the prefix? 🤔

Great point... I just followed the prior convention from here:

polaris/integration-tests/src/main/java/org/apache/polaris/service/it/test/CatalogFederationIntegrationTest.java

Lines 168 to 171 in 95607db

localCatalogName = "test_catalog_local_" + UUID.randomUUID().toString().replace("-", "");

localCatalogRoleName = "test-catalog-role_" + UUID.randomUUID().toString().replace("-", "");

federatedCatalogName = "test_catalog_external_" + UUID.randomUUID().toString().replace("-", "");

federatedCatalogRoleName = "test-catalog-role_" + UUID.randomUUID().toString().replace("-", "");

no worries... I was just wondering :)

dimas-b · 2025-12-25T16:51:22Z

...s/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaPolicyCatalogHandlerIT.java

+    rootToken = getRootToken();
+    catalogName = "opa-policy-" + UUID.randomUUID().toString().replace("-", "");
+    namespace = "ns_" + UUID.randomUUID().toString().replace("-", "");
+    baseLocation = java.nio.file.Files.createTempDirectory("opa-policy").toUri().toString();


nit: why not use @TempDir everywhere?

missed that one - done!

dimas-b · 2026-01-08T16:04:59Z

This PR is two weeks old 😅 (although it was over holidays) if there are no further comments, I'm going to merge tomorrow.

sungwy · 2026-01-13T00:49:05Z

@dimas-b and @adutra - thank you for the reviews. Agree that it's about time we merged this in. 🙂
Could I ask for your help with that?

* increase test coverage * update docstrings * clean up * make use of same helper method * remove duplicate tests from OpaIntegrationTest * use tempdir

* feat: Add AWS STS Session Tags support for credential vending (apache#3327) * feat: Add AWS STS Session Tags support for credential vending Fixes apache#3325 * Minor fixes * Review comments * Review comments. * Remove request_id from the PR * Review comment: added activated roles * Review comments * Update AwsSessionTagsBuilder.java * Spotless fixes * Update StorageCredentialCacheKey.java * fix failing tests * Review comments. * Reverting last commit based on review comments for cache collisions. --------- Co-authored-by: Anand Kumar Sankaran <anand.sankaran@workday.com> * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.24-2.1767639862 (apache#3383) * Added HttpRoute and Gateway to Helm Chart (apache#3314) * Added httproute and gateway * added to readme * updated helm site docs * updated changelog * Added tests * fixed broken test * fixed test part 2 * removed odd comment * added check for httproute and gateway * shuffled the gateway documentation * better gateway instructions * removed extra case in validateRouting * Errorprone: prepare for v2.46.0 (apache#3384) This tackles the current failure in apache#3382: `-XDaddTypeAnnotationsToSymbol=true is required by Error Prone on JDK 21` * NoSQL: Metastore maintenance (apache#3268) Implementation of the NoSQL meta-store maintenance implementation. It adds the meta-store specific handling to the existing NoSQL maintenance service to purge unreferenced and unneeded data from the database. * Update release workflows to use the new GPG private key (apache#3399) * [tech debt] Cleanup `gradle/libs.versions.toml` (apache#3394) This change removes unreferenced dependency references, inlines single usages of a version-reference and adds a dependency for checkstyle to get that version managed by renovate. * fix(deps): update dependency org.keycloak:keycloak-admin-client to v26.0.8 (apache#3405) * fix(deps): update dependency com.google.errorprone:error_prone_core to v2.46.0 (apache#3382) * [doc]: Add Minio OSS disclaimer (apache#3390) * [Releasy] Let Maven artifact publication propagate failures (apache#3407) The Gradle build to publish the Maven artifacts is invoked like `./gradlew ... | tee <log>`. The (overall) exit code of pipes is the exit code of the _last_ command. The exit codes of all pipe "parts" is available in bash's `PIPESTATUS` array and needs to be checked. * Fix Gradle up-to-date of jars (apache#3401) The change apache#1036 added the Maven pom.xml to all built jars. However, the `GenerateMavenPom` Gradle task type is never up-to-date, in consequence no jar can ever be up-to-date, leading to unnecessarily longer build times. This change ensures that the pom.xml is included in release builds, but not in developer/snapshot builds. * fix(deps): update dependency net.ltgt.gradle:gradle-errorprone-plugin to v4.4.0 (apache#3406) * fix(deps): update dependency com.puppycrawl.tools:checkstyle to v13 (apache#3403) * fix(deps): update dependency io.opentelemetry:opentelemetry-bom to v1.58.0 (apache#3408) * fix(deps): update dependency software.amazon.awssdk:bom to v2.41.5 (apache#3416) * chore(deps): update dependency jupyterlab to v4.5.2 (apache#3419) * Release workflows should retry svn checkout in case of failure (apache#3393) * Change parser option to be required (apache#3413) * Support hierarchical namespace in Azure (apache#3347) * Support hierarchical namespace in Azure * Add `hierarchical` to `AzureStorageConfigInfo` (the default is unset translating to current behaviour). * Use `DataLakeDirectoryClient` instead of `DataLakeFileSystemClient` for generating SAS tokens when `hierarchical` is set to `true`. * Add `cloudTest` classes for testing with credential vending in ADLS. * chore(test): Increase Authorization Test Coverage (apache#3332) * increase test coverage * update docstrings * clean up * make use of same helper method * remove duplicate tests from OpaIntegrationTest * use tempdir * fix(deps): update dependency io.micrometer:micrometer-bom to v1.16.2 (apache#3422) * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.24-2.1767878250 (apache#3421) * Last merged commit 1996156 * Add free-disk-space action * Add free-disk-space action to regtest + spark_client_regtests --------- Co-authored-by: Anand K Sankaran <lists@anands.net> Co-authored-by: Anand Kumar Sankaran <anand.sankaran@workday.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: cccs-cat001 <56204545+cccs-cat001@users.noreply.github.com> Co-authored-by: Pierre Laporte <pierre@pingtimeout.fr> Co-authored-by: Yong Zheng <yongzheng0809@gmail.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Sung Yun <107272191+sungwy@users.noreply.github.com>

increase test coverage

700ec86

Copilot AI review requested due to automatic review settings December 24, 2025 20:19

github-project-automation bot added this to Basic Kanban Board Dec 24, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Dec 24, 2025

Copilot started reviewing on behalf of sungwy December 24, 2025 20:19 View session

Copilot AI reviewed Dec 24, 2025

View reviewed changes

sungwy added 2 commits December 24, 2025 15:32

update docstrings

cfd5df7

clean up

b880af0

sungwy requested a review from Copilot December 24, 2025 20:55

Copilot started reviewing on behalf of sungwy December 24, 2025 20:55 View session

Copilot AI reviewed Dec 24, 2025

View reviewed changes

...opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaAdminServiceIT.java Outdated Show resolved Hide resolved

...opa/tests/src/intTest/java/org/apache/polaris/extension/auth/opa/test/OpaAdminServiceIT.java Outdated Show resolved Hide resolved

sungwy added 2 commits December 24, 2025 16:06

make use of same helper method

23e7f96

remove duplicate tests from OpaIntegrationTest

eba05b2

dimas-b reviewed Dec 24, 2025

View reviewed changes

adopt review feedback - thanks Dmitri!

3f148ab

dimas-b previously approved these changes Dec 25, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Dec 25, 2025

use tempdir

23eb1f3

sungwy dismissed dimas-b’s stale review via 23eb1f3 December 26, 2025 11:10

dimas-b approved these changes Dec 29, 2025

View reviewed changes

adutra approved these changes Jan 8, 2026

View reviewed changes

dimas-b merged commit 635bb62 into apache:main Jan 13, 2026
15 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jan 13, 2026

sungwy deleted the auth-test-cov branch January 13, 2026 01:52

	localCatalogName = "test_catalog_local_" + UUID.randomUUID().toString().replace("-", "");
	localCatalogRoleName = "test-catalog-role_" + UUID.randomUUID().toString().replace("-", "");
	federatedCatalogName = "test_catalog_external_" + UUID.randomUUID().toString().replace("-", "");
	federatedCatalogRoleName = "test-catalog-role_" + UUID.randomUUID().toString().replace("-", "");

Conversation

sungwy commented Dec 24, 2025

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sungwy Dec 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b commented Jan 8, 2026

Uh oh!

sungwy commented Jan 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sungwy Dec 24, 2025 •

edited

Loading