dimas-b commented on code in PR #4405:
URL: https://github.com/apache/polaris/pull/4405#discussion_r3227326129
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -34,35 +35,45 @@ public interface PolarisPrincipal extends Principal {
* Creates a new instance of {@link PolarisPrincipal} from the given {@link
PrincipalEntity} and
* roles.
*
- * <p>The created principal will have the same ID and name as the {@link
PrincipalEntity}, and its
- * properties will be derived from the internal properties of the entity.
+ * <p>The created principal will have the same name as the {@link
PrincipalEntity}, and its
+ * properties will be the merger of the entity's user-defined properties and
its internal
+ * properties. If a key appears in both maps, the internal-property value
wins, so that
+ * system-managed values (for example the {@code client_id}) cannot be
shadowed by user input.
*
* @param principalEntity the principal entity representing the user or
service
* @param roles the set of roles associated with the principal
*/
static PolarisPrincipal of(PrincipalEntity principalEntity, Set<String>
roles) {
- return of(
- principalEntity.getName(),
- principalEntity.getInternalPropertiesAsMap(),
- roles,
- Optional.empty());
+ return of(principalEntity, roles, Optional.empty());
}
/**
* Creates a new instance of {@link PolarisPrincipal} from the given {@link
PrincipalEntity} and
* roles.
*
- * <p>The created principal will have the same ID and name as the {@link
PrincipalEntity}, and its
- * properties will be derived from the internal properties of the entity.
+ * <p>The created principal will have the same name as the {@link
PrincipalEntity}, and its
+ * properties will be the merger of the entity's user-defined properties and
its internal
Review Comment:
nit: `the merger` is the actor who merges or the process of merging, but
it's not the result, right? 🤔
##########
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisPrincipal.java:
##########
@@ -34,35 +35,45 @@ public interface PolarisPrincipal extends Principal {
* Creates a new instance of {@link PolarisPrincipal} from the given {@link
PrincipalEntity} and
* roles.
*
- * <p>The created principal will have the same ID and name as the {@link
PrincipalEntity}, and its
- * properties will be derived from the internal properties of the entity.
+ * <p>The created principal will have the same name as the {@link
PrincipalEntity}, and its
+ * properties will be the merger of the entity's user-defined properties and
its internal
+ * properties. If a key appears in both maps, the internal-property value
wins, so that
+ * system-managed values (for example the {@code client_id}) cannot be
shadowed by user input.
*
* @param principalEntity the principal entity representing the user or
service
* @param roles the set of roles associated with the principal
*/
static PolarisPrincipal of(PrincipalEntity principalEntity, Set<String>
roles) {
- return of(
- principalEntity.getName(),
- principalEntity.getInternalPropertiesAsMap(),
- roles,
- Optional.empty());
+ return of(principalEntity, roles, Optional.empty());
}
/**
* Creates a new instance of {@link PolarisPrincipal} from the given {@link
PrincipalEntity} and
* roles.
*
- * <p>The created principal will have the same ID and name as the {@link
PrincipalEntity}, and its
- * properties will be derived from the internal properties of the entity.
+ * <p>The created principal will have the same name as the {@link
PrincipalEntity}, and its
+ * properties will be the merger of the entity's user-defined properties and
its internal
+ * properties. If a key appears in both maps, the internal-property value
wins, so that
+ * system-managed values (for example the {@code client_id}) cannot be
shadowed by user input.
*
* @param principalEntity the principal entity representing the user or
service
* @param roles the set of roles associated with the principal
* @param token the access token of the current user
*/
static PolarisPrincipal of(
PrincipalEntity principalEntity, Set<String> roles, Optional<String>
token) {
- return of(
- principalEntity.getName(),
principalEntity.getInternalPropertiesAsMap(), roles, token);
+ return of(principalEntity.getName(),
mergeEntityProperties(principalEntity), roles, token);
Review Comment:
This change looks fine in isolation. However, from #4291 I guess that the
real intent is to ensure the `Authenticator` implementations forward
user-settable principal properties to `PolarisPrincipal`
Note: Authenticators are pluggable, so whether that happens in all
environments is not 100% certain.
This is not a blocker, just a notice for awareness.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
