Skip to content

Convert HttpListener to use public NegotiateAuthentication API - #86956

Merged
wfurt merged 3 commits into
dotnet:mainfrom
filipnavara:httplistener-ntauth
Jun 21, 2023
Merged

Convert HttpListener to use public NegotiateAuthentication API#86956
wfurt merged 3 commits into
dotnet:mainfrom
filipnavara:httplistener-ntauth

Conversation

@filipnavara

@filipnavara filipnavara commented May 31, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Removes the last user of internal NTAuthentication API that's outside of System.Net.Security. This should allow some code cleanup and refactoring, and also reduce size by having single implementation in one assembly.

@filipnavara
filipnavara requested a review from wfurt May 31, 2023 14:32
@ghost ghost added area-System.Net.Http community-contribution Indicates that the PR has been added by a community member labels May 31, 2023
@ghost

ghost commented May 31, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: filipnavara
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@filipnavara filipnavara changed the title Convert HttpListener to public NegotiateAuthentication API Convert HttpListener to use public NegotiateAuthentication API May 31, 2023
@filipnavara
filipnavara marked this pull request as ready for review May 31, 2023 15:32
@stephentoub

stephentoub commented May 31, 2023

Copy link
Copy Markdown
Member

Do we have sufficient test coverage of this? Our HttpListener coverage has historically been... spotty.

@filipnavara

filipnavara commented May 31, 2023

Copy link
Copy Markdown
Member Author

Do we have sufficient test coverage of this? Our HttpListener coverage has historically been... spotty.

There're few tests here: #86956 (comment)

They definitely don't cover everything but they should test at least the basic code path for NTLM authentication. I am aware that there is no code that tests modifying the claims on the Windows Principal object. I mimicked the feedback from ASP.NET PR from last year in regards to that part, but I highly doubt anyone uses that.

This was referenced May 31, 2023
Closed
Closed
@wfurt

wfurt commented Jun 16, 2023

Copy link
Copy Markdown
Member

Do we have sufficient test coverage of this? Our HttpListener coverage has historically been... spotty.

While I share the same sentiment, I also feel this should be probably ok. The NegotiateAuthentication is basically publicly exposed old PAL.

wfurt
wfurt reviewed Jun 16, 2023
View reviewed changes

@wfurt wfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks god @filipnavara. I like that it is mostly deletion and naming cleanup.

@filipnavara filipnavara mentioned this pull request Jun 16, 2023
Closed
@build-analysis build-analysis Bot mentioned this pull request Jun 16, 2023
Closed
@filipnavara
filipnavara requested a review from wfurt June 21, 2023 10:03
wfurt
wfurt approved these changes Jun 21, 2023
View reviewed changes

@wfurt wfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice. thanks.

@wfurt
wfurt merged commit e5bba27 into dotnet:main Jun 21, 2023
@karelz karelz added this to the 8.0.0 milestone Jul 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2023
@filipnavara
filipnavara deleted the httplistener-ntauth branch June 5, 2025 07:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurt wfurt wfurt approved these changes

Assignees

No one assigned

Labels

area-System.Net.Http community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

4 participants

@filipnavara @stephentoub @wfurt @karelz