sergehuber commented on code in PR #760:
URL: https://github.com/apache/unomi/pull/760#discussion_r3348484070
##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java:
##########
@@ -133,14 +180,31 @@ private boolean isAuthenticatedUser(HttpServletRequest
req) {
}
});
loginContext.login();
- Subject subject = loginContext.getSubject();
- boolean success = subject != null;
+ Subject loginSubject = loginContext.getSubject();
+ boolean success = loginSubject != null;
if (success) {
req.setAttribute(REMOTE_USER, username);
+ // Set the security context for JAAS authentication
+ securityService.setCurrentSubject(loginSubject);
+
+ // Check for tenant ID header
+ String tenantId = req.getHeader(UNOMI_TENANT_ID_HEADER);
+ if (tenantId != null && !tenantId.trim().isEmpty()) {
+ // Validate tenant exists
+ Tenant tenant = tenantService.getTenant(tenantId);
+ if (tenant != null) {
+
executionContextManager.setCurrentContext(executionContextManager.createContext(tenantId));
+ } else {
+ LOG.warn("Invalid tenant ID provided in header: {}",
tenantId);
+
executionContextManager.setCurrentContext(ExecutionContext.systemContext());
Review Comment:
**[C2 CRITICAL] JAAS user with unknown tenant ID receives full system
privileges**
When JAAS authentication succeeds but `X-Unomi-Tenant-Id` names a tenant
that does not exist, this line grants `systemContext()` — which sets `isSystem
= true` and bypasses every `validateAccess()` check. Any Karaf user can exploit
this by passing a random non-existent tenant ID.
Safe fix: drop the context override and let the request continue under the
JAAS user’s own subject-derived context.
```suggestion
// Unknown tenant — do not grant system privileges;
leave context derived from JAAS login
```
##########
services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java:
##########
@@ -0,0 +1,333 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.services.common.security;
+
+import org.apache.karaf.jaas.boot.principal.RolePrincipal;
+import org.apache.karaf.jaas.boot.principal.UserPrincipal;
+import org.apache.unomi.api.security.*;
+import org.apache.unomi.api.tenants.AuditService;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.security.auth.Subject;
+import java.security.AccessController;
+import java.security.Principal;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class KarafSecurityService implements SecurityService {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(KarafSecurityService.class);
+
+ public static final String SYSTEM_TENANT = "system";
+ private final Subject SYSTEM_SUBJECT;
+
+ private SecurityServiceConfiguration configuration;
+ private EncryptionService encryptionService;
+ private AuditService tenantAuditService;
+
+ private final ThreadLocal<Subject> currentSubject = new ThreadLocal<>();
+ private final ThreadLocal<Subject> privilegedSubject = new ThreadLocal<>();
+
+ public KarafSecurityService() {
+ SYSTEM_SUBJECT = createSystemSubject();
+ }
+
+ private Subject createSystemSubject() {
+ Subject subject = new Subject();
+ subject.getPrincipals().add(new UserPrincipal("system"));
+ subject.getPrincipals().add(new
RolePrincipal(UnomiRoles.ADMINISTRATOR));
+ subject.getPrincipals().add(new
RolePrincipal(UnomiRoles.TENANT_ADMINISTRATOR));
+ subject.getPrincipals().add(new
RolePrincipal(UnomiRoles.SYSTEM_MAINTENANCE));
+ return subject;
+ }
+
+ public void init() {
+ if (configuration == null) {
+ configuration = new SecurityServiceConfiguration();
+ }
+ updateSystemSubject();
+ }
+
+ public void destroy() {
+ // Cleanup
+ }
+
+ private void updateSystemSubject() {
+ SYSTEM_SUBJECT.getPrincipals().clear();
+ SYSTEM_SUBJECT.getPrincipals().add(new TenantPrincipal(SYSTEM_TENANT));
+ SYSTEM_SUBJECT.getPrincipals().add(new UserPrincipal("system"));
+ for (String role : configuration.getSystemRoles()) {
+ SYSTEM_SUBJECT.getPrincipals().add(new RolePrincipal(role));
+ }
+ }
+
+ public void setTenantAuditService(AuditService tenantAuditService) {
+ this.tenantAuditService = tenantAuditService;
+ }
+
+ public void setConfiguration(SecurityServiceConfiguration configuration) {
+ this.configuration = configuration;
+ }
+
+ public void bindEncryptionService(EncryptionService encryptionService) {
+ this.encryptionService = encryptionService;
+ }
+
+ public void unbindEncryptionService(EncryptionService encryptionService) {
+ this.encryptionService = null;
+ }
+
+ @Override
+ public Subject getCurrentSubject() {
+ // First check JAAS context
+ Subject jaasSubject =
Subject.getSubject(AccessController.getContext());
+ if (jaasSubject != null) {
+ return jaasSubject;
+ }
+
+ // Then check privileged subject
+ Subject privSubject = privilegedSubject.get();
+ if (privSubject != null) {
+ return privSubject;
+ }
+
+ // Finally return current request subject
+ return currentSubject.get();
+ }
+
+ @Override
+ public Principal getCurrentPrincipal() {
+ Subject subject = getCurrentSubject();
+ return subject != null ? getFirstPrincipal(subject) : null;
+ }
+
+ @Override
+ public void setCurrentSubject(Subject subject) {
+ currentSubject.set(subject);
+ }
+
+ @Override
+ public void clearCurrentSubject() {
+ currentSubject.remove();
+ privilegedSubject.remove();
+ }
+
+ /**
+ * Sets a temporary privileged subject for operations that require
elevated permissions.
+ * This subject will be used in addition to the current subject for
permission checks.
+ *
+ * @param subject the privileged subject to set
+ */
+ public void setPrivilegedSubject(Subject subject) {
+ privilegedSubject.set(subject);
+ }
+
+ /**
+ * Clears the temporary privileged subject.
+ */
+ public void clearPrivilegedSubject() {
+ privilegedSubject.remove();
+ }
+
+ @Override
+ public boolean hasRole(String role) {
+ // Check JAAS context first
+ Subject jaasSubject =
Subject.getSubject(AccessController.getContext());
+ if (jaasSubject != null && hasRoleInSubject(jaasSubject, role)) {
+ return true;
+ }
+
+ // Then check privileged subject
+ Subject privileged = privilegedSubject.get();
+ if (privileged != null && hasRoleInSubject(privileged, role)) {
+ return true;
+ }
+
+ // Finally check current subject
+ Subject current = currentSubject.get();
+ return current != null && hasRoleInSubject(current, role);
+ }
+
+ @Override
+ public boolean isAdmin() {
+ return hasRole(UnomiRoles.ADMINISTRATOR);
+ }
+
+ @Override
+ public boolean hasSystemAccess() {
+ return hasRole(UnomiRoles.ADMINISTRATOR) ||
hasRole(UnomiRoles.TENANT_ADMINISTRATOR);
+ }
+
+ @Override
+ public boolean hasTenantAccess(String tenantId) {
+ if (hasRole(UnomiRoles.TENANT_ADMINISTRATOR)) {
Review Comment:
**[C1 CRITICAL] Tenant isolation bypass — `tenantId` parameter is ignored**
Every API-key-authenticated tenant receives `TENANT_ADMINISTRATOR` (see
`createSubject()` line 323). This check then returns `true` for **any**
`tenantId` when that role is held — Tenant A’s private key grants full
read/write access to Tenant B’s data.
Fix:
```java
@Override
public boolean hasTenantAccess(String tenantId) {
if (hasRole(UnomiRoles.ADMINISTRATOR)) {
return true; // system admin bypasses tenant boundary
}
String subjectTenantId = getCurrentSubjectTenantId();
return tenantId != null && tenantId.equals(subjectTenantId);
}
```
##########
tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-10-tenantInitialization.groovy:
##########
@@ -0,0 +1,88 @@
+import org.apache.unomi.shell.migration.service.MigrationContext
+import org.apache.unomi.shell.migration.utils.MigrationUtils
+import java.time.ZonedDateTime
+import java.time.format.DateTimeFormatter
+import static org.apache.unomi.shell.migration.service.MigrationConfig.*
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+MigrationContext context = migrationContext
+String esAddress = context.getConfigString("esAddress")
+String indexPrefix = context.getConfigString("indexPrefix")
+String tenantId = context.getConfigString(TENANT_ID)
+ZonedDateTime unifiedDate = ZonedDateTime.now()
+String isoDate = unifiedDate.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME)
+
+// Create the default tenant index and items
+context.performMigrationStep("3.1.0-create-tenant-index", () -> {
+ String baseSettings = MigrationUtils.resourceAsString(bundleContext,
"requestBody/2.0.0/base_index_mapping.json")
+ String mapping = MigrationUtils.extractMappingFromBundles(bundleContext,
"tenant.json")
+ String newIndexSettings =
MigrationUtils.buildIndexCreationRequest(baseSettings, mapping, context, false)
+
+ if (!MigrationUtils.indexExists(context.getHttpClient(), esAddress,
"${indexPrefix}-tenant")) {
+ context.printMessage("Creating tenant index: ${indexPrefix}-tenant")
+ MigrationUtils.createIndex(context.getHttpClient(), esAddress,
"${indexPrefix}-tenant", newIndexSettings)
+
+ // Create the default tenant (this might be adjusted based on actual
tenant structure)
+ String defaultTenantJson = """{
+ "itemId": "${tenantId}",
+ "itemType": "tenant",
+ "name": "Default Tenant",
+ "tenantId": "system",
+ "description": "Default tenant created during migration to Unomi
V3",
+ "createdBy": "system-migration-3.1.0",
+ "lastModifiedBy": "system-migration-3.1.0",
+ "creationDate": "${isoDate}",
+ "lastModificationDate": "${isoDate}",
+ "version": 1,
+ "status": "ACTIVE",
+ "apiKeys" : [
+ {
+ "itemId" : "5a3f11a8-38a7-41b0-9fe8-d1ef0b4ad8ca",
+ "itemType" : "apiKey",
+ "createdBy": "system-migration-3.1.0",
+ "lastModifiedBy": "system-migration-3.1.0",
+ "creationDate" : "${isoDate}",
+ "lastModificationDate" : "${isoDate}",
+ "key" :
"C606D77D1D219509637A82C062BCD17F13D6DF1501702DC396D4A12D63D4E5F2",
+ "keyType" : "PUBLIC",
+ "revoked" : false
+ },
+ {
+ "itemId" : "3c595ea8-000e-4d0b-a329-0d259cc4d176",
+ "itemType" : "apiKey",
+ "createdBy": "system-migration-3.1.0",
+ "lastModifiedBy": "system-migration-3.1.0",
+ "creationDate" : "${isoDate}",
+ "lastModificationDate" : "${isoDate}",
+ "key" :
"503BAABB3A14AEB4B50ACF3C82982FBABECDBAEA83879CA8AECA016A6A9EEA85",
Review Comment:
**[C3 CRITICAL] Private API key credential hardcoded in source — committed
to git history permanently**
Both the public key (`C606D77D…` line 62) and this private key (`503BAABB…`)
will bootstrap the system tenant on every migration run. Anyone with repository
access has working credentials. Private keys must never be committed to source.
Replace both key values with keys generated at migration time:
```groovy
import java.security.SecureRandom
SecureRandom rng = SecureRandom.getInstanceStrong()
byte[] pubBytes = new byte[32]; rng.nextBytes(pubBytes)
byte[] privBytes = new byte[32]; rng.nextBytes(privBytes)
String generatedPublicKey = pubBytes.collect { String.format('%02X', it)
}.join()
String generatedPrivateKey = privBytes.collect { String.format('%02X', it)
}.join()
// use generatedPublicKey / generatedPrivateKey in the JSON body
```
The fixed `itemId` UUIDs on lines 56 and 67 compound the issue — an attacker
who knows the key values also knows the exact Elasticsearch document IDs.
Consider generating those at runtime too.
##########
services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.services.common.security;
+
+import org.apache.karaf.jaas.boot.principal.RolePrincipal;
+import org.apache.unomi.api.ExecutionContext;
+import org.apache.unomi.api.security.SecurityService;
+import org.apache.unomi.api.security.TenantPrincipal;
+import org.apache.unomi.api.security.UnomiRoles;
+import org.apache.unomi.api.services.ExecutionContextManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.security.auth.Subject;
+import java.security.AccessController;
+import java.security.Principal;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Supplier;
+
+public class ExecutionContextManagerImpl implements ExecutionContextManager {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ExecutionContextManagerImpl.class);
+
+ private final ThreadLocal<ExecutionContext> currentContext = new
ThreadLocal<>();
+ private SecurityService securityService;
+
+ public void setSecurityService(SecurityService securityService) {
+ this.securityService = securityService;
+ }
+
+ @Override
+ public ExecutionContext getCurrentContext() {
+ ExecutionContext context = currentContext.get();
+ if (context == null) {
+ context = createContext(securityService.getCurrentSubject());
+ currentContext.set(context);
+ }
+ return context;
+ }
+
+ @Override
+ public void setCurrentContext(ExecutionContext context) {
+ if (context == null) {
+ currentContext.remove();
+ } else {
+ currentContext.set(context);
+ }
+ }
+
+ @Override
+ public <T> T executeAsSystem(Supplier<T> operation) {
+ ExecutionContext previousContext = currentContext.get();
+ Subject previousSubject = securityService.getCurrentSubject();
+ try {
+ if (operation == null) {
+ throw new IllegalArgumentException("System operation cannot be
null");
+ }
+
+ Subject systemSubject = securityService.getSystemSubject();
+ if (systemSubject == null) {
+ throw new SecurityException("Failed to obtain system subject");
+ }
+
+ securityService.setCurrentSubject(systemSubject);
+ Set<String> roles =
securityService.extractRolesFromSubject(systemSubject);
+ if (!roles.contains(UnomiRoles.ADMINISTRATOR)) {
+ throw new SecurityException("System subject does not have
required administrator role");
+ }
+
+ Set<String> permissions = getPermissionsForRoles(roles);
+ ExecutionContext systemContext = new ExecutionContext(
+ ExecutionContext.SYSTEM_TENANT,
+ roles,
+ permissions
+ );
+ currentContext.set(systemContext);
+
+ try {
+ return operation.get();
+ } catch (Exception e) {
+ LOGGER.error("Error executing system operation: {}",
e.getMessage(), e);
+ throw e;
+ }
+ } finally {
+ try {
+ if (previousContext != null) {
+ currentContext.set(previousContext);
+ } else {
+ currentContext.remove();
+ }
+ securityService.setCurrentSubject(previousSubject);
+ } catch (Exception e) {
+ LOGGER.error("Error restoring previous context: {}",
e.getMessage(), e);
+ // Still throw the error to ensure it's not silently ignored
+ throw new SecurityException("Failed to restore security
context", e);
Review Comment:
**[H1 HIGH] Original operation exception suppressed when context restore
also fails**
If `operation.get()` throws `E1` and `setCurrentSubject()` then throws `E2`
in the `finally` catch, this `throw new SecurityException(…)` replaces `E1`.
The caller receives a generic restore-failure message and the actual root cause
is silently lost (not even added via `Throwable.addSuppressed()`).
Remove the re-throw — the error is already logged, and rethrowing here
swallows `E1` every time both the operation and the restore fail together. Also
update the stale comment on line 109.
```suggestion
// Context restore failed — already logged above. Do not
rethrow to avoid suppressing the original operation exception.
```
##########
tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-00-tenantDocumentIds.groovy:
##########
@@ -0,0 +1,179 @@
+import org.apache.unomi.shell.migration.service.MigrationContext
+import org.apache.unomi.shell.migration.utils.MigrationUtils
+import org.apache.unomi.shell.migration.utils.HttpUtils
+import java.time.ZonedDateTime
+import java.time.format.DateTimeFormatter
+import static org.apache.unomi.shell.migration.service.MigrationConfig.*
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+MigrationContext context = migrationContext
+String esAddress = context.getConfigString(CONFIG_ES_ADDRESS)
+String indexPrefix = context.getConfigString(INDEX_PREFIX)
+String tenantId = context.getConfigString(TENANT_ID)
+String systemTenantId = "system" // System tenant ID for system-level items
+String rolloverPolicyName = indexPrefix + "-unomi-rollover-policy"
+String rolloverSessionAlias = indexPrefix + "-session"
+String rolloverEventAlias = indexPrefix + "-event"
+ZonedDateTime unifiedDate = ZonedDateTime.now()
+String isoDate = unifiedDate.format(DateTimeFormatter.ISO_OFFSET_DATE_TIME)
+
+// Define index-specific configurations
+def indexConfigs = [
+ "profile": [
+ baseSettings: "requestBody/2.0.0/base_index_mapping.json",
+ mapping: "profile.json",
+ useRollover: false
+ ],
+ "session": [
+ baseSettings:
"requestBody/2.2.0/base_index_withRollover_request.json",
+ mapping: "session.json",
+ useRollover: true,
+ alias: { indexPrefix + "-session" }
+ ],
+ "event": [
+ baseSettings:
"requestBody/2.2.0/base_index_withRollover_request.json",
+ mapping: "event.json",
+ useRollover: true,
+ alias: { indexPrefix + "-event" }
+ ],
+ "systemitems": [
+ baseSettings: "requestBody/2.0.0/base_index_mapping.json",
+ mapping: "systemItems.json",
+ useRollover: false
+ ],
+ "geonameentry": [
+ baseSettings: "requestBody/2.0.0/base_index_mapping.json",
+ mapping: "geonameEntry.json",
+ useRollover: false
+ ],
+ "personasession": [
+ baseSettings: "requestBody/2.0.0/base_index_mapping.json",
+ mapping: "personaSession.json",
+ useRollover: false
+ ],
+ "generic": [
+ baseSettings: "requestBody/2.0.0/base_index_mapping.json",
+ mapping: null, // Will be determined dynamically from resolved
item type
+ useRollover: false
+ ]
+]
+
+// Helper function to resolve item type from index name
+def resolveItemType = { String indexName ->
+ def type = indexConfigs.find { type, config ->
+ indexName.startsWith("${indexPrefix}-${type}")
+ }
+ return type ? type.key : "generic"
+}
+
+// Helper function to get index configuration
+def getIndexConfig = { String itemType ->
+ return indexConfigs[itemType] ?: indexConfigs["generic"]
+}
+
+// Verify environment is ready for migration
+context.performMigrationStep("3.1.0-environment-check", () -> {
+ String elasticMajorVersion =
MigrationUtils.getElasticMajorVersion(context.getHttpClient(), esAddress)
+ context.printMessage("ElasticSearch major version: " + elasticMajorVersion)
+})
+
+// Get list of all index names and system items
+context.performMigrationStep("3.1.0-get-all-indices", () -> {
+ Set<String> allIndices =
MigrationUtils.getIndexesPrefixedBy(context.getHttpClient(), esAddress,
indexPrefix)
+ context.printMessage("Found " + allIndices.size() + " indices with prefix
" + indexPrefix)
+
+ Set<String> allItemTypes =
MigrationUtils.getAllItemTypes(context.getHttpClient(), esAddress, indexPrefix,
"*", bundleContext)
+ context.printMessage("Found " + allItemTypes.size() + " item types")
+
+ // Get all system items from the systemitems index
+ Set<String> systemItems =
MigrationUtils.getAllItemTypes(context.getHttpClient(), esAddress, indexPrefix,
"systemitems", bundleContext)
+ context.printMessage("Found " + systemItems.size() + " system items")
+
+ // Create base parameters
+ Map<String, Object> baseParams = new HashMap<>()
+ baseParams.put("date", isoDate)
+ baseParams.put("tenantId", tenantId)
+ baseParams.put("systemTenantId", systemTenantId)
+ baseParams.put("systemItems", systemItems)
+
+ context.printMessage("Using tenant ID: " + tenantId)
+
+ // Get the Painless script
+ String updateScript = MigrationUtils.getFileWithoutComments(bundleContext,
"requestBody/3.1.0/initialize_tenant_and_audit_fields.painless")
+
+ // Process each index (reindex them)
+ allIndices.each { indexName ->
+ context.printMessage("Processing index: " + indexName)
+
+ // Determine item type and get configuration
+ String itemType = resolveItemType(indexName)
+ def indexConfig = getIndexConfig(itemType)
+
+ // Add item type to parameters
+ Map<String, Object> params = new HashMap<>(baseParams)
+ params.put("itemType", itemType)
+
+ // Get base settings and mapping
+ String baseSettings = MigrationUtils.resourceAsString(bundleContext,
indexConfig.baseSettings)
+ String mapping = indexConfig.mapping ?
+ MigrationUtils.extractMappingFromBundles(bundleContext,
indexConfig.mapping) :
+ MigrationUtils.extractMappingFromBundles(bundleContext,
"${itemType}.json")
+
+ // Build index settings
+ String newIndexSettings
+ if (indexConfig.useRollover) {
+ newIndexSettings =
MigrationUtils.buildIndexCreationRequestWithRollover(baseSettings, mapping,
context, rolloverPolicyName, indexConfig.alias(indexPrefix))
+ } else {
+ newIndexSettings =
MigrationUtils.buildIndexCreationRequest(baseSettings, mapping, context, false)
+ }
+
+ // Execute reindex
+ MigrationUtils.reIndex(context.getHttpClient(), bundleContext,
esAddress, indexName, newIndexSettings, updateScript, params, context,
"3.1.0-${itemType}-update")
Review Comment:
**[H2 HIGH] Non-idempotent step key — multiple rollover indices of the same
type silently skipped**
For rollover indices (`session`, `event`) there are typically several
physical backing indices that all resolve to the same `itemType`.
`performMigrationStep` marks a step `COMPLETED` after the first index of that
type is processed; every subsequent same-type index is then silently skipped on
the current run and on any retry. Data in those indices is never migrated.
Use the unique physical `indexName` instead of `itemType` as the step key:
```suggestion
MigrationUtils.reIndex(context.getHttpClient(), bundleContext,
esAddress, indexName, newIndexSettings, updateScript, params, context,
"3.1.0-${indexName}-update")
```
##########
api/src/main/java/org/apache/unomi/api/security/SecurityService.java:
##########
@@ -0,0 +1,234 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.api.security;
+
+import javax.security.auth.Subject;
+import java.security.Principal;
+import java.util.Set;
+
+/**
+ * A service to manage security-related operations in Apache Unomi.
+ * This service provides comprehensive security management including:
+ * - Subject management (authentication and authorization)
+ * - Role-based access control (RBAC)
+ * - Tenant isolation and access control
+ * - Operation validation
+ * - System and privileged operations
+ * - Encryption key management
+ */
+public interface SecurityService {
+ /** The system tenant identifier used for system-wide operations */
+ String SYSTEM_TENANT = "SYSTEM_TENANT";
Review Comment:
**[I1 IMPORTANT] `SecurityService.SYSTEM_TENANT` constant has the wrong
value**
Every other definition in the codebase uses `"system"`:
- `ExecutionContext.SYSTEM_TENANT = "system"`
- `TenantService.SYSTEM_TENANT = "system"`
- `KarafSecurityService.SYSTEM_TENANT = "system"`
Any caller who writes `tenantId.equals(SecurityService.SYSTEM_TENANT)` will
silently **never match** the system tenant, defeating whatever isolation guard
they intended.
```suggestion
String SYSTEM_TENANT = "system";
```
##########
tools/shell-commands/src/main/resources/META-INF/cxs/migration/migrate-3.1.0-00-tenantDocumentIds.groovy:
##########
@@ -0,0 +1,179 @@
+import org.apache.unomi.shell.migration.service.MigrationContext
Review Comment:
**[I3 IMPORTANT] Filename conflict —
`migrate-3.1.0-00-fixProfileNbOfVisits.groovy` already exists on `master`**
`migrate-3.1.0-00-fixProfileNbOfVisits.groovy` was added by UNOMI-922
(commit `c0f145bb0`) and is already in `master`. Both scripts share step
priority `0`. They both execute (the `TreeSet` orders them alphabetically by
name when priority is equal, so `fixProfileNbOfVisits` runs first), but:
1. `migrate-3.1.0-05-fixSystemItemIds.groovy` refers to _“the 3.1.0-00
migration script”_ in the singular — now ambiguous.
2. Future operators and developers cannot tell which `00` was intentional.
Please rename this file to `migrate-3.1.0-01-tenantDocumentIds.groovy` (or
another unambiguous number that makes the ordering relative to
`fixSystemItemIds` explicit).
##########
services/src/main/java/org/apache/unomi/services/impl/scheduler/SchedulerServiceImpl.java:
##########
@@ -17,57 +17,2015 @@
package org.apache.unomi.services.impl.scheduler;
+import org.apache.unomi.api.PartialList;
import org.apache.unomi.api.services.SchedulerService;
+import org.apache.unomi.api.tasks.ScheduledTask;
+import org.apache.unomi.api.tasks.ScheduledTask.TaskStatus;
+import org.apache.unomi.api.tasks.TaskExecutor;
+import org.osgi.framework.BundleContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.time.Duration;
import java.time.ZonedDateTime;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledExecutorService;
+import java.util.*;
+import java.util.concurrent.*;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
/**
+ * Implementation of the SchedulerService that provides task scheduling and
execution capabilities.
+ * This implementation supports:
+ * - Persistent and in-memory tasks
+ * - Single-node and cluster execution
+ * - Task dependencies and waiting queues
+ * - Lock management and crash recovery
+ * - Execution history and metrics tracking
+ * - Pending operations queue for initialization
+ *
+ * Task Lifecycle:
+ * 1. SCHEDULED: Initial state, task is ready to execute
+ * 2. WAITING: Task is waiting for dependencies or lock
+ * 3. RUNNING: Task is currently executing
+ * 4. COMPLETED/FAILED/CANCELLED/CRASHED: Terminal states
+ *
+ * Lock Management:
+ * - Tasks can be configured to allow/disallow parallel execution
+ * - Locks are managed differently for persistent and in-memory tasks
+ * - Lock timeout mechanism prevents deadlocks
+ *
+ * Clustering Support:
+ * - Tasks can be configured to run on specific nodes or all nodes
+ * - Lock ownership prevents duplicate execution
+ * - Crash recovery handles node failures
+ *
+ * Pending Operations:
+ * - Operations that require subservices are queued during initialization
+ * - Operations are executed once all required services are available
+ * - Supports different operation types with appropriate handling
+ *
* @author dgaillard
Review Comment:
**[A1 Advisory] `@author` tag — violates ASF attribution policy**
The Apache Software Foundation asks projects not to use `@author` Javadoc
tags; author attribution belongs in `git log`.
```suggestion
*
```
##########
services-common/src/main/java/org/apache/unomi/services/common/security/KarafSecurityService.java:
##########
@@ -0,0 +1,333 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.services.common.security;
+
+import org.apache.karaf.jaas.boot.principal.RolePrincipal;
+import org.apache.karaf.jaas.boot.principal.UserPrincipal;
+import org.apache.unomi.api.security.*;
+import org.apache.unomi.api.tenants.AuditService;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.security.auth.Subject;
+import java.security.AccessController;
+import java.security.Principal;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+public class KarafSecurityService implements SecurityService {
+ private static final Logger LOGGER =
LoggerFactory.getLogger(KarafSecurityService.class);
+
+ public static final String SYSTEM_TENANT = "system";
+ private final Subject SYSTEM_SUBJECT;
+
+ private SecurityServiceConfiguration configuration;
+ private EncryptionService encryptionService;
+ private AuditService tenantAuditService;
+
+ private final ThreadLocal<Subject> currentSubject = new ThreadLocal<>();
+ private final ThreadLocal<Subject> privilegedSubject = new ThreadLocal<>();
+
+ public KarafSecurityService() {
+ SYSTEM_SUBJECT = createSystemSubject();
+ }
+
+ private Subject createSystemSubject() {
+ Subject subject = new Subject();
+ subject.getPrincipals().add(new UserPrincipal("system"));
+ subject.getPrincipals().add(new
RolePrincipal(UnomiRoles.ADMINISTRATOR));
+ subject.getPrincipals().add(new
RolePrincipal(UnomiRoles.TENANT_ADMINISTRATOR));
+ subject.getPrincipals().add(new
RolePrincipal(UnomiRoles.SYSTEM_MAINTENANCE));
+ return subject;
+ }
+
+ public void init() {
+ if (configuration == null) {
+ configuration = new SecurityServiceConfiguration();
+ }
+ updateSystemSubject();
+ }
+
+ public void destroy() {
+ // Cleanup
+ }
+
+ private void updateSystemSubject() {
+ SYSTEM_SUBJECT.getPrincipals().clear();
+ SYSTEM_SUBJECT.getPrincipals().add(new TenantPrincipal(SYSTEM_TENANT));
+ SYSTEM_SUBJECT.getPrincipals().add(new UserPrincipal("system"));
+ for (String role : configuration.getSystemRoles()) {
+ SYSTEM_SUBJECT.getPrincipals().add(new RolePrincipal(role));
+ }
+ }
+
+ public void setTenantAuditService(AuditService tenantAuditService) {
+ this.tenantAuditService = tenantAuditService;
+ }
+
+ public void setConfiguration(SecurityServiceConfiguration configuration) {
+ this.configuration = configuration;
+ }
+
+ public void bindEncryptionService(EncryptionService encryptionService) {
+ this.encryptionService = encryptionService;
+ }
+
+ public void unbindEncryptionService(EncryptionService encryptionService) {
+ this.encryptionService = null;
+ }
+
+ @Override
+ public Subject getCurrentSubject() {
+ // First check JAAS context
+ Subject jaasSubject =
Subject.getSubject(AccessController.getContext());
+ if (jaasSubject != null) {
+ return jaasSubject;
+ }
+
+ // Then check privileged subject
+ Subject privSubject = privilegedSubject.get();
+ if (privSubject != null) {
+ return privSubject;
+ }
+
+ // Finally return current request subject
+ return currentSubject.get();
+ }
+
+ @Override
+ public Principal getCurrentPrincipal() {
+ Subject subject = getCurrentSubject();
+ return subject != null ? getFirstPrincipal(subject) : null;
+ }
+
+ @Override
+ public void setCurrentSubject(Subject subject) {
+ currentSubject.set(subject);
+ }
+
+ @Override
+ public void clearCurrentSubject() {
+ currentSubject.remove();
+ privilegedSubject.remove();
+ }
+
+ /**
+ * Sets a temporary privileged subject for operations that require
elevated permissions.
+ * This subject will be used in addition to the current subject for
permission checks.
+ *
+ * @param subject the privileged subject to set
+ */
+ public void setPrivilegedSubject(Subject subject) {
+ privilegedSubject.set(subject);
+ }
+
+ /**
+ * Clears the temporary privileged subject.
+ */
+ public void clearPrivilegedSubject() {
+ privilegedSubject.remove();
+ }
+
+ @Override
+ public boolean hasRole(String role) {
+ // Check JAAS context first
+ Subject jaasSubject =
Subject.getSubject(AccessController.getContext());
+ if (jaasSubject != null && hasRoleInSubject(jaasSubject, role)) {
+ return true;
+ }
+
+ // Then check privileged subject
+ Subject privileged = privilegedSubject.get();
+ if (privileged != null && hasRoleInSubject(privileged, role)) {
+ return true;
+ }
+
+ // Finally check current subject
+ Subject current = currentSubject.get();
+ return current != null && hasRoleInSubject(current, role);
+ }
+
+ @Override
+ public boolean isAdmin() {
+ return hasRole(UnomiRoles.ADMINISTRATOR);
+ }
+
+ @Override
+ public boolean hasSystemAccess() {
+ return hasRole(UnomiRoles.ADMINISTRATOR) ||
hasRole(UnomiRoles.TENANT_ADMINISTRATOR);
+ }
+
+ @Override
+ public boolean hasTenantAccess(String tenantId) {
+ if (hasRole(UnomiRoles.TENANT_ADMINISTRATOR)) {
+ return true;
+ }
+ return hasSystemAccess();
+ }
+
+ @Override
+ public boolean hasPermission(String permission) {
+ // First check JAAS context
+ Subject jaasSubject =
Subject.getSubject(AccessController.getContext());
+ if (jaasSubject != null && hasPermissionInSubject(jaasSubject,
permission)) {
+ return true;
+ }
+
+ // Then check privileged subject
+ Subject privSubject = privilegedSubject.get();
+ if (privSubject != null && hasPermissionInSubject(privSubject,
permission)) {
+ return true;
+ }
+
+ // Finally check current subject
+ Subject subject = currentSubject.get();
+ return subject != null && hasPermissionInSubject(subject, permission);
+ }
+
+ private boolean hasRoleInSubject(Subject subject, String role) {
+ return subject.getPrincipals(RolePrincipal.class).stream()
+ .anyMatch(p -> p.getName().equals(role));
+ }
+
+ private boolean hasPermissionInSubject(Subject subject, String permission)
{
+ Set<String> roles = extractRolesFromSubject(subject);
+ String[] requiredRoles =
configuration.getRequiredRolesForPermission(permission);
+
+ return requiredRoles != null &&
+ roles.stream().anyMatch(role ->
Arrays.asList(requiredRoles).contains(role));
+ }
+
+ @Override
+ public void auditTenantOperation(String tenantId, String operation) {
+ tenantAuditService.logTenantOperation(tenantId, operation);
+ }
+
+ private Principal getFirstPrincipal(Subject subject) {
+ if (subject == null) {
+ return null;
+ }
+ Set<Principal> principals = subject.getPrincipals();
+ if (principals == null || principals.isEmpty()) {
+ return null;
+ }
+ return principals.iterator().next();
+ }
+
+ @Override
+ public void executeWithPrivilegedSubject(Subject subject, Runnable
operation) {
+ Subject oldPrivileged = privilegedSubject.get();
+ try {
+ privilegedSubject.set(subject);
+ operation.run();
+ } finally {
+ if (oldPrivileged != null) {
+ privilegedSubject.set(oldPrivileged);
+ } else {
+ privilegedSubject.remove();
+ }
+ }
+ }
+
+ @Override
+ public String getCurrentSubjectTenantId() {
+ Subject subject = getCurrentSubject();
+ if (subject != null) {
+ Set<TenantPrincipal> tenantPrincipals =
subject.getPrincipals(TenantPrincipal.class);
+ if (!tenantPrincipals.isEmpty()) {
+ return tenantPrincipals.iterator().next().getTenantId();
+ }
+ }
+ return SYSTEM_TENANT;
+ }
+
+ @Override
+ public boolean isOperatingOnSystemTenant() {
+ return false;
Review Comment:
**[I6 IMPORTANT] `isOperatingOnSystemTenant()` is a non-functional stub —
always returns `false`**
This method is part of the `SecurityService` API contract. Any caller that
uses it to guard a system-only operation will silently never activate that
guard.
```suggestion
return getCurrentSubjectTenantId().equals(SYSTEM_TENANT);
```
##########
api/src/main/java/org/apache/unomi/api/tenants/ApiKey.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.api.tenants;
+
+import org.apache.unomi.api.Item;
+
+import java.util.Date;
+
+/**
+ * Represents an API key for tenant authentication and authorization.
+ * This class extends the base Item class and provides functionality for
managing
+ * API keys including their lifecycle (creation, expiration, revocation) and
metadata.
+ */
+public class ApiKey extends Item {
+ /**
+ * The item type for an API key.
+ */
+ public static final String ITEM_TYPE = "apiKey";
+
+ /**
+ * Enum defining the types of API keys.
+ */
+ public enum ApiKeyType {
+ /**
+ * Public API key for context.json, event collector and other
public-facing endpoints
+ */
+ PUBLIC,
+
+ /**
+ * Private API key for protected endpoints including login and
updateProperties
+ */
+ PRIVATE
+ }
+
+ /**
+ * The API key value.
+ */
+ private String key;
Review Comment:
**[C4 CRITICAL] Raw API key value serialised to Elasticsearch and exposed by
the REST layer**
This field has no `@JsonIgnore`. Since `Tenant extends Item` and is
persisted to Elasticsearch, the entire `List<ApiKey>` — including raw `key`
values — is stored in plaintext and returned by `GET /tenants/{id}` to any
caller with the `ADMINISTRATOR` role. The `@XmlTransient` annotations on the
derived getters suppress JAXB only, not Jackson.
Private keys are authentication credentials and should be stored as a secure
hash, returned to the caller only once at creation time.
Minimal immediate fix (also add `import
com.fasterxml.jackson.annotation.JsonIgnore;` to the class imports):
```suggestion
@com.fasterxml.jackson.annotation.JsonIgnore
private String key;
```
##########
api/src/main/java/org/apache/unomi/api/tasks/ScheduledTask.java:
##########
@@ -0,0 +1,873 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.api.tasks;
+
+import org.apache.unomi.api.Item;
+
+import java.io.Serializable;
+import java.util.Date;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import java.util.HashSet;
+
+/**
+ * Represents a persistent scheduled task that can be executed across a
cluster.
+ * This class provides a comprehensive model for task scheduling and execution
with features including:
+ * <ul>
+ * <li>Task lifecycle management through states (SCHEDULED, WAITING,
RUNNING, etc.)</li>
+ * <li>Lock management for cluster coordination</li>
+ * <li>Execution history and checkpoint data for recovery</li>
+ * <li>Support for one-shot and periodic execution</li>
+ * <li>Task dependencies and parallel execution control</li>
+ * <li>Cluster-wide task distribution</li>
+ * </ul>
+ */
+public class ScheduledTask extends Item implements Serializable {
+
+ public static final String ITEM_TYPE = "scheduledTask";
Review Comment:
**[I4 IMPORTANT] `Serializable` class without `serialVersionUID` —
rolling-upgrade hazard**
`ScheduledTask` implements `Serializable` and has ~25 fields (`Map`, `Set`,
`TimeUnit`, `Date`, inner `TaskStatus` enum). Without a declared
`serialVersionUID` the JVM computes one from the class structure. Adding or
removing any field will invalidate persisted serialised blobs in a rolling
cluster upgrade and throw `InvalidClassException`.
```suggestion
private static final long serialVersionUID = 1L;
public static final String ITEM_TYPE = "scheduledTask";
```
##########
graphql/cxs-impl/src/main/java/org/apache/unomi/graphql/servlet/auth/GraphQLServletSecurityValidator.java:
##########
@@ -74,6 +96,10 @@ public boolean validate(String query, String operationName,
HttpServletRequest r
}
private boolean isPublicOperation(String query) {
+ if (query == null) {
+ return false;
+ }
+
final Document queryDoc = parser.parseDocument(query);
final Definition<?> def = queryDoc.getDefinitions().get(0);
Review Comment:
**[I2 IMPORTANT] `IndexOutOfBoundsException` on empty or fragment-only
GraphQL documents**
`getDefinitions().get(0)` throws if the parsed document has no operation
definitions (e.g. a fragment-only document, or a body that parses without error
but contains no operations). Because `isPublicOperation` runs before the
`Authorization` check, this is reachable unauthenticated and returns a 500
instead of a clean 400.
```suggestion
if (queryDoc.getDefinitions().isEmpty()) {
return false;
}
final Definition<?> def = queryDoc.getDefinitions().get(0);
```
##########
services-common/src/main/java/org/apache/unomi/services/common/security/ExecutionContextManagerImpl.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.unomi.services.common.security;
+
+import org.apache.karaf.jaas.boot.principal.RolePrincipal;
+import org.apache.unomi.api.ExecutionContext;
+import org.apache.unomi.api.security.SecurityService;
+import org.apache.unomi.api.security.TenantPrincipal;
+import org.apache.unomi.api.security.UnomiRoles;
+import org.apache.unomi.api.services.ExecutionContextManager;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.security.auth.Subject;
+import java.security.AccessController;
+import java.security.Principal;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.function.Supplier;
+
+public class ExecutionContextManagerImpl implements ExecutionContextManager {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(ExecutionContextManagerImpl.class);
+
+ private final ThreadLocal<ExecutionContext> currentContext = new
ThreadLocal<>();
+ private SecurityService securityService;
+
+ public void setSecurityService(SecurityService securityService) {
+ this.securityService = securityService;
+ }
+
+ @Override
+ public ExecutionContext getCurrentContext() {
+ ExecutionContext context = currentContext.get();
+ if (context == null) {
+ context = createContext(securityService.getCurrentSubject());
+ currentContext.set(context);
+ }
+ return context;
+ }
+
+ @Override
+ public void setCurrentContext(ExecutionContext context) {
+ if (context == null) {
+ currentContext.remove();
+ } else {
+ currentContext.set(context);
+ }
+ }
+
+ @Override
+ public <T> T executeAsSystem(Supplier<T> operation) {
+ ExecutionContext previousContext = currentContext.get();
+ Subject previousSubject = securityService.getCurrentSubject();
+ try {
+ if (operation == null) {
+ throw new IllegalArgumentException("System operation cannot be
null");
+ }
+
+ Subject systemSubject = securityService.getSystemSubject();
+ if (systemSubject == null) {
+ throw new SecurityException("Failed to obtain system subject");
+ }
+
+ securityService.setCurrentSubject(systemSubject);
+ Set<String> roles =
securityService.extractRolesFromSubject(systemSubject);
+ if (!roles.contains(UnomiRoles.ADMINISTRATOR)) {
+ throw new SecurityException("System subject does not have
required administrator role");
+ }
+
+ Set<String> permissions = getPermissionsForRoles(roles);
+ ExecutionContext systemContext = new ExecutionContext(
+ ExecutionContext.SYSTEM_TENANT,
+ roles,
+ permissions
+ );
+ currentContext.set(systemContext);
+
+ try {
+ return operation.get();
+ } catch (Exception e) {
+ LOGGER.error("Error executing system operation: {}",
e.getMessage(), e);
+ throw e;
+ }
+ } finally {
+ try {
+ if (previousContext != null) {
+ currentContext.set(previousContext);
+ } else {
+ currentContext.remove();
+ }
+ securityService.setCurrentSubject(previousSubject);
+ } catch (Exception e) {
+ LOGGER.error("Error restoring previous context: {}",
e.getMessage(), e);
+ // Still throw the error to ensure it's not silently ignored
+ throw new SecurityException("Failed to restore security
context", e);
+ }
+ }
+ }
+
+ @Override
+ public void executeAsSystem(Runnable operation) {
+ executeAsSystem(() -> {
+ operation.run();
+ return null;
+ });
+ }
+
+ @Override
+ public ExecutionContext createContext(String tenantId) {
+ Subject subject = securityService.getCurrentSubject();
+ Set<String> roles = securityService.extractRolesFromSubject(subject);
+ Set<String> permissions = getPermissionsForRoles(roles);
+ return new ExecutionContext(tenantId, roles, permissions);
+ }
+
+ @Override
+ public <T> T executeAsTenant(String tenantId, Supplier<T> operation) {
+ ExecutionContext previousContext = currentContext.get();
+ try {
+ ExecutionContext tenantContext = createContext(tenantId);
+ currentContext.set(tenantContext);
+ return operation.get();
+ } finally {
+ if (previousContext != null) {
+ currentContext.set(previousContext);
+ } else {
+ currentContext.remove();
+ }
+ }
+ }
+
+ @Override
+ public void executeAsTenant(String tenantId, Runnable operation) {
+ executeAsTenant(tenantId, () -> {
+ operation.run();
+ return null;
+ });
+ }
+
+ private Set<String> getCurrentRoles() {
Review Comment:
**[I5 IMPORTANT] Dead code calling `AccessController.getContext()` — removed
in Java 21**
`getCurrentRoles()` calls `AccessController.getContext()` which was
deprecated for removal in Java 17 and removed entirely in Java 21. A
project-wide search confirms this method has **zero call sites** — it is
unreachable dead code.
If ever invoked on a Java 21+ runtime it will throw `NoSuchMethodError`.
Please delete the entire method (lines 155–166). If the functionality is ever
needed it should be re-implemented using the `securityService` instance already
available in this class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
