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/07/24 12:30:55 UTC

[PR] [DRAFT] Switch principal attributes to AttributeMap [polaris]

adutra opened a new pull request, #5140:
URL: https://github.com/apache/polaris/pull/5140

   Requires #5139 
   
   ## 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] Switch principal attributes to AttributeMap [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5456578892

   > > 1.7.0 breaking-change note: it still documents the generic Map attributes bag, constants on PolarisPrincipal, and getAttribute(key, type). Could we update it to match the AttributeMap/PolarisPrincipalAttributes API?
   > 
   > We cannot change the changelog of a released version, but you are right that we should inform users of the change. I will add a new note under "breaking changes" for the next release.
   > 
   > > I would model them independently rather than treat all three as one class of attributes.
   > 
   > Can we tackle that as a follow-up? Especially given that a [design doc](https://docs.google.com/document/d/1Zztz-owFDzZxAFgNHusd-X_7XLJx0fu8NpCOedrKm7w/edit?tab=t.0) has been proposed to rationalize principal attributes, I think not only the attributes may change where they are declared, but the keys themselves may change.
   
   Yes, I’m okay handling the role-selection/JWT shape in a follow-up. One caveat: the linked provenance design explicitly makes redesigning PolarisPrincipal and removing PRINCIPAL_ENTITY_ATTRIBUTE_KEY non-goals, so it does not actually own that follow-up; please track it separately. The raw SecurityIdentity passthrough is gone and the changelog now covers this migration, so those earlier points no longer block 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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5167129792

   > I agree with the type-safety concern: AttributeKey<T> does not enforce T when keys compare by their string value.
   
   That's by design: if keys compared by string value and type, that would be very awkward. So the contract is that consumers of the map know in advance the keys and their types. I think that's a net improvement over untyped maps.
   
   The attribute map pattern is widely used by Quarkus itself, Netty, AWS SDK, Spark, etc. etc. I really don't understand why it's so unthinkable to have the same pattern in Polaris.
   
   > For the latter, provenance/namespace and an explicit forwarding allowlist matter more than nominal generic typing: an authorizer must be able to distinguish trusted Polaris-derived facts from IdP claims or other supplied attributes.
   
   Fair enough: let's introduce a `PolarisPrincipalAttribute<T>` interface exposing a key, value, along with provenance, namespace and whatnot. We'd still need an `AttributeMap` though to store and retrieve those.


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3657983713


##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -19,53 +19,14 @@
 package org.apache.polaris.core.auth;
 
 import java.security.Principal;
-import java.util.Map;
-import java.util.Optional;
 import java.util.Set;
+import org.apache.polaris.core.collection.AttributeMap;
 import org.apache.polaris.immutables.PolarisImmutable;
-import org.immutables.value.Value;
 
 /** Represents a {@link Principal} in the Polaris system. */
 @PolarisImmutable
 public interface PolarisPrincipal extends Principal {
 
-  /**
-   * Attribute key for the principal entity attribute, of type {@link
-   * org.apache.polaris.core.entity.PrincipalEntity}.
-   *
-   * <p>Note: callers must never assume that this attribute is present.
-   */
-  String PRINCIPAL_ENTITY_ATTRIBUTE_KEY = "org.apache.polaris.core.auth.PRINCIPAL_ENTITY";

Review Comment:
   Moved to a separate `PolarisPrincipalAttributes` class.



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.

dimas-b commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3658959529


##########
runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticatingAugmentor.java:
##########
@@ -61,11 +64,14 @@ public Uni<SecurityIdentity> augment(
 
   private SecurityIdentity authenticatePolarisPrincipal(SecurityIdentity identity) {
     PolarisPrincipal polarisPrincipal = authenticator.authenticate(identity);
+    Map<String, Object> attributes =
+        polarisPrincipal.getAttributes().entrySet().stream()
+            .collect(Collectors.toMap(entry -> entry.key().key(), AttributeMap.Attribute::value));
     return QuarkusSecurityIdentity.builder(identity)
         .setAnonymous(false)
         .setPrincipal(polarisPrincipal)
         .addRoles(polarisPrincipal.getRoles())
-        .addAttributes(polarisPrincipal.getAttributes())
+        .addAttributes(attributes)

Review Comment:
   Looks like this code is gone :sweat_smile: 



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5331047298

   FYI I rebased to fix the merge conflicts, PTAL again 🙏 


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5357770598

   > 1.7.0 breaking-change note: it still documents the generic Map attributes bag, constants on PolarisPrincipal, and getAttribute(key, type). Could we update it to match the AttributeMap/PolarisPrincipalAttributes API?
   
   We cannot change the changelog of a released version, but you are right that we should inform users of the change. I will add a new note under "breaking changes" for the next release.
   
   > I would model them independently rather than treat all three as one class of attributes.
   
   Can we tackle that as a follow-up? Especially given that a [design doc](https://docs.google.com/document/d/1Zztz-owFDzZxAFgNHusd-X_7XLJx0fu8NpCOedrKm7w/edit?tab=t.0) has been proposed to rationalize principal attributes, I think not only the attributes may change where they are declared, but the keys themselves may change.


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "Copilot (via GitHub)" <gi...@apache.org>.

Copilot commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3658123358


##########
runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticatingAugmentor.java:
##########
@@ -61,11 +64,14 @@ public Uni<SecurityIdentity> augment(
 
   private SecurityIdentity authenticatePolarisPrincipal(SecurityIdentity identity) {
     PolarisPrincipal polarisPrincipal = authenticator.authenticate(identity);
+    Map<String, Object> attributes =
+        polarisPrincipal.getAttributes().entrySet().stream()
+            .collect(Collectors.toMap(entry -> entry.key().key(), AttributeMap.Attribute::value));

Review Comment:
   `Collectors.toMap(...)` uses `Map.merge(...)` internally, which throws if any collected value is `null`. `AttributeMap` explicitly allows `null` values, so converting attributes this way can fail at runtime when a principal attribute is present-but-null.



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3887080446


##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -74,23 +35,16 @@ public interface PolarisPrincipal extends Principal {
    * @param attributes the attributes of the principal
    * @param roles the set of roles associated with the principal
    */
-  static PolarisPrincipal of(String name, Map<String, Object> attributes, Set<String> roles) {
+  static PolarisPrincipal of(String name, AttributeMap attributes, Set<String> roles) {
     return ImmutablePolarisPrincipal.builder()
         .name(name)
-        .attributes(attributes)
+        .attributes(AttributeMap.copyOf(attributes))

Review Comment:
   Good point. There is actually a much simpler solution: make `getAttributes()` return `ImmutableAttributeMap`.



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5410660748

   @snazy @flyingImer can I have a second review here please? 🙏 


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5096032660

   > > I guess I'm a bit late to this topic. I wonder is the intent supporting ABAC authz? do we have any pointer to the design doc that can better navigate me throughout prs?
   > 
   > The intent is not to support ABAC, but just to introduce a type-safe mechanism to retrieve principal attributes.
   > 
   > Principal attributes predate this PR. Granted, one could leverage principal attributes to implement some form of ABAC, but this PR does not make any visible step in that direction.
   
   Thanks Alex for the quick turnaround. In that case, could you please point me or share on why we need this type-safe mechanism change?
   
   I noticed https://github.com/apache/polaris/pull/5139, but not quite caught up on why 


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5095920205

   > I guess I'm a bit late to this topic. I wonder is the intent supporting ABAC authz? do we have any pointer to the design doc that can better navigate me throughout prs?
   
   The intent is not to support ABAC, but just to introduce a type-safe mechanism to retrieve principal attributes.
   
   Principal attributes predate this PR. Granted, one could leverage principal attributes to implement some form of ABAC, but this PR does not make any visible step in that direction.


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5167120249

> There are two different kinds of data on the principal today. The first kind: 3 values Polaris itself produces, where Polaris always knows the type ahead of time. That's the resolved PrincipalEntity, the PRINCIPAL_ROLE_ALL boolean, and an optional JWT string.

No. Polaris does not "always know" these attributes. In fact, they are very specific to the internal authentication mechanism. If/when external principals are enacted (see #5119) both the resolved PrincipalEntity and the PRINCIPAL_ROLE_ALL boolean attribute become an implementation detail of the internal auth path. Similarly, I know of Polaris deployments today that do not use OAuth2; for those deployments, the JWT attribute simply does not make any sense.

I am strongly opposed to promoting these attributes to the `PolarisPrincipal` API.

> The second kind: an open set of raw attributes copied from the identity provider, where Polaris does not control the keys or the types.

Again, not entirely true. The `SecurityIdentity` API exposes _a very similar type-safe attribute retrieval_ API compared to what `PolarisPrincipal` exposes:

https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/identity/SecurityIdentity.java#L112

So yes, we are copying attributes from `SecurityIdentity` to `PolarisPrincipal` in bulk, but the result is if a caller is able to retrieve a typed attribute from `SecurityIdentity.getAttribute()` then it is also able to retrieve the exact same attribute from `AttributeMap.get()`.

If that's the main point of concern, I'm happy to remove the code that copies attributes from `SecurityIdentity` to `PolarisPrincipal`.

--
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5331055083

   FYI, attributes are not copied anymore between `SecurityIdentity` and `PolarisPrincipal`.


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3685656379


##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -159,17 +162,19 @@ protected PrincipalEntity resolvePrincipalEntity(PolarisCredential credentials)
     return principal;
   }
 
-  protected Map<String, Object> resolvePrincipalAttributes(
+  protected AttributeMap resolvePrincipalAttributes(
       SecurityIdentity identity, PrincipalEntity principalEntity, boolean allRolesRequested) {
-    ImmutableMap.Builder<String, Object> principalAttributes =
-        ImmutableMap.<String, Object>builder()
-            .putAll(identity.getAttributes())
-            .put(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, principalEntity)
-            .put(PolarisPrincipal.PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY, allRolesRequested);
+    ImmutableAttributeMap.Builder principalAttributes = ImmutableAttributeMap.builder();
+    identity
+        .getAttributes()
+        .forEach((key, value) -> principalAttributes.put(new AttributeKey<>(key), value));

Review Comment:
   IIUC, new AttributeKey<>(key) infers AttributeKey<Object> here, since value's static type from identity.getAttributes() is Object. This entry never gets a narrower type than a raw string key would have had. See the general comment for why I think the raw attributes passthrough through AttributeMap isn't earning its keep right now.



##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipalAttributes.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.core.auth;
+
+import org.apache.polaris.core.collection.AttributeMap.AttributeKey;
+import org.apache.polaris.core.entity.PrincipalEntity;
+
+public final class PolarisPrincipalAttributes {
+
+  private PolarisPrincipalAttributes() {}
+
+  /**
+   * Attribute key for the principal entity attribute, of type {@link PrincipalEntity}.
+   *
+   * <p>Note: callers must never assume that this attribute is present.
+   */
+  public static final AttributeKey<PrincipalEntity> PRINCIPAL_ENTITY_ATTRIBUTE_KEY =
+      new AttributeKey<>("org.apache.polaris.core.auth.PRINCIPAL_ENTITY");
+
+  /**
+   * Attribute key, of type {@link Boolean}, used to determine how authorizers should resolve
+   * principal roles.
+   *
+   * <p>When absent or false, authorizers should resolve only the roles returned by {@link
+   * PolarisPrincipal#getRoles()}; if an empty set is returned, then no principal roles should be
+   * resolved.
+   *
+   * <p>When present and true, authorizers should resolve all roles granted to the principal at the
+   * time of check, ignoring roles returned by {@link PolarisPrincipal#getRoles()} (even if an empty
+   * set is returned).
+   *
+   * <p>This attribute is generally present and true when the principal presented a credential
+   * including the pseudo-role {@code PRINCIPAL_ROLE:ALL}.
+   *
+   * <p>Note: Callers must not assume that this attribute is always present.
+   */
+  public static final AttributeKey<Boolean> PRINCIPAL_ROLE_ALL_ATTRIBUTE_KEY =
+      new AttributeKey<>("org.apache.polaris.core.auth.PRINCIPAL_ROLE:ALL");
+
+  /**
+   * Attribute key used to store or retrieve a JSON Web Token (JWT) associated with the {@link
+   * PolarisPrincipal}, of type {@link String}.
+   *
+   * <p>The associated value is expected to represent the JWT provided during authentication and may
+   * be used for further validation or for deriving additional claims.
+   *
+   * <p>Note: callers must never assume that this attribute is present.
+   */
+  public static final AttributeKey<String> JWT_ATTRIBUTE_KEY =
+      new AttributeKey<>("org.apache.polaris.core.auth.JWT");

Review Comment:
   Since PolarisPrincipal only has one implementation (@PolarisImmutable), these 3 constants could be named accessors on the interface instead of AttributeKey entries. That gives a real compile-time type guarantee instead of the string-key convention callers of this class currently have to hold to by hand IMO.



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5101548168

   > why we need this type-safe mechanism change?
   
   Simply to avoid unsafe type casts when retrieving attributes.


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.

dimas-b commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3658180443


##########
runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticatingAugmentor.java:
##########
@@ -61,11 +64,14 @@ public Uni<SecurityIdentity> augment(
 
   private SecurityIdentity authenticatePolarisPrincipal(SecurityIdentity identity) {
     PolarisPrincipal polarisPrincipal = authenticator.authenticate(identity);
+    Map<String, Object> attributes =
+        polarisPrincipal.getAttributes().entrySet().stream()
+            .collect(Collectors.toMap(entry -> entry.key().key(), AttributeMap.Attribute::value));
     return QuarkusSecurityIdentity.builder(identity)
         .setAnonymous(false)
         .setPrincipal(polarisPrincipal)
         .addRoles(polarisPrincipal.getRoles())
-        .addAttributes(polarisPrincipal.getAttributes())
+        .addAttributes(attributes)

Review Comment:
   tangential: Do we really need Polaris-specific principal attributes in `SecurityIdentity`?
   
   Not adding them might resolve some of the concerns with exposing too much or too specific data to potential (server-side) readers. WDYT?



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5763806920

   @flyingImer could you please take another look?


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra merged PR #5140:
URL: https://github.com/apache/polaris/pull/5140


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5092534128

   This PR is ready for review.


-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "adutra (via GitHub)" <gi...@apache.org>.

adutra commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3659092356


##########
runtime/service/src/main/java/org/apache/polaris/service/auth/AuthenticatingAugmentor.java:
##########
@@ -61,11 +64,14 @@ public Uni<SecurityIdentity> augment(
 
   private SecurityIdentity authenticatePolarisPrincipal(SecurityIdentity identity) {
     PolarisPrincipal polarisPrincipal = authenticator.authenticate(identity);
+    Map<String, Object> attributes =
+        polarisPrincipal.getAttributes().entrySet().stream()
+            .collect(Collectors.toMap(entry -> entry.key().key(), AttributeMap.Attribute::value));
     return QuarkusSecurityIdentity.builder(identity)
         .setAnonymous(false)
         .setPrincipal(polarisPrincipal)
         .addRoles(polarisPrincipal.getRoles())
-        .addAttributes(polarisPrincipal.getAttributes())
+        .addAttributes(attributes)

Review Comment:
   It's still there, but I refactored following Copilot's suggestion.
   
   Exposing the exact same attributes on both sides brings coherence and avoids surprises imho. As for exposing too much information: any component that can consume a `PolarisPrincipal` already has access to this information; injecting a `SecurityIdentity` wouldn't expose more. So in the end, I think exposing the same attributes is slightly preferrable. But I'm open to other ideas!



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "snazy (via GitHub)" <gi...@apache.org>.

snazy commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5166883645

I agree with the type-safety concern: `AttributeKey<T>` does not enforce `T` when keys compare by their string value, and wrapping raw IdP attributes as `AttributeKey<Object>` buys very little.

I do not agree that all three reserved values should therefore become named `PolarisPrincipal` accessors, specifically the resolved `PrincipalEntity`. That would move a Polaris persistence/domain object into the authentication and authorization contract and encourage consumers to rely on its presence and internal property shape. An entity-backed principal can use it internally without exposing the entity itself.

I think the contract should instead separate stable, deliberately exposed authentication facts from an open attribute set. For the latter, provenance/namespace and an explicit forwarding allowlist matter more than nominal generic typing: an authorizer must be able to distinguish trusted Polaris-derived facts from IdP claims or other supplied attributes.

So I would keep `PrincipalEntity` behind the principal implementation and use this PR to define the projected authorization facts and provenance boundary, rather than making the backing entity first-class.

--
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] Switch principal attributes to AttributeMap [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on code in PR #5140:
URL: https://github.com/apache/polaris/pull/5140#discussion_r3883296799


##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -74,23 +35,16 @@ public interface PolarisPrincipal extends Principal {
    * @param attributes the attributes of the principal
    * @param roles the set of roles associated with the principal
    */
-  static PolarisPrincipal of(String name, Map<String, Object> attributes, Set<String> roles) {
+  static PolarisPrincipal of(String name, AttributeMap attributes, Set<String> roles) {
     return ImmutablePolarisPrincipal.builder()
         .name(name)
-        .attributes(attributes)
+        .attributes(AttributeMap.copyOf(attributes))

Review Comment:
   The defensive copy in PolarisPrincipal.of(...) only covers that factory path. 
   
   IIUC, immutables treats AttributeMap as a scalar, so ImmutablePolarisPrincipal.builder().from(polarisPrincipal).build(), which TaskExecutorImpl already uses, retains a MutableAttributeMap returned by a custom Authenticator: mutating that source later also changes the clone’s authorization attributes. 
   
   Could we make every ImmutablePolarisPrincipal construction snapshot its attributes and cover builder/from with a source-map mutation test? One option is to normalize through AttributeMap.copyOf in a @Value.Check.
   



-- 
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] Switch principal attributes to AttributeMap [polaris]

Posted by "flyingImer (via GitHub)" <gi...@apache.org>.

flyingImer commented on PR #5140:
URL: https://github.com/apache/polaris/pull/5140#issuecomment-5348213277

   With the raw passthrough removed, I don't think the broader principal-contract redesign needs to block this PR. The remaining blocker for me is the 1.7.0 breaking-change note: it still documents the generic Map attributes bag, constants on PolarisPrincipal, and getAttribute(key, type). Could we update it to match the AttributeMap/PolarisPrincipalAttributes API?
   
   @adutra Thanks for removing the raw SecurityIdentity passthrough; that resolves that part of my review. I agree the three values are not universal, but presence is separate from API shape. Optional/default semantics work for external principals and non-OAuth deployments too. Role selection has stable in-tree semantics, while JWT has the token-propagation use case from #3236, so I would model them independently rather than treat all three as one class of attributes.
   
   > keep PrincipalEntity behind the principal implementation
   
   @snazy I agree with that boundary. My original all-three-accessors suggestion was too broad, but the current public AttributeKey<PrincipalEntity> still exposes the full persistence entity through PolarisPrincipal; changing the lookup shape does not keep it behind the implementation. A projected authorization-facts type seems like the cleaner contract.


-- 
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