-
Notifications
You must be signed in to change notification settings - Fork 1k
[Server] Fix generation of UserTokenPolicyIDs #3525
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[Server] Fix generation of UserTokenPolicyIDs #3525
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3525 +/- ##
==========================================
+ Coverage 51.86% 59.98% +8.11%
==========================================
Files 370 378 +8
Lines 78618 78978 +360
Branches 13650 13812 +162
==========================================
+ Hits 40779 47376 +6597
+ Misses 33705 27180 -6525
- Partials 4134 4422 +288 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request addresses issue #3522, which involves incorrect UserTokenPolicyId generation when an OPC UA server listens on multiple IP addresses. The fix changes the ID generation strategy from using a simple global counter to using semantic equality-based deduplication of UserTokenPolicy objects.
Changes:
- Adds
IEquatable<UserTokenPolicy>implementation to enable semantic equality comparison based on TokenType, SecurityPolicyUri, IssuedTokenType, and IssuerEndpointUrl (intentionally excluding PolicyId) - Modifies
ServerBase.GetUserTokenPoliciesto maintain a server-wide collection of unique policies and reuse existing PolicyIds for semantically equivalent policies - Ensures PolicyIds remain consistent regardless of the order in which endpoints are processed
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| Stack/Opc.Ua.Core/Stack/Types/UserTokenPolicy.cs | Implements IEquatable with Equals, GetHashCode, and operator overloads to enable semantic equality comparison |
| Stack/Opc.Ua.Core/Stack/Server/ServerBase.cs | Adds UserTokenPolicys collection for deduplicating policies and modifies GetUserTokenPolicies to assign consistent PolicyIds based on semantic equality |
| public override bool Equals(object obj) | ||
| { | ||
| return Equals(obj as UserTokenPolicy); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public bool Equals(UserTokenPolicy other) | ||
| { | ||
|
|
||
| if (ReferenceEquals(null, other)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| if (ReferenceEquals(this, other)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return | ||
| TokenType == other.TokenType && | ||
| string.Equals(SecurityPolicyUri, other.SecurityPolicyUri, StringComparison.Ordinal) && | ||
| string.Equals(IssuedTokenType, other.IssuedTokenType, StringComparison.Ordinal) && | ||
| string.Equals(IssuerEndpointUrl, other.IssuerEndpointUrl, StringComparison.Ordinal); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public override int GetHashCode() | ||
| { | ||
| return HashCode.Combine( | ||
| TokenType, | ||
| SecurityPolicyUri, | ||
| IssuedTokenType, | ||
| IssuerEndpointUrl); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public static bool operator ==(UserTokenPolicy left, UserTokenPolicy right) | ||
| { | ||
| if (ReferenceEquals(left, right)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (ReferenceEquals(left, null)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| return left.Equals(right); | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public static bool operator !=(UserTokenPolicy left, UserTokenPolicy right) | ||
| { | ||
| return !(left == right); | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new Equals, GetHashCode, and operator overloads added to UserTokenPolicy lack test coverage. Given that this implementation is critical for the bug fix (ensuring correct PolicyId generation), tests should be added to verify:
- Equality comparison works correctly for policies with the same semantic properties (TokenType, SecurityPolicyUri, IssuedTokenType, IssuerEndpointUrl) but different PolicyIds
- Hash codes are consistent for equal objects
- Operator overloads (== and !=) work as expected
- Edge cases like null values for optional string properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| var existingPolicy = UserTokenPolicys.FirstOrDefault(o => o.Equals(policy)); | ||
|
|
||
| // Ensure each policy has a unique ID within the context of the Server | ||
| if (existingPolicy == null) | ||
| { | ||
| clone.PolicyId = Utils.Format("{0}", UserTokenPolicys.Count + 1); | ||
| UserTokenPolicys.Add(clone); | ||
| existingPolicy = clone; | ||
| } | ||
|
|
||
| policies.Add(clone); | ||
| policies.Add(existingPolicy); | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The modified logic in GetUserTokenPolicies that uses the UserTokenPolicys collection to deduplicate policies based on semantic equality should have test coverage. Tests should verify:
- That policies with the same semantic properties receive the same PolicyId
- That PolicyIds are stable across multiple calls to
GetUserTokenPolicieswith different endpoint descriptions - That the SecurityPolicyUri modifications (lines 906, 911) are handled correctly
- That the fix resolves the original issue where servers with multiple IP addresses had inconsistent PolicyIds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| var existingPolicy = UserTokenPolicys.FirstOrDefault(o => o.Equals(policy)); | ||
|
|
||
| // Ensure each policy has a unique ID within the context of the Server | ||
| if (existingPolicy == null) | ||
| { | ||
| clone.PolicyId = Utils.Format("{0}", UserTokenPolicys.Count + 1); | ||
| UserTokenPolicys.Add(clone); | ||
| existingPolicy = clone; | ||
| } | ||
|
|
||
| policies.Add(clone); | ||
| policies.Add(existingPolicy); | ||
| } |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UserTokenPolicys collection uses IList<UserTokenPolicy> which is not thread-safe. While GetUserTokenPolicies is likely only called during server initialization, there's no explicit documentation or synchronization to prevent concurrent calls. The code uses FirstOrDefault (read), Count (read), and Add (write) operations on lines 915, 920-921 which could result in race conditions if called concurrently.
Consider either:
- Documenting that this method should only be called during initialization, or
- Adding proper synchronization (e.g., using a concurrent collection or locking) if concurrent access is possible.
| /// Defines constants for key user token policies. | ||
| /// </summary> | ||
| public partial class UserTokenPolicy : IFormattable | ||
| public partial class UserTokenPolicy : IFormattable, IEquatable<UserTokenPolicy> |
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class 'UserTokenPolicy' implements 'ICloneable'.
3abd8a2 to
810e06d
Compare
810e06d to
823cd17
Compare
|
@raserle Thank you very much for your fix, with this proper Ids are generated and the ctt can properly run tests requiring user auth. Also GetEndpoints returns the same Ids for same UserTokenPolicies |
|
The changes have already been approved by another reviewer, but it seems that the license agreement is still not being recognized. |
|
@raserle you need to register the Email configured in your local git as commit Email in your GitHub Account. Then the license cla will work |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
I can only approve and accept the CLA using my GitHub account raserle, which is associated with the verified email address. |
|
@raserle some Tests in the Opc.Ua.Core.Stack.Server area are now failing (e.g. TranslateEndpointDescriptionsTest), can you check it out? |
…erTokenPolicy Ids
|
@raserle I provided a fix, so we can get this into the next update |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Proposed changes
Change generation of UserTokenPolicyId's
Add IEquatable to UserTokenPolicy.cs
Related Issues
Types of changes
Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.