flyrain commented on code in PR #5112:
URL: https://github.com/apache/polaris/pull/5112#discussion_r3616159021
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/DefaultAuthenticator.java:
##########
@@ -180,19 +180,21 @@ protected Set<String> resolvePrincipalRoles(
.filter(entity -> entity.getType() ==
PolarisEntityType.PRINCIPAL_ROLE)
.map(PrincipalRoleEntity::of)
.map(PrincipalRoleEntity::getName)
- .filter(includeRoleFilter)
+ .filter(
+ role -> requestedRoles.allRolesRequested() ||
requestedRoles.roles().contains(role))
.collect(Collectors.toSet());
- if (requestedRoles != ALL_ROLES_REQUESTED &&
!activeRoles.containsAll(requestedRoles)) {
+ if (!requestedRoles.allRolesRequested() &&
!activeRoles.containsAll(requestedRoles.roles())) {
LOGGER
.atWarn()
.addKeyValue("principal", principal.getName())
.addKeyValue("credentials", credentials)
.addKeyValue("roles", activeRoles)
.log("Some principal roles were not found in the principal's
grants");
+ throw new NotAuthorizedException("Unable to authenticate");
Review Comment:
This flips a client-facing behavior, not just a clarification. Before, a
client requesting a role it wasn't granted got a downscoped success (the role
was filtered out); now the whole authentication throws `NotAuthorizedException`
(401). It's a good fail-closed tightening and the deny-path tests are updated
to match, just confirm it's intended and give it a release note + a mention in
the PR description, since deployments relying on the old lenient downscoping
will start seeing 401s.
--
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]
