adutra commented on code in PR #2925:
URL: https://github.com/apache/polaris/pull/2925#discussion_r2470099896
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -791,22 +788,22 @@ private List<ResolvedPolarisEntity>
resolveAllPrincipalRoles(
PolarisEntityType.PRINCIPAL_ROLE,
PolarisEntityConstants.getRootEntityId(),
gr.getSecurableId()))
+ .filter(Objects::nonNull)
.collect(Collectors.toList());
}
/**
- * Resolve the specified list of principal roles. The SecurityContext is
used to determine whether
- * the principal actually has the roles specified.
+ * Resolve the specified list of principal roles. The PolarisPrincipal is
used to determine
+ * whether the principal actually has the roles specified.
*
- * @param toValidate
- * @param roleNames
* @return the filtered list of resolved principal roles
*/
private List<ResolvedPolarisEntity> resolvePrincipalRolesByName(
Review Comment:
I still think that there is something fishy with the fact that we re-resolve
the principal entity and principal role entities here, at each pass.
The only authoritative source of truth for principal names and roles should
be the `SecurityIdentity` produced during authentication.
If the principal and/or principal roles are modified _after_ the
authentication by some other process, we could get in the awkward situation
where we would fetch different entities from the database, causing a mismatch
with what `SecurityIdentity` exposes.
IMHO, we should stop resolving principals and principal roles altogether
here. That's a prerogative of the `Authenticator`. This class should focus on
resolving _catalog roles_ and _paths_.
--
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]
