Skip to content

Commit

Permalink
fix skip non revocable attribute logic
Browse files Browse the repository at this point in the history 
Signed-off-by: Henry Avetisyan <[email protected]>
  • Loading branch information
@havetisyan
havetisyan committed Aug 8, 2024
1 parent ac9d467 commit 61567ba
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 63 deletions.
61 changes: 34 additions & 27 deletions servers/zms/src/main/java/com/yahoo/athenz/zms/DBService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6968,7 +6968,7 @@ private boolean isEarlierDueDate(long newDueDateMillis, Timestamp currentDueDate
}

int getMemberUserAuthorityState(final String roleMemberName, final Set<String> authorityFilterSet,
int currentState, boolean skipRevocableAttributes) {
int currentState) {

boolean bUser = PrincipalUtils.isUserDomainPrincipal(roleMemberName, zmsConfig.getUserDomainPrefix(),
zmsConfig.getAddlUserCheckDomainPrefixList());
Expand All @@ -6979,7 +6979,7 @@ int getMemberUserAuthorityState(final String roleMemberName, final Set<String> 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();
Expand All @@ -6990,12 +6990,11 @@ int getMemberUserAuthorityState(final String roleMemberName, final Set<String> a
return newState;
}

boolean updateUserAuthorityFilter(RoleMember roleMember, final Set<String> userAuthorityFilterSet,
boolean skipRevocableAttributes) {
boolean updateUserAuthorityFilter(RoleMember roleMember, final Set<String> userAuthorityFilterSet) {

int currentState = roleMember.getSystemDisabled() == null ? 0 : roleMember.getSystemDisabled();
int newState = getMemberUserAuthorityState(roleMember.getMemberName(), userAuthorityFilterSet,
currentState, skipRevocableAttributes);
currentState);

if (newState != currentState) {
roleMember.setSystemDisabled(newState);
Expand All @@ -7004,12 +7003,11 @@ boolean updateUserAuthorityFilter(RoleMember roleMember, final Set<String> userA
return false;
}

boolean updateUserAuthorityFilter(GroupMember groupMember, final Set<String> userAuthorityFilterSet,
boolean skipRevocableAttributes) {
boolean updateUserAuthorityFilter(GroupMember groupMember, final Set<String> userAuthorityFilterSet) {

int currentState = groupMember.getSystemDisabled() == null ? 0 : groupMember.getSystemDisabled();
int newState = getMemberUserAuthorityState(groupMember.getMemberName(), userAuthorityFilterSet,
currentState, skipRevocableAttributes);
currentState);

if (newState != currentState) {
groupMember.setSystemDisabled(newState);
Expand Down Expand Up @@ -7105,8 +7103,7 @@ List<RoleMember> getRoleMembersWithUpdatedDisabledState(List<RoleMember> 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) {
Expand Down Expand Up @@ -7145,8 +7142,7 @@ List<GroupMember> getGroupMembersWithUpdatedDisabledState(List<GroupMember> 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) {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -8490,7 +8486,7 @@ Map<String, List<String>> applyTemplatesForListOfDomains(Map<String, Integer> 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)) {
Expand Down Expand Up @@ -8535,14 +8531,20 @@ void enforceRoleUserAuthorityRestrictions(final String domainName, final String
if (userAuthorityFilter != null) {
List<RoleMember> updatedMembers = new ArrayList<>();
Set<String> 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) {
Expand All @@ -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)) {
Expand Down Expand Up @@ -8602,14 +8604,19 @@ void enforceGroupUserAuthorityRestrictions(final String domainName, final String
List<GroupMember> updatedMembers = new ArrayList<>();
Set<String> 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) {
Expand Down
2 changes: 1 addition & 1 deletion servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
15 changes: 9 additions & 6 deletions servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ZMSUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,21 +291,24 @@ public static String extractServiceName(String domainName, String fullServiceNam
}

public static boolean isUserAuthorityFilterValid(Authority userAuthority, final Set<String> 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;
}
}
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<String> userAuthorityFilterSet) {
for (String filterItem : userAuthorityFilterSet) {
if (userAuthority.isAttributeRevocable(filterItem)) {
return true;
}
}
return false;
}

public static String combineUserAuthorityFilters(final String roleUserAuthorityFilter, final String domainUserAuthorityFilter) {
Expand Down
41 changes: 20 additions & 21 deletions servers/zms/src/test/java/com/yahoo/athenz/zms/DBServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -9668,20 +9667,20 @@ public void testUpdateUserAuthorityFilterRoleMember() {

RoleMember roleMemberJohn = new RoleMember().setMemberName("user.john").setSystemDisabled(null);
Set<String> 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
Expand All @@ -9705,20 +9704,20 @@ public void testUpdateUserAuthorityFilterGroupMember() {

GroupMember groupMemberJohn = new GroupMember().setMemberName("user.john").setSystemDisabled(null);
Set<String> 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
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 61567ba

Please sign in to comment.