From a3074d2d95084f242e260568fd477862fc6da346 Mon Sep 17 00:00:00 2001 From: Kevin Herron Date: Fri, 17 Oct 2025 11:30:44 -0500 Subject: [PATCH] Add attribute ID validation and fix null UserWriteMask handling - Validate attribute IDs in read and write access checks - Return Bad_AttributeIdInvalid for invalid attribute IDs - Allow operations when UserWriteMask is null so they fail with Bad_NodeIdUnknown instead of Bad_UserAccessDenied - Add javadoc for AccessResult constants - Add tests for invalid attribute IDs and null UserWriteMask handling Fixes #1624 --- .../servicesets/impl/AccessController.java | 8 +++ .../impl/DefaultAccessController.java | 36 +++++++--- .../impl/DefaultAccessControllerTest.java | 71 +++++++++++++++++++ 3 files changed, 104 insertions(+), 11 deletions(-) diff --git a/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/AccessController.java b/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/AccessController.java index 63a3d94a99..da575d9cef 100644 --- a/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/AccessController.java +++ b/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/AccessController.java @@ -102,8 +102,16 @@ Map checkDeleteReferencesAccess( /** The result of an access control check. */ sealed interface AccessResult { + /** Access is allowed. */ AccessResult ALLOWED = new Allowed(); + + /** Access is denied due to an invalid attribute id. */ + AccessResult DENIED_ATTRIBUTE_ID_INVALID = new Denied(StatusCodes.Bad_AttributeIdInvalid); + + /** Access is denied due to insufficient user access rights. */ AccessResult DENIED_USER_ACCESS = new Denied(StatusCodes.Bad_UserAccessDenied); + + /** Access is denied due to insufficient security mode. */ AccessResult DENIED_SECURITY_MODE = new Denied(StatusCodes.Bad_SecurityModeInsufficient); /** Access is allowed. */ diff --git a/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessController.java b/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessController.java index a4d62914e1..13d01f7d95 100644 --- a/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessController.java +++ b/opc-ua-sdk/sdk-server/src/main/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessController.java @@ -69,6 +69,12 @@ static Map checkReadAccess( Map attributes = context.readAccessControlAttributes(nodeIds); + for (PendingResult p : pending) { + if (!AttributeId.isValid(p.value.getAttributeId())) { + p.result = AccessResult.DENIED_ATTRIBUTE_ID_INVALID; + } + } + checkAccessRestrictions(context, pending, attributes, ReadValueId::getNodeId); for (PendingResult p : pending) { @@ -122,12 +128,19 @@ public Map checkWriteAccess( static Map checkWriteAccess( AccessControlContext context, List writeValues) { + List> pending = writeValues.stream().map(PendingResult::new).toList(); List nodeIds = writeValues.stream().map(WriteValue::getNodeId).toList(); Map attributes = context.readAccessControlAttributes(nodeIds); + for (PendingResult p : pending) { + if (!AttributeId.isValid(p.value.getAttributeId())) { + p.result = AccessResult.DENIED_ATTRIBUTE_ID_INVALID; + } + } + checkAccessRestrictions(context, pending, attributes, WriteValue::getNodeId); for (PendingResult p : pending) { @@ -177,19 +190,20 @@ static Map checkWriteAccess( } } else { UInteger userWriteMask = attributes.get(nodeId).userWriteMask(); - Set userWriteMasks = - userWriteMask == null ? Set.of() : WriteMask.fromMask(userWriteMask); + if (userWriteMask != null) { + Set userWriteMasks = WriteMask.fromMask(userWriteMask); - // The value of the UserWriteMask attribute implicitly accounts for whether Roles and - // Permission are configured and if the current Session is assigned a role that includes the - // WriteAttribute PermissionType bit. - boolean hasAccess = - AttributeId.from(attributeId) - .map(id -> userWriteMasks.contains(WriteMask.forAttribute(id))) - .orElse(false); + // The value of the UserWriteMask attribute implicitly accounts for whether Roles and + // Permission are configured and if the current Session is assigned a role that includes + // the WriteAttribute PermissionType bit. + boolean hasAccess = + AttributeId.from(attributeId) + .map(id -> userWriteMasks.contains(WriteMask.forAttribute(id))) + .orElse(false); - if (!hasAccess) { - p.result = AccessResult.DENIED_USER_ACCESS; + if (!hasAccess) { + p.result = AccessResult.DENIED_USER_ACCESS; + } } } } diff --git a/opc-ua-sdk/sdk-server/src/test/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessControllerTest.java b/opc-ua-sdk/sdk-server/src/test/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessControllerTest.java index 8ac5c90e0e..38b7ec293b 100644 --- a/opc-ua-sdk/sdk-server/src/test/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessControllerTest.java +++ b/opc-ua-sdk/sdk-server/src/test/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessControllerTest.java @@ -134,6 +134,21 @@ void checkReadAccess_RolePermissions_Denied() { assertEquals(AccessResult.DENIED_USER_ACCESS, result); } + @Test + void checkReadAccess_InvalidAttributeId() { + var nodeId = new NodeId(1, "foo"); + var invalidAttributeId = UInteger.valueOf(9999); + var readValueId = new ReadValueId(nodeId, invalidAttributeId, null, null); + + var attributes = new AccessControlAttributes(null, null, null, null, null, null); + attributesMap.put(nodeId, attributes); + + AccessResult result = + DefaultAccessController.checkReadAccess(context, List.of(readValueId)).get(readValueId); + + assertEquals(AccessResult.DENIED_ATTRIBUTE_ID_INVALID, result); + } + @Test void checkWriteAccess_Value_Allowed() { var nodeId = new NodeId(1, "foo"); @@ -170,6 +185,22 @@ void checkWriteAccess_Value_Denied() { assertEquals(AccessResult.DENIED_USER_ACCESS, result); } + @Test + void checkWriteAccess_InvalidAttributeId() { + var nodeId = new NodeId(1, "foo"); + var invalidAttributeId = UInteger.valueOf(9999); + var writeValue = + new WriteValue(nodeId, invalidAttributeId, null, DataValue.valueOnly(Variant.NULL_VALUE)); + + var attributes = new AccessControlAttributes(null, null, null, null, null, null); + attributesMap.put(nodeId, attributes); + + AccessResult result = + DefaultAccessController.checkWriteAccess(context, List.of(writeValue)).get(writeValue); + + assertEquals(AccessResult.DENIED_ATTRIBUTE_ID_INVALID, result); + } + @Test void checkWriteAccess_AttributeProtectedByUserWriteMask_Allowed() { var nodeId = new NodeId(1, "wm-node"); @@ -228,6 +259,46 @@ void checkWriteAccess_AttributeProtectedByUserWriteMask_Denied() { assertEquals(AccessResult.DENIED_USER_ACCESS, results.get(wvBrowseName)); } + @Test + void checkWriteAccess_NullUserWriteMask_Allowed() { + // Simulate a non-existent node - all attributes would be null/unreadable + var nonExistentNodeId = new NodeId(1, "non-existent-node"); + + var wvDisplayName = + new WriteValue( + nonExistentNodeId, + AttributeId.DisplayName.uid(), + null, + DataValue.valueOnly(Variant.NULL_VALUE)); + var wvDescription = + new WriteValue( + nonExistentNodeId, + AttributeId.Description.uid(), + null, + DataValue.valueOnly(Variant.NULL_VALUE)); + var wvBrowseName = + new WriteValue( + nonExistentNodeId, + AttributeId.BrowseName.uid(), + null, + DataValue.valueOnly(Variant.NULL_VALUE)); + + // For a non-existent node, all attributes including userWriteMask would be null. + // The access check should return ALLOWED so the operation proceeds and fails + // later with Bad_NodeIdUnknown rather than incorrectly returning Bad_UserAccessDenied. + attributesMap.put( + nonExistentNodeId, new AccessControlAttributes(null, null, null, null, null, null)); + Mockito.when(context.getRoleIds()).thenReturn(Optional.of(List.of())); + + var results = + DefaultAccessController.checkWriteAccess( + context, List.of(wvDisplayName, wvDescription, wvBrowseName)); + + assertEquals(AccessResult.ALLOWED, results.get(wvDisplayName)); + assertEquals(AccessResult.ALLOWED, results.get(wvDescription)); + assertEquals(AccessResult.ALLOWED, results.get(wvBrowseName)); + } + @Test void checkWriteAccess_RolePermissions() { var nodeId = new NodeId(1, "foo");