RANGER-5787 : Refactoring the security zone API - #1229
dhavalshah9131 wants to merge 2 commits into
Conversation
| if (bizUtil.isAdmin()) { | ||
| blockAdminFromKMSService(submittedZone); | ||
|
|
||
| if (zoneId != null) { |
There was a problem hiding this comment.
The existing-zone check here blocks the only way to fix a zone that already contains a KMS service (e.g. one created on an older build before this check existed). Delete is blocked below, and ROLE_KEY_ADMIN can't operate on zones, so an admin who submits this zone with the KMS service removed is still rejected — the zone can't be edited or deleted through the API.
The check also adds nothing: if the submitted zone still contains the KMS service, blockAdminFromKMSService(submittedZone) catches it; if it doesn't, that's the removal we want to allow. Suggest checking only the submitted zone on update, dropping the securityZoneStore.getSecurityZone(zoneId) read, and adding a test that an admin can remove a KMS service from an existing zone (PUT without KMS → DELETE should then succeed).
| if (EmbeddedServiceDefsUtil.KMS_IMPL_CLASS_NAME.equals(xServiceDef.getImplclassname())) { | ||
| throw restErrorUtil.createRESTException("KMS Services/Service-Defs are not accessible for Zone operations", MessageEnums.OPER_NOT_ALLOWED_FOR_ENTITY); | ||
| } | ||
| if (EmbeddedServiceDefsUtil.EMBEDDED_SERVICEDEF_KMS_NAME.equals(serviceType)) { |
There was a problem hiding this comment.
This changes what "KMS service" means for the gate. Every other sys-admin/key-admin boundary in security-admin keys on implclassname == KMS_IMPL_CLASS_NAME ServiceREST (getServices filtering, policy CRUD, validateConfig) and RangerBizUtil (the "System Admin cannot create/update/delete KMS Service-Def" check). A key admin can legitimately create a service-def with a different name and the KMS impl class; services under it are key-admin-only everywhere else but would pass this gate for a sys admin.
If the motivation was the two-step lookup / latent NPE on a null xServiceDef, a findServiceDefImplClassByServiceName named query keeps the single round-trip without changing semantics. If name-based is intentional, worth stating in the JIRA since it's a behavior change in a refactor. @mneethiraj for a second opinion.
There was a problem hiding this comment.
Coverage gaps: (1) the old create-path mocks were removed but there's no test asserting create is still blocked for a KMS service; (2) delete-by-id / by-name where the KMS service sits alongside non-KMS services (current tests use a single-entry map); (3) the V2 RangerSecurityZoneChangeRequest path adding a KMS service; (4) findServiceDefTypeByServiceName returning null for an unknown service name.
3 participants
What changes were proposed in this pull request?
Improvement and refactoring the security zone API
How was this patch tested?
Successfully validated the Security Zone API.
Build succeeded with unit tests