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

Fix repeated suspensions #831

Merged
merged 5 commits into from
Aug 9, 2024
Merged

Fix repeated suspensions #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3497623
Bump version to 1.10.1
enricovianello Aug 9, 2024
55c6e73
Check if user is active before suspending it
enricovianello Aug 9, 2024
9f6a0a1
Add test there's no update every time a suspended user is handled
enricovianello Aug 9, 2024
310118d
Avoid useless multiple AccountLabelSetEvent
enricovianello Aug 9, 2024
247eb5c
Improve coverage
enricovianello Aug 9, 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
2 changes: 1 addition & 1 deletion iam-common/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>it.infn.mw.iam-parent</groupId>
<artifactId>iam-parent</artifactId>
<version>1.10.0</version>
<version>1.10.1</version>
</parent>

<groupId>it.infn.mw.iam-common</groupId>
Expand Down
2 changes: 1 addition & 1 deletion iam-login-service/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
<parent>
<groupId>it.infn.mw.iam-parent</groupId>
<artifactId>iam-parent</artifactId>
<version>1.10.0</version>
<version>1.10.1</version>
</parent>

<groupId>it.infn.mw.iam-login-service</groupId>
Expand Down
77 changes: 53 additions & 24 deletions iam-login-service/src/main/java/it/infn/mw/iam/core/lifecycle/ExpiredAccountsHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,19 @@
package it.infn.mw.iam.core.lifecycle;

import static com.google.common.collect.Sets.newHashSet;
import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.AccountLifecycleStatus.PENDING_REMOVAL;
import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.AccountLifecycleStatus.PENDING_SUSPENSION;
import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.AccountLifecycleStatus.SUSPENDED;

import java.time.Clock;
import java.time.Instant;
import java.time.temporal.ChronoUnit;
import java.util.Date;
import java.util.Optional;
import java.util.Set;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.data.domain.Page;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
Expand All @@ -43,10 +46,7 @@
public class ExpiredAccountsHandler implements Runnable {

public enum AccountLifecycleStatus {
OK,
PENDING_SUSPENSION,
PENDING_REMOVAL,
SUSPENDED
OK, PENDING_SUSPENSION, PENDING_REMOVAL, SUSPENDED
}

public static final String LIFECYCLE_TIMESTAMP_LABEL = "lifecycle.timestamp";
Expand All @@ -67,7 +67,6 @@ public enum AccountLifecycleStatus {

private Set<IamAccount> accountsScheduledForRemoval = newHashSet();

@Autowired
public ExpiredAccountsHandler(Clock clock, LifecycleProperties properties,
IamAccountRepository repo, IamAccountService service) {
this.clock = clock;
Expand Down Expand Up @@ -96,40 +95,71 @@ private boolean pastRemovalGracePeriod(IamAccount expiredAccount) {
properties.getAccount().getExpiredAccountPolicy().getRemovalGracePeriodDays());
}

private void addLastCheckedLabel(IamAccount expiredAccount) {
accountService.setLabel(expiredAccount,
IamLabel.builder()
.name(LIFECYCLE_TIMESTAMP_LABEL)
.value(String.valueOf(checkTime.toEpochMilli()))
.build());
}

private void addStatusLabel(IamAccount expiredAccount, AccountLifecycleStatus status) {
accountService.setLabel(expiredAccount,
IamLabel.builder().name(LIFECYCLE_STATUS_LABEL).value(status.name()).build());
}

private boolean checkAccountStatusIs(IamAccount a, AccountLifecycleStatus status) {
Optional<IamLabel> lifecycleStatus = a.getLabelByName(LIFECYCLE_STATUS_LABEL);
if (lifecycleStatus.isEmpty()) {
return false;
}
return status.name().equals(lifecycleStatus.get().getValue());
}

private void suspendAccount(IamAccount expiredAccount) {

LOG.info("Suspeding account {} expired on {} ({} days ago)", expiredAccount.getUsername(),
if (expiredAccount.isActive()) {
LOG.info("Suspeding account {} expired on {} ({} days ago)", expiredAccount.getUsername(),
expiredAccount.getEndTime(),
ChronoUnit.DAYS.between(expiredAccount.getEndTime().toInstant(), checkTime));
accountService.disableAccount(expiredAccount);
if (properties.getAccount().getExpiredAccountPolicy().isRemoveExpiredAccounts()) {
addStatusLabel(expiredAccount, AccountLifecycleStatus.PENDING_REMOVAL);
accountService.disableAccount(expiredAccount);
} else {
// nothing to do
LOG.debug("Account {} expired on {} has been already suspended", expiredAccount.getUsername(), expiredAccount.getEndTime());
}
else {
addStatusLabel(expiredAccount, AccountLifecycleStatus.SUSPENDED);
if (properties.getAccount().getExpiredAccountPolicy().isRemoveExpiredAccounts()) {
markAsPendingRemoval(expiredAccount);
} else {
markAsSuspended(expiredAccount);
}
addLastCheckedLabel(expiredAccount);
}

private void markAsPendingSuspension(IamAccount expiredAccount) {
if (checkAccountStatusIs(expiredAccount, PENDING_SUSPENSION)) {
LOG.debug("Account {} expired on {} has been already marked as pending suspension",
expiredAccount.getUsername(), expiredAccount.getEndTime());
return;
}
LOG.info("Marking account {} (expired on {} ({} days ago)) as pending suspension",
expiredAccount.getUsername(), expiredAccount.getEndTime(),
ChronoUnit.DAYS.between(expiredAccount.getEndTime().toInstant(), checkTime));
addStatusLabel(expiredAccount, AccountLifecycleStatus.PENDING_SUSPENSION);
addLastCheckedLabel(expiredAccount);
addStatusLabel(expiredAccount, PENDING_SUSPENSION);
}

private void markAsPendingRemoval(IamAccount expiredAccount) {
if (checkAccountStatusIs(expiredAccount, PENDING_REMOVAL)) {
LOG.debug("Account {} expired on {} has been already marked as pending removal",
expiredAccount.getUsername(), expiredAccount.getEndTime());
return;
}
LOG.info("Marking account {} (expired on {} ({} days ago)) as pending removal",
expiredAccount.getUsername(), expiredAccount.getEndTime(),
ChronoUnit.DAYS.between(expiredAccount.getEndTime().toInstant(), checkTime));
addStatusLabel(expiredAccount, PENDING_REMOVAL);
}

private void markAsSuspended(IamAccount expiredAccount) {
if (checkAccountStatusIs(expiredAccount, SUSPENDED)) {
LOG.debug("Account {} expired on {} has been already marked as suspended",
expiredAccount.getUsername(), expiredAccount.getEndTime());
return;
}
LOG.info("Marking account {} (expired on {} ({} days ago)) as suspended",
expiredAccount.getUsername(), expiredAccount.getEndTime(),
ChronoUnit.DAYS.between(expiredAccount.getEndTime().toInstant(), checkTime));
addStatusLabel(expiredAccount, SUSPENDED);
}

private void removeAccount(IamAccount expiredAccount) {
Expand All @@ -139,7 +169,6 @@ private void removeAccount(IamAccount expiredAccount) {
accountService.deleteAccount(expiredAccount);
}


private void scheduleAccountRemoval(IamAccount expiredAccount) {
accountsScheduledForRemoval.add(expiredAccount);
}
Expand Down
19 changes: 9 additions & 10 deletions ...test/java/it/infn/mw/iam/test/lifecycle/AccountLifecycleNoRemovalExpiredAccountTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
package it.infn.mw.iam.test.lifecycle;

import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.LIFECYCLE_STATUS_LABEL;
import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.LIFECYCLE_TIMESTAMP_LABEL;
import static java.lang.String.valueOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

Expand Down Expand Up @@ -64,18 +62,24 @@ public void testSuspendedLabelWorks() {
assertThat(testAccount.isActive(), is(true));
testAccount.setEndTime(Date.from(EIGHT_DAYS_AGO));
repo.save(testAccount);
Date lastUpdateTime = testAccount.getLastUpdateTime();

handler.handleExpiredAccounts();

testAccount =
repo.findByUuid(TEST_USER_UUID).orElseThrow(assertionError(EXPECTED_ACCOUNT_NOT_FOUND));

assertThat(testAccount.isActive(), is(false));
assertThat(testAccount.getLastUpdateTime().compareTo(lastUpdateTime) > 0, is(true));
lastUpdateTime = testAccount.getLastUpdateTime();

Optional<IamLabel> timestampLabel = testAccount.getLabelByName(LIFECYCLE_TIMESTAMP_LABEL);
handler.handleExpiredAccounts();

assertThat(timestampLabel.isPresent(), is(true));
assertThat(timestampLabel.get().getValue(), is(valueOf(NOW.toEpochMilli())));
testAccount =
repo.findByUuid(TEST_USER_UUID).orElseThrow(assertionError(EXPECTED_ACCOUNT_NOT_FOUND));

assertThat(testAccount.isActive(), is(false));
assertThat(testAccount.getLastUpdateTime().compareTo(lastUpdateTime) == 0, is(true));

Optional<IamLabel> statusLabel = testAccount.getLabelByName(LIFECYCLE_STATUS_LABEL);
assertThat(statusLabel.isPresent(), is(true));
Expand All @@ -99,11 +103,6 @@ public void testNoRemovalWorks() {

assertThat(testAccount.isActive(), is(false));

Optional<IamLabel> timestampLabel = testAccount.getLabelByName(LIFECYCLE_TIMESTAMP_LABEL);

assertThat(timestampLabel.isPresent(), is(true));
assertThat(timestampLabel.get().getValue(), is(valueOf(NOW.toEpochMilli())));

Optional<IamLabel> statusLabel = testAccount.getLabelByName(LIFECYCLE_STATUS_LABEL);
assertThat(statusLabel.isPresent(), is(true));
assertThat(statusLabel.get().getValue(),
Expand Down
7 changes: 0 additions & 7 deletions ...test/java/it/infn/mw/iam/test/lifecycle/AccountLifecycleNoSuspensionGracePeriodTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,8 @@
package it.infn.mw.iam.test.lifecycle;

import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.LIFECYCLE_STATUS_LABEL;
import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.LIFECYCLE_TIMESTAMP_LABEL;
import static it.infn.mw.iam.test.api.TestSupport.EXPECTED_ACCOUNT_NOT_FOUND;
import static it.infn.mw.iam.test.api.TestSupport.TEST_USER_UUID;
import static java.lang.String.valueOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

Expand Down Expand Up @@ -92,11 +90,6 @@ public void testZeroDaysSuspensionGracePeriod() {

assertThat(testAccount.isActive(), is(false));

Optional<IamLabel> timestampLabel = testAccount.getLabelByName(LIFECYCLE_TIMESTAMP_LABEL);

assertThat(timestampLabel.isPresent(), is(true));
assertThat(timestampLabel.get().getValue(), is(valueOf(NOW.toEpochMilli())));

Optional<IamLabel> statusLabel = testAccount.getLabelByName(LIFECYCLE_STATUS_LABEL);
assertThat(statusLabel.isPresent(), is(true));
assertThat(statusLabel.get().getValue(),
Expand Down
31 changes: 21 additions & 10 deletions iam-login-service/src/test/java/it/infn/mw/iam/test/lifecycle/AccountLifecycleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@
package it.infn.mw.iam.test.lifecycle;

import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.LIFECYCLE_STATUS_LABEL;
import static it.infn.mw.iam.core.lifecycle.ExpiredAccountsHandler.LIFECYCLE_TIMESTAMP_LABEL;
import static java.lang.String.valueOf;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

Expand Down Expand Up @@ -83,23 +81,30 @@ public void testSuspensionGracePeriodWorks() {

testAccount.setEndTime(Date.from(FOUR_DAYS_AGO));
repo.save(testAccount);
Date lastUpdateTime = testAccount.getLastUpdateTime();

handler.handleExpiredAccounts();

testAccount =
repo.findByUuid(TEST_USER_UUID).orElseThrow(assertionError(EXPECTED_ACCOUNT_NOT_FOUND));

assertThat(testAccount.isActive(), is(true));

Optional<IamLabel> timestampLabel = testAccount.getLabelByName(LIFECYCLE_TIMESTAMP_LABEL);

assertThat(timestampLabel.isPresent(), is(true));
assertThat(timestampLabel.get().getValue(), is(valueOf(NOW.toEpochMilli())));
assertThat(testAccount.getLastUpdateTime().compareTo(lastUpdateTime) > 0, is(true));
lastUpdateTime = testAccount.getLastUpdateTime();

Optional<IamLabel> statusLabel = testAccount.getLabelByName(LIFECYCLE_STATUS_LABEL);
assertThat(statusLabel.isPresent(), is(true));
assertThat(statusLabel.get().getValue(),
is(ExpiredAccountsHandler.AccountLifecycleStatus.PENDING_SUSPENSION.name()));

handler.handleExpiredAccounts();

testAccount =
repo.findByUuid(TEST_USER_UUID).orElseThrow(assertionError(EXPECTED_ACCOUNT_NOT_FOUND));

assertThat(testAccount.isActive(), is(true));
assertThat(testAccount.getLastUpdateTime().compareTo(lastUpdateTime) == 0, is(true));

}

@Test
Expand All @@ -110,18 +115,24 @@ public void testRemovalGracePeriodWorks() {
assertThat(testAccount.isActive(), is(true));
testAccount.setEndTime(Date.from(EIGHT_DAYS_AGO));
repo.save(testAccount);
Date lastUpdateTime = testAccount.getLastUpdateTime();

handler.handleExpiredAccounts();

testAccount =
repo.findByUuid(TEST_USER_UUID).orElseThrow(assertionError(EXPECTED_ACCOUNT_NOT_FOUND));

assertThat(testAccount.isActive(), is(false));
assertThat(testAccount.getLastUpdateTime().compareTo(lastUpdateTime) > 0, is(true));
lastUpdateTime = testAccount.getLastUpdateTime();

Optional<IamLabel> timestampLabel = testAccount.getLabelByName(LIFECYCLE_TIMESTAMP_LABEL);
handler.handleExpiredAccounts();

assertThat(timestampLabel.isPresent(), is(true));
assertThat(timestampLabel.get().getValue(), is(valueOf(NOW.toEpochMilli())));
testAccount =
repo.findByUuid(TEST_USER_UUID).orElseThrow(assertionError(EXPECTED_ACCOUNT_NOT_FOUND));

assertThat(testAccount.isActive(), is(false));
assertThat(testAccount.getLastUpdateTime().compareTo(lastUpdateTime) == 0, is(true));

Optional<IamLabel> statusLabel = testAccount.getLabelByName(LIFECYCLE_STATUS_LABEL);
assertThat(statusLabel.isPresent(), is(true));
Expand Down
2 changes: 1 addition & 1 deletion iam-persistence/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
<parent>
<groupId>it.infn.mw.iam-parent</groupId>
<artifactId>iam-parent</artifactId>
<version>1.10.0</version>
<version>1.10.1</version>
</parent>

<groupId>it.infn.mw.iam-persistence</groupId>
Expand Down
2 changes: 1 addition & 1 deletion iam-test-client/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
<parent>
<groupId>it.infn.mw.iam-parent</groupId>
<artifactId>iam-parent</artifactId>
<version>1.10.0</version>
<version>1.10.1</version>
</parent>

<groupId>it.infn.mw.iam-test-client</groupId>
Expand Down
2 changes: 1 addition & 1 deletion iam-voms-aa/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
<parent>
<groupId>it.infn.mw.iam-parent</groupId>
<artifactId>iam-parent</artifactId>
<version>1.10.0</version>
<version>1.10.1</version>
</parent>

<groupId>it.infn.mw.iam-voms-aa</groupId>
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

<groupId>it.infn.mw.iam-parent</groupId>
<artifactId>iam-parent</artifactId>
<version>1.10.0</version>
<version>1.10.1</version>
<packaging>pom</packaging>

<name>INDIGO Identity and Access Manager (IAM) - Parent POM</name>
Expand Down
Loading