You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "psy-ns (via GitHub)" <gi...@apache.org> on 2026/08/04 10:28:46 UTC

[PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

psy-ns opened a new pull request, #5237:
URL: https://github.com/apache/polaris/pull/5237

   This PR adds dedicated, isolated unit test coverage for the protected extractRequestedRoles and resolvePrincipalRoles methods in DefaultAuthenticator, addressing Issue #5092.
   
   While behavioral coverage existed via the main authenticate() method, these localized tests act as direct regression guards for the role resolution logic itself.
   
   Key additions in DefaultAuthenticatorTest.java:
   
   extractRequestedRoles tests: Verified prefix stripping (PRINCIPAL_ROLE:), handling of the :ALL flag, filtering of unprefixed invalid scopes, and empty credential scopes.
   
   resolvePrincipalRoles tests: Verified integration with the metastore using the pre-created principalEntity, ensuring that valid subsets and the :ALL flag return correctly granted roles, while requested but ungranted roles strictly throw an AuthenticationFailedException.
   
   ## Checklist
   - [x] ๐Ÿ›ก๏ธ Don't disclose security issues! (contact security@apache.org)
   - [x] ๐Ÿ”— Clearly explained why the changes are needed, or linked related issues: Fixes #5092 
   - [x] ๐Ÿงช Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] ๐Ÿ’ก Added comments for complex logic
   - [ ] ๐Ÿงพ Updated `CHANGELOG.md` (if needed)
   - [ ] ๐Ÿ“š Updated documentation in `site/content/in-dev/unreleased` (if needed)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #5237:
URL: https://github.com/apache/polaris/pull/5237#issuecomment-5214469780

   > One more case is worth adding. It is on the attribute map, not the role set. resolvePrincipalAttributes copies every incoming identity attribute in first. It writes Polaris's own reserved keys second. Then it calls buildKeepingLast, and that's the part worth pausing on. Guava's build() throws on a duplicate key. buildKeepingLast trades that hard failure for a silent overwrite, so if an incoming attribute happens to use the same string as PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY, Polaris's later write wins instead of the build blowing up. That's the right outcome.
   
   Again, I've already offered to remove this bulk copy, which I introduced simply out of convenience. And I'm not opposed to switching to `build()` instead of `buildKeepingLast()`.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5237:
URL: https://github.com/apache/polaris/pull/5237#discussion_r3732554844


##########
runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java:
##########
@@ -518,4 +518,158 @@ private void assertUnauthorized(PolarisCredential credentials) {
         .isInstanceOf(AuthenticationFailedException.class)
         .hasMessageContaining("Unable to authenticate");
   }
-}
+  // --- Tests for extractRequestedRoles ---
+
+  @Test
+  void testExtractRequestedRoles_allReturnsEmptyWithFlag() {
+    PolarisCredential credentials =
+            PolarisCredential.of(null, PRINCIPAL_NAME, Set.of(DefaultAuthenticator.PRINCIPAL_ROLE_ALL));
+    DefaultAuthenticator.PrincipalRoleSelection selection =
+            authenticator.extractRequestedRoles(credentials);
+
+    assertThat(selection.allRolesRequested()).isTrue();
+    assertThat(selection.roles()).isEmpty();
+  }
+
+  @Test
+  void testExtractRequestedRoles_allTakesPrecedenceOverIndividual() {
+    PolarisCredential credentials =
+            PolarisCredential.of(
+                    null,
+                    PRINCIPAL_NAME,
+                    Set.of(
+                            DefaultAuthenticator.PRINCIPAL_ROLE_ALL,
+                            DefaultAuthenticator.PRINCIPAL_ROLE_PREFIX + PRINCIPAL_ROLE1));
+    DefaultAuthenticator.PrincipalRoleSelection selection =
+            authenticator.extractRequestedRoles(credentials);
+
+    assertThat(selection.allRolesRequested()).isTrue();
+    assertThat(selection.roles()).isEmpty();
+  }
+
+  @Test
+  void testExtractRequestedRoles_individualStripsPrefix() {
+    PolarisCredential credentials =
+            PolarisCredential.of(
+                    null,
+                    PRINCIPAL_NAME,
+                    Set.of(
+                            DefaultAuthenticator.PRINCIPAL_ROLE_PREFIX + PRINCIPAL_ROLE1,
+                            DefaultAuthenticator.PRINCIPAL_ROLE_PREFIX + PRINCIPAL_ROLE2));
+    DefaultAuthenticator.PrincipalRoleSelection selection =
+            authenticator.extractRequestedRoles(credentials);
+
+    assertThat(selection.allRolesRequested()).isFalse();
+    assertThat(selection.roles()).containsExactlyInAnyOrder(PRINCIPAL_ROLE1, PRINCIPAL_ROLE2);
+  }
+
+  @Test
+  void testExtractRequestedRoles_onlyInvalidPrefixesFilteredOut() {
+    PolarisCredential credentials =
+            PolarisCredential.of(null, PRINCIPAL_NAME, Set.of("unprefixed", "also-bad"));
+    DefaultAuthenticator.PrincipalRoleSelection selection =
+            authenticator.extractRequestedRoles(credentials);
+
+    assertThat(selection.allRolesRequested()).isFalse();
+    assertThat(selection.roles()).isEmpty();
+  }
+
+  @Test
+  void testExtractRequestedRoles_mixedKeepsOnlyPrefixed() {
+    PolarisCredential credentials =
+            PolarisCredential.of(
+                    null,
+                    PRINCIPAL_NAME,
+                    Set.of(DefaultAuthenticator.PRINCIPAL_ROLE_PREFIX + PRINCIPAL_ROLE1, "unprefixed"));
+    DefaultAuthenticator.PrincipalRoleSelection selection =
+            authenticator.extractRequestedRoles(credentials);
+
+    assertThat(selection.allRolesRequested()).isFalse();
+    assertThat(selection.roles()).containsExactly(PRINCIPAL_ROLE1);
+  }
+
+  @Test
+  void testExtractRequestedRoles_emptyReturnsEmpty() {
+    PolarisCredential credentials = PolarisCredential.of(null, PRINCIPAL_NAME, Set.of());
+    DefaultAuthenticator.PrincipalRoleSelection selection =
+            authenticator.extractRequestedRoles(credentials);
+
+    assertThat(selection.allRolesRequested()).isFalse();
+    assertThat(selection.roles()).isEmpty();
+  }
+
+  // --- Tests for resolvePrincipalRoles ---
+
+  @Test
+  void testResolvePrincipalRoles_allRequestedReturnsAllGranted() {

Review Comment:
   These pin how allRolesRequested gets computed from the credential. Nothing pins how it survives resolvePrincipalAttributes. I'd suggest to add a new test case:
   - Given: credentials naming one specific role, plus an incoming identity that already carries PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY set to true
   - When: authenticate
   - Then: the flag on the result is false and the roles are just that one role.
   
   Worth covering PRINCIPAL_ENTITY_ATTRIBUTE_KEY the same way. Same overwrite path, and the resolved PrincipalEntity is what authorization reads downstream.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5237:
URL: https://github.com/apache/polaris/pull/5237#issuecomment-5861904722

   This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on code in PR #5237:
URL: https://github.com/apache/polaris/pull/5237#discussion_r3884585584


##########
runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java:
##########
@@ -518,4 +518,158 @@ private void assertUnauthorized(PolarisCredential credentials) {
         .isInstanceOf(AuthenticationFailedException.class)
         .hasMessageContaining("Unable to authenticate");
   }
-}
+  // --- Tests for extractRequestedRoles ---
+
+  @Test
+  void testExtractRequestedRoles_allReturnsEmptyWithFlag() {
+    PolarisCredential credentials =
+            PolarisCredential.of(null, PRINCIPAL_NAME, Set.of(DefaultAuthenticator.PRINCIPAL_ROLE_ALL));
+    DefaultAuthenticator.PrincipalRoleSelection selection =
+            authenticator.extractRequestedRoles(credentials);

Review Comment:
   I need to revise my earlier comment that these were the right two methods to pin down directly: I don't think this whole block should pin the helpers directly unless the current helper boundary and PrincipalRoleSelection shape are an intended subclass contract. 
   
   The existing authenticate() tests already cover the caller-visible all/subset/empty/unprefixed/ungranted outcomes, so these direct calls mainly add coupling to that helper shape. 
   
   If subclasses are expected to rely on it, could we make that contract explicit and cover it with a compact parameterized test? Otherwise, one authenticate() test for PRINCIPAL_ROLE:ALL taking precedence over a named role seems to cover the only missing caller-visible behavior.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #5237: Add unit tests for DefaultAuthenticator role resolution logic
URL: https://github.com/apache/polaris/pull/5237


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #5237:
URL: https://github.com/apache/polaris/pull/5237#issuecomment-5214846144

   > > One more case is worth adding. It is on the attribute map, not the role set. resolvePrincipalAttributes copies every incoming identity attribute in first. It writes Polaris's own reserved keys second. Then it calls buildKeepingLast, and that's the part worth pausing on. Guava's build() throws on a duplicate key. buildKeepingLast trades that hard failure for a silent overwrite, so if an incoming attribute happens to use the same string as PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY, Polaris's later write wins instead of the build blowing up. That's the right outcome.
   > 
   > Again, I've already offered to remove this bulk copy, which I introduced simply out of convenience. And I'm not opposed to switching to `build()` instead of `buildKeepingLast()`.
   
   https://github.com/apache/polaris/pull/5254


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add unit tests for DefaultAuthenticator role resolution logic [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.
flyingImer commented on PR #5237:
URL: https://github.com/apache/polaris/pull/5237#issuecomment-5458593259

   > > > One more case is worth adding. It is on the attribute map, not the role set. resolvePrincipalAttributes copies every incoming identity attribute in first. It writes Polaris's own reserved keys second. Then it calls buildKeepingLast, and that's the part worth pausing on. Guava's build() throws on a duplicate key. buildKeepingLast trades that hard failure for a silent overwrite, so if an incoming attribute happens to use the same string as PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY, Polaris's later write wins instead of the build blowing up. That's the right outcome.
   > > 
   > > 
   > > Again, I've already offered to remove this bulk copy, which I introduced simply out of convenience. And I'm not opposed to switching to `build()` instead of `buildKeepingLast()`.
   > 
   > #5254
   
   Thanks, #5254 removes the SecurityIdentity attribute bulk copy, so the overwrite/collision case I asked for is no longer part of this flow. My inline is obsolete and can be resolved.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org