diff --git a/servers/zms/src/main/java/com/yahoo/athenz/zms/DBService.java b/servers/zms/src/main/java/com/yahoo/athenz/zms/DBService.java index 4a9c8d73f76..277ba78b9ec 100644 --- a/servers/zms/src/main/java/com/yahoo/athenz/zms/DBService.java +++ b/servers/zms/src/main/java/com/yahoo/athenz/zms/DBService.java @@ -6968,7 +6968,7 @@ private boolean isEarlierDueDate(long newDueDateMillis, Timestamp currentDueDate } int getMemberUserAuthorityState(final String roleMemberName, final Set authorityFilterSet, - int currentState, boolean skipRevocableAttributes) { + int currentState) { boolean bUser = PrincipalUtils.isUserDomainPrincipal(roleMemberName, zmsConfig.getUserDomainPrefix(), zmsConfig.getAddlUserCheckDomainPrefixList()); @@ -6979,7 +6979,7 @@ int getMemberUserAuthorityState(final String roleMemberName, final Set a int newState; if (bUser) { if (ZMSUtils.isUserAuthorityFilterValid(zmsConfig.getUserAuthority(), authorityFilterSet, - roleMemberName, skipRevocableAttributes)) { + roleMemberName)) { newState = currentState & ~Principal.State.AUTHORITY_FILTER_DISABLED.getValue(); } else { newState = currentState | Principal.State.AUTHORITY_FILTER_DISABLED.getValue(); @@ -6990,12 +6990,11 @@ int getMemberUserAuthorityState(final String roleMemberName, final Set a return newState; } - boolean updateUserAuthorityFilter(RoleMember roleMember, final Set userAuthorityFilterSet, - boolean skipRevocableAttributes) { + boolean updateUserAuthorityFilter(RoleMember roleMember, final Set userAuthorityFilterSet) { int currentState = roleMember.getSystemDisabled() == null ? 0 : roleMember.getSystemDisabled(); int newState = getMemberUserAuthorityState(roleMember.getMemberName(), userAuthorityFilterSet, - currentState, skipRevocableAttributes); + currentState); if (newState != currentState) { roleMember.setSystemDisabled(newState); @@ -7004,12 +7003,11 @@ boolean updateUserAuthorityFilter(RoleMember roleMember, final Set userA return false; } - boolean updateUserAuthorityFilter(GroupMember groupMember, final Set userAuthorityFilterSet, - boolean skipRevocableAttributes) { + boolean updateUserAuthorityFilter(GroupMember groupMember, final Set userAuthorityFilterSet) { int currentState = groupMember.getSystemDisabled() == null ? 0 : groupMember.getSystemDisabled(); int newState = getMemberUserAuthorityState(groupMember.getMemberName(), userAuthorityFilterSet, - currentState, skipRevocableAttributes); + currentState); if (newState != currentState) { groupMember.setSystemDisabled(newState); @@ -7105,8 +7103,7 @@ List getRoleMembersWithUpdatedDisabledState(List roleMem if (userAuthorityFilterSet == null) { newState = currentState & ~Principal.State.AUTHORITY_FILTER_DISABLED.getValue(); } else { - newState = getMemberUserAuthorityState(roleMember.getMemberName(), userAuthorityFilterSet, - currentState, false); + newState = getMemberUserAuthorityState(roleMember.getMemberName(), userAuthorityFilterSet, currentState); } if (newState != currentState) { @@ -7145,8 +7142,7 @@ List getGroupMembersWithUpdatedDisabledState(List grou if (userAuthorityFilterSet == null) { newState = currentState & ~Principal.State.AUTHORITY_FILTER_DISABLED.getValue(); } else { - newState = getMemberUserAuthorityState(groupMember.getMemberName(), userAuthorityFilterSet, - currentState, false); + newState = getMemberUserAuthorityState(groupMember.getMemberName(), userAuthorityFilterSet, currentState); } if (newState != currentState) { @@ -8422,7 +8418,7 @@ void processRoleUserAuthorityRestrictions() { for (PrincipalRole role : roles) { try { enforceRoleUserAuthorityRestrictions(role.getDomainName(), role.getRoleName(), - role.getDomainUserAuthorityFilter(), role.getDomainMemberExpiryDays(), true); + role.getDomainUserAuthorityFilter(), role.getDomainMemberExpiryDays()); } catch (Exception ex) { LOG.error("Unable to process user authority restrictions for {}:role.{} - {}", role.getDomainName(), role.getRoleName(), ex.getMessage()); @@ -8458,7 +8454,7 @@ void processGroupUserAuthorityRestrictions() { for (PrincipalGroup group : groups) { try { enforceGroupUserAuthorityRestrictions(group.getDomainName(), group.getGroupName(), - group.getDomainUserAuthorityFilter(), true); + group.getDomainUserAuthorityFilter()); } catch (Exception ex) { LOG.error("Unable to process user authority restrictions for {}:group.{} - {}", group.getDomainName(), group.getGroupName(), ex.getMessage()); @@ -8490,7 +8486,7 @@ Map> applyTemplatesForListOfDomains(Map te } void enforceRoleUserAuthorityRestrictions(final String domainName, final String roleName, - final String domainUserAuthorityFilter, int domainMemberExpiryDays, boolean skipRevocableAttributes) { + final String domainUserAuthorityFilter, int domainMemberExpiryDays) { final String caller = "enforceRoleUserAuthorityRestrictions"; try (ObjectStoreConnection con = store.getConnection(true, true)) { @@ -8535,14 +8531,20 @@ void enforceRoleUserAuthorityRestrictions(final String domainName, final String if (userAuthorityFilter != null) { List updatedMembers = new ArrayList<>(); Set userAuthorityFilterSet = Set.of(userAuthorityFilter.split(",")); - for (RoleMember roleMember : roleMembers) { - if (updateUserAuthorityFilter(roleMember, userAuthorityFilterSet, skipRevocableAttributes)) { - updatedMembers.add(roleMember); + + // if all the attributes are not revocable then there is no point + // to check all the members again, so we're going to skip + + if (ZMSUtils.enforceUserAuthorityFilterCheck(zmsConfig.getUserAuthority(), userAuthorityFilterSet)) { + for (RoleMember roleMember : roleMembers) { + if (updateUserAuthorityFilter(roleMember, userAuthorityFilterSet)) { + updatedMembers.add(roleMember); + } } - } - filterDBUpdated = updateRoleMemberDisabledState(null, con, updatedMembers, domainName, + filterDBUpdated = updateRoleMemberDisabledState(null, con, updatedMembers, domainName, roleName, ZMSConsts.SYS_AUTH_MONITOR, AUDIT_REF_USER_AUTHORITY, caller); + } } if (expiryDBUpdated || filterDBUpdated) { @@ -8557,7 +8559,7 @@ void enforceRoleUserAuthorityRestrictions(final String domainName, final String } void enforceGroupUserAuthorityRestrictions(final String domainName, final String groupName, - final String domainUserAuthorityFilter, boolean skipRevocableAttributes) { + final String domainUserAuthorityFilter) { final String caller = "enforceGroupUserAuthorityRestrictions"; try (ObjectStoreConnection con = store.getConnection(true, true)) { @@ -8602,14 +8604,19 @@ void enforceGroupUserAuthorityRestrictions(final String domainName, final String List updatedMembers = new ArrayList<>(); Set userAuthorityFilterSet = Set.of(userAuthorityFilter.split(",")); - for (GroupMember groupMember : groupMembers) { - if (updateUserAuthorityFilter(groupMember, userAuthorityFilterSet, skipRevocableAttributes)) { - updatedMembers.add(groupMember); + // if all the attributes are not revocable then there is no point + // to check all the members again, so we're going to skip + + if (ZMSUtils.enforceUserAuthorityFilterCheck(zmsConfig.getUserAuthority(), userAuthorityFilterSet)) { + for (GroupMember groupMember : groupMembers) { + if (updateUserAuthorityFilter(groupMember, userAuthorityFilterSet)) { + updatedMembers.add(groupMember); + } } - } - filterDBUpdated = updateGroupMemberDisabledState(null, con, updatedMembers, domainName, - groupName, ZMSConsts.SYS_AUTH_MONITOR, AUDIT_REF_USER_AUTHORITY, caller); + filterDBUpdated = updateGroupMemberDisabledState(null, con, updatedMembers, domainName, + groupName, ZMSConsts.SYS_AUTH_MONITOR, AUDIT_REF_USER_AUTHORITY, caller); + } } if (expiryDBUpdated || filterDBUpdated) { 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 7d6cf321424..64f43e5821c 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 @@ -4407,7 +4407,7 @@ void validateUserPrincipal(final String memberName, boolean validateUserMember, // authority filter configured, we'll check that as well if (userAuthorityFilterSet != null) { - if (!ZMSUtils.isUserAuthorityFilterValid(userAuthority, userAuthorityFilterSet, memberName, false)) { + if (!ZMSUtils.isUserAuthorityFilterValid(userAuthority, userAuthorityFilterSet, memberName)) { throw ZMSUtils.requestError("Invalid member: " + memberName + ". Required user authority filter not valid for the member", caller); } diff --git a/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ZMSUtils.java b/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ZMSUtils.java index 6c9c1f82efb..1d9a80b30ca 100644 --- a/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ZMSUtils.java +++ b/servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ZMSUtils.java @@ -291,11 +291,10 @@ public static String extractServiceName(String domainName, String fullServiceNam } public static boolean isUserAuthorityFilterValid(Authority userAuthority, final Set filterSet, - final String memberName, boolean skipRevocableAttributes) { + final String memberName) { for (String filterItem : filterSet) { - if (!skipAttributeCheck(userAuthority, filterItem, skipRevocableAttributes) - && !userAuthority.isAttributeSet(memberName, filterItem)) { + if (!userAuthority.isAttributeSet(memberName, filterItem)) { LOG.error("Principal {} does not satisfy user authority {} filter", memberName, filterItem); return false; } @@ -303,9 +302,13 @@ public static boolean isUserAuthorityFilterValid(Authority userAuthority, final return true; } - public static boolean skipAttributeCheck(Authority userAuthority, final String attributeName, - boolean skipRevocableAttributes) { - return skipRevocableAttributes || !userAuthority.isAttributeRevocable(attributeName); + public static boolean enforceUserAuthorityFilterCheck(Authority userAuthority, Set userAuthorityFilterSet) { + for (String filterItem : userAuthorityFilterSet) { + if (userAuthority.isAttributeRevocable(filterItem)) { + return true; + } + } + return false; } public static String combineUserAuthorityFilters(final String roleUserAuthorityFilter, final String domainUserAuthorityFilter) { diff --git a/servers/zms/src/test/java/com/yahoo/athenz/zms/DBServiceTest.java b/servers/zms/src/test/java/com/yahoo/athenz/zms/DBServiceTest.java index f8eea85c154..bc4e2662517 100644 --- a/servers/zms/src/test/java/com/yahoo/athenz/zms/DBServiceTest.java +++ b/servers/zms/src/test/java/com/yahoo/athenz/zms/DBServiceTest.java @@ -27,7 +27,6 @@ import com.yahoo.athenz.auth.util.Crypto; import com.yahoo.athenz.common.server.audit.AuditReferenceValidator; import com.yahoo.athenz.common.server.notification.NotificationManager; -import com.yahoo.athenz.common.server.util.AuthzHelper; import com.yahoo.athenz.common.server.util.ResourceUtils; import com.yahoo.athenz.common.server.util.config.dynamic.DynamicConfigInteger; import com.yahoo.athenz.zms.DBService.DataCache; @@ -9203,8 +9202,8 @@ public void testEnforceRoleUserAuthorityRestrictionsEmptyRoles() { // calling the enforce twice - first time we should get null role // and second time role with no members - zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0, false); - zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0, false); + zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0); + zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0); zms.dbService.store = savedStore; } @@ -9244,7 +9243,7 @@ public void testEnforceRoleUserAuthorityExpiryRestrictionsUpdate() { // the request should complete successfully - zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0, false); + zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0); zms.dbService.zmsConfig.setUserAuthority(savedAuthority); zms.dbService.store = savedStore; @@ -9287,7 +9286,7 @@ public void testEnforceRoleUserAuthorityFilterRestrictionsUpdate() { // the request should complete successfully - zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0, false); + zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0); zms.dbService.zmsConfig.setUserAuthority(savedAuthority); zms.dbService.store = savedStore; @@ -9329,7 +9328,7 @@ public void testEnforceRoleUserAuthorityRestrictionsNoUpdate() { // the request should complete successfully - zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0, false); + zms.dbService.enforceRoleUserAuthorityRestrictions(domainName, roleName, null, 0); zms.dbService.zmsConfig.setUserAuthority(savedAuthority); zms.dbService.store = savedStore; @@ -9668,20 +9667,20 @@ public void testUpdateUserAuthorityFilterRoleMember() { RoleMember roleMemberJohn = new RoleMember().setMemberName("user.john").setSystemDisabled(null); Set attributeSet = Set.of("employee"); - assertFalse(zms.dbService.updateUserAuthorityFilter(roleMemberJohn, attributeSet, false)); + assertFalse(zms.dbService.updateUserAuthorityFilter(roleMemberJohn, attributeSet)); roleMemberJohn.setSystemDisabled(0); - assertFalse(zms.dbService.updateUserAuthorityFilter(roleMemberJohn, attributeSet, false)); + assertFalse(zms.dbService.updateUserAuthorityFilter(roleMemberJohn, attributeSet)); roleMemberJohn.setSystemDisabled(1); - assertTrue(zms.dbService.updateUserAuthorityFilter(roleMemberJohn, attributeSet, false)); + assertTrue(zms.dbService.updateUserAuthorityFilter(roleMemberJohn, attributeSet)); assertEquals(roleMemberJohn.getSystemDisabled(), Integer.valueOf(0)); RoleMember roleMemberJane = new RoleMember().setMemberName("user.jane").setSystemDisabled(null); - assertTrue(zms.dbService.updateUserAuthorityFilter(roleMemberJane, attributeSet, false)); + assertTrue(zms.dbService.updateUserAuthorityFilter(roleMemberJane, attributeSet)); assertEquals(roleMemberJane.getSystemDisabled(), Integer.valueOf(1)); - assertFalse(zms.dbService.updateUserAuthorityFilter(roleMemberJane, attributeSet, false)); + assertFalse(zms.dbService.updateUserAuthorityFilter(roleMemberJane, attributeSet)); assertEquals(roleMemberJane.getSystemDisabled(), Integer.valueOf(1)); // reset authority to its original value @@ -9705,20 +9704,20 @@ public void testUpdateUserAuthorityFilterGroupMember() { GroupMember groupMemberJohn = new GroupMember().setMemberName("user.john").setSystemDisabled(null); Set attributeSet = Set.of("employee"); - assertFalse(zms.dbService.updateUserAuthorityFilter(groupMemberJohn, attributeSet, false)); + assertFalse(zms.dbService.updateUserAuthorityFilter(groupMemberJohn, attributeSet)); groupMemberJohn.setSystemDisabled(0); - assertFalse(zms.dbService.updateUserAuthorityFilter(groupMemberJohn, attributeSet, false)); + assertFalse(zms.dbService.updateUserAuthorityFilter(groupMemberJohn, attributeSet)); groupMemberJohn.setSystemDisabled(1); - assertTrue(zms.dbService.updateUserAuthorityFilter(groupMemberJohn, attributeSet, false)); + assertTrue(zms.dbService.updateUserAuthorityFilter(groupMemberJohn, attributeSet)); assertEquals(groupMemberJohn.getSystemDisabled(), Integer.valueOf(0)); GroupMember groupMemberJane = new GroupMember().setMemberName("user.jane").setSystemDisabled(null); - assertTrue(zms.dbService.updateUserAuthorityFilter(groupMemberJane, attributeSet, false)); + assertTrue(zms.dbService.updateUserAuthorityFilter(groupMemberJane, attributeSet)); assertEquals(groupMemberJane.getSystemDisabled(), Integer.valueOf(1)); - assertFalse(zms.dbService.updateUserAuthorityFilter(groupMemberJane, attributeSet, false)); + assertFalse(zms.dbService.updateUserAuthorityFilter(groupMemberJane, attributeSet)); assertEquals(groupMemberJane.getSystemDisabled(), Integer.valueOf(1)); // reset authority to its original value @@ -10467,8 +10466,8 @@ public void testEnforceGroupUserAuthorityRestrictionsEmptyGroups() { // calling the enforce twice - first time we should get null group // and second time group with no members - zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null, false); - zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null, false); + zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null); + zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null); zms.dbService.store = savedStore; } @@ -10512,7 +10511,7 @@ public void testEnforceGroupUserAuthorityExpiryRestrictionsUpdate() { // the request should complete successfully - zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null, false); + zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null); zms.dbService.zmsConfig.setUserAuthority(savedAuthority); zms.dbService.store = savedStore; @@ -10559,7 +10558,7 @@ public void testEnforceGroupUserAuthorityFilterRestrictionsUpdate() { // the request should complete successfully - zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null, false); + zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null); zms.dbService.zmsConfig.setUserAuthority(savedAuthority); zms.dbService.store = savedStore; @@ -10605,7 +10604,7 @@ public void testEnforceGroupUserAuthorityRestrictionsNoUpdate() { // the request should complete successfully - zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null, false); + zms.dbService.enforceGroupUserAuthorityRestrictions(domainName, groupName, null); zms.dbService.zmsConfig.setUserAuthority(savedAuthority); zms.dbService.store = savedStore; diff --git a/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ZMSUtilsTest.java b/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ZMSUtilsTest.java index 1ac5a9c7f16..0fa97a06366 100644 --- a/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ZMSUtilsTest.java +++ b/servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ZMSUtilsTest.java @@ -208,23 +208,23 @@ public void testIsUserAuthorityFilterValid() { Mockito.when(mockAuthority.isAttributeRevocable(anyString())).thenReturn(true); // non-users are always false - assertFalse(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, Set.of("filterList"), "athenz.test", false)); + assertFalse(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, Set.of("filterList"), "athenz.test")); // single filter value - assertTrue(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, Set.of("contractor"), "user.john", false)); - assertFalse(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, Set.of("employee"), "user.john", false)); + assertTrue(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, Set.of("contractor"), "user.john")); + assertFalse(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, Set.of("employee"), "user.john")); // multiple values assertTrue(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, - Set.of("contractor", "local"), "user.john", false)); + Set.of("contractor", "local"), "user.john")); assertTrue(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, - Set.of("local", "contractor"), "user.john", false)); + Set.of("local", "contractor"), "user.john")); assertFalse(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, - Set.of("local", "contractor", "employee"), "user.john", false)); + Set.of("local", "contractor", "employee"), "user.john")); assertFalse(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, - Set.of("local", "employee" ," contractor"), "user.john", false)); + Set.of("local", "employee" ," contractor"), "user.john")); assertFalse(ZMSUtils.isUserAuthorityFilterValid(mockAuthority, - Set.of("employee", "contractor"), "user.john", false)); + Set.of("employee", "contractor"), "user.john")); } @Test @@ -428,4 +428,25 @@ public void testSmallestExpiry() { assertEquals(ZMSUtils.smallestExpiry(expiry1, expiry2), expiry2); assertEquals(ZMSUtils.smallestExpiry(expiry2, expiry1), expiry2); } + + @Test + public void testEnforceUserAuthorityFilterCheck() { + Authority mockAuthority = Mockito.mock(Authority.class); + Mockito.when(mockAuthority.isAttributeRevocable("attr1")).thenReturn(true); + Mockito.when(mockAuthority.isAttributeRevocable("attr2")).thenReturn(false); + Mockito.when(mockAuthority.isAttributeRevocable("attr3")).thenReturn(true); + Mockito.when(mockAuthority.isAttributeRevocable("attr4")).thenReturn(false); + + assertTrue(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr1"))); + assertFalse(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr2"))); + assertTrue(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr3"))); + assertFalse(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr4"))); + + assertTrue(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr1", "attr3"))); + assertTrue(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr1", "attr2"))); + assertTrue(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr2", "attr3"))); + assertTrue(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr1", "attr2", "attr3"))); + + assertFalse(ZMSUtils.enforceUserAuthorityFilterCheck(mockAuthority, Set.of("attr2", "attr4"))); + } }