From d6956a7876fcd4ef35bfef289c380d47e806090d Mon Sep 17 00:00:00 2001 From: Henry Avetisyan Date: Thu, 5 Sep 2024 12:01:20 -0700 Subject: [PATCH] merge #2702 into jetty 1.12.x branch Signed-off-by: Henry Avetisyan --- .../java/com/yahoo/athenz/zms/ZMSImpl.java | 44 ++++--- .../com/yahoo/athenz/zms/ZMSImplTest.java | 120 +++++++++++++++++- 2 files changed, 140 insertions(+), 24 deletions(-) diff --git a/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java b/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java index a05a3f06595..ea54fc6c934 100644 --- a/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java +++ b/servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java @@ -9981,26 +9981,28 @@ private void validatePutMembershipDecisionAuthorization(final Principal principa // out the authorization in the sys.auth.audit domains if (role.getAuditEnabled() == Boolean.TRUE) { + if (!isAllowedAuditRoleMembershipApproval(principal, domain)) { throw ZMSUtils.forbiddenError("principal " + principal.getFullName() + " is not authorized to approve / reject members", caller); } - return; - } - // otherwise, we're going to do a standard check if the principal - // is authorized to update the domain role membership + } else { + + // otherwise, we're going to do a standard check if the principal + // is authorized to update the domain role membership - if (!isAllowedPutMembershipAccess(principal, domain, role.getName())) { - throw ZMSUtils.forbiddenError("principal " + principal.getFullName() - + " is not authorized to approve / reject members", caller); + if (!isAllowedPutMembershipAccess(principal, domain, role.getName())) { + throw ZMSUtils.forbiddenError("principal " + principal.getFullName() + + " is not authorized to approve / reject members", caller); + } } // if the user is allowed to make changes in the domain but - // the role is review enabled then we need to make sure + // the role is review or audit enabled then we need to make sure // the approver cannot be the same as the requester - if (role.getReviewEnabled() == Boolean.TRUE) { + if (role.getReviewEnabled() == Boolean.TRUE || role.getAuditEnabled() == Boolean.TRUE) { Membership pendingMember = dbService.getMembership(domain.getName(), ZMSUtils.extractRoleName(domain.getName(), role.getName()), @@ -10008,7 +10010,7 @@ private void validatePutMembershipDecisionAuthorization(final Principal principa if (principal.getFullName().equalsIgnoreCase(pendingMember.getRequestPrincipal())) { throw ZMSUtils.forbiddenError("principal " + principal.getFullName() - + " cannot approve his/her own request", caller); + + " cannot approve / reject own request", caller); } } } @@ -10022,26 +10024,28 @@ private void validatePutGroupMembershipDecisionAuthorization(final Principal pri // out the authorization in the sys.auth.audit domains if (group.getAuditEnabled() == Boolean.TRUE) { + if (!isAllowedAuditRoleMembershipApproval(principal, domain)) { throw ZMSUtils.forbiddenError("principal " + principal.getFullName() + " is not authorized to approve / reject members", caller); } - return; - } - // otherwise, we're going to do a standard check if the principal - // is authorized to update the domain group membership + } else { + + // otherwise, we're going to do a standard check if the principal + // is authorized to update the domain group membership - if (!isAllowedPutMembershipAccess(principal, domain, group.getName())) { - throw ZMSUtils.forbiddenError("principal " + principal.getFullName() - + " is not authorized to approve / reject members", caller); + if (!isAllowedPutMembershipAccess(principal, domain, group.getName())) { + throw ZMSUtils.forbiddenError("principal " + principal.getFullName() + + " is not authorized to approve / reject members", caller); + } } // if the user is allowed to make changes in the domain but - // the role is review enabled then we need to make sure + // the role is review or audit enabled then we need to make sure // the approver cannot be the same as the requester - if (group.getReviewEnabled() == Boolean.TRUE) { + if (group.getReviewEnabled() == Boolean.TRUE || group.getAuditEnabled() == Boolean.TRUE) { GroupMembership pendingMember = dbService.getGroupMembership(domain.getName(), ZMSUtils.extractGroupName(domain.getName(), group.getName()), @@ -10049,7 +10053,7 @@ private void validatePutGroupMembershipDecisionAuthorization(final Principal pri if (principal.getFullName().equalsIgnoreCase(pendingMember.getRequestPrincipal())) { throw ZMSUtils.forbiddenError("principal " + principal.getFullName() - + " cannot approve his/her own request", caller); + + " cannot approve / reject own request", caller); } } } diff --git a/servers/zms/src/test/java/com/yahoo/athenz/zms/ZMSImplTest.java b/servers/zms/src/test/java/com/yahoo/athenz/zms/ZMSImplTest.java index 0bf12988648..44a2fdfe86c 100644 --- a/servers/zms/src/test/java/com/yahoo/athenz/zms/ZMSImplTest.java +++ b/servers/zms/src/test/java/com/yahoo/athenz/zms/ZMSImplTest.java @@ -21581,7 +21581,7 @@ public void testPutMembershipDecisionReviewEnabledRoleApproveAddState() { zmsImpl.putMembershipDecision(ctx, domainName, roleName, "user.bob", auditRef, mbr); fail(); } catch (ResourceException ex) { - assertTrue(ex.getMessage().contains("cannot approve his/her own request")); + assertTrue(ex.getMessage().contains("cannot approve / reject own request")); } // revert back to admin principal @@ -21681,7 +21681,7 @@ public void testPutMembershipDecisionReviewEnabledRoleApproveDeleteState() { zmsImpl.putMembershipDecision(ctx, domainName, roleName, "user.bob", auditRef, mbr); fail(); } catch (ResourceException ex) { - assertTrue(ex.getMessage().contains("cannot approve his/her own request")); + assertTrue(ex.getMessage().contains("cannot approve / reject own request")); } // revert back to admin principal @@ -21864,6 +21864,62 @@ public void testPutMembershipDecisionAuditEnabledRoleByOrg() { zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef, null); } + @Test + public void testPutMembershipDecisionAuditEnabledRejectSelfApproval() { + + ZMSImpl zmsImpl = zmsTestInitializer.getZms(); + RsrcCtxWrapper ctx = zmsTestInitializer.getMockDomRsrcCtx(); + final String auditRef = zmsTestInitializer.getAuditRef(); + + final String domainName = "testdomain1"; + final String roleName = "testrole1"; + + TopLevelDomain dom1 = zmsTestInitializer.createTopLevelDomainObject(domainName, "Approval Test Domain1", + "testOrg", "user.user1"); + zmsImpl.postTopLevelDomain(ctx, auditRef, null, dom1); + DomainMeta meta = zmsTestInitializer.createDomainMetaObject("Domain Meta for approval test", "testOrg", + true, true, "12345", 1001); + zmsImpl.putDomainMeta(ctx, domainName, auditRef, null, meta); + zmsImpl.putDomainSystemMeta(ctx, domainName, "auditenabled", auditRef, meta); + + Role auditedRole = zmsTestInitializer.createRoleObject(domainName, roleName, null, "user.john", "user.jane"); + zmsImpl.putRole(ctx, domainName, roleName, auditRef, false, null, auditedRole); + RoleSystemMeta rsm = ZMSTestUtils.createRoleSystemMetaObject(true); + zmsImpl.putRoleSystemMeta(ctx, domainName, roleName, "auditenabled", auditRef, rsm); + + Membership mbr = new Membership(); + mbr.setMemberName("user.bob"); + mbr.setActive(false); + mbr.setApproved(false); + zmsImpl.putMembership(ctx, domainName, roleName, "user.bob", auditRef, false, null, mbr); + + Role resrole = zmsImpl.getRole(ctx, domainName, roleName, false, false, true); + assertEquals(resrole.getRoleMembers().size(), 3); + for (RoleMember rmem : resrole.getRoleMembers()) { + if ("user.bob".equals(rmem.getMemberName())) { + assertFalse(rmem.getApproved()); + } + } + + setupPrincipalAuditedRoleApprovalByOrg(zmsImpl, "user.user1", "testOrg"); + + mbr = new Membership(); + mbr.setMemberName("user.bob"); + mbr.setActive(true); + mbr.setApproved(true); + + try { + zmsImpl.putMembershipDecision(ctx, domainName, roleName, "user.bob", auditRef, mbr); + fail(); + } catch (ResourceException ex) { + assertEquals(ex.getCode(), ResourceException.FORBIDDEN); + assertTrue(ex.getMessage().contains("cannot approve / reject own request")); + } + + cleanupPrincipalAuditedRoleApprovalByOrg(zmsImpl, "testOrg"); + zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef, null); + } + @Test public void testPutMembershipDecisionAuditEnabledRoleByDomain() { @@ -26221,6 +26277,62 @@ public void testPutGroupMembershipDecisionAuditEnabledGroupByDomain() { zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef, null); } + @Test + public void testPutGroupMembershipDecisionAuditEnabledGroupRejectSelfApproval() { + + ZMSImpl zmsImpl = zmsTestInitializer.getZms(); + RsrcCtxWrapper ctx = zmsTestInitializer.getMockDomRsrcCtx(); + final String auditRef = zmsTestInitializer.getAuditRef(); + + final String domainName = "group-dec-by-domain"; + final String groupName = "group1"; + + TopLevelDomain dom1 = zmsTestInitializer.createTopLevelDomainObject(domainName, "Approval Test Domain1", + "testOrg", "user.user1"); + zmsImpl.postTopLevelDomain(ctx, auditRef, null, dom1); + DomainMeta meta = zmsTestInitializer.createDomainMetaObject("Domain Meta for approval test", "testOrg", + true, true, "12345", 1001); + zmsImpl.putDomainMeta(ctx, domainName, auditRef, null, meta); + zmsImpl.putDomainSystemMeta(ctx, domainName, "auditenabled", auditRef, meta); + + Group auditedGroup = zmsTestInitializer.createGroupObject(domainName, groupName, "user.john", "user.jane"); + zmsImpl.putGroup(ctx, domainName, groupName, auditRef, false, null, auditedGroup); + GroupSystemMeta rsm = createGroupSystemMetaObject(true); + zmsImpl.putGroupSystemMeta(ctx, domainName, groupName, "auditenabled", auditRef, rsm); + + GroupMembership mbr = new GroupMembership(); + mbr.setMemberName("user.bob"); + mbr.setActive(false); + mbr.setApproved(false); + zmsImpl.putGroupMembership(ctx, domainName, groupName, "user.bob", auditRef, false, null, mbr); + + Group resgroup = zmsImpl.getGroup(ctx, domainName, groupName, false, true); + assertEquals(resgroup.getGroupMembers().size(), 3); + for (GroupMember rmem : resgroup.getGroupMembers()) { + if ("user.bob".equals(rmem.getMemberName())) { + assertFalse(rmem.getApproved()); + } + } + + setupPrincipalAuditedRoleApprovalByDomain(zmsImpl, "user.user1", domainName); + + mbr = new GroupMembership(); + mbr.setMemberName("user.bob"); + mbr.setActive(true); + mbr.setApproved(true); + + try { + zmsImpl.putGroupMembershipDecision(ctx, domainName, groupName, "user.bob", auditRef, mbr); + fail(); + } catch (ResourceException ex) { + assertEquals(ex.getCode(), ResourceException.FORBIDDEN); + assertTrue(ex.getMessage().contains("cannot approve / reject own request")); + } + + cleanupPrincipalAuditedRoleApprovalByDomain(zmsImpl, domainName); + zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef, null); + } + @Test public void testPutGroupMembershipDecisionAuditEnabledGroupInvalidUser() { @@ -27327,7 +27439,7 @@ public void testPutGroupMembershipDecisionReviewEnabledGroupApproveAddState() { zmsImpl.putGroupMembershipDecision(ctx, domainName, groupName, "user.bob", auditRef, mbr); fail(); } catch (ResourceException ex) { - assertTrue(ex.getMessage().contains("cannot approve his/her own request")); + assertTrue(ex.getMessage().contains("cannot approve / reject own request")); } // revert back to admin principal @@ -27428,7 +27540,7 @@ public void testPutGroupMembershipDecisionReviewEnabledGroupApproveDeleteState() zmsImpl.putGroupMembershipDecision(ctx, domainName, groupName, "user.bob", auditRef, mbr); fail(); } catch (ResourceException ex) { - assertTrue(ex.getMessage().contains("cannot approve his/her own request")); + assertTrue(ex.getMessage().contains("cannot approve / reject own request")); } // revert back to admin principal