From 2db058c632490197f9371d37214b327f4790cbe8 Mon Sep 17 00:00:00 2001 From: Ian Patterson Date: Wed, 11 Nov 2026 14:59:51 -0600 Subject: [PATCH 1/2] [Core] Fix BasicSecurityRealm state save/load for users of other modules (#366) --- .../impl/security/BasicSecurityRealm.java | 69 +++++- .../impl/security/TestBasicSecurityRealm.java | 230 ++++++++++++++++++ 2 files changed, 290 insertions(+), 9 deletions(-) create mode 100644 sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java diff --git a/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java b/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java index 3f8c1b22a..83d3f9107 100644 --- a/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java +++ b/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java @@ -17,6 +17,7 @@ import java.io.IOException; import java.io.InputStreamReader; import java.io.OutputStreamWriter; +import java.io.StringWriter; import java.util.Collection; import java.util.Map; import java.util.concurrent.ConcurrentSkipListMap; @@ -52,41 +53,68 @@ class UserPermissionsStore extends ConcurrentSkipLis { public void addAllowPermissions(String id, Collection permissions) { - var config = (PermissionsConfig)get(id); + var config = getPermissionsConfig(id); + if (config == null) + return; config.allowList.addAll(permissions); saveState(); } public void addDenyPermissions(String id, Collection permissions) { - var config = (PermissionsConfig)get(id); + var config = getPermissionsConfig(id); + if (config == null) + return; config.denyList.addAll(permissions); saveState(); } public void removeAllowPermissions(String id, Collection permissions) { - var config = (PermissionsConfig)get(id); + var config = getPermissionsConfig(id); + if (config == null) + return; config.allowList.removeAll(permissions); saveState(); } public void removeDenyPermissions(String id, Collection permissions) { - var config = (PermissionsConfig)get(id); + var config = getPermissionsConfig(id); + if (config == null) + return; config.denyList.removeAll(permissions); saveState(); } + /* + * The registry is a map, so other modules can add entries of their own + * class. This realm only owns the permissions of its config entries. + */ + protected PermissionsConfig getPermissionsConfig(String id) + { + var entity = get(id); + if (entity instanceof PermissionsConfig) + return (PermissionsConfig)entity; + + if (entity == null) + getLogger().debug("Permission update skipped for '{}': no such entry", id); + else + getLogger().debug("Permission update skipped for '{}': the entry is managed by another module", id); + return null; + } + protected void saveState() { try { var stateManager = getParentHub().getModuleRegistry().getStateManager(getLocalID()); + if (stateManager == null) + return; var fileName = this == users ? USER_PERMISSIONS_FILENAME : ROLE_PERMISSIONS_FILENAME; savePermissionJsonFile(this, stateManager, fileName); } - catch (SensorHubException e) + catch (SensorHubException | RuntimeException e) { getLogger().error(e.getMessage(), e); } @@ -166,12 +194,18 @@ public void saveState(IModuleStateManager saver) throws SensorHubException protected void savePermissionJsonFile(Map permissions, IModuleStateManager saver, String fileName) throws SensorHubException { - try (var w = new JsonWriter(new OutputStreamWriter(saver.getOutputStream(fileName)))) + // build the whole document first so the file is never left truncated + var json = new StringWriter(); + try (var w = new JsonWriter(json)) { w.setIndent(" "); w.beginObject(); for (var e: permissions.entrySet()) { + // skip entries added by other modules + if (!(e.getValue() instanceof PermissionsConfig)) + continue; + var id = e.getKey(); var entity = (PermissionsConfig)e.getValue(); @@ -199,6 +233,15 @@ protected void savePermissionJsonFile(Map pe } w.endObject(); } + catch (IOException | RuntimeException e) + { + throw new SensorHubException("Error writing " + fileName, e); + } + + try (var w = new OutputStreamWriter(saver.getOutputStream(fileName))) + { + w.write(json.toString()); + } catch (IOException e) { throw new SensorHubException("Error writing " + fileName, e); @@ -229,9 +272,14 @@ protected void readPermissionJsonFile(Map pe while (r.hasNext()) { var id = r.nextName(); - var entity = (PermissionsConfig)permissions.get(id); - if (entity == null) + + // skip entries that are unknown or were added by other modules + if (!(permissions.get(id) instanceof PermissionsConfig)) + { + r.skipValue(); continue; + } + var entity = (PermissionsConfig)permissions.get(id); r.beginObject(); while (r.hasNext()) @@ -259,12 +307,15 @@ else if ("deny".equals(propName)) } r.endArray(); } + + else + r.skipValue(); } r.endObject(); } r.endObject(); } - catch (IOException e) + catch (IOException | RuntimeException e) { throw new SensorHubException("Error reading " + fileName, e); } diff --git a/sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java b/sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java new file mode 100644 index 000000000..0e1b30649 --- /dev/null +++ b/sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java @@ -0,0 +1,230 @@ +/***************************** BEGIN LICENSE BLOCK *************************** + +The contents of this file are subject to the Mozilla Public License, v. 2.0. +If a copy of the MPL was not distributed with this file, You can obtain one +at http://mozilla.org/MPL/2.0/. + +Software distributed under the License is distributed on an "AS IS" basis, +WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License +for the specific language governing rights and limitations under the License. + +******************************* END LICENSE BLOCK ***************************/ + +package org.sensorhub.impl.security; + +import static org.junit.Assert.*; +import java.io.File; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.sensorhub.api.common.SensorHubException; +import org.sensorhub.api.module.IModuleStateManager; +import org.sensorhub.api.security.IPermissionPath; +import org.sensorhub.api.security.IUserInfo; +import org.sensorhub.impl.SensorHub; +import org.sensorhub.impl.module.DefaultModuleStateManager; +import org.sensorhub.impl.security.BasicSecurityRealmConfig.RoleConfig; +import org.sensorhub.impl.security.BasicSecurityRealmConfig.UserConfig; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; + + +public class TestBasicSecurityRealm +{ + static final String RUNTIME_PERM = "*/get"; + static final String OTHER_USER = "bob"; + + @Rule + public TemporaryFolder tmp = new TemporaryFolder(); + + SensorHub hub; + IModuleStateManager state; + File userFile; + + + @Before + public void setup() throws Exception + { + hub = new SensorHub(); + hub.start(); + state = new DefaultModuleStateManager(tmp.getRoot().getAbsolutePath(), "realm"); + userFile = new File(state.getDataDirectory(), BasicSecurityRealm.USER_PERMISSIONS_FILENAME); + } + + + /* realm with config users 'admin' and 'user1' */ + private BasicSecurityRealm newRealm() throws Exception + { + var config = new BasicSecurityRealmConfig(); + config.id = "realm"; + + var role = new RoleConfig(); + role.roleID = "admin"; + role.allow.add("*"); + config.roles.add(role); + + for (var id: List.of("admin", "user1")) + { + var user = new UserConfig(); + user.userID = id; + user.password = "pwd"; + config.users.add(user); + } + + var realm = new BasicSecurityRealm(); + realm.setParentHub(hub); + realm.setConfiguration(config); + realm.doInit(); + return realm; + } + + + /* user of a class that is not owned by the realm */ + private IUserInfo otherUser(String id) + { + return new IUserInfo() { + public String getId() { return id; } + public String getName() { return id; } + public String getDescription() { return null; } + public String getPassword() { return "pwd"; } + public Collection getRoles() { return Collections.emptyList(); } + public Map getAttributes() { return Collections.emptyMap(); } + public Collection getAllowList() { return Collections.emptyList(); } + public Collection getDenyList() { return Collections.emptyList(); } + }; + } + + + private Collection runtimePerm() + { + return List.of(PermissionFactory.newPermissionSetting(hub.getModuleRegistry(), RUNTIME_PERM)); + } + + + private List allowStrings(BasicSecurityRealm realm, String userID) + { + return realm.users.get(userID).getAllowList().stream() + .map(PermissionFactory::getPermissionString) + .toList(); + } + + + private JsonObject readUserFile() throws Exception + { + return JsonParser.parseString(Files.readString(userFile.toPath())).getAsJsonObject(); + } + + + private void writeUserFile(String json) throws Exception + { + Files.writeString(userFile.toPath(), json, StandardCharsets.UTF_8); + } + + + @Test + public void testSaveSkipsUsersOfOtherModules() throws Exception + { + var realm = newRealm(); + realm.users.put(OTHER_USER, otherUser(OTHER_USER)); + + realm.saveState(state); + + var json = readUserFile(); + assertTrue(json.has("admin")); + assertTrue(json.has("user1")); + assertFalse(json.has(OTHER_USER)); + } + + + @Test + public void testSaveThenLoad() throws Exception + { + var realm = newRealm(); + realm.users.put(OTHER_USER, otherUser(OTHER_USER)); + ((UserConfig)realm.users.get("user1")).allowList.addAll(runtimePerm()); + realm.saveState(state); + + var realm2 = newRealm(); + assertTrue(allowStrings(realm2, "user1").isEmpty()); + realm2.loadState(state); + assertEquals(List.of(RUNTIME_PERM), allowStrings(realm2, "user1")); + assertTrue(allowStrings(realm2, "admin").isEmpty()); + } + + + @Test + public void testLoadSkipsUnknownAndOtherUsers() throws Exception + { + // 'aaa' is not in the registry, 'bob' is of another class; both come before 'user1' + writeUserFile("{" + + "\"aaa\": {\"allow\": [\"*/x\"], \"deny\": []}," + + "\"" + OTHER_USER + "\": {\"allow\": [\"*/x\"], \"deny\": []}," + + "\"user1\": {\"other\": 1, \"allow\": [\"" + RUNTIME_PERM + "\"], \"deny\": []}" + + "}"); + + var realm = newRealm(); + realm.users.put(OTHER_USER, otherUser(OTHER_USER)); + realm.loadState(state); + + assertEquals(List.of(RUNTIME_PERM), allowStrings(realm, "user1")); + } + + + @Test + public void testLoadTruncatedFile() throws Exception + { + var truncated = "{\n \"admin\": {\n \"allow\": [],\n \"deny\": []\n }\n"; + writeUserFile(truncated); + + var realm = newRealm(); + assertThrows(SensorHubException.class, () -> realm.loadState(state)); + + // file is kept and config users are intact + assertEquals(truncated, Files.readString(userFile.toPath())); + assertTrue(realm.users.containsKey("admin")); + assertTrue(realm.users.containsKey("user1")); + } + + + @Test + public void testLoadInvalidFile() throws Exception + { + writeUserFile("{\"user1\": [1, 2]}"); + var realm = newRealm(); + assertThrows(SensorHubException.class, () -> realm.loadState(state)); + } + + + @Test + public void testPermissionUpdateOnUserOfOtherModule() throws Exception + { + var realm = newRealm(); + realm.users.put(OTHER_USER, otherUser(OTHER_USER)); + + // no exception, no change + realm.users.addAllowPermissions(OTHER_USER, runtimePerm()); + realm.users.addDenyPermissions(OTHER_USER, runtimePerm()); + realm.users.removeAllowPermissions(OTHER_USER, runtimePerm()); + realm.users.removeDenyPermissions(OTHER_USER, runtimePerm()); + realm.users.addAllowPermissions("nobody", runtimePerm()); + assertTrue(realm.users.get(OTHER_USER).getAllowList().isEmpty()); + } + + + @Test + public void testPermissionUpdateOnConfigUser() throws Exception + { + var realm = newRealm(); + realm.users.put(OTHER_USER, otherUser(OTHER_USER)); + + realm.users.addAllowPermissions("user1", runtimePerm()); + assertEquals(List.of(RUNTIME_PERM), allowStrings(realm, "user1")); + } +} From 0e229e21e4db458020b40527bfb6419d89a6a6e9 Mon Sep 17 00:00:00 2001 From: Ian Patterson Date: Fri, 13 Nov 2026 14:27:53 -0600 Subject: [PATCH 2/2] [Core] Warn on permission update for users of other modules (#366) --- .../impl/security/BasicSecurityRealm.java | 8 +-- .../impl/security/TestBasicSecurityRealm.java | 52 ++++++++++++++++++- 2 files changed, 56 insertions(+), 4 deletions(-) diff --git a/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java b/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java index 83d3f9107..b179080f4 100644 --- a/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java +++ b/sensorhub-core/src/main/java/org/sensorhub/impl/security/BasicSecurityRealm.java @@ -89,7 +89,9 @@ public void removeDenyPermissions(String id, Collection permiss /* * The registry is a map, so other modules can add entries of their own - * class. This realm only owns the permissions of its config entries. + * class. This realm holds run-time permissions only for entries that are + * PermissionsConfig. A module that wants its users to hold run-time + * permissions must use a subclass of UserConfig. */ protected PermissionsConfig getPermissionsConfig(String id) { @@ -98,9 +100,9 @@ protected PermissionsConfig getPermissionsConfig(String id) return (PermissionsConfig)entity; if (entity == null) - getLogger().debug("Permission update skipped for '{}': no such entry", id); + getLogger().warn("Permissions of '{}' not changed: no such entry", id); else - getLogger().debug("Permission update skipped for '{}': the entry is managed by another module", id); + getLogger().warn("Permissions of '{}' not changed: the entry was added by another module ({})", id, entity.getClass().getName()); return null; } diff --git a/sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java b/sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java index 0e1b30649..15f4ad23d 100644 --- a/sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java +++ b/sensorhub-core/src/test/java/org/sensorhub/impl/security/TestBasicSecurityRealm.java @@ -29,6 +29,7 @@ import org.sensorhub.api.security.IPermissionPath; import org.sensorhub.api.security.IUserInfo; import org.sensorhub.impl.SensorHub; +import org.sensorhub.impl.SensorHubConfig; import org.sensorhub.impl.module.DefaultModuleStateManager; import org.sensorhub.impl.security.BasicSecurityRealmConfig.RoleConfig; import org.sensorhub.impl.security.BasicSecurityRealmConfig.UserConfig; @@ -52,7 +53,8 @@ public class TestBasicSecurityRealm @Before public void setup() throws Exception { - hub = new SensorHub(); + // module data path is set so that permission updates are written to file + hub = new SensorHub(new SensorHubConfig(null, tmp.getRoot().getAbsolutePath())); hub.start(); state = new DefaultModuleStateManager(tmp.getRoot().getAbsolutePath(), "realm"); userFile = new File(state.getDataDirectory(), BasicSecurityRealm.USER_PERMISSIONS_FILENAME); @@ -226,5 +228,53 @@ public void testPermissionUpdateOnConfigUser() throws Exception realm.users.addAllowPermissions("user1", runtimePerm()); assertEquals(List.of(RUNTIME_PERM), allowStrings(realm, "user1")); + + // the update is written to a complete file, without the other user + var json = readUserFile(); + assertEquals(RUNTIME_PERM, json.getAsJsonObject("user1").getAsJsonArray("allow").get(0).getAsString()); + assertTrue(json.has("admin")); + assertFalse(json.has(OTHER_USER)); + } + + + @Test + public void testPermissionUpdateOnUserConfigOfOtherModule() throws Exception + { + var realm = newRealm(); + var user = new UserConfig() {}; + user.userID = OTHER_USER; + realm.users.put(OTHER_USER, user); + + // a user class that extends UserConfig holds run-time permissions + realm.users.addAllowPermissions(OTHER_USER, runtimePerm()); + assertEquals(List.of(RUNTIME_PERM), allowStrings(realm, OTHER_USER)); + assertTrue(readUserFile().has(OTHER_USER)); + } + + + @Test + public void testFailedSaveKeepsFile() throws Exception + { + var realm = newRealm(); + realm.users.addAllowPermissions("user1", runtimePerm()); + var saved = Files.readString(userFile.toPath()); + + // entry that fails when it is written + var badUser = new UserConfig() { + @Override + public Collection getAllowList() + { + throw new IllegalStateException("test"); + } + }; + badUser.userID = "zzz"; + realm.users.put("zzz", badUser); + + assertThrows(SensorHubException.class, () -> realm.saveState(state)); + assertEquals(saved, Files.readString(userFile.toPath())); + + // a failed save does not fail the permission update + realm.users.addAllowPermissions("admin", runtimePerm()); + assertEquals(saved, Files.readString(userFile.toPath())); } }