Skip to content

Commit

Permalink
fix: tokens should be removed when user is disabled
Browse files Browse the repository at this point in the history
fixes AM-4062

(cherry picked from commit d3e0ba6)
  • Loading branch information
lgw-gravitee committed Jan 14, 2025
1 parent 460236b commit 3ffeece
Show file tree
Hide file tree
Showing 13 changed files with 262 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
import io.gravitee.am.service.PasswordService;
import io.gravitee.am.service.RateLimiterService;
import io.gravitee.am.service.RoleService;
import io.gravitee.am.service.TokenService;
import io.gravitee.am.service.UserActivityService;
import io.gravitee.am.service.VerifyAttemptService;
import io.gravitee.am.service.exception.AbstractManagementException;
Expand Down Expand Up @@ -151,6 +152,9 @@ public class UserServiceImpl implements UserService {
@Autowired
private PasswordPolicyManager passwordPolicyManager;

@Autowired
private TokenService tokenService;

@Override
public Single<ListResponse<User>> list(Filter filter, int startIndex, int size, String baseUrl) {
LOGGER.debug("Find users by domain: {}", domain.getId());
Expand Down Expand Up @@ -365,7 +369,7 @@ public Single<User> innerUpdate(io.gravitee.am.model.User userIntoDb, User scimU
userToUpdate.setLastPasswordReset(new Date());
}

return userRepository.update(userToUpdate, UpdateActions.build(existingUser, userToUpdate));
return updateUser(userToUpdate, UpdateActions.build(existingUser, userToUpdate));
})
.onErrorResumeNext(ex -> {
if (ex instanceof UserNotFoundException ||
Expand All @@ -374,7 +378,7 @@ public Single<User> innerUpdate(io.gravitee.am.model.User userIntoDb, User scimU
// idp user does not exist, only update AM user
// clear password
userToUpdate.setPassword(null);
return userRepository.update(userToUpdate, UpdateActions.build(existingUser, userToUpdate));
return updateUser(userToUpdate, UpdateActions.build(existingUser, userToUpdate));
}
return Single.error(ex);
})
Expand Down Expand Up @@ -403,6 +407,12 @@ public Single<User> innerUpdate(io.gravitee.am.model.User userIntoDb, User scimU
});
}

private Single<io.gravitee.am.model.User> updateUser(io.gravitee.am.model.User userToUpdate, UpdateActions updateActions){
Completable revokeTokens = userToUpdate.isDisabled() ?
tokenService.deleteByUser(userToUpdate) : Completable.complete();
return revokeTokens.andThen(userRepository.update(userToUpdate, updateActions));
}

@Override
public Single<User> patch(String userId, PatchOp patchOp, String idp, String baseUrl, io.gravitee.am.identityprovider.api.User principal, Client client) {
LOGGER.debug("Patch user {}", userId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,15 @@
import io.gravitee.am.service.PasswordService;
import io.gravitee.am.service.RateLimiterService;
import io.gravitee.am.service.RoleService;
import io.gravitee.am.service.TokenService;
import io.gravitee.am.service.UserActivityService;
import io.gravitee.am.service.VerifyAttemptService;
import io.gravitee.am.service.exception.UserInvalidException;
import io.gravitee.am.service.impl.PasswordHistoryService;
import io.gravitee.am.service.validators.email.EmailValidatorImpl;
import io.gravitee.am.service.validators.user.UserValidator;
import io.gravitee.am.service.validators.user.UserValidatorImpl;
import io.reactivex.rxjava3.core.Completable;
import io.reactivex.rxjava3.core.Flowable;
import io.reactivex.rxjava3.core.Maybe;
import io.reactivex.rxjava3.core.Single;
Expand Down Expand Up @@ -164,6 +166,8 @@ public class UserServiceTest {

private static final String DOMAIN_ID = "domain";

@Mock
private TokenService tokenService;

@Before
public void setUp() {
Expand Down Expand Up @@ -390,9 +394,55 @@ public void shouldUpdateUser_status_enabled() {
verify(userProvider).create(any());
verify(userProvider, never()).update(anyString(), any());
verify(userProvider, never()).updatePassword(any(), eq(PASSWORD));
verify(tokenService, never()).deleteByUser(any());
assertTrue(userCaptor.getValue().isEnabled());
}

@Test
public void shouldUpdateUser_status_disabled_and_tokens_revoked() {
io.gravitee.am.model.User existingUser = mock(io.gravitee.am.model.User.class);
when(existingUser.getId()).thenReturn("user-id");
when(existingUser.getSource()).thenReturn("user-idp");
when(existingUser.getUsername()).thenReturn("username");

User scimUser = mock(User.class);
when(scimUser.getPassword()).thenReturn(PASSWORD);
when(scimUser.isActive()).thenReturn(false);

io.gravitee.am.identityprovider.api.User idpUser = mock(io.gravitee.am.identityprovider.api.User.class);

UserProvider userProvider = mock(UserProvider.class);
when(userProvider.create(any())).thenReturn(Single.just(idpUser));

Set<Role> roles = new HashSet<>();
Role role1 = new Role();
role1.setId("role-1");
Role role2 = new Role();
role2.setId("role-2");
roles.add(role1);
roles.add(role2);

when(userRepository.findById(existingUser.getId())).thenReturn(Maybe.just(existingUser));
when(identityProviderManager.getUserProvider(anyString())).thenReturn(Maybe.just(userProvider));
when(identityProviderManager.getIdentityProvider(anyString())).thenReturn(new IdentityProvider());
when(tokenService.deleteByUser(any())).thenReturn(Completable.complete());
ArgumentCaptor<io.gravitee.am.model.User> userCaptor = ArgumentCaptor.forClass(io.gravitee.am.model.User.class);
when(userRepository.update(any(), any())).thenReturn(Single.just(existingUser));
when(groupService.findByMember(existingUser.getId())).thenReturn(Flowable.empty());
when(passwordService.isValid(eq(PASSWORD), any(), any())).thenReturn(true);

TestObserver<User> testObserver = userService.update(existingUser.getId(), scimUser, null, "/", null, null).test();
testObserver.assertNoErrors();
testObserver.assertComplete();

verify(userRepository, times(1)).update(userCaptor.capture(), any());
verify(userProvider).create(any());
verify(userProvider, never()).update(anyString(), any());
verify(userProvider, never()).updatePassword(any(), eq(PASSWORD));
verify(tokenService, times(1)).deleteByUser(any());
assertFalse(userCaptor.getValue().isEnabled());
}

@Test
public void shouldUpdateUser_roles_entitlements() {
io.gravitee.am.model.User existingUser = new io.gravitee.am.model.User();
Expand Down Expand Up @@ -579,6 +629,7 @@ public void shouldPatchUser() throws Exception {
when(userRepository.findById(userId)).thenReturn(Maybe.just(patchedUser));
when(identityProviderManager.getIdentityProvider(anyString())).thenReturn(new IdentityProvider());
when(identityProviderManager.getUserProvider(anyString())).thenReturn(Maybe.just(userProvider));
when(tokenService.deleteByUser(any())).thenReturn(Completable.complete());
doAnswer(invocation -> {
io.gravitee.am.model.User userToUpdate = invocation.getArgument(0);
Assert.assertEquals("my user 2", userToUpdate.getDisplayName());
Expand Down Expand Up @@ -671,6 +722,7 @@ public void shouldPatchUser_customGraviteeUser() throws Exception {
when(userRepository.findById(userId)).thenReturn(Maybe.just(patchedUser));
when(identityProviderManager.getIdentityProvider(anyString())).thenReturn(new IdentityProvider());
when(identityProviderManager.getUserProvider(anyString())).thenReturn(Maybe.just(userProvider));
when(tokenService.deleteByUser(any())).thenReturn(Completable.complete());
doAnswer(invocation -> {
io.gravitee.am.model.User userToUpdate = invocation.getArgument(0);
Assert.assertTrue(userToUpdate.getAdditionalInformation().containsKey("customClaim"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.gravitee.am.management.handlers.management.api.resources.organizations.settings.SettingsResource;
import io.gravitee.am.management.handlers.management.api.resources.organizations.tags.TagsResource;
import io.gravitee.am.management.handlers.management.api.resources.organizations.users.OrganizationUsersResource;

import jakarta.ws.rs.Path;
import jakarta.ws.rs.container.ResourceContext;
import jakarta.ws.rs.core.Context;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ public void updateUserStatus(
checkAnyPermission(organizationId, environmentId, domain, Permission.DOMAIN_USER, Acl.UPDATE)
.andThen(domainService.findById(domain)
.switchIfEmpty(Maybe.error(new DomainNotFoundException(domain)))
.flatMapSingle(irrelevant -> userService.updateStatus(ReferenceType.DOMAIN, domain, user, status.isEnabled(), authenticatedUser)))
.flatMapSingle(irrelevant -> userService.updateStatus(domain, user, status.isEnabled(), authenticatedUser)))
.subscribe(response::resume, response::resume);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,7 @@
import jakarta.ws.rs.PathParam;
import jakarta.ws.rs.Produces;
import jakarta.ws.rs.container.AsyncResponse;
import jakarta.ws.rs.container.ResourceContext;
import jakarta.ws.rs.container.Suspended;
import jakarta.ws.rs.core.Context;
import jakarta.ws.rs.core.Response;
import org.springframework.beans.factory.annotation.Autowired;

Expand All @@ -69,9 +67,6 @@
@SuppressWarnings("ResultOfMethodCallIgnored")
public class OrganizationUserResource extends AbstractResource {

@Context
private ResourceContext resourceContext;

@Autowired
@Named("managementOrganizationUserService")
private OrganizationUserService organizationUserService;
Expand Down Expand Up @@ -216,8 +211,8 @@ public void updateUserStatus(
final io.gravitee.am.identityprovider.api.User authenticatedUser = getAuthenticatedUser();

checkPermission(ReferenceType.ORGANIZATION, organizationId, Permission.ORGANIZATION_USER, Acl.UPDATE)
.andThen(organizationUserService.updateStatus(ReferenceType.ORGANIZATION, organizationId, user, status.isEnabled(), authenticatedUser)
.map(UserEntity::new))
.andThen(organizationUserService.updateStatus(organizationId, user, status.isEnabled(), authenticatedUser)
.map(UserEntity::new))
.subscribe(response::resume, response::resume);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.times;

/**
* @author Titouan COMPIEGNE (titouan.compiegne at graviteesource.com)
Expand Down Expand Up @@ -301,7 +302,34 @@ public void shouldNotUpdateUsername_domainNotFound() {
}

@Test
public void shouldUpdateStatus() {
public void shouldUpdateStatus_enabled() {
final String domainId = "domain-id";
final Domain mockDomain = new Domain();
mockDomain.setId(domainId);

final String userId = "userId";
final User mockUser = new User();
mockUser.setId(userId);
mockUser.setUsername("user-username");
mockUser.setReferenceType(ReferenceType.DOMAIN);
mockUser.setReferenceId(domainId);
mockUser.setEnabled(false);

var statusEntity = new StatusEntity();
statusEntity.setEnabled(false);
doReturn(Maybe.just(mockDomain)).when(domainService).findById(domainId);
doReturn(Single.just(mockUser)).when(userService).updateStatus(eq(ReferenceType.DOMAIN), eq(domainId), eq(userId), eq(statusEntity.isEnabled()), any());

final Response response = target("domains").path(domainId).path("users").path(userId).path("status").request().put(Entity.json(statusEntity));
assertEquals(HttpStatusCode.OK_200, response.getStatus());
final User user = readEntity(response, User.class);
assertEquals(domainId, user.getReferenceId());
assertEquals(statusEntity.isEnabled(), user.isEnabled());
Mockito.verifyNoInteractions(tokenService);
}

@Test
public void shouldUpdateStatus_disabled() {
final String domainId = "domain-id";
final Domain mockDomain = new Domain();
mockDomain.setId(domainId);
Expand Down Expand Up @@ -353,6 +381,8 @@ public void shouldUpdateStatus_organization() {
assertEquals(mockUser.getUsername(), user.getUsername());
assertNull(user.getPassword());
assertEquals(statusEntity.isEnabled(), user.isEnabled());
Mockito.verifyNoInteractions(tokenService);

}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,6 @@ public interface OrganizationUserService extends CommonUserService {
Single<User> findByAccessToken(String tokenId, String tokenValue);

Maybe<AccountAccessToken> revokeToken(String organizationId, String userId, String tokenId, io.gravitee.am.identityprovider.api.User authenticatedUser);
Single<User> updateStatus(String organizationId, String id, boolean status, io.gravitee.am.identityprovider.api.User principal);

}
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,6 @@ default Single<User> update(String domain, String id, UpdateUser updateUser) {
return update(domain, id, updateUser, null);
}

default Single<User> updateStatus(String domain, String userId, boolean status) {
return updateStatus(domain, userId, status, null);
}
default Completable unlock(ReferenceType referenceType, String referenceId, String userId) {
return unlock(referenceType, referenceId, userId, null);
}
Expand All @@ -79,8 +76,4 @@ default Single<User> revokeRoles(ReferenceType referenceType, String referenceId
return revokeRoles(referenceType, referenceId, userId, roles, null);
}

default Single<User> enrollFactors(String userId, List<EnrolledFactor> factors) {
return enrollFactors(userId, factors, null);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,9 @@ private Single<UpdateUser> updateWithUserProvider(UpdateUser updateUser, User
}

@Override
public Single<User> updateStatus(ReferenceType referenceType, String referenceId, String id, boolean status, io.
public Single<User> updateStatus(ReferenceType referenceType, String referenceId, String userId, boolean status, io.
gravitee.am.identityprovider.api.User principal) {
return getUserService().findById(referenceType, referenceId, id)
return getUserService().findById(referenceType, referenceId, userId)
.flatMap(user -> {
user.setEnabled(status);
return getUserService().update(user);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@

import static io.gravitee.am.management.service.impl.IdentityProviderManagerImpl.IDP_GRAVITEE;
import static org.springframework.util.StringUtils.hasText;
import static io.gravitee.am.model.ReferenceType.ORGANIZATION;

/**
* @author Titouan COMPIEGNE (titouan.compiegne at graviteesource.com)
Expand Down Expand Up @@ -275,4 +276,8 @@ public Single<User> delete(ReferenceType referenceType, String referenceId, Stri
return super.delete(referenceType, referenceId, userId, principal)
.flatMap(user -> getUserService().revokeUserAccessTokens(user.getReferenceType(), user.getReferenceId(), user.getId()).toSingleDefault(user));
}

public Single<User> updateStatus(String organizationId, String userId, boolean status, io.gravitee.am.identityprovider.api.User principal) {
return updateStatus(ORGANIZATION, organizationId, userId, status, principal);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public class UserServiceImpl extends AbstractUserService<io.gravitee.am.service.
private DomainService domainService;

@Autowired
protected io.gravitee.am.service.UserService userService;
private io.gravitee.am.service.UserService userService;

@Autowired
protected TokenService tokenService;
Expand Down Expand Up @@ -291,8 +291,21 @@ public Single<User> delete(ReferenceType referenceType, String referenceId, Stri
}

@Override
public Single<User> updateStatus(String domain, String id, boolean status, io.gravitee.am.identityprovider.api.User principal) {
return updateStatus(DOMAIN, domain, id, status, principal);
public Single<User> updateStatus(String domainId, String userId, boolean status, io.gravitee.am.identityprovider.api.User principal) {
return updateStatus(DOMAIN, domainId, userId, status, principal);
}

@Override
public Single<User> updateStatus(ReferenceType referenceType, String referenceId, String userId, boolean status, io.
gravitee.am.identityprovider.api.User principal) {
Completable removeTokens = status ? Completable.complete() : tokenService.deleteByUser(User.simpleUser(userId, referenceType, referenceId));
return getUserService().findById(referenceType, referenceId, userId)
.flatMap(user -> {
user.setEnabled(status);
return removeTokens.andThen(getUserService().update(user));
})
.doOnSuccess(user1 -> auditService.report(AuditBuilder.builder(UserAuditBuilder.class).principal(principal).type((status ? EventType.USER_ENABLED : EventType.USER_DISABLED)).user(user1)))
.doOnError(throwable -> auditService.report(AuditBuilder.builder(UserAuditBuilder.class).principal(principal).type((status ? EventType.USER_ENABLED : EventType.USER_DISABLED)).throwable(throwable)));
}

@Override
Expand Down
13 changes: 13 additions & 0 deletions gravitee-am-model/src/main/java/io/gravitee/am/model/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -547,4 +547,17 @@ public void unlockUser() {
setAccountLockedAt(null);
setAccountLockedUntil(null);
}

public boolean isDisabled(){
return Boolean.FALSE.equals(enabled);
}

public static User simpleUser(String userId, ReferenceType referenceType, String referenceId) {
User user = new User();
user.setId(userId);
user.setReferenceType(referenceType);
user.setReferenceId(referenceId);
return user;
}

}
Loading

0 comments on commit 3ffeece

Please sign in to comment.