Re: [PR] making the external authorizers consume the user-defined properties from the entity attribute [polaris]

["flyingImer (via GitHub)" <gi...@apache.org>]

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


##########
runtime/service/src/main/java/org/apache/polaris/service/auth/PrincipalAttributeAugmentor.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.service.auth;
+
+import io.quarkus.security.identity.AuthenticationRequestContext;
+import io.quarkus.security.identity.SecurityIdentity;
+import io.quarkus.security.identity.SecurityIdentityAugmentor;
+import io.quarkus.security.runtime.QuarkusSecurityIdentity;
+import io.smallrye.mutiny.Uni;
+import jakarta.enterprise.context.ApplicationScoped;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import org.apache.polaris.core.auth.PolarisPrincipal;
+import org.apache.polaris.core.auth.PolarisPrincipalAttributeNamespaces;
+import org.apache.polaris.core.entity.PolarisEntityConstants;
+import org.apache.polaris.core.entity.PrincipalEntity;
+
+/**
+ * A {@link SecurityIdentityAugmentor} that runs after {@link AuthenticatingAugmentor} and projects
+ * selected {@link PrincipalEntity} fields onto namespaced {@link PolarisPrincipal} attributes.
+ *
+ * <p>User-defined properties become {@code polaris.user.*}. Selected internal properties become
+ * {@code polaris.system.*}. If the principal has no {@code PRINCIPAL_ENTITY} attribute (federated
+ * or detached principals), this augmentor is a no-op.
+ */
+@ApplicationScoped
+public class PrincipalAttributeAugmentor implements SecurityIdentityAugmentor {
+
+  /** Must run after {@link AuthenticatingAugmentor}. */
+  public static final int PRIORITY = AuthenticatingAugmentor.PRIORITY - 100;
+
+  @Override
+  public int priority() {
+    return PRIORITY;
+  }
+
+  @Override
+  public Uni<SecurityIdentity> augment(
+      SecurityIdentity identity, AuthenticationRequestContext context) {
+    if (identity.isAnonymous() || !(identity.getPrincipal() instanceof PolarisPrincipal)) {
+      return Uni.createFrom().item(identity);
+    }
+    return Uni.createFrom().item(enrich(identity));
+  }
+
+  static SecurityIdentity enrich(SecurityIdentity identity) {
+    PolarisPrincipal principal = (PolarisPrincipal) identity.getPrincipal();
+    PrincipalEntity entity =
+        principal
+            .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
+            .orElse(null);
+    if (entity == null) {
+      return identity;
+    }
+    Map<String, Object> derived = deriveFromEntity(entity);
+    if (derived.isEmpty()) {
+      return identity;
+    }
+    Map<String, Object> attributes = new LinkedHashMap<>(principal.getAttributes());
+    attributes.putAll(derived);
+    PolarisPrincipal enriched =
+        PolarisPrincipal.of(principal.getName(), attributes, principal.getRoles());
+    return QuarkusSecurityIdentity.builder(identity).setPrincipal(enriched).build();
+  }
+
+  static Map<String, Object> deriveFromEntity(PrincipalEntity entity) {
+    Map<String, Object> derived = new LinkedHashMap<>();
+    Map<String, String> internal = entity.getInternalPropertiesAsMap();
+    putIfPresent(
+        derived,
+        PolarisPrincipalAttributeNamespaces.SYSTEM_CLIENT_ID,
+        internal.get(PolarisEntityConstants.getClientIdPropertyName()));
+    putIfPresent(
+        derived,
+        PolarisPrincipalAttributeNamespaces.SYSTEM_CREDENTIAL_ROTATION_REQUIRED,
+        internal.get(PolarisEntityConstants.PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE));
+    for (Map.Entry<String, String> property : entity.getPropertiesAsMap().entrySet()) {
+      String key = property.getKey();
+      if (key != null && !key.isBlank()) {
+        putIfPresent(
+            derived, PolarisPrincipalAttributeNamespaces.USER_PREFIX + key, property.getValue());
+      }
+    }
+    return derived;
+  }
+
+  private static void putIfPresent(Map<String, Object> derived, String key, String value) {
+    if (value != null && !value.isBlank()) {

Review Comment:
   putIfPresent is also used for user properties, so isBlank() turns a stored value into a missing authorization attribute. The Principal API accepts string property values without a non-blank constraint, and the create and update paths preserve them, so department="" can exist on PrincipalEntity while the derived view omits polaris.user.department. OPA therefore sees absence instead of the stored value, and every consumer of derivedStringAttributes inherits the same collapse. Could we either preserve every non-null user-property value here, or reject blank values at the management boundary and cover that contract in tests and documentation?
   



##########
extensions/auth/ranger/src/main/java/org/apache/polaris/extension/auth/ranger/utils/RangerUtils.java:
##########
@@ -128,12 +128,8 @@ private static Map<String, Object> getResourceAttributes(
   }
 
   private static Map<String, Object> getUserAttributes(PolarisPrincipal principal) {
-    Map<String, String> properties =
-        principal
-            .getAttribute(PolarisPrincipal.PRINCIPAL_ENTITY_ATTRIBUTE_KEY, PrincipalEntity.class)
-            .map(PrincipalEntity::getInternalPropertiesAsMap)
-            .orElse(Collections.emptyMap());
-
-    return properties.isEmpty() ? Collections.emptyMap() : new HashMap<>(properties);
+    Map<String, String> derived =
+        PolarisPrincipalAttributeNamespaces.derivedStringAttributes(principal);

Review Comment:
   IIUC, this derived map stops at the intermediate RangerUserInfo DTO. RangerPolarisAuthorizer sends it through RangerEmbeddedAuthorizer, but the pinned Ranger path builds RangerAccessRequestImpl from only the user name, groups, and roles, and never copies RangerUserInfo.attributes ([source](https://github.com/apache/ranger/blob/0aad6958efb0bd40b1fcac28a7cab2837e80e20e/authz-embedded/src/main/java/org/apache/ranger/authz/embedded/RangerAuthzPlugin.java#L90-L109)). As a result, the Polaris-supplied polaris.user.* values cannot affect a Ranger policy decision. The new test only asserts the DTO, so could we add an authorization-level test whose decision depends on one of these attributes and either wire them into the request surface Ranger actually evaluates, or limit the Ranger claim until that path exists?



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