Session is not provided by "ClearChangeMasks" when a change is notified

[Filippo-Oliva-ABB]

Type of issue

• Bug
• Enhancement
• Compliance
• Question
• Help wanted

Current Behavior

• A single OPC UA server providing a single “NodeManager” that:

  • Creates and exposes the whole address space.
  • Notifies each subscribed client through ClearChangeMasks method.
  • Supports User Authentication and Authorization by checking the UserIdentity.
  • Checks the session identity and sets up the UserAccessLevel property of the Node in the hook OnReadUserAccessLevel.

• Two clients (Client1 and Client2) connected (and subscribed to the same set of variables) to the server with two different users (User1 and User2).

  • User1 doesn’t have the permission to read.
  • User2 has the permission to read.

When Client1 and Client2 subscribe for the first time to the variable changes everything works, meaning that Client1 get an error during the subscription while Client2 can complete the subscription and reads the first value as expected.

When the “NodeManager” calls the ClearChangeMasks method it happens that both Client1 and Clien2 can’t read the just changed value.
Tracking the session acquired by the OnReadUserAccessLevel event handler, it appears empty.
This seems to be since the context passed to the ClearChangeMasks the one referenced by the “NodeNanager” doesn’t have a session in the time of that call.

Expected Behavior

When the “NodeManager” calls the ClearChangeMasks method it is expected that Client1 will still receive an authorization error while Client2 will be able to read the changed value.

Steps To Reproduce

• A single OPC UA server providing a single “NodeManager” that:

  • Creates and exposes the whole address space.
  • Notifies each subscribed client through ClearChangeMasks method.
  • Supports User Authentication and Authorization by checking the UserIdentity.
  • Checks the session identity and sets up the “UserAccessLevel” property of the Node in the hook OnReadUserAccessLevel.

• Two clients (Client1 and Client2) connected (and subscribed to the same set of variables) to the server with two different users (User1 and User2).

  • User1 doesn’t have the permission to read.
  • User2 has the permission to read.

Make the "NodeManager" calls the ClearChangeMasks method on one variable among those both clients are subscribed to, notifying a value change passing as the first argument the instance of "ISystemContext" provided by the "SystemContext" property of the "NodeManager".

Environment

- OS: Windows 10
- Environment: Visual Studio 2019 16.11.33
- Runtime: .NET Framework 4.8
- Nuget Version: 1.5.364.36
- Component: Opc.Ua.Server
- Server: Quickstarts.UserAuthenticationServer
- Client: UA Expert

Anything else?

I tried to fix the lack of session changing the context passed to the ClearChangeMasks method by looping through all available session. To do this I got the list of active session from the referenced “SessionManager” and, for each of them I created a copy of “SytemContext” providing it a new instance of “OperationContext” including the session and I passed the new context to the ClearChangeMasks.

As a result, when the “NodeManager” triggers the ClearChangeMasks it happens that both Client1 and Client2 can or can’t read depending on which was the first session connected.
Tracking the session acquired by the OnReadUserAccessLevel event handler, it appears that the identity included in it is always the one of the first session connected.

Thus, I thoroughly analyzed and debugged the library and I found a possible solution to this bug which involves a fix to the "Opc.Ua.Server" library.

[romanett]

After further investigation I came to the following conclusion:

• creating a MI is correctly validating the role permissions
• however changing the user identity after the MI exists you are still allowed to receive data changes.

Test Setup:

• Reference Server
• Node: ns=2 nodeId=AccessRights_RolePermissions_ConfigureAdmin

node creation:

Client 1: Configure Admin (sysadmin) ->sucessfully monitor node
Client 2: Anonymous -> cant create MI
Client 1: -> change user identity to anonymous -> still monitors node
Client 2: -> write node
Client 1-> receives Data change even though it should not be able to

fixed by: #2809

[romanett]

@Filippo-Oliva-ABB please test with #2809 if this fixes the issue also in your tests

[Filippo-Oliva-ABB]

Hi @romanett, first of all thank you for your effort in solving this issue, I really appreciate it.
Unfortunately what we are trying to achieve is a dynamic permission management, based on a set of remote configurations that may change asynchronously, so I can't statically assign a permission to a node.

[romanett]

@Filippo-Oliva-ABB you can assign the permissions at runtime as far as i know, it was just easier to reproduce for me.

[Filippo-Oliva-ABB]

Ok, sorry @romanett , I misunderstood your intentions.
As for your PR #2809, I think it will not solve the problem I found for these reasons:

1. Since the NodeManager.ValidateRolePermissions method returns "Good" in case the session is null, in that case the "MonitoredItem" would be queued and when that happens, it is done with the original ServerSystemContext which does not contain any information related to the user session.
2. The problem with my PR #2656 Fix for - Session is not provided by ClearChangeMasks when a change is notified #2772 is related to a NullArgumentException which, in my opinion, could be related to a concurrency issue that any "null check" that could be added before queueing could only mitigate but not eliminate the risk of the exception being thrown.

[romanett]

@Filippo-Oliva-ABB

regarding 1. ithe function ValidateRolePermissions is called with a new OperationContext initialized by the MI, therefore the check will be performed, with the session of the MonitoredItem.

Can you still reproduce the issue with my fix?

For me it seems the check should be enough

[Filippo-Oliva-ABB]

Hi @romanett, I meant that in the case of the MonitoredItem.Session would be null for some reason, the ValidaterRolePermission method would returns "Good" letting the Monitored.Item be queued.
You purposed fix can't solve the problem that I'm experiencing because I always need a session associated to the Context that is passed as an argument of the OnReadUserAccessLevel event handler.

My PR would fix the problem but increases the probability to experience the NullArgumentException because this part of the code is executed every time a change needs to be reported to each subscribed client.

At this point, in my humble opinion, there are two options:

1. My concerns about the any concurrency issue related to the MonitoredItem.Session management is unfounded. In this case it may be simply possible to add a "null check" before creating a new OperationContext passing the MonitoredItem.Session as an argument.
2. My concerns are founded and a deeper analysis is needed to fix this different issue.

[romanett]

@Filippo-Oliva-ABB The case a MI without associated session exists can occur when a session is closed but the subscription lifetime is not up yet. In this case we definitely need to continue sampling.

I have two more questions:

• how does your fix help in case a Monitored Item without an associated session exists?
• is it described anywhere in the spec that OnReadUserAccessLevel /UserAcccessLevel shall be checked directly by the server. Isn´t this what RolePermissions Checks are made for to not directly needing to interact with the UserAccessLevel of the Nodes? Because to me it seems those checks relying on OperationContext should happen the same way as for events.

Regarding: #2777 When does the ISystemContext reporting an event contain a session and what does this mean for the event (are session local events a thing desribed in the spec?)

To be able to verify role Permissions in case the session is closed several changes are needed:

• MI reports SavedUserIdentity of the Susbscription (Interface change / extension of IMonitoredItem needed)
• OperationContext is initialzed with savedUserIdenty if session is null
• MasterNodeManager.ValidateRolePermissions reads user Identity directly from the OperationContext instead of associated session.

[Filippo-Oliva-ABB]

Hi @romanett, trying to respond with order:

• how does your fix help in case a Monitored Item without an associated session exists?
  Simply doesn't because at the time I purposed it, I supposed that a MI shall always have an associated session.
• is it described anywhere in the spec that OnReadUserAccessLevel /UserAcccessLevel shall be checked directly by the server. Isn´t this what RolePermissions Checks are made for to not directly needing to interact with the UserAccessLevel of the Nodes?
  I'm currently doing that check in the server side, I think I didn't get the point of you question. Besides that I need a place where I can check the permissions the user has on the subscribed variable, and I this permissions may change any time. So If there is a better place where I can customize this behavior, feel free to suggest it to me, I really appreciate it.
• When does the ISystemContext reporting an event contain a session and what does this mean for the event (are session local events a thing desribed in the spec?)
  The case was the ConditionRefresh call. When a client invokes it, without my PR, each client would get the events and not only the one that sent the request.

All things considered, I need a clarification about this:
"The case a MI without associated session exists can occur when a session is closed but the subscription lifetime is not up yet. In this case we definitely need to continue sampling."

• Why It's needed that the sampling continues?
• Shuldn't all MonitoredItem be removed for that session since no client is interested in its changes anymore?

[romanett]

@Filippo-Oliva-ABB regarding checking UserAcccessLevel vs. RolePermissions checking. I was not familiar with checking the UserAccessLevel so i thougth maybe using the existing RolePermissions check at the necessary places (Read /Write from Client is checked, Event MI is also covered and DataChangeMI would be coverd with my PR) would be a more common solution. However I am not shure about your exact needs so maybe checking UserAccessLevel directly can achieve a differnt usecase.

Why It's needed that the sampling continues?
--> This is the intended design of Subscriptions
Shuldn't all MonitoredItem be removed for that session since no client is interested in its changes anymore?
-> Only when the lifetime is up shall the subscription be cleaned up

Subscriptions are designed to work independent of the actual communication connection between OPC UA Client and Server and independent of a Session. Short communication interruptions can be handled without losing data or events.

see: https://reference.opcfoundation.org/Core/Part4/v105/docs/5.13.1