diff --git a/openaev-api/src/main/java/io/openaev/api/platform/users/PlatformUserApi.java b/openaev-api/src/main/java/io/openaev/api/platform/users/PlatformUserApi.java index 9b822cd986f..fb2b659adf5 100644 --- a/openaev-api/src/main/java/io/openaev/api/platform/users/PlatformUserApi.java +++ b/openaev-api/src/main/java/io/openaev/api/platform/users/PlatformUserApi.java @@ -8,6 +8,7 @@ import io.openaev.api.users.dto.UserOutput; import io.openaev.database.model.Action; import io.openaev.database.model.ResourceType; +import io.openaev.service.UserCreationScope; import io.openaev.service.UserService; import io.openaev.utils.pagination.SearchPaginationInput; import io.swagger.v3.oas.annotations.Operation; @@ -38,7 +39,7 @@ public class PlatformUserApi { @ResponseStatus(HttpStatus.CREATED) @Transactional public UserOutput create(@Valid @RequestBody UserInput input) { - return toPlatformOutput(userService.createUser(input)); + return toPlatformOutput(userService.createUser(input, UserCreationScope.PLATFORM)); } // -- READ -- diff --git a/openaev-api/src/main/java/io/openaev/config/security/SecurityService.java b/openaev-api/src/main/java/io/openaev/config/security/SecurityService.java index f7411d2c181..ac7bec32f6c 100644 --- a/openaev-api/src/main/java/io/openaev/config/security/SecurityService.java +++ b/openaev-api/src/main/java/io/openaev/config/security/SecurityService.java @@ -69,7 +69,9 @@ public User userManagement( String.class, ""); userMappingService.mapCurrentUserWithGroup(groupsManagementObject, user, groups); - attachTenant(registrationId, user); + attachTenant(registrationId, user) + .ifPresent( + tenantId -> userService.assignAutoAssignGroups(user.getId(), List.of(tenantId))); return this.userService.saveUser(user); } else { // If user exists, update it @@ -86,7 +88,10 @@ public User userManagement( String.class, ""); userMappingService.mapCurrentUserWithGroup(groupsManagementObject, currentUser, groups); - attachTenant(registrationId, currentUser); + attachTenant(registrationId, currentUser) + .ifPresent( + tenantId -> + userService.assignAutoAssignGroups(currentUser.getId(), List.of(tenantId))); return this.userService.saveUser(currentUser); } } @@ -103,21 +108,27 @@ public String getAudience(@NotBlank final String registrationId) { // -- PRIVATE -- /** Attaches the user to the tenant configured for the given SSO provider registration. */ - private void attachTenant(String registrationId, User user) { + /** + * Attaches the user to the tenant configured for this identity provider, and returns that tenant + * ID when the user just entered it. Returns empty when nothing was attached, so a group + * deliberately removed from the user is never re-granted on a later login. + */ + private Optional attachTenant(String registrationId, User user) { String configuredTenantId = env.getProperty( OPENAEV_PROVIDER_PATH_PREFIX + registrationId + TENANT_ID_SUFFIX, String.class, ""); String tenantId = hasText(configuredTenantId) ? configuredTenantId : Tenant.DEFAULT_TENANT_UUID; boolean alreadyAttached = user.getTenants().stream().anyMatch(t -> t.getId().equals(tenantId)); if (alreadyAttached) { - return; + return Optional.empty(); } if (!tenantRepository.existsById(tenantId)) { log.warn("SSO tenant ID '{}' configured but not found in database", tenantId); - return; + return Optional.empty(); } Tenant tenant = tenantRepository.getReferenceById(tenantId); user.getTenants().add(tenant); + return Optional.of(tenantId); } private List getAdminRoles(@NotBlank final String registrationId) { diff --git a/openaev-api/src/main/java/io/openaev/service/UserCreationScope.java b/openaev-api/src/main/java/io/openaev/service/UserCreationScope.java new file mode 100644 index 00000000000..2177e2592c2 --- /dev/null +++ b/openaev-api/src/main/java/io/openaev/service/UserCreationScope.java @@ -0,0 +1,20 @@ +package io.openaev.service; + +/** + * Scope a user is created from. It decides which auto-assign groups the new user inherits: the + * creator can only hand out groups it has authority over. + */ +public enum UserCreationScope { + /** + * Creation from the platform: the platform administrator has authority over every tenant, so the + * platform auto-assign groups are granted, plus those of each tenant carried by the input. + */ + PLATFORM, + + /** + * Creation from within a tenant: the creator has no authority over the platform, so no + * platform-wide group is ever granted and the tenants carried by the input are ignored. The + * caller attaches the user to its own tenant, then grants that tenant's auto-assign groups. + */ + TENANT +} diff --git a/openaev-api/src/main/java/io/openaev/service/UserService.java b/openaev-api/src/main/java/io/openaev/service/UserService.java index 0f3fc779a15..d3d79b8f4c1 100644 --- a/openaev-api/src/main/java/io/openaev/service/UserService.java +++ b/openaev-api/src/main/java/io/openaev/service/UserService.java @@ -126,8 +126,12 @@ public long globalCount() { // -- CREATE -- + /** + * Creates a user. The {@link UserCreationScope} decides which auto-assign groups are granted, and + * whether the tenants carried by the input are honoured. + */ @Transactional(rollbackFor = Exception.class) - public User createUser(UserInput input) { + public User createUser(UserInput input, UserCreationScope scope) { if (!StringUtils.hasLength(input.plainPassword())) { throw new IllegalArgumentException("Password is required when creating a user"); } @@ -137,24 +141,32 @@ public User createUser(UserInput input) { "User with email " + input.email() + " already exists"); } PrivilegeEscalationValidator.assertAdminFlagUnchanged(input.admin(), false); + // A tenant creator has no authority over other tenants: it never attaches any, the caller + // attaches its own right after. + List tenantIds = scope == UserCreationScope.PLATFORM ? input.tenantIds() : List.of(); User user = new User(); user.setUpdateAttributes(input); user.setTags(referenceResolver.resolve(input.tagIds(), Tag.class, tagRepository::countByIdIn)); user.setOrganization(referenceResolver.resolve(input.organizationId(), Organization.class)); user.setTenants( new ArrayList<>( - referenceResolver.resolve( - input.tenantIds(), Tenant.class, tenantRepository::countByIdIn))); + referenceResolver.resolve(tenantIds, Tenant.class, tenantRepository::countByIdIn))); // The user's id is generated on save (UUID generator), not before: evict only after // persisting, using the saved user's id, or evictForUser is called with a null key. - User createdUser = createUser(user, input.plainPassword(), UUID.randomUUID().toString()); - if (!CollectionUtils.isEmpty(input.tenantIds())) { - tenantMembershipCacheManager.evictForUser(createdUser.getId(), input.tenantIds()); + User createdUser = createUser(user, input.plainPassword(), UUID.randomUUID().toString(), scope); + if (!CollectionUtils.isEmpty(tenantIds)) { + tenantMembershipCacheManager.evictForUser(createdUser.getId(), tenantIds); } return createdUser; } - /** Creates a user for internal/technical purposes (SSO login, connector provisioning). */ + /** + * Creates a user for internal/technical purposes (SSO login, connector provisioning, service + * accounts). Such a user always lands in a tenant, attached by the caller right after: the + * platform auto-assign groups are therefore never granted. Callers own the group assignment — + * either explicitly (technical accounts) or through {@link #assignAutoAssignGroups(String, + * Collection)} once the tenant is attached (SSO). + */ @Transactional(rollbackFor = Exception.class) public User createInternalUser( String email, String firstname, String lastname, boolean isAdmin, String token) { @@ -166,15 +178,19 @@ public User createInternalUser( user.setFirstname(firstname); user.setLastname(lastname); user.setAdmin(isAdmin); - return createUser(user, null, token); + return createUser(user, null, token, UserCreationScope.TENANT); } - private User createUser(User user, String password, String token) { + private User createUser(User user, String password, String token, UserCreationScope scope) { if (StringUtils.hasLength(password)) { user.setPassword(this.encodeUserPassword(password)); } - // Creation enters every scope at once: platform, plus each tenant attached in the input. - assignAutoAssignGroups(user, user.getTenants().stream().map(Tenant::getId).toList(), true); + // Creation enters every scope at once: the platform when created from the platform screen, + // plus each tenant attached in the input. + assignAutoAssignGroups( + user, + user.getTenants().stream().map(Tenant::getId).toList(), + scope == UserCreationScope.PLATFORM); User savedUser = userRepository.save(user); this.createUserToken(savedUser, token); return savedUser; diff --git a/openaev-api/src/main/java/io/openaev/service/tenants/TenantUserService.java b/openaev-api/src/main/java/io/openaev/service/tenants/TenantUserService.java index aa34719c539..05a4b1971d3 100644 --- a/openaev-api/src/main/java/io/openaev/service/tenants/TenantUserService.java +++ b/openaev-api/src/main/java/io/openaev/service/tenants/TenantUserService.java @@ -19,6 +19,7 @@ import io.openaev.database.specification.UserSpecification; import io.openaev.multitenancy.DependenciesManager; import io.openaev.rest.exception.ElementNotFoundException; +import io.openaev.service.UserCreationScope; import io.openaev.service.UserService; import io.openaev.service.account.PrivilegeEscalationValidator; import io.openaev.service.account.ReservedKeyValidator; @@ -62,7 +63,7 @@ public UserOutput createOrAttach(UserInput input) { User reloaded = userRepository.findById(userId).orElseThrow(); return UserMapper.toOutput(reloaded); } - User user = userService.createUser(input); + User user = userService.createUser(input, UserCreationScope.TENANT); attachToTenant(user.getId(), tenantId); userService.assignAutoAssignGroups(user.getId(), List.of(tenantId)); // Reload user after @Modifying queries cleared the persistence context diff --git a/openaev-api/src/test/java/io/openaev/service/UserServiceTest.java b/openaev-api/src/test/java/io/openaev/service/UserServiceTest.java index 3bfb4e07334..eec8f2a4ccf 100644 --- a/openaev-api/src/test/java/io/openaev/service/UserServiceTest.java +++ b/openaev-api/src/test/java/io/openaev/service/UserServiceTest.java @@ -16,6 +16,7 @@ import jakarta.persistence.EntityManager; import jakarta.persistence.PersistenceContext; import java.util.List; +import java.util.UUID; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; @@ -40,7 +41,7 @@ void given_validInput_should_createUser() { UserInput input = getUserInputWithPasswordAndPhone( "create@test.invalid", "John", "Doe", "secureP@ss1", "+33612345678"); - User created = userService.createUser(input); + User created = userService.createUser(input, UserCreationScope.PLATFORM); // -- ASSERT -- assertThat(created.getId()).isNotNull(); @@ -69,7 +70,7 @@ void given_inputWithTenantIds_should_createUserAndEvictMembershipCache() { List.of(tenant.getId())); // -- ACT -- - User created = userService.createUser(input); + User created = userService.createUser(input, UserCreationScope.PLATFORM); // -- ASSERT -- assertThat(created.getId()).isNotNull(); @@ -93,7 +94,8 @@ void given_platformCreationWithTenants_should_assignAutoAssignGroups() { "Auto", "Assign", "secureP@ss1", - List.of(tenant.getId()))); + List.of(tenant.getId())), + UserCreationScope.PLATFORM); // -- ASSERT -- entityManager.flush(); @@ -104,6 +106,32 @@ void given_platformCreationWithTenants_should_assignAutoAssignGroups() { .contains(platformGroup.getId(), tenantGroup.getId()); } + @Test + @DisplayName("given_internalUserCreation_should_notAssignPlatformAutoAssignGroups") + void given_internalUserCreation_should_notAssignPlatformAutoAssignGroups() { + // -- ARRANGE -- + // Internal accounts (SSO, connectors, service accounts) always land in a tenant attached by + // the caller: they must never inherit the platform-wide auto-assign groups. + Group platformGroup = autoAssignGroup("platform-auto-assign-internal", null); + + // -- ACT -- + User created = + userService.createInternalUser( + "internal-auto-assign@test.invalid", + "Internal", + "Account", + false, + UUID.randomUUID().toString()); + + // -- ASSERT -- + entityManager.flush(); + entityManager.clear(); + User reloaded = userService.user(created.getId()); + assertThat(reloaded.getUnscopedGroups()) + .extracting(Group::getId) + .doesNotContain(platformGroup.getId()); + } + @Test @DisplayName("given_platformUpdateAttachingTenant_should_assignTenantAutoAssignGroups") void given_platformUpdateAttachingTenant_should_assignAutoAssignGroups() { @@ -148,7 +176,7 @@ void given_groupRemovedInTenant_should_notReassignItOnPlatformUpdate() { "Removed", "secureP@ss1", List.of(tenant.getId())); - User created = userService.createUser(input); + User created = userService.createUser(input, UserCreationScope.PLATFORM); // The tenant admin removes the auto-assign group from the user, from within the tenant. created.getUnscopedGroups().remove(tenantGroup); userService.saveUser(created); @@ -181,7 +209,7 @@ void given_alreadyAttachedTenant_should_notAssignAutoAssignGroupsOnUpdate() { "Unchanged", "secureP@ss1", List.of(tenant.getId())); - User created = userService.createUser(input); + User created = userService.createUser(input, UserCreationScope.PLATFORM); entityManager.flush(); entityManager.clear(); // The auto-assign group only appears after the user already belongs to the tenant. @@ -216,7 +244,8 @@ void given_platformUpdateDetachingTenant_should_revokeTenantGroups() { "Detach", "Tenant", "secureP@ss1", - List.of(kept.getId(), left.getId()))); + List.of(kept.getId(), left.getId())), + UserCreationScope.PLATFORM); entityManager.flush(); entityManager.clear(); @@ -256,7 +285,8 @@ void given_revokeTenantGroups_should_onlyDropTheGivenTenantGroups() { "Revoke", "Scoped", "secureP@ss1", - List.of(tenant.getId()))); + List.of(tenant.getId())), + UserCreationScope.PLATFORM); entityManager.flush(); entityManager.clear(); diff --git a/openaev-api/src/test/java/io/openaev/service/tenants/TenantUserServiceTest.java b/openaev-api/src/test/java/io/openaev/service/tenants/TenantUserServiceTest.java index 741fa78438f..0566a937670 100644 --- a/openaev-api/src/test/java/io/openaev/service/tenants/TenantUserServiceTest.java +++ b/openaev-api/src/test/java/io/openaev/service/tenants/TenantUserServiceTest.java @@ -16,11 +16,14 @@ import io.openaev.database.raw.RawUser; import io.openaev.database.repository.GroupRepository; import io.openaev.database.repository.TenantRepository; +import io.openaev.database.repository.UserRepository; import io.openaev.rest.exception.ElementNotFoundException; import io.openaev.utils.fixtures.PaginationFixture; import io.openaev.utils.fixtures.TenantGroupFixture; import io.openaev.utils.fixtures.composers.TenantGroupComposer; import io.openaev.utils.fixtures.composers.UserComposer; +import io.openaev.utils.fixtures.platform.PlatformGroupComposer; +import io.openaev.utils.fixtures.platform.PlatformGroupFixture; import io.openaev.utils.fixtures.tenants.TenantComposer; import io.openaev.utils.mockUser.WithMockUser; import io.openaev.utils.pagination.SearchPaginationInput; @@ -45,7 +48,9 @@ class TenantUserServiceTest extends IntegrationTest { @Autowired private UserComposer userComposer; @Autowired private TenantComposer tenantComposer; @Autowired private TenantGroupComposer tenantGroupComposer; + @Autowired private PlatformGroupComposer platformGroupComposer; @Autowired private GroupRepository groupRepository; + @Autowired private UserRepository userRepository; @Autowired private EntityManager entityManager; private Tenant tenant; @@ -348,5 +353,65 @@ void given_existingUser_should_autoAssignOnAttach() { Group reloaded = groupRepository.findById(autoGroup.getId()).orElseThrow(); assertThat(reloaded.getUsers()).extracting(User::getId).contains(existingUser.getId()); } + + @Test + @DisplayName("Given a platform auto-assign group, should not assign a user created in a tenant") + void given_platformAutoAssignGroup_should_notAssignTenantCreatedUser() { + // -- ARRANGE -- + Group platformAutoGroup = PlatformGroupFixture.getPlatformGroup("AutoAssignPlatform"); + platformAutoGroup.setDefaultUserAssignation(true); + platformGroupComposer.forPlatformGroup(platformAutoGroup).persist(); + Group tenantAutoGroup = TenantGroupFixture.getGroup("AutoAssignTenantScoped"); + tenantAutoGroup.setDefaultUserAssignation(true); + tenantGroupComposer.forGroup(tenantAutoGroup).persist(); + entityManager.flush(); + + UserInput input = getUserInput("tenant-scoped@test.invalid", "Tenant", "Scoped"); + + // -- ACT -- + UserOutput result = tenantUserService.createOrAttach(input); + + // -- ASSERT -- + entityManager.flush(); + entityManager.clear(); + Group reloadedPlatform = groupRepository.findById(platformAutoGroup.getId()).orElseThrow(); + assertThat(reloadedPlatform.getUsers()).extracting(User::getId).doesNotContain(result.id()); + Group reloadedTenant = groupRepository.findById(tenantAutoGroup.getId()).orElseThrow(); + assertThat(reloadedTenant.getUsers()).extracting(User::getId).contains(result.id()); + } + + @Test + @DisplayName("Given tenants in the input, should ignore them when creating from a tenant") + void given_tenantIdsInInput_should_ignoreThem() { + // -- ARRANGE -- + Tenant otherTenant = tenantComposer.forTenant(getTenant("Other tenant")).persist().get(); + entityManager.flush(); + UserInput base = getUserInput("cross-tenant@test.invalid", "Cross", "Tenant"); + UserInput input = + new UserInput( + base.email(), + base.firstname(), + base.lastname(), + base.plainPassword(), + base.pgpKey(), + base.phone(), + base.phone2(), + base.organizationId(), + base.tagIds(), + base.admin(), + List.of(otherTenant.getId())); + + // -- ACT -- + UserOutput result = tenantUserService.createOrAttach(input); + + // -- ASSERT -- + entityManager.flush(); + entityManager.clear(); + User reloaded = userRepository.findById(result.id()).orElseThrow(); + assertThat(reloaded.getTenants()) + .extracting(Tenant::getId) + .containsExactly(tenant.getId()) + .doesNotContain(otherTenant.getId()); + } } }