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

[4.4.x] fix: tokens should be removed when user is disabled #5353

Merged
merged 1 commit into from
Jan 15, 2025
Merged

[4.4.x] fix: tokens should be removed when user is disabled #5353

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
13 changes: 11 additions & 2 deletions ...-scim/src/main/java/io/gravitee/am/gateway/handler/scim/service/impl/UserServiceImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,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 @@ -148,6 +149,9 @@ public class UserServiceImpl implements UserService {
@Autowired
private PasswordPolicyManager passwordPolicyManager;

@Autowired
private TokenService tokenService;

@Override
public Single<ListResponse<User>> list(Filter filter, int page, int size, String baseUrl) {
LOGGER.debug("Find users by domain: {}", domain.getId());
Expand Down Expand Up @@ -403,7 +407,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 @@ -412,7 +416,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 @@ -441,6 +445,11 @@ 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
53 changes: 53 additions & 0 deletions ...ndler-scim/src/test/java/io/gravitee/am/gateway/handler/scim/service/UserServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,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 @@ -150,6 +152,9 @@ public class UserServiceTest {
@Mock
private PasswordPolicyManager passwordPolicyManager;

@Mock
private TokenService tokenService;

@Before
public void setUp() {
when(passwordHistoryService.addPasswordToHistory(any(), any(), any(), any(), any(), any())).thenReturn(Maybe.just(new PasswordHistory()));
Expand Down Expand Up @@ -383,9 +388,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 = mock(io.gravitee.am.model.User.class);
Expand Down Expand Up @@ -579,6 +630,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.assertTrue(userToUpdate.getDisplayName().equals("my user 2"));
Expand Down Expand Up @@ -632,6 +684,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
9 changes: 3 additions & 6 deletions ...e/am/management/handlers/management/api/resources/organizations/OrganizationResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@
import io.gravitee.am.management.handlers.management.api.resources.organizations.roles.RolesResource;
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.UsersResource;
import io.gravitee.am.management.handlers.management.api.resources.platform.plugins.PluginsResource;
import io.gravitee.am.service.OrganizationService;
import org.springframework.beans.factory.annotation.Autowired;
import io.gravitee.am.management.handlers.management.api.resources.organizations.users.OrganizationUsersResource;

import jakarta.ws.rs.Path;
import jakarta.ws.rs.container.ResourceContext;
Expand Down Expand Up @@ -85,8 +82,8 @@ public IdentityProvidersResource getIdentityProvidersResource() {
}

@Path("users")
public UsersResource getUsersResource() {
return resourceContext.getResource(UsersResource.class);
public OrganizationUsersResource getUsersResource() {
return resourceContext.getResource(OrganizationUsersResource.class);
}

@Path("settings")
Expand Down
2 changes: 1 addition & 1 deletion ...nt/handlers/management/api/resources/organizations/environments/domains/UserResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,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
11 changes: 3 additions & 8 deletions ...ces/organizations/users/UserResource.java → ...tions/users/OrganizationUserResource.java
View file
Open in desktop
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 @@ -67,10 +65,7 @@
* @author GraviteeSource Team
*/
@SuppressWarnings("ResultOfMethodCallIgnored")
public class UserResource extends AbstractResource {

@Context
private ResourceContext resourceContext;
public class OrganizationUserResource extends AbstractResource {

@Autowired
@Named("managementOrganizationUserService")
Expand Down Expand Up @@ -214,8 +209,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
6 changes: 3 additions & 3 deletions ...es/organizations/users/UsersResource.java → ...ions/users/OrganizationUsersResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
* @author GraviteeSource Team
*/
@Tags({@Tag(name= "user")})
public class UsersResource extends AbstractUsersResource {
public class OrganizationUsersResource extends AbstractUsersResource {

@Context
private ResourceContext resourceContext;
Expand Down Expand Up @@ -139,8 +139,8 @@ public void create(
}

@Path("{user}")
public UserResource getUserResource() {
return resourceContext.getResource(UserResource.class);
public OrganizationUserResource getUserResource() {
return resourceContext.getResource(OrganizationUserResource.class);
}

private Single<User> filterUserInfos(Map<Permission, Set<Acl>> organizationPermissions, User user) {
Expand Down
48 changes: 39 additions & 9 deletions ...st/java/io/gravitee/am/management/handlers/management/api/resources/UserResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,14 @@
import org.assertj.core.api.Assertions;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.Test;
import org.mockito.Mockito;

import java.util.Date;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static io.gravitee.am.model.User.simpleUser;
import static jakarta.ws.rs.HttpMethod.PATCH;
import static org.glassfish.jersey.client.HttpUrlConnectorProvider.SET_METHOD_WORKAROUND;
import static org.junit.Assert.assertEquals;
Expand All @@ -52,6 +54,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 @@ -256,7 +259,8 @@ public void shouldDeleteUser() {

final String userId = "userId";
doReturn(Maybe.just(mockDomain)).when(domainService).findById(domainId);
doReturn(Completable.complete()).when(userService).delete(eq(ReferenceType.DOMAIN), eq(domainId), eq(userId), any());
doReturn(Single.just(simpleUser("id", ReferenceType.DOMAIN, domainId)))
.when(userService).delete(eq(ReferenceType.DOMAIN), eq(domainId), eq(userId), any());
doReturn(Completable.complete()).when(userActivityService).deleteByDomainAndUser(domainId, userId);

final Response response = target("domains").path(domainId).path("users").path(userId).request().delete();
Expand Down Expand Up @@ -300,7 +304,7 @@ 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);
Expand All @@ -318,6 +322,33 @@ public void shouldUpdateStatus() {
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);

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(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);
Expand All @@ -341,7 +372,7 @@ public void shouldUpdateStatus_organization() {
var statusEntity = new StatusEntity();
statusEntity.setEnabled(false);
doReturn(Single.just(mockUser)).when(organizationUserService)
.updateStatus(eq(ReferenceType.ORGANIZATION), eq(referenceId), eq(userId), eq(statusEntity.isEnabled()), any());
.updateStatus(eq(referenceId), eq(userId), eq(statusEntity.isEnabled()), any());

final Response response = target("organizations").path(referenceId).path("users").path(userId).path("status").request().put(Entity.json(statusEntity));
assertEquals(HttpStatusCode.OK_200, response.getStatus());
Expand All @@ -352,6 +383,8 @@ public void shouldUpdateStatus_organization() {
assertEquals(mockUser.getUsername(), user.getUsername());
assertNull(user.getPassword());
assertEquals(statusEntity.isEnabled(), user.isEnabled());
Mockito.verifyNoInteractions(tokenService);

}

@Test
Expand Down Expand Up @@ -472,20 +505,17 @@ public void shouldCreateAccountToken() {

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

final String userId = "user-id";
doReturn(Maybe.empty()).when(identityProviderService).findById(any());
doReturn(Maybe.just(mockDomain)).when(domainService).findById(domainId);

var accessToken1 = AccountAccessToken.builder().tokenId("1").build();
var accessToken2 = AccountAccessToken.builder().tokenId("2").build();

doReturn(Flowable.just(List.of(accessToken1, accessToken2))).when(organizationUserService).findAccountAccessTokens("DEFAULT", userId);
doReturn(Flowable.just(accessToken1, accessToken2)).when(organizationUserService).findAccountAccessTokens(organizationId, userId);

final Response response = target("domains").path(domainId).path("users").path(userId).path("tokens").request().get();
final Response response = target("organizations").path(organizationId).path("users").path(userId).path("tokens").request().get();
assertEquals(HttpStatusCode.OK_200, response.getStatus());

final List<AccountAccessToken> tokens = readListEntity(response, AccountAccessToken.class);
Expand Down
3 changes: 3 additions & 0 deletions ...-api-service/src/main/java/io/gravitee/am/management/service/OrganizationUserService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,4 +46,7 @@ public interface OrganizationUserService extends CommonUserService {
Single<User> findByAccessToken(String tokenId, String tokenValue);

Completable 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);

}
7 changes: 0 additions & 7 deletions ...m-management-api-service/src/main/java/io/gravitee/am/management/service/UserService.java
View file
Open in desktop
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);
}

}
Loading