Core: Extract shared credential rotation pre-condition check (Closes #4026)#4420
Core: Extract shared credential rotation pre-condition check (Closes #4026)#4420visit2rahul wants to merge 2 commits into
Conversation
…4026) Move the duplicated PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATE check from PolarisAuthorizerImpl and RangerPolarisAuthorizer into a shared AuthorizationPreConditions utility class. Both authorizer implementations had identical logic to enforce credential rotation before allowing non-ROTATE_CREDENTIALS operations. This change eliminates the duplication as suggested in the PR apache#3928 review.
There was a problem hiding this comment.
I believe this is not used anymore with your change.
There was a problem hiding this comment.
Done in 4244163 — removed OPERATION_NOT_ALLOWED_FOR_USER_ERROR from RangerPolarisAuthorizer. Confirmed via grep it wasn't referenced anywhere after the extraction.
| * Common pre-condition checks shared across authorizer implementations for credential-related | ||
| * operations. | ||
| */ | ||
| public final class AuthorizationPreConditions { |
There was a problem hiding this comment.
I would suggest to add a unit test with the different conditions:
- flag off, no exception, regardless of property/op
- flag on + rotation property absent, no exception
- flag on + rotation property present + op == ROTATE_CREDENTIALS, no exception
- flag on + rotation property present + op != ROTATE_CREDENTIALS, ForbiddenException
There was a problem hiding this comment.
Thanks @jbonofre Added in 4244163 — AuthorizationPreConditionsTest covers all four cases:
testFlagOff_noException— flag off, no exception regardless of property/optestFlagOn_nonRotationOp_propertyNotSet_noException— flag on + property absenttestFlagOn_rotateCredentialsOp_noException— flag on + property present + op == ROTATE_CREDENTIALStestFlagOn_nonRotationOp_propertySet_throwsForbidden— flag on + property present + op != ROTATE_CREDENTIALS
|
@jbonofre thanks for the review — addressed both points in 4244163:
|
2 participants
Description:
Closes #4026
Summary
Extracts the duplicated
PRINCIPAL_CREDENTIAL_ROTATION_REQUIRED_STATEpre-condition check fromPolarisAuthorizerImplandRangerPolarisAuthorizerinto a sharedAuthorizationPreConditionsutility class.Both authorizer implementations had identical logic to block non-
ROTATE_CREDENTIALSoperations when a principal is flagged for mandatory credential rotation. This duplication was flagged during the PR #3928review.
Changes
AuthorizationPreConditions.checkCredentialRotationRequired()utility method inpolaris-corePolarisAuthorizerImplandRangerPolarisAuthorizernow delegate to the shared methodPolarisEntityConstantsimport fromRangerPolarisAuthorizerTest Plan
polaris-core:test— auth tests passpolaris-extensions-auth-ranger-tests:test— Ranger tests pass