Fix role permission authorization checks

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<ReadValueId, AccessResult> 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<WriteValue, AccessResult> 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<WriteValue, AccessResult> 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;
@@ -240,8 +239,7 @@ static Map<NodeId, AccessResult> checkBrowseAccess(
       RolePermissionType[] userRolePermissions = attributes.get(nodeId).userRolePermissions();
 
       if (roleIds != null && userRolePermissions != null) {
-        boolean hasAccess =
-            Stream.of(userRolePermissions).anyMatch(rp -> rp.getPermissions().getBrowse());
+        boolean hasAccess = hasPermission(roleIds, userRolePermissions, PermissionType::getBrowse);
 
         if (!hasAccess) {
           p.result = AccessResult.DENIED_USER_ACCESS;
@@ -303,10 +301,10 @@ static Map<CallMethodRequest, AccessResult> 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 +369,7 @@ static Map<AddReferencesItem, AccessResult> 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 +414,7 @@ static Map<DeleteNodesItem, AccessResult> 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 +462,7 @@ static Map<DeleteReferencesItem, AccessResult> 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 +475,15 @@ static Map<DeleteReferencesItem, AccessResult> checkDeleteReferencesAccess(
 
   // endregion
 
+  private static boolean hasPermission(
+      List<NodeId> roleIds,
+      RolePermissionType[] userRolePermissions,
+      Predicate<PermissionType> permission) {
+
+    return Stream.of(userRolePermissions)
+        .anyMatch(rp -> roleIds.contains(rp.getRoleId()) && permission.test(rp.getPermissions()));
+  }
+
   private static <T> void checkAccessRestrictions(
       AccessControlContext context,
       List<PendingResult<T>> pending,

opc-ua-sdk/sdk-server/src/test/java/org/eclipse/milo/opcua/sdk/server/servicesets/impl/DefaultAccessControllerTest.java

@@ -692,4 +692,150 @@ 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())
+    };
+  }
 }