This is an automated email from the ASF dual-hosted git repository. sergehuber pushed a commit to branch UNOMI-875-pr4-security-tenant in repository https://gitbox.apache.org/repos/asf/unomi.git
commit 9f993faec9184656b4fd1734a23ae267454db62c Author: Serge Huber <[email protected]> AuthorDate: Sun Jul 5 11:16:17 2026 +0200 UNOMI-941: Harden security service thread-safety and tenant authorization Derive @RequiresTenant checks from the authenticated subject instead of X-Unomi-Tenant header. Make the system Subject immutable via AtomicReference and clear thread-local subjects with remove() after executeAsSystem. --- .../apache/unomi/rest/security/SecurityFilter.java | 4 ++-- .../security/ExecutionContextManagerImpl.java | 6 ++++- .../common/security/KarafSecurityService.java | 17 ++++++++------ .../common/security/KarafSecurityServiceTest.java | 23 +++++++++++++++++++ .../impl/ExecutionContextManagerImplTest.java | 26 ++++++++++++++++++++++ 5 files changed, 66 insertions(+), 10 deletions(-) diff --git a/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java b/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java index d7359d82f..4c4551058 100644 --- a/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java +++ b/rest/src/main/java/org/apache/unomi/rest/security/SecurityFilter.java @@ -73,7 +73,7 @@ public class SecurityFilter implements ContainerRequestFilter { // Check tenants-based access if (tenantAnnotation != null) { - String tenantId = requestContext.getHeaderString("X-Unomi-Tenant"); + String tenantId = securityService.getCurrentSubjectTenantId(); if (tenantId == null) { requestContext.abortWith(Response.status(Response.Status.BAD_REQUEST) .entity("Tenant ID is required") @@ -82,7 +82,7 @@ public class SecurityFilter implements ContainerRequestFilter { } if (!securityService.hasTenantAccess(tenantId)) { requestContext.abortWith(Response.status(Response.Status.FORBIDDEN) - .entity("User does not have access to tenants") + .entity("User does not have access to tenant") .build()); return; } diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java b/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java index 6cf39713a..926850c54 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java @@ -116,7 +116,11 @@ public class ExecutionContextManagerImpl implements ExecutionContextManager { } else { currentContext.remove(); } - securityService.setCurrentSubject(previousSubject); + if (previousSubject == null) { + securityService.clearCurrentSubject(); + } else { + securityService.setCurrentSubject(previousSubject); + } } catch (Exception e) { LOGGER.error("Error restoring previous context: {}", e.getMessage(), e); // Do not rethrow — would suppress the original operation exception if both fail together diff --git a/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java b/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java index b084dff8a..6cbc6d8f7 100644 --- a/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java +++ b/services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java @@ -30,6 +30,7 @@ import java.util.Arrays; import java.util.HashSet; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; /** @@ -47,7 +48,7 @@ public class KarafSecurityService implements SecurityService { /** The system tenant identifier used for system-wide operations. */ public static final String SYSTEM_TENANT = "system"; - private final Subject SYSTEM_SUBJECT; + private final AtomicReference<Subject> systemSubject = new AtomicReference<>(); private SecurityServiceConfiguration configuration; private EncryptionService encryptionService; @@ -60,7 +61,7 @@ public class KarafSecurityService implements SecurityService { * Creates the security service and initializes the system subject. */ public KarafSecurityService() { - SYSTEM_SUBJECT = createSystemSubject(); + systemSubject.set(createSystemSubject()); } private Subject createSystemSubject() { @@ -90,12 +91,14 @@ public class KarafSecurityService implements SecurityService { } private void updateSystemSubject() { - SYSTEM_SUBJECT.getPrincipals().clear(); - SYSTEM_SUBJECT.getPrincipals().add(new TenantPrincipal(SYSTEM_TENANT)); - SYSTEM_SUBJECT.getPrincipals().add(new UserPrincipal("system")); + Subject subject = new Subject(); + subject.getPrincipals().add(new TenantPrincipal(SYSTEM_TENANT)); + subject.getPrincipals().add(new UserPrincipal("system")); for (String role : configuration.getSystemRoles()) { - SYSTEM_SUBJECT.getPrincipals().add(new RolePrincipal(role)); + subject.getPrincipals().add(new RolePrincipal(role)); } + subject.setReadOnly(); + systemSubject.set(subject); } /** @@ -315,7 +318,7 @@ public class KarafSecurityService implements SecurityService { @Override public Subject getSystemSubject() { - return SYSTEM_SUBJECT; + return systemSubject.get(); } @Override diff --git a/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java b/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java index a69244f65..686b0f03e 100644 --- a/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java +++ b/services-common/src/test/java/org/apache/unomi/services/common/security/KarafSecurityServiceTest.java @@ -78,6 +78,7 @@ public class KarafSecurityServiceTest { public void testGetSystemSubject() { Subject systemSubject = securityService.getSystemSubject(); assertNotNull("System subject should not be null", systemSubject); + assertTrue("System subject should be read-only", systemSubject.isReadOnly()); Set<Principal> principals = systemSubject.getPrincipals(); assertTrue("System subject should have UserPrincipal", @@ -94,6 +95,28 @@ public class KarafSecurityServiceTest { roles.contains(UnomiRoles.SYSTEM_MAINTENANCE)); } + @Test + public void testSystemSubjectImmutableAfterReconfiguration() { + Subject originalSystemSubject = securityService.getSystemSubject(); + Set<String> originalRoles = extractRoles(originalSystemSubject.getPrincipals()); + + SecurityServiceConfiguration newConfig = new SecurityServiceConfiguration(); + newConfig.setSystemRoles(new HashSet<>(Collections.singletonList(UnomiRoles.ADMINISTRATOR))); + securityService.setConfiguration(newConfig); + securityService.init(); + + Subject updatedSystemSubject = securityService.getSystemSubject(); + assertNotSame("System subject should be replaced atomically", + originalSystemSubject, updatedSystemSubject); + assertTrue("Updated system subject should be read-only", updatedSystemSubject.isReadOnly()); + assertEquals("Original subject principals should be unchanged", originalRoles, + extractRoles(originalSystemSubject.getPrincipals())); + assertTrue("Updated system subject should reflect new configuration", + extractRoles(updatedSystemSubject.getPrincipals()).contains(UnomiRoles.ADMINISTRATOR)); + assertFalse("Updated system subject should not retain removed roles", + extractRoles(updatedSystemSubject.getPrincipals()).contains(UnomiRoles.SYSTEM_MAINTENANCE)); + } + @Test public void testCurrentSubjectManagement() { // Test initial state diff --git a/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java b/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java index 4a732c83d..e8740f6c5 100644 --- a/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java +++ b/services/src/test/java/org/apache/unomi/services/impl/ExecutionContextManagerImplTest.java @@ -16,6 +16,7 @@ */ package org.apache.unomi.services.impl; +import org.apache.karaf.jaas.boot.principal.UserPrincipal; import org.apache.unomi.api.ExecutionContext; import org.apache.unomi.api.security.SecurityService; import org.apache.unomi.api.security.SecurityServiceConfiguration; @@ -34,6 +35,7 @@ import java.util.Set; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -88,6 +90,7 @@ public class ExecutionContextManagerImplTest { Set<String> systemPermissions = new HashSet<>(Arrays.asList("READ", "WRITE", SecurityServiceConfiguration.PERMISSION_DELETE, "ADMIN")); // Mock security service behavior + when(securityService.getCurrentSubject()).thenReturn(null); when(securityService.getSystemSubject()).thenReturn(systemSubject); when(securityService.extractRolesFromSubject(systemSubject)).thenReturn(systemRoles); when(securityService.getPermissionsForRole(UnomiRoles.ADMINISTRATOR)).thenReturn(systemPermissions); @@ -106,6 +109,29 @@ public class ExecutionContextManagerImplTest { verify(securityService).getSystemSubject(); verify(securityService).extractRolesFromSubject(systemSubject); verify(securityService).getPermissionsForRole(UnomiRoles.ADMINISTRATOR); + verify(securityService).clearCurrentSubject(); + verify(securityService, never()).setCurrentSubject(null); + } + + @Test + public void testExecuteAsSystemRestoresPreviousSubject() { + Subject previousSubject = new Subject(); + previousSubject.getPrincipals().add(new UserPrincipal("previous")); + Subject systemSubject = new Subject(); + systemSubject.getPrincipals().add(new UserPrincipal("system")); + Set<String> systemRoles = new HashSet<>(Arrays.asList(UnomiRoles.ADMINISTRATOR)); + Set<String> systemPermissions = new HashSet<>(Arrays.asList("ADMIN")); + + when(securityService.getCurrentSubject()).thenReturn(previousSubject); + when(securityService.getSystemSubject()).thenReturn(systemSubject); + when(securityService.extractRolesFromSubject(systemSubject)).thenReturn(systemRoles); + when(securityService.getPermissionsForRole(UnomiRoles.ADMINISTRATOR)).thenReturn(systemPermissions); + + contextManager.executeAsSystem(() -> "done"); + + verify(securityService).setCurrentSubject(systemSubject); + verify(securityService).setCurrentSubject(previousSubject); + verify(securityService, never()).clearCurrentSubject(); } @Test
