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

Jump to bottom

set role/group last review date check differenly for new and updated objects #2533

Merged
merged 1 commit into from
Mar 4, 2024
Merged

set role/group last review date check differenly for new and updated objects #2533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 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 @@ -538,3 +538,17 @@ athenz.zms.no_auth_uri_list=/zms/v1/schema
# The server will validate that the environment specified in the domain
# is one of the values specified in this list.
#athenz.zms.domain_environments=production,integration,staging,sandbox,qa,development

# The setting specifies the number of days that the server will allow the
# user to set the lastReviewedDate when updating an existing role/group object
# in the past. The default value is 7 days. This setting is used to prevent
# administrators from claiming that they have reviewed the role/group in the
# past but never applied the changes to the server.
#athenz.zms.review_date_offset_days_updated_objects=7

# The setting specifies the number of days that the server will allow the
# user to set the lastReviewedDate when creating a new role/group object
# in the past. The default value is 365 days. With new objects, we don't
# have the same concerns as with updated objects, so we're allowing a much
# larger offset.
#athenz.zms.review_date_offset_days_new_objects=365
41 changes: 27 additions & 14 deletions servers/zms/src/main/java/com/yahoo/athenz/zms/DBService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ public class DBService implements RolesProvider {
int defaultOpTimeout;
ZMSConfig zmsConfig;
private final int maxPolicyVersions;
long maxLastReviewDateOffsetMillis;
long maxLastReviewDateOffsetMillisForNewObjects;
long maxLastReviewDateOffsetMillisForUpdatedObjects;

final String awsAssumeRoleAction;
final String gcpAssumeRoleAction;
Expand Down Expand Up @@ -178,9 +179,14 @@ public DBService(ObjectStore store, AuditLogger auditLogger, ZMSConfig zmsConfig
ZMSConsts.ZMS_PROP_PURGE_TASK_LIMIT_PER_CALL, ZMSConsts.ZMS_PURGE_TASK_LIMIT_PER_CALL_DEF);
purgeMemberExpiryDays = new DynamicConfigInteger(CONFIG_MANAGER,
ZMSConsts.ZMS_PROP_PURGE_MEMBER_EXPIRY_DAYS, ZMSConsts.ZMS_PURGE_MEMBER_EXPIRY_DAYS_DEF);
int maxLastReviewDateOffsetDays = Integer.parseInt(System.getProperty(ZMSConsts.ZMS_PROP_REVIEW_DATE_OFFSET_DAYS,
ZMSConsts.ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_DEFAULT));
maxLastReviewDateOffsetMillis = TimeUnit.MILLISECONDS.convert(maxLastReviewDateOffsetDays, TimeUnit.DAYS);
int maxLastReviewDateOffsetDays = Integer.parseInt(System.getProperty(
ZMSConsts.ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_UPDATED_OBJECT,
ZMSConsts.ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_UPDATED_OBJECT_DEFAULT));
maxLastReviewDateOffsetMillisForUpdatedObjects = TimeUnit.MILLISECONDS.convert(maxLastReviewDateOffsetDays, TimeUnit.DAYS);
maxLastReviewDateOffsetDays = Integer.parseInt(System.getProperty(
ZMSConsts.ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_NEW_OBJECT,
ZMSConsts.ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_NEW_OBJECT_DEFAULT));
maxLastReviewDateOffsetMillisForNewObjects = TimeUnit.MILLISECONDS.convert(maxLastReviewDateOffsetDays, TimeUnit.DAYS);

minReviewDaysPercentage = new DynamicConfigInteger(CONFIG_MANAGER,
ZMSConsts.ZMS_PROP_REVIEW_DAYS_PERCENTAGE, ZMSConsts.ZMS_PROP_REVIEW_DAYS_PERCENTAGE_DEFAULT);
Expand Down Expand Up @@ -1561,9 +1567,10 @@ Role executePutRole(ResourceContext ctx, String domainName, String roleName, Rol

// we need to validate and transfer the last reviewed date if necessary

Timestamp originalLastReviewedTime = originalRole == null ? null : originalRole.getLastReviewedDate();
boolean isNewRole = originalRole == null;
Timestamp originalLastReviewedTime = isNewRole ? null : originalRole.getLastReviewedDate();
role.setLastReviewedDate(objectLastReviewDate(role.getLastReviewedDate(),
originalLastReviewedTime, caller));
originalLastReviewedTime, isNewRole, caller));

// now process the request

Expand Down Expand Up @@ -1597,34 +1604,39 @@ Role executePutRole(ResourceContext ctx, String domainName, String roleName, Rol
}
}

Timestamp objectLastReviewDate(Timestamp newLastReviewedDate, Timestamp oldLastReviwedDate, final String caller) {
Timestamp objectLastReviewDate(Timestamp newLastReviewedDate, Timestamp oldLastReviewedDate,
boolean isNewObject, final String caller) {

// if the new last reviewed date is not specified then
// we'll just return the old value

if (newLastReviewedDate == null) {
return oldLastReviwedDate;
return oldLastReviewedDate;
}

// if the new last reviewed timestamp is the same as the old value
// then no further validation is required

if (newLastReviewedDate.equals(oldLastReviwedDate)) {
if (newLastReviewedDate.equals(oldLastReviewedDate)) {
return newLastReviewedDate;
}

// otherwise we're going to make sure to validate that the date
// specified is not in the future (we'll allow an offset of 5 minutes
// in case the client/server times are not in sync) and within the configured
// offset days from the current time (we don't want admins to specify
// review date way in the past)
// review date way in the past unless we're dealing with a new object
// in which case we don't really care much about the strict review
// date validation and allow a much larger offset).

long currentTime = System.currentTimeMillis();
long reviewTime = newLastReviewedDate.millis();
if (reviewTime > currentTime + 300 * 1000) {
throw ZMSUtils.requestError("Last reviewed date: " + newLastReviewedDate +
" is in the future", caller);
}
long maxLastReviewDateOffsetMillis = isNewObject ? maxLastReviewDateOffsetMillisForNewObjects :
maxLastReviewDateOffsetMillisForUpdatedObjects;
if (currentTime - reviewTime > maxLastReviewDateOffsetMillis) {
throw ZMSUtils.requestError("Last reviewed date: " + newLastReviewedDate +
" is too far in the past", caller);
Expand Down Expand Up @@ -1664,9 +1676,10 @@ Group executePutGroup(ResourceContext ctx, final String domainName, final String

// we need to validate and transfer the last reviewed date if necessary

Timestamp originalLastReviewedTime = originalGroup == null ? null : originalGroup.getLastReviewedDate();
boolean isNewGroup = originalGroup == null;
Timestamp originalLastReviewedTime = isNewGroup ? null : originalGroup.getLastReviewedDate();
group.setLastReviewedDate(objectLastReviewDate(group.getLastReviewedDate(),
originalLastReviewedTime, ctx.getApiName()));
originalLastReviewedTime, isNewGroup, ctx.getApiName()));

// now process the request

Expand Down Expand Up @@ -6578,7 +6591,7 @@ void updateRoleMetaFields(Role role, RoleMeta meta, final String caller) {
role.setSelfRenewMins(meta.getSelfRenewMins());
}
role.setLastReviewedDate(objectLastReviewDate(meta.getLastReviewedDate(),
role.getLastReviewedDate(), caller));
role.getLastReviewedDate(), false, caller));
}

public Role executePutRoleMeta(ResourceContext ctx, String domainName, String roleName, Role originalRole,
Expand Down Expand Up @@ -6711,7 +6724,7 @@ void updateGroupMetaFields(Group group, GroupMeta meta, final String caller) {
group.setSelfRenewMins(meta.getSelfRenewMins());
}
group.setLastReviewedDate(objectLastReviewDate(meta.getLastReviewedDate(),
group.getLastReviewedDate(), caller));
group.getLastReviewedDate(), false, caller));
}

public Group executePutGroupMeta(ResourceContext ctx, final String domainName, final String groupName,
Expand Down
7 changes: 5 additions & 2 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 @@ -424,8 +424,11 @@ public final class ZMSConsts {
public static final String ZMS_PROP_JSON_MAX_NUMBER_LENGTH = "athenz.zms.json_max_number_length";
public static final String ZMS_PROP_JSON_MAX_STRING_LENGTH = "athenz.zms.json_max_string_length";

public static final String ZMS_PROP_REVIEW_DATE_OFFSET_DAYS = "athenz.zms.review_date_offset_days";
public static final String ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_DEFAULT = "3";
public static final String ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_NEW_OBJECT = "athenz.zms.review_date_offset_days_new_objects";
public static final String ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_NEW_OBJECT_DEFAULT = "365";

public static final String ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_UPDATED_OBJECT = "athenz.zms.review_date_offset_days_updated_objects";
public static final String ZMS_PROP_REVIEW_DATE_OFFSET_DAYS_UPDATED_OBJECT_DEFAULT = "7";

public static final String ZMS_PROP_REVIEW_DAYS_PERCENTAGE = "athenz.zms.review_days_percentage";
public static final Integer ZMS_PROP_REVIEW_DAYS_PERCENTAGE_DEFAULT = 68;
Expand Down
108 changes: 104 additions & 4 deletions servers/zms/src/test/java/com/yahoo/athenz/zms/ZMSObjectReviewTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,7 +471,7 @@ public void testRoleWithLastReviewedDate() {

// let's change the last reviewed date to only 1 day

zmsImpl.dbService.maxLastReviewDateOffsetMillis = TimeUnit.MILLISECONDS.convert(1, TimeUnit.DAYS);
zmsImpl.dbService.maxLastReviewDateOffsetMillisForUpdatedObjects = TimeUnit.MILLISECONDS.convert(1, TimeUnit.DAYS);

// our update should still work since we're not going to change the value

Expand Down Expand Up @@ -530,7 +530,58 @@ public void testRoleWithLastReviewedDate() {
assertEquals(role.getServiceReviewDays(), Integer.valueOf(50));
assertEquals(role.getDescription(), "test role with last reviewed date - updated");

zmsImpl.dbService.maxLastReviewDateOffsetMillis = TimeUnit.MILLISECONDS.convert(3, TimeUnit.DAYS);
zmsImpl.dbService.maxLastReviewDateOffsetMillisForUpdatedObjects = TimeUnit.MILLISECONDS.convert(7, TimeUnit.DAYS);
zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef);
}

@Test
public void testRoleWithLastReviewedDateNewObject() {

ZMSImpl zmsImpl = zmsTestInitializer.getZms();
RsrcCtxWrapper ctx = zmsTestInitializer.getMockDomRsrcCtx();
final String auditRef = zmsTestInitializer.getAuditRef();

final String domainName = "new-role-last-reviewed-date";
final String roleName = "role1";

TopLevelDomain dom1 = zmsTestInitializer.createTopLevelDomainObject(domainName, "Test Domain1",
"testOrg", zmsTestInitializer.getAdminUser());
dom1.getAdminUsers().add("user.user1");
zmsImpl.postTopLevelDomain(ctx, auditRef, dom1);

// the default setting last reviewed date is allowed for new object
// is set to 365 days. So we should not be able to set the last
// reviewed date to more than 365 days in the past

Timestamp moreThanYearAgo = Timestamp.fromMillis(
System.currentTimeMillis() - TimeUnit.MILLISECONDS.convert(366, TimeUnit.DAYS));
Role role1 = zmsTestInitializer.createRoleObject(domainName, roleName, null, Collections.emptyList());
role1.setLastReviewedDate(moreThanYearAgo);
role1.setMemberExpiryDays(10);
role1.setCertExpiryMins(60);
role1.setDescription("test role with last reviewed date");

try {
zmsImpl.putRole(ctx, domainName, roleName, auditRef, false, role1);
fail();
} catch (ResourceException ex) {
assertEquals(ex.getCode(), ResourceException.BAD_REQUEST);
assertTrue(ex.getMessage().contains("in the past"));
}

// now let's update our role object to be less than one year and
// the request must complete successfully

Timestamp lessThanYearAgo = Timestamp.fromMillis(
System.currentTimeMillis() - TimeUnit.MILLISECONDS.convert(364, TimeUnit.DAYS));
role1.setLastReviewedDate(lessThanYearAgo);
zmsImpl.putRole(ctx, domainName, roleName, auditRef, false, role1);

Role role = zmsImpl.getRole(ctx, domainName, roleName, false, false, false);
assertNotNull(role);

assertEquals(role.getLastReviewedDate(), lessThanYearAgo);

zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef);
}

Expand Down Expand Up @@ -617,7 +668,7 @@ public void testGroupWithLastReviewedDate() {

// let's change the last reviewed date to only 1 day

zmsImpl.dbService.maxLastReviewDateOffsetMillis = TimeUnit.MILLISECONDS.convert(1, TimeUnit.DAYS);
zmsImpl.dbService.maxLastReviewDateOffsetMillisForUpdatedObjects = TimeUnit.MILLISECONDS.convert(1, TimeUnit.DAYS);

// our update should still work since we're not going to change the value

Expand Down Expand Up @@ -673,7 +724,56 @@ public void testGroupWithLastReviewedDate() {
assertEquals(group.getServiceExpiryDays(), Integer.valueOf(120));
assertTrue(group.getDeleteProtection());

zmsImpl.dbService.maxLastReviewDateOffsetMillis = TimeUnit.MILLISECONDS.convert(3, TimeUnit.DAYS);
zmsImpl.dbService.maxLastReviewDateOffsetMillisForUpdatedObjects = TimeUnit.MILLISECONDS.convert(7, TimeUnit.DAYS);
zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef);
}

@Test
public void testGroupWithLastReviewedDateNewObject() {

ZMSImpl zmsImpl = zmsTestInitializer.getZms();
RsrcCtxWrapper ctx = zmsTestInitializer.getMockDomRsrcCtx();
final String auditRef = zmsTestInitializer.getAuditRef();

final String domainName = "new-group-last-reviewed-date";
final String groupName = "group1";

TopLevelDomain dom1 = zmsTestInitializer.createTopLevelDomainObject(domainName, "Test Domain1",
"testOrg", zmsTestInitializer.getAdminUser());
dom1.getAdminUsers().add("user.user1");
zmsImpl.postTopLevelDomain(ctx, auditRef, dom1);

// the default setting last reviewed date is allowed for new object
// is set to 365 days. So we should not be able to set the last
// reviewed date to more than 365 days in the past

Timestamp moreThanYearAgo = Timestamp.fromMillis(
System.currentTimeMillis() - TimeUnit.MILLISECONDS.convert(366, TimeUnit.DAYS));
Group group1 = zmsTestInitializer.createGroupObject(domainName, groupName, null, null);
group1.setLastReviewedDate(moreThanYearAgo);
group1.setMemberExpiryDays(10);

try {
zmsImpl.putGroup(ctx, domainName, groupName, auditRef, false, group1);
fail();
} catch (ResourceException ex) {
assertEquals(ex.getCode(), ResourceException.BAD_REQUEST);
assertTrue(ex.getMessage().contains("in the past"));
}

// now let's update our role object to be less than one year and
// the request must complete successfully

Timestamp lessThanYearAgo = Timestamp.fromMillis(
System.currentTimeMillis() - TimeUnit.MILLISECONDS.convert(364, TimeUnit.DAYS));
group1.setLastReviewedDate(lessThanYearAgo);
zmsImpl.putGroup(ctx, domainName, groupName, auditRef, false, group1);

// now let's get our group object

Group group = zmsImpl.getGroup(ctx, domainName, groupName, false, false);
assertNotNull(group);

zmsImpl.deleteTopLevelDomain(ctx, domainName, auditRef);
}
}