Repository: AthenZ/athenz

Issue:
membership decision should reject if the approver is the same as the member being reviewed
if the role is review and/or audit enabled, zms only verifies that the approver is the not same as the requestor. However, it does not verify that the member being reviewed is not the same as the  approver. For example:

- user1 adds user2 to the audit/review enabled role.
- user2 is one of the external approvers.
- user2 reviews the request and approves it so effectively adds himself to the role.

What should have happened is that ZMS should reject this request and user3 needs to review the request and approve user2 

Discussion:

Solution patch:
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 04e3c05cf..dd80b016b 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
@@ -10618,10 +10618,11 @@ public class ZMSImpl implements Authorizer, KeyStore, ZMSHandler {
         // if this is an audit enabled domain then we're going to carry
         // out the authorization in the sys.auth.audit domains
 
+        final String principalName = principal.getFullName();
         if (role.getAuditEnabled() == Boolean.TRUE) {
 
             if (!isAllowedAuditRoleMembershipApproval(principal, domain)) {
-                throw ZMSUtils.forbiddenError("principal " + principal.getFullName()
+                throw ZMSUtils.forbiddenError("principal " + principalName
                         + " is not authorized to approve / reject members", caller);
             }
 
@@ -10631,40 +10632,47 @@ public class ZMSImpl implements Authorizer, KeyStore, ZMSHandler {
             // is authorized to update the domain role membership
 
             if (!isAllowedPutMembershipAccess(principal, domain, role.getName())) {
-                throw ZMSUtils.forbiddenError("principal " + principal.getFullName()
+                throw ZMSUtils.forbiddenError("principal " + principalName
                         + " is not authorized to approve / reject members", caller);
             }
         }
 
         // if the user is allowed to make changes in the domain but
         // the role is review or audit enabled then we need to make sure
-        // the approver cannot be the same as the requester
+        // the approver cannot be the same as the requester or the one
+        // being approved / rejected
 
         if (role.getReviewEnabled() == Boolean.TRUE || role.getAuditEnabled() == Boolean.TRUE) {
 
+            if (principalName.equalsIgnoreCase(roleMember.getMemberName())) {
+                throw ZMSUtils.forbiddenError("principal " + principalName
+                        + " cannot approve / reject own membership", caller);
+            }
+
             Membership pendingMember = dbService.getMembership(domain.getName(),
                     ZMSUtils.extractRoleName(domain.getName(), role.getName()),
                     roleMember.getMemberName(), 0, true);
 
-            if (principal.getFullName().equalsIgnoreCase(pendingMember.getRequestPrincipal())) {
-                throw ZMSUtils.forbiddenError("principal " + principal.getFullName()
+            if (principalName.equalsIgnoreCase(pendingMember.getRequestPrincipal())) {
+                throw ZMSUtils.forbiddenError("principal " + principalName
                         + " cannot approve / reject own request", caller);
             }
         }
     }
 
     private void validatePutGroupMembershipDecisionAuthorization(final Principal principal, final AthenzDomain domain,
-                                                                 final Group group, final GroupMember groupMember) {
+            final Group group, final GroupMember groupMember) {
 
         final String caller = "putgroupmembershipdecision";
 
         // if this is an audit enabled domain then we're going to carry
         // out the authorization in the sys.auth.audit domains
 
+        final String principalName = principal.getFullName();
         if (group.getAuditEnabled() == Boolean.TRUE) {
 
             if (!isAllowedAuditRoleMembershipApproval(principal, domain)) {
-                throw ZMSUtils.forbiddenError("principal " + principal.getFullName()
+                throw ZMSUtils.forbiddenError("principal " + principalName
                         + " is not authorized to approve / reject members", caller);
             }
 
@@ -10674,23 +10682,29 @@ public class ZMSImpl implements Authorizer, KeyStore, ZMSHandler {
             // is authorized to update the domain group membership
 
             if (!isAllowedPutMembershipAccess(principal, domain, group.getName())) {
-                throw ZMSUtils.forbiddenError("principal " + principal.getFullName()
+                throw ZMSUtils.forbiddenError("principal " + principalName
                         + " is not authorized to approve / reject members", caller);
             }
         }
 
         // if the user is allowed to make changes in the domain but
         // the role is review or audit enabled then we need to make sure
-        // the approver cannot be the same as the requester
+        // the approver cannot be the same as the requester or the one
+        // being approved / rejected
 
         if (group.getReviewEnabled() == Boolean.TRUE || group.getAuditEnabled() == Boolean.TRUE) {
 
+            if (principalName.equalsIgnoreCase(groupMember.getMemberName())) {
+                throw ZMSUtils.forbiddenError("principal " + principalName
+                        + " cannot approve / reject own membership", caller);
+            }
+
             GroupMembership pendingMember = dbService.getGroupMembership(domain.getName(),
                     ZMSUtils.extractGroupName(domain.getName(), group.getName()),
                     groupMember.getMemberName(), 0, true);
 
-            if (principal.getFullName().equalsIgnoreCase(pendingMember.getRequestPrincipal())) {
-                throw ZMSUtils.forbiddenError("principal " + principal.getFullName()
+            if (principalName.equalsIgnoreCase(pendingMember.getRequestPrincipal())) {
+                throw ZMSUtils.forbiddenError("principal " + principalName
                         + " cannot approve / reject own request", caller);
             }
         }
diff --git a/servers/zts/src/main/java/com/yahoo/athenz/zts/ZTSImpl.java b/servers/zts/src/main/java/com/yahoo/athenz/zts/ZTSImpl.java
index 20bc3f901..f5b5c1ed9 100644
--- a/servers/zts/src/main/java/com/yahoo/athenz/zts/ZTSImpl.java
+++ b/servers/zts/src/main/java/com/yahoo/athenz/zts/ZTSImpl.java
@@ -2666,9 +2666,9 @@ public class ZTSImpl implements ZTSHandler {
         Set<String> subjectRoles = new HashSet<>();
         dataStore.getAccessibleRoles(data, domainName, subjectIdentity, requestedRoles, false, subjectRoles, false);
 
-        // we return failure if we don't have access to all the roles requested
+        // we return failure if we don't have access to any roles
 
-        if (subjectRoles.size() != requestedRoles.length) {
+        if (subjectRoles.isEmpty()) {
             throw forbiddenError(tokenErrorMessage(caller, subjectIdentity, domainName, requestedRoles),
                     caller, domainName, principalDomain);
         }



Test patch:
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 a03c3e85d..6bda02bca 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
@@ -20680,6 +20680,27 @@ public class ZMSImplTest {
             assertTrue(ex.getMessage().contains("cannot approve / reject own request"));
         }
 
+        // now let's check that we first reject if the member is the approver
+
+        mbr = new Membership();
+        mbr.setMemberName("user.user1");
+        mbr.setActive(false);
+        mbr.setApproved(false);
+        zmsImpl.putMembership(ctx, domainName, roleName, "user.user1", auditRef, false, null, mbr);
+
+        mbr = new Membership();
+        mbr.setMemberName("user.user1");
+        mbr.setActive(true);
+        mbr.setApproved(true);
+
+        try {
+            zmsImpl.putMembershipDecision(ctx, domainName, roleName, "user.user1", auditRef, mbr);
+            fail();
+        } catch (ResourceException ex) {
+            assertEquals(ex.getCode(), ResourceException.FORBIDDEN);
+            assertTrue(ex.getMessage().contains("cannot approve / reject own membership"));
+        }
+
         cleanupPrincipalAuditedRoleApprovalByOrg(zmsImpl, "testOrg");
         zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef, null);
     }
@@ -25231,6 +25252,27 @@ public class ZMSImplTest {
             assertTrue(ex.getMessage().contains("cannot approve / reject own request"));
         }
 
+        // now let's check that we first reject if the member is the approver
+
+        mbr = new GroupMembership();
+        mbr.setMemberName("user.user1");
+        mbr.setActive(false);
+        mbr.setApproved(false);
+        zmsImpl.putGroupMembership(ctx, domainName, groupName, "user.user1", auditRef, false, null, mbr);
+
+        mbr = new GroupMembership();
+        mbr.setMemberName("user.user1");
+        mbr.setActive(true);
+        mbr.setApproved(true);
+
+        try {
+            zmsImpl.putGroupMembershipDecision(ctx, domainName, groupName, "user.user1", auditRef, mbr);
+            fail();
+        } catch (ResourceException ex) {
+            assertEquals(ex.getCode(), ResourceException.FORBIDDEN);
+            assertTrue(ex.getMessage().contains("cannot approve / reject own membership"));
+        }
+        
         cleanupPrincipalAuditedRoleApprovalByDomain(zmsImpl, domainName);
         zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef, null);
     }
diff --git a/servers/zts/src/test/java/com/yahoo/athenz/zts/ZTSImplAccessTokenTest.java b/servers/zts/src/test/java/com/yahoo/athenz/zts/ZTSImplAccessTokenTest.java
index e9dc6bbe3..8e36b983b 100644
--- a/servers/zts/src/test/java/com/yahoo/athenz/zts/ZTSImplAccessTokenTest.java
+++ b/servers/zts/src/test/java/com/yahoo/athenz/zts/ZTSImplAccessTokenTest.java
@@ -3028,23 +3028,22 @@ public class ZTSImplAccessTokenTest {
         ResourceContext context = createResourceContext(principal);
         TokenConfigOptions tokenConfigOptions = createTokenConfigOptions(ztsImpl);
 
-        try {
-            // Request both roles but subject only has access to one
-            AccessTokenRequest accessTokenRequest = new AccessTokenRequest(
-                    "grant_type=urn:ietf:params:oauth:grant-type:token-exchange"
-                    + "&requested_token_type=urn:ietf:params:oauth:token-type:id-jag"
-                    + "&subject_token=" + subjectToken + "&audience=https://athenz.io"
-                    + "&subject_token_type=urn:ietf:params:oauth:token-type:id_token"
-                    + "&scope=coretech:role.writers coretech:role.readers",
-                    tokenConfigOptions);
+        // Request both roles but subject only has access to one
+        AccessTokenRequest accessTokenRequest = new AccessTokenRequest(
+                "grant_type=urn:ietf:params:oauth:grant-type:token-exchange"
+                + "&requested_token_type=urn:ietf:params:oauth:token-type:id-jag"
+                + "&subject_token=" + subjectToken + "&audience=https://athenz.io"
+                + "&subject_token_type=urn:ietf:params:oauth:token-type:id_token"
+                + "&scope=coretech:role.writers coretech:role.readers",
+                tokenConfigOptions);
 
-            ztsImpl.processJAGTokenIssueRequest(context, principal,
-                    accessTokenRequest, "user_domain", "postAccessTokenRequest");
-            fail("Expected ResourceException for partial access");
-        } catch (ResourceException ex) {
-            assertEquals(ex.getCode(), ResourceException.FORBIDDEN);
-        }
-        
+        AccessTokenResponse response = ztsImpl.processJAGTokenIssueRequest(context, principal,
+                accessTokenRequest, "user_domain", "postAccessTokenRequest");
+        assertNotNull(response);
+
+        // verify the response contains a single scope in the response
+
+        assertEquals(response.getScope(), "coretech:role.writers");
         cloudStore.close();
     }