Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting of server default values for member expiry days #2840

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Allow setting of server default values for member expiry days #2840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed2c6e9
[feat] Allow setting of server default values for member expiry days
TakuyaMatsu Dec 26, 2024
de5eb4f
add config
TakuyaMatsu Dec 26, 2024
5f493dd
fix
TakuyaMatsu Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions servers/zms/conf/zms.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,3 +577,10 @@ athenz.zms.no_auth_uri_list=/zms/v1/schema
# of results returned to the specified value. The default value is 100. This prevents
# the server from returning a large number of results when the search criteria is too broad.
#athenz.zms.search_service_limit=100

# This property specifies the maximum expiry duration in days for user/service/group.
# The value must be an integer, and the default value is 0.
# If set to 0, it indicates that there is no expiry limit.
#athenz.zms.default_user_expiry_days=0
Copy link
Collaborator

@havetisyan havetisyan Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's rename these three variables to be athenz.zms.default_max_... (add the max_ component). default value indicates that we'll set this if no value is specified but here we're doing 2 things: first if no value is set, we default to this value and second, if a value is specified then we do not allow the value to be bigger than this (which is max part and not necessarily default).

#athenz.zms.default_service_expiry_days=0
#athenz.zms.default_group_expiry_days=0
4 changes: 4 additions & 0 deletions servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSConsts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ public final class ZMSConsts {
public static final String ZMS_PROP_DOMAIN_ENVIRONMENTS = "athenz.zms.domain_environments";
public static final String ZMS_DEFAULT_DOMAIN_ENVIRONMENTS = "production,integration,staging,sandbox,qa,development";

public static final String ZMS_PROP_DEFAULT_USER_EXPIRY = "athenz.zms.default_user_expiry_days";
public static final String ZMS_PROP_DEFAULT_SERVICE_EXPIRY = "athenz.zms.default_service_expiry_days";
public static final String ZMS_PROP_DEFAULT_GROUP_EXPIRY = "athenz.zms.default_group_expiry_days";

public static final String ZMS_PROP_VALIDATE_USER_MEMBERS = "athenz.zms.validate_user_members";
public static final String ZMS_PROP_VALIDATE_SERVICE_MEMBERS = "athenz.zms.validate_service_members";
public static final String ZMS_PROP_VALIDATE_ASSERTION_ROLES = "athenz.zms.validate_policy_assertion_roles";
Expand Down
47 changes: 14 additions & 33 deletions servers/zms/src/main/java/com/yahoo/athenz/zms/ZMSImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4820,6 +4820,9 @@ public DomainRoleMembers getOverdueReview(ResourceContext ctx, String domainName
}

Timestamp getMemberDueDate(long cfgDueDateMillis, Timestamp memberDueDate) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the testGetMemberDueDate unit test case to include the cfgDueDateMillis == 0 case.

if (cfgDueDateMillis == 0) {
return memberDueDate;
}
if (memberDueDate == null) {
return Timestamp.fromMillis(cfgDueDateMillis);
} else if (memberDueDate.millis() > cfgDueDateMillis) {
Expand Down Expand Up @@ -4893,23 +4896,6 @@ private <T> void updateMemberDueDate(MemberDueDays memberDueDays,
}
}

Timestamp memberDueDateTimestamp(Integer domainDueDateDays, Integer roleDueDateDays, Timestamp memberDueDate) {

long cfgExpiryMillis = ZMSUtils.configuredDueDateMillis(domainDueDateDays, roleDueDateDays);

// if we have no value configured then return
// the membership expiration as is

if (cfgExpiryMillis == 0) {
return memberDueDate;
}

// otherwise compare the configured expiry days with the specified
// membership value and choose the smallest expiration value

return getMemberDueDate(cfgExpiryMillis, memberDueDate);
}

@Override
public Response putMembership(ResourceContext ctx, String domainName, String roleName,
String memberName, String auditRef, Boolean returnObj, String resourceOwner, Membership membership) {
Expand Down Expand Up @@ -5066,6 +5052,7 @@ Timestamp getUserAuthorityExpiry(final String userName, final String expiryAttrV
void setRoleMemberExpiration(final AthenzDomain domain, final Role role, final RoleMember roleMember,
final Membership membership, final String caller) {

MemberDueDays memberExpiryDueDays = new MemberDueDays(domain.getDomain(), role, MemberDueDays.Type.EXPIRY);
switch (Principal.Type.getType(roleMember.getPrincipalType())) {

case USER:
Expand All @@ -5077,44 +5064,39 @@ void setRoleMemberExpiration(final AthenzDomain domain, final Role role, final R

Timestamp userAuthorityExpiry = getUserAuthorityExpiry(roleMember.memberName,
role.getUserAuthorityExpiration(), caller);
Timestamp memberExpiry = memberDueDateTimestamp(domain.getDomain().getMemberExpiryDays(),
role.getMemberExpiryDays(), membership.getExpiration());
Timestamp memberExpiry = getMemberDueDate(memberExpiryDueDays.getUserDueDateMillis(), membership.getExpiration());
roleMember.setExpiration(ZMSUtils.smallestExpiry(memberExpiry, userAuthorityExpiry));
break;

case SERVICE:
case USER_HEADLESS:

roleMember.setExpiration(memberDueDateTimestamp(domain.getDomain().getServiceExpiryDays(),
role.getServiceExpiryDays(), membership.getExpiration()));
roleMember.setExpiration(getMemberDueDate(memberExpiryDueDays.getServiceDueDateMillis(), membership.getExpiration()));
break;

case GROUP:

roleMember.setExpiration(memberDueDateTimestamp(domain.getDomain().getGroupExpiryDays(),
role.getGroupExpiryDays(), membership.getExpiration()));
roleMember.setExpiration(getMemberDueDate(memberExpiryDueDays.getGroupDueDateMillis(), membership.getExpiration()));
break;
}
}

void setRoleMemberReview(final Role role, final RoleMember roleMember,
final Membership membership) {

MemberDueDays memberReminderDueDays = new MemberDueDays(null, role, MemberDueDays.Type.REMINDER);
switch (Principal.Type.getType(roleMember.getPrincipalType())) {
case USER:
roleMember.setReviewReminder(memberDueDateTimestamp(null,
role.getMemberReviewDays(), membership.getReviewReminder()));
roleMember.setReviewReminder(getMemberDueDate(memberReminderDueDays.getUserDueDateMillis(), membership.getReviewReminder()));
break;

case SERVICE:
case USER_HEADLESS:
roleMember.setReviewReminder(memberDueDateTimestamp(null,
role.getServiceReviewDays(), membership.getReviewReminder()));
roleMember.setReviewReminder(getMemberDueDate(memberReminderDueDays.getServiceDueDateMillis(), membership.getReviewReminder()));
break;

case GROUP:
roleMember.setReviewReminder(memberDueDateTimestamp(null,
role.getGroupReviewDays(), membership.getReviewReminder()));
roleMember.setReviewReminder(getMemberDueDate(memberReminderDueDays.getGroupDueDateMillis(), membership.getReviewReminder()));
break;
}
}
Expand Down Expand Up @@ -11164,22 +11146,21 @@ public GroupMembership getGroupMembership(ResourceContext ctx, String domainName
void setGroupMemberExpiration(final AthenzDomain domain, final Group group, final GroupMember groupMember,
final GroupMembership membership, final String caller) {

MemberDueDays memberExpiryDueDays = new MemberDueDays(domain.getDomain(), group);
switch (Principal.Type.getType(groupMember.getPrincipalType())) {

case USER:

Timestamp userAuthorityExpiry = getUserAuthorityExpiry(groupMember.memberName,
group.getUserAuthorityExpiration(), caller);
Timestamp memberExpiry = memberDueDateTimestamp(domain.getDomain().getMemberExpiryDays(),
group.getMemberExpiryDays(), membership.getExpiration());
Timestamp memberExpiry = getMemberDueDate(memberExpiryDueDays.getUserDueDateMillis(), membership.getExpiration());
groupMember.setExpiration(ZMSUtils.smallestExpiry(memberExpiry, userAuthorityExpiry));
break;

case SERVICE:
case USER_HEADLESS:

groupMember.setExpiration(memberDueDateTimestamp(domain.getDomain().getServiceExpiryDays(),
group.getServiceExpiryDays(), membership.getExpiration()));
groupMember.setExpiration(getMemberDueDate(memberExpiryDueDays.getServiceDueDateMillis(), membership.getExpiration()));
break;

case GROUP:
Expand Down
15 changes: 10 additions & 5 deletions servers/zms/src/main/java/com/yahoo/athenz/zms/config/MemberDueDays.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,14 @@
import com.yahoo.athenz.zms.Group;
import com.yahoo.athenz.zms.Role;
import com.yahoo.athenz.zms.utils.ZMSUtils;
import com.yahoo.athenz.zms.ZMSConsts;

public class MemberDueDays {

private static final int DEFAULT_USER_EXPIRY = Integer.parseInt(System.getProperty(ZMSConsts.ZMS_PROP_DEFAULT_USER_EXPIRY, "0"));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here. let's name these DEFAULT_MAX_USER_EXPIRY, etc to correctly indicate the max aspect of the value.

private static final int DEFAULT_SERVICE_EXPIRY = Integer.parseInt(System.getProperty(ZMSConsts.ZMS_PROP_DEFAULT_SERVICE_EXPIRY, "0"));
private static final int DEFAULT_GROUP_EXPIRY = Integer.parseInt(System.getProperty(ZMSConsts.ZMS_PROP_DEFAULT_GROUP_EXPIRY, "0"));

final long userDueDateMillis;
final long serviceDueDateMillis;
final long groupDueDateMillis;
Expand Down Expand Up @@ -59,9 +64,9 @@ public MemberDueDays(Domain domain, Role role, Type type) {
roleGroupDays = role.getGroupReviewDays();
}

userDueDateMillis = ZMSUtils.configuredDueDateMillis(domainUserDays, roleUserDays);
serviceDueDateMillis = ZMSUtils.configuredDueDateMillis(domainServiceDays, roleServiceDays);
groupDueDateMillis = ZMSUtils.configuredDueDateMillis(domainGroupDays, roleGroupDays);
userDueDateMillis = ZMSUtils.configuredDueDateMillis(DEFAULT_USER_EXPIRY, domainUserDays, roleUserDays);
serviceDueDateMillis = ZMSUtils.configuredDueDateMillis(DEFAULT_SERVICE_EXPIRY, domainServiceDays, roleServiceDays);
groupDueDateMillis = ZMSUtils.configuredDueDateMillis(DEFAULT_GROUP_EXPIRY, domainGroupDays, roleGroupDays);
}

public MemberDueDays(Domain domain, Group group) {
Expand All @@ -74,8 +79,8 @@ public MemberDueDays(Domain domain, Group group) {
Integer groupUserDays = group.getMemberExpiryDays();
Integer groupServiceDays = group.getServiceExpiryDays();

userDueDateMillis = ZMSUtils.configuredDueDateMillis(domainUserDays, groupUserDays);
serviceDueDateMillis = ZMSUtils.configuredDueDateMillis(domainServiceDays, groupServiceDays);
userDueDateMillis = ZMSUtils.configuredDueDateMillis(DEFAULT_USER_EXPIRY, domainUserDays, groupUserDays);
serviceDueDateMillis = ZMSUtils.configuredDueDateMillis(DEFAULT_SERVICE_EXPIRY, domainServiceDays, groupServiceDays);
groupDueDateMillis = 0;
}

Expand Down
9 changes: 8 additions & 1 deletion servers/zms/src/main/java/com/yahoo/athenz/zms/utils/ZMSUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,7 @@ public static boolean metaValueChanged(Object domainValue, Object metaValue) {
return metaValue != null && !metaValue.equals(domainValue);
}

public static long configuredDueDateMillis(Integer domainDueDateDays, Integer roleDueDateDays) {
public static long configuredDueDateMillis(Integer serverDefaultDueDateDays, Integer domainDueDateDays, Integer roleDueDateDays) {
Copy link
Collaborator

@havetisyan havetisyan Dec 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we have serverDefaultDueDateDays (should also be called serverDefaultMaxDueDateDays) set as Integer? It's int so we should define the function as such and it also eliminates the boxing and unnecessary null check later.


// the role expiry days settings overrides the domain one if one configured

Expand All @@ -459,6 +459,13 @@ public static long configuredDueDateMillis(Integer domainDueDateDays, Integer ro
} else if (domainDueDateDays != null && domainDueDateDays > 0) {
expiryDays = domainDueDateDays;
}

if (serverDefaultDueDateDays != null && serverDefaultDueDateDays > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once serverDefaultMaxDueDateDays is defined as int, the null check is not necessary here

if (expiryDays == 0 || expiryDays > serverDefaultDueDateDays) {
expiryDays = serverDefaultDueDateDays;
}
}

return expiryDays == 0 ? 0 : System.currentTimeMillis() + TimeUnit.MILLISECONDS.convert(expiryDays, TimeUnit.DAYS);
}

Expand Down
24 changes: 5 additions & 19 deletions servers/zms/src/test/java/com/yahoo/athenz/zms/ZMSImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22957,24 +22957,6 @@ public void testGetMemberDueDate() {
assertEquals(zmsImpl.getMemberDueDate(100, Timestamp.fromMillis(150)), Timestamp.fromMillis(100));
}

@Test
public void testMemberDueDateTimestamp() {
ZMSImpl zmsImpl = zmsTestInitializer.getZms();
assertEquals(zmsImpl.memberDueDateTimestamp(null, null, Timestamp.fromMillis(100)), Timestamp.fromMillis(100));
assertEquals(zmsImpl.memberDueDateTimestamp(-1, 0, Timestamp.fromMillis(100)), Timestamp.fromMillis(100));
assertEquals(zmsImpl.memberDueDateTimestamp(-3, -2, Timestamp.fromMillis(100)), Timestamp.fromMillis(100));

long ext50Millis = TimeUnit.MILLISECONDS.convert(50, TimeUnit.DAYS);
long ext75Millis = TimeUnit.MILLISECONDS.convert(75, TimeUnit.DAYS);
long ext100Millis = TimeUnit.MILLISECONDS.convert(100, TimeUnit.DAYS);

Timestamp stamp = zmsImpl.memberDueDateTimestamp(100, 50, Timestamp.fromMillis(System.currentTimeMillis() + ext75Millis));
assertTrue(ZMSTestUtils.validateDueDate(stamp.millis(), ext50Millis));

stamp = zmsImpl.memberDueDateTimestamp(75, null, Timestamp.fromMillis(System.currentTimeMillis() + ext100Millis));
assertTrue(ZMSTestUtils.validateDueDate(stamp.millis(), ext75Millis));
}

@Test
public void testUpdateRoleMemberReview() {

Expand Down Expand Up @@ -24545,11 +24527,15 @@ public void testSetGroupMemberExpiration() {
public void testSetGroupMemberExpirationGroupRejected() {

ZMSImpl zmsImpl = zmsTestInitializer.getZms();
AthenzDomain domain = new AthenzDomain("coretech");
domain.setDomain(new Domain());

Group group = zmsTestInitializer.createGroupObject(domain.getName(), "group1", "user.joe", "user.jane");

GroupMember groupMember = new GroupMember().setMemberName("dev-group")
.setPrincipalType(Principal.Type.GROUP.getValue());
try {
zmsImpl.setGroupMemberExpiration(null, null, groupMember, null, "unit-test");
zmsImpl.setGroupMemberExpiration(domain, group, groupMember, null, "unit-test");
fail();
} catch (ResourceException ex) {
assertEquals(ex.getCode(), ResourceException.BAD_REQUEST);
Expand Down
48 changes: 32 additions & 16 deletions servers/zms/src/test/java/com/yahoo/athenz/zms/utils/ZMSUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,33 +408,49 @@ public void testMetaValueChanged() {
@Test
public void testConfiguredExpiryMillis() {

assertEquals(ZMSUtils.configuredDueDateMillis(null, null), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(null, -3), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(null, 0), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(-3, null), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, null), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(-3, -3), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, 0), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(null, null, null), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, null, null), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, null, -3), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, null, 0), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, -3, null), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, 0, null), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, -3, -3), 0);
assertEquals(ZMSUtils.configuredDueDateMillis(0, 0, 0), 0);

long extMillis = TimeUnit.MILLISECONDS.convert(10, TimeUnit.DAYS);
long millis = ZMSUtils.configuredDueDateMillis(null, 10);
long millis = ZMSUtils.configuredDueDateMillis(0, null, 10);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(null, 10);
millis = ZMSUtils.configuredDueDateMillis(0, null, 10);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(-1, 10);
millis = ZMSUtils.configuredDueDateMillis(0, -1, 10);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(0, 10);
millis = ZMSUtils.configuredDueDateMillis(0, 0, 10);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(5, 10);
millis = ZMSUtils.configuredDueDateMillis(0, 5, 10);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(20, 10);
millis = ZMSUtils.configuredDueDateMillis(0, 20, 10);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));

millis = ZMSUtils.configuredDueDateMillis(10, null);
millis = ZMSUtils.configuredDueDateMillis(0, 10, null);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(10, -1);
millis = ZMSUtils.configuredDueDateMillis(0, 10, -1);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(10, 0);
millis = ZMSUtils.configuredDueDateMillis(0, 10, 0);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));

millis = ZMSUtils.configuredDueDateMillis(10, 0, 0);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(20, 10, 0);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(10, 100, 0);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(10, 100, 20);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(20, 0, 10);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(10, 0, 100);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
millis = ZMSUtils.configuredDueDateMillis(10, 20, 100);
assertTrue(ZMSTestUtils.validateDueDate(millis, extMillis));
}

Expand Down
Loading