-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Switch DirectoryControl to use AsnWriter, AsnDecoder #101512
Merged
Merged
Changes from 17 commits
Commits
Show all changes
24 commits
Select commit
Hold shift + click to select a range
9b87b4e
Removed BerConverter.Decode usage
edwardneal 31600a4
Removed BerConverter.Encode usage
edwardneal a152c84
Regression in CrossDomainMoveControl
edwardneal 9461057
Correcting test values
edwardneal cb07eb4
Removed SortKeyInterop from .csproj
edwardneal 73dd1bc
Small optimisations
edwardneal c5c9598
AD compatibility test corrections
edwardneal 33e4f25
Performance improvement, bugfix
edwardneal 6b487c2
Following code review
edwardneal b4bbdc7
Implemented further code review comments
edwardneal e12d447
Remainder of code review feedback
edwardneal 57fc5f6
First response to code review feedback
edwardneal f1aca42
Further code review changes
edwardneal 2a6915b
Merge remote-tracking branch 'upstream/main' into issue-97540
edwardneal 96790e0
Update tests from PR107201
edwardneal a215542
Tightened validation based on updated tests
edwardneal 2d87369
Eliminated OS-specific test condition
edwardneal 5ca1fad
Merge remote-tracking branch 'upstream/main' into issue-97540
edwardneal 610b5b2
Code review: SortResponseControl
edwardneal c2df730
Code review: VlvRequestControl
edwardneal 3056ec7
Code review: TransformControls
edwardneal 98ef3e6
Disabled the new VlvRequestControl on .NET Framework
edwardneal 80cb7af
Cleanup GetWriter
edwardneal 0af321f
Renamed WriteLdapString to WriteStringAsOctetString
edwardneal File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
18 changes: 0 additions & 18 deletions
18
...Services.Protocols/src/System/DirectoryServices/Protocols/Interop/SortKeyInterop.Linux.cs
This file was deleted.
Oops, something went wrong.
18 changes: 0 additions & 18 deletions
18
...rvices.Protocols/src/System/DirectoryServices/Protocols/Interop/SortKeyInterop.Windows.cs
This file was deleted.
Oops, something went wrong.
35 changes: 0 additions & 35 deletions
35
...ectoryServices.Protocols/src/System/DirectoryServices/Protocols/Interop/SortKeyInterop.cs
This file was deleted.
Oops, something went wrong.
36 changes: 36 additions & 0 deletions
36
...ryServices.Protocols/src/System/DirectoryServices/Protocols/common/AsnWriterExtensions.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using System.Formats.Asn1; | ||
| using System.Text; | ||
|
|
||
| namespace System.DirectoryServices.Protocols | ||
| { | ||
| internal static class AsnWriterExtensions | ||
| { | ||
| public static void WriteLdapString(this AsnWriter writer, string value, Encoding stringEncoding, bool mandatory = true, Asn1Tag? tag = null) | ||
| { | ||
| // A typical stack allocation threshold would be 256 bytes. A higher threshold has been chosen because an LdapString can be | ||
| // used to serialize server names. A server name is defined by RF1035, which specifies that a label in a domain name should | ||
| // be < 64 characters. If a server name is specified as an FQDN, this will be at least three labels in an AD environment - | ||
| // up to 192 characters. Doubling this to allow for Unicode encoding, then rounding to the nearest power of two yields 512. | ||
| const int StackAllocationThreshold = 512; | ||
|
|
||
| if (!string.IsNullOrEmpty(value)) | ||
| { | ||
| int octetStringLength = stringEncoding.GetByteCount(value); | ||
| // Allocate space on the stack. There's a modest codegen advantage to a constant-value stackalloc. | ||
| Span<byte> tmpValue = octetStringLength <= StackAllocationThreshold | ||
| ? stackalloc byte[StackAllocationThreshold].Slice(0, octetStringLength) | ||
| : new byte[octetStringLength]; | ||
|
|
||
| stringEncoding.GetBytes(value, tmpValue); | ||
| writer.WriteOctetString(tmpValue, tag); | ||
| } | ||
| else if (mandatory) | ||
| { | ||
| writer.WriteOctetString([], tag); | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I'm not sure this is a good name. There's no construct (that I see) called
LdapString, and not all strings in LDAP are sent as "A Utf8String, except using tag 04 instead of 0C".WriteUtf8OctetString, maybe?The
bool mandatoryhas no peer on AsnWriter methods. I recommend removing it here (making it always behave astrue, and making the one "optional" caller bring that logic closer to home... so it looks like any other conditional write for an ASN OPTIONAL or DEFAULT value.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.
I guess one caller passes Encoding.Unicode. So either two functions, or "WriteStringAsOctetString" might be a better name for the current shape.
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 name
LdapStringpartially comes from RFC2251, as the backing type forAttributeDescription. Do you still want the name to change?It was primarily used for writing the sort controls, and the other control logic piggybacks on the same method by explicitly specifying the encoding. I'll see if two methods would be clearer for this.
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.
I've tried a couple of different methods to see what the semantics look like, and agree -
WriteStringAsOctetStringit is. That's rolled up and done now.