visit2rahul commented on code in PR #4420:
URL: https://github.com/apache/polaris/pull/4420#discussion_r3282846343
##########
extensions/auth/ranger/impl/src/main/java/org/apache/polaris/extension/auth/ranger/RangerPolarisAuthorizer.java:
##########
@@ -136,14 +131,8 @@ public void authorizeOrThrow(
}
try {
- if (enforceCredentialRotationRequiredState
- && authzOp != PolarisAuthorizableOperation.ROTATE_CREDENTIALS
- && polarisPrincipal
- .getProperties()
-
.containsKey(PolarisEntityConstants.PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE))
{
- throw new ForbiddenException(
- OPERATION_NOT_ALLOWED_FOR_USER_ERROR, polarisPrincipal.getName(),
authzOp.name());
- }
+ AuthorizationPreConditions.checkCredentialRotationRequired(
Review Comment:
Thank you @dimas-b. Good suggestion - I checked, and `OpaPolarisAuthorizer`
doesn't currently perform the credential-rotation-required check at all (no
reference to `ENFORCE_PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_CHECKING` or
`checkCredentialRotationRequired`).
Adding it there would be a new feature for the OPA authorizer rather than a
refactor of existing code. Would it work to keep this PR scoped to the existing
call sites (`PolarisAuthorizerImpl` + `RangerPolarisAuthorizer`) and file a
follow-up issue for `OpaPolarisAuthorizer`? That would also let @sungwy weigh
in on whether OPA should enforce this at the Java layer or via Rego policy,
which seems like a separate design discussion.
Happy to file the follow-up issue and link it here if that approach works -
thoughts?
@jbonofre @dimas-b please review as your time permits.
--
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]
