dimas-b commented on code in PR #2925:
URL: https://github.com/apache/polaris/pull/2925#discussion_r2470210296
##########
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:
Good point, @adutra ! I agree that roles should be established during
authentication and not re-resolved after that.
However, I may be a pretty big change. IIRC, in case of Polaris-manages
Principals the idea is that the state of permission assignments should be
consistent with catalog data during request execution. This probably means that
the roles entities used by the authenticator should be made available to the
`Resolver` in order to take their version numbers into account when traversing
grant records.
I think the change to use Principal role names in this PR is consistent with
current code. Let's merge this PR and refactor more in follow-up PRs. 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
