From 5f8ef35ff9ff322acb361805aba28826e8b5e4c7 Mon Sep 17 00:00:00 2001 From: Kevin Herron Date: Sun, 17 May 2026 08:59:26 -0700 Subject: [PATCH 1/2] Fix role permission authorization checks --- .../impl/DefaultAccessController.java | 32 ++-- .../impl/DefaultAccessControllerTest.java | 153 ++++++++++++++++++ 2 files changed, 173 insertions(+), 12 deletions(-) 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 13d01f7d95..43469228f6 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 @@ -17,6 +17,7 @@ import java.util.Optional; import java.util.Set; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; import org.eclipse.milo.opcua.sdk.core.AccessLevel; @@ -37,6 +38,7 @@ import org.eclipse.milo.opcua.stack.core.types.structured.CallMethodRequest; import org.eclipse.milo.opcua.stack.core.types.structured.DeleteNodesItem; import org.eclipse.milo.opcua.stack.core.types.structured.DeleteReferencesItem; +import org.eclipse.milo.opcua.stack.core.types.structured.PermissionType; import org.eclipse.milo.opcua.stack.core.types.structured.ReadValueId; import org.eclipse.milo.opcua.stack.core.types.structured.RolePermissionType; import org.eclipse.milo.opcua.stack.core.types.structured.WriteValue; @@ -101,8 +103,7 @@ static Map checkReadAccess( if (roleIds != null && userRolePermissions != null) { boolean hasAccess = - Stream.of(userRolePermissions) - .anyMatch(rp -> rp.getPermissions().getReadRolePermissions()); + hasPermission(roleIds, userRolePermissions, PermissionType::getReadRolePermissions); if (!hasAccess) { p.result = AccessResult.DENIED_USER_ACCESS; @@ -167,8 +168,7 @@ static Map checkWriteAccess( if (roleIds != null && userRolePermissions != null) { boolean hasAccess = - Stream.of(userRolePermissions) - .anyMatch(rp -> rp.getPermissions().getWriteRolePermissions()); + hasPermission(roleIds, userRolePermissions, PermissionType::getWriteRolePermissions); if (!hasAccess) { p.result = AccessResult.DENIED_USER_ACCESS; @@ -181,8 +181,7 @@ static Map checkWriteAccess( if (roleIds != null && userRolePermissions != null) { boolean hasAccess = - Stream.of(userRolePermissions) - .anyMatch(rp -> rp.getPermissions().getWriteHistorizing()); + hasPermission(roleIds, userRolePermissions, PermissionType::getWriteHistorizing); if (!hasAccess) { p.result = AccessResult.DENIED_USER_ACCESS; @@ -241,7 +240,7 @@ static Map checkBrowseAccess( if (roleIds != null && userRolePermissions != null) { boolean hasAccess = - Stream.of(userRolePermissions).anyMatch(rp -> rp.getPermissions().getBrowse()); + hasPermission(roleIds, userRolePermissions, PermissionType::getBrowse); if (!hasAccess) { p.result = AccessResult.DENIED_USER_ACCESS; @@ -303,10 +302,10 @@ static Map checkCallAccess( if (roleIds != null && objectPermissions != null && methodPermissions != null) { boolean objectPermission = - Stream.of(objectPermissions).anyMatch(rp -> rp.getPermissions().getCall()); + hasPermission(roleIds, objectPermissions, PermissionType::getCall); boolean methodPermission = - Stream.of(methodPermissions).anyMatch(rp -> rp.getPermissions().getCall()); + hasPermission(roleIds, methodPermissions, PermissionType::getCall); if (!objectPermission || !methodPermission) { p0.result = AccessResult.DENIED_USER_ACCESS; @@ -371,7 +370,7 @@ static Map checkAddReferencesAccess( if (roleIds != null && userRolePermissions != null) { boolean hasAccess = - Stream.of(userRolePermissions).anyMatch(rp -> rp.getPermissions().getAddReference()); + hasPermission(roleIds, userRolePermissions, PermissionType::getAddReference); if (!hasAccess) { p.result = AccessResult.DENIED_USER_ACCESS; @@ -416,7 +415,7 @@ static Map checkDeleteNodesAccess( if (roleIds != null && userRolePermissions != null) { boolean hasAccess = - Stream.of(userRolePermissions).anyMatch(rp -> rp.getPermissions().getDeleteNode()); + hasPermission(roleIds, userRolePermissions, PermissionType::getDeleteNode); if (!hasAccess) { p.result = AccessResult.DENIED_USER_ACCESS; @@ -464,7 +463,7 @@ static Map checkDeleteReferencesAccess( if (roleIds != null && userRolePermissions != null) { boolean hasAccess = - Stream.of(userRolePermissions).anyMatch(rp -> rp.getPermissions().getRemoveReference()); + hasPermission(roleIds, userRolePermissions, PermissionType::getRemoveReference); if (!hasAccess) { p.result = AccessResult.DENIED_USER_ACCESS; @@ -477,6 +476,15 @@ static Map checkDeleteReferencesAccess( // endregion + private static boolean hasPermission( + List roleIds, + RolePermissionType[] userRolePermissions, + Predicate permission) { + + return Stream.of(userRolePermissions) + .anyMatch(rp -> roleIds.contains(rp.getRoleId()) && permission.test(rp.getPermissions())); + } + private static void checkAccessRestrictions( AccessControlContext context, List> pending, 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 38b7ec293b..e66e0506ce 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 @@ -692,4 +692,157 @@ void checkDeleteReferences() { assertEquals(AccessResult.ALLOWED, result); } } + + @Test + void checkRolePermissionAccess_DeniesPermissionsForUnassignedRoles() { + Mockito.when(context.getRoleIds()).thenReturn(Optional.of(List.of(ROLE_B))); + + var readRolePermissionsNodeId = new NodeId(1, "readRolePermissions"); + var readValueId = + new ReadValueId( + readRolePermissionsNodeId, AttributeId.RolePermissions.uid(), null, null); + attributesMap.put( + readRolePermissionsNodeId, + new AccessControlAttributes( + null, + null, + null, + null, + null, + rolePermissions(PermissionType.Field.ReadRolePermissions))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkReadAccess(context, List.of(readValueId)).get(readValueId)); + + var writeRolePermissionsNodeId = new NodeId(1, "writeRolePermissions"); + var writeRolePermissionsValue = + new WriteValue( + writeRolePermissionsNodeId, + AttributeId.RolePermissions.uid(), + null, + DataValue.valueOnly(Variant.NULL_VALUE)); + attributesMap.put( + writeRolePermissionsNodeId, + new AccessControlAttributes( + null, + null, + null, + null, + null, + rolePermissions(PermissionType.Field.WriteRolePermissions))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkWriteAccess(context, List.of(writeRolePermissionsValue)) + .get(writeRolePermissionsValue)); + + var writeHistorizingNodeId = new NodeId(1, "writeHistorizing"); + var writeHistorizingValue = + new WriteValue( + writeHistorizingNodeId, + AttributeId.Historizing.uid(), + null, + DataValue.valueOnly(Variant.NULL_VALUE)); + attributesMap.put( + writeHistorizingNodeId, + new AccessControlAttributes( + null, + null, + null, + null, + null, + rolePermissions(PermissionType.Field.WriteHistorizing))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkWriteAccess(context, List.of(writeHistorizingValue)) + .get(writeHistorizingValue)); + + var browseNodeId = new NodeId(1, "browse"); + attributesMap.put( + browseNodeId, + new AccessControlAttributes( + null, null, null, null, null, rolePermissions(PermissionType.Field.Browse))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkBrowseAccess(context, List.of(browseNodeId)) + .get(browseNodeId)); + + var objectNodeId = new NodeId(1, "object"); + var methodNodeId = new NodeId(1, "method"); + var callMethodRequest = new CallMethodRequest(objectNodeId, methodNodeId, null); + attributesMap.put( + objectNodeId, + new AccessControlAttributes( + null, null, null, null, null, rolePermissions(PermissionType.Field.Call))); + attributesMap.put( + methodNodeId, + new AccessControlAttributes( + null, null, null, null, null, rolePermissions(PermissionType.Field.Call))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkCallAccess(context, List.of(callMethodRequest)) + .get(callMethodRequest)); + + var addReferenceSourceNodeId = new NodeId(1, "addReferenceSource"); + var addReferenceTargetNodeId = new NodeId(1, "addReferenceTarget"); + var addReferencesItem = + new AddReferencesItem( + addReferenceSourceNodeId, + NodeIds.HasComponent, + true, + null, + addReferenceTargetNodeId.expanded(), + NodeClass.Object); + attributesMap.put( + addReferenceSourceNodeId, + new AccessControlAttributes( + null, null, null, null, null, rolePermissions(PermissionType.Field.AddReference))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkAddReferencesAccess(context, List.of(addReferencesItem)) + .get(addReferencesItem)); + + var deleteNodesItem = new DeleteNodesItem(new NodeId(1, "deleteNode"), true); + attributesMap.put( + deleteNodesItem.getNodeId(), + new AccessControlAttributes( + null, null, null, null, null, rolePermissions(PermissionType.Field.DeleteNode))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkDeleteNodesAccess(context, List.of(deleteNodesItem)) + .get(deleteNodesItem)); + + var deleteReferenceSourceNodeId = new NodeId(1, "deleteReferenceSource"); + var deleteReferenceTargetNodeId = new NodeId(1, "deleteReferenceTarget"); + var deleteReferencesItem = + new DeleteReferencesItem( + deleteReferenceSourceNodeId, + NodeIds.HasComponent, + true, + deleteReferenceTargetNodeId.expanded(), + true); + attributesMap.put( + deleteReferenceSourceNodeId, + new AccessControlAttributes( + null, null, null, null, null, rolePermissions(PermissionType.Field.RemoveReference))); + + assertEquals( + AccessResult.DENIED_USER_ACCESS, + DefaultAccessController.checkDeleteReferencesAccess( + context, List.of(deleteReferencesItem)) + .get(deleteReferencesItem)); + } + + private static RolePermissionType[] rolePermissions(PermissionType.Field field) { + return new RolePermissionType[] { + new RolePermissionType(ROLE_A, PermissionType.of(field)), + new RolePermissionType(ROLE_B, PermissionType.of()) + }; + } } From bee7fc7e7227cc59174ea18230fdc8d14d0cb540 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 17 May 2026 16:21:37 +0000 Subject: [PATCH 2/2] Run spotless apply for formatting Agent-Logs-Url: https://github.com/kevinherron/milo/sessions/72d88e37-d3c3-4ece-939e-b4e8473510fb Co-authored-by: kevinherron <340273+kevinherron@users.noreply.github.com> --- .../servicesets/impl/DefaultAccessController.java | 3 +-- .../impl/DefaultAccessControllerTest.java | 13 +++---------- 2 files changed, 4 insertions(+), 12 deletions(-) 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 43469228f6..f61413755c 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 @@ -239,8 +239,7 @@ static Map checkBrowseAccess( RolePermissionType[] userRolePermissions = attributes.get(nodeId).userRolePermissions(); if (roleIds != null && userRolePermissions != null) { - boolean hasAccess = - hasPermission(roleIds, userRolePermissions, PermissionType::getBrowse); + boolean hasAccess = hasPermission(roleIds, userRolePermissions, PermissionType::getBrowse); 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 e66e0506ce..0a23a1361c 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 @@ -699,8 +699,7 @@ void checkRolePermissionAccess_DeniesPermissionsForUnassignedRoles() { var readRolePermissionsNodeId = new NodeId(1, "readRolePermissions"); var readValueId = - new ReadValueId( - readRolePermissionsNodeId, AttributeId.RolePermissions.uid(), null, null); + new ReadValueId(readRolePermissionsNodeId, AttributeId.RolePermissions.uid(), null, null); attributesMap.put( readRolePermissionsNodeId, new AccessControlAttributes( @@ -747,12 +746,7 @@ void checkRolePermissionAccess_DeniesPermissionsForUnassignedRoles() { attributesMap.put( writeHistorizingNodeId, new AccessControlAttributes( - null, - null, - null, - null, - null, - rolePermissions(PermissionType.Field.WriteHistorizing))); + null, null, null, null, null, rolePermissions(PermissionType.Field.WriteHistorizing))); assertEquals( AccessResult.DENIED_USER_ACCESS, @@ -834,8 +828,7 @@ void checkRolePermissionAccess_DeniesPermissionsForUnassignedRoles() { assertEquals( AccessResult.DENIED_USER_ACCESS, - DefaultAccessController.checkDeleteReferencesAccess( - context, List.of(deleteReferencesItem)) + DefaultAccessController.checkDeleteReferencesAccess(context, List.of(deleteReferencesItem)) .get(deleteReferencesItem)); }