You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2026/08/07 08:42:19 UTC
[PR] DefaultAuthenticator: do not merge attributes [polaris]
adutra opened a new pull request, #5254:
URL: https://github.com/apache/polaris/pull/5254
In #5085, typed attributes were introduced in the PolarisPrincipal interface.
That same PR also started merging attributes from `SecurityIdentity` to `PolarisPrincipal` and vice versa, as a convenience to expose the same attributes on both interfaces.
In hindsight, that was probably a bad choice: both interfaces expose typed attributes, so a bulk merge from one to the other could break the type safety guarantees.
Furthermore, the merge was using Guava's `buildKeepingLast()`: likely another bad choice, as duplicate attributes would be silently overrridden.
This PR removes the attribute merge logic entirely.
<!--
📝 Describe what changes you're proposing, especially breaking or user-facing changes.
📖 See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## 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)
--
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] DefaultAuthenticator: do not merge attributes [polaris]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #5254:
URL: https://github.com/apache/polaris/pull/5254#discussion_r3734561887
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -162,15 +162,16 @@ protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials)
protected Map<String, Object> resolvePrincipalAttributes(
SecurityIdentity identity, PrincipalEntity principalEntity, boolean allRolesRequested) {
+ // Do not merge the security identity's attributes into the principal attributes:
+ // these must stay separate.
Review Comment:
This changes previously tested behavior: arbitrary `SecurityIdentity` attributes are no longer propagated into `PolarisPrincipal`. If external integrations relied on reading identity attributes from the principal (or vice versa), this is a potentially breaking behavior change; please check whether a CHANGELOG and/or user-facing auth documentation note is needed.
##########
runtime/service/src/test/java/org/apache/polaris/service/auth/AuthenticatingAugmentorTest.java:
##########
@@ -98,6 +100,7 @@ public void testAugmentSuccessfulAuthentication() {
// Then
assertThat(result).isNotNull();
assertThat(result.getPrincipal()).isSameAs(polarisPrincipal);
- assertThat(result.getPrincipal().getName()).isEqualTo("user1");
+ // principal attributes should not be merged
+ assertThat(result.getAttributes()).isEmpty();
Review Comment:
The assertion `result.getAttributes().isEmpty()` is stricter than the test’s intent (“principal attributes should not be merged”). `QuarkusSecurityIdentity.builder(identity)` preserves any pre-existing identity attributes (e.g., tenant config), so this test could become brittle if identity attributes are added elsewhere; it’s enough to assert that the principal attribute key is not present.
--
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] DefaultAuthenticator: do not merge attributes [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5254:
URL: https://github.com/apache/polaris/pull/5254#issuecomment-5274178654
@dimas-b, I have double checked the #5085. The merging behavior was actually introduced in the PR. I was just a bit concerned about the behavior change. It should be good since we reverted it in this PR.
--
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] DefaultAuthenticator: do not merge attributes [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #5254:
URL: 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] DefaultAuthenticator: do not merge attributes [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #5254:
URL: https://github.com/apache/polaris/pull/5254#discussion_r3734577136
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -162,15 +162,16 @@ protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials)
protected Map<String, Object> resolvePrincipalAttributes(
SecurityIdentity identity, PrincipalEntity principalEntity, boolean allRolesRequested) {
+ // Do not merge the security identity's attributes into the principal attributes:
+ // these must stay separate.
Review Comment:
It's indeed breaking, but I don't believe anybody has had time to actually start leveraging the attribute merge 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] DefaultAuthenticator: do not merge attributes [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5254:
URL: https://github.com/apache/polaris/pull/5254#issuecomment-5274056458
> why do we merge the attributes in the PR #5085?
IIRC, #5085 just continued the old approach when `SecurityIdentity` had Principal attributes.
I think this PR clarifies the whole flow. `SecurityIdentity` has `PolarisPrincipal`, the principal has attributes. No duplication. Single "source of truth".
--
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] DefaultAuthenticator: do not merge attributes [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on PR #5254:
URL: https://github.com/apache/polaris/pull/5254#issuecomment-5274118561
^ right, but "convenience" was basically based on old code patterns that used to put more data into `SecurityIdentity`, IIRC.
In any case @adutra posted `In hindsight, that was probably a bad choice`.
--
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] DefaultAuthenticator: do not merge attributes [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on PR #5254:
URL: https://github.com/apache/polaris/pull/5254#issuecomment-5274086134
> IIRC, https://github.com/apache/polaris/pull/5085 just continued the old approach when SecurityIdentity had (some) Principal attributes.
I think @adutra mentioned in the PR description that merging was introduce in #5085.
> That same PR also started merging attributes from SecurityIdentity to PolarisPrincipal and vice versa, as a convenience to expose the same attributes on both interfaces.
--
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