Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Refactoring the Windows specific code in NegotiateStream#5772

Merged
stephentoub merged 1 commit into
dotnet:dev/negotiatestreamfrom
shrutigarg:devNegoFirst
Feb 15, 2016
Merged

Refactoring the Windows specific code in NegotiateStream#5772
stephentoub merged 1 commit into
dotnet:dev/negotiatestreamfrom
shrutigarg:devNegoFirst

Conversation

@shrutigarg
Copy link
Copy Markdown
Contributor

summary

Renaming NegoState.Windows.cs to InternalNegoState.cs as it had much common code applicable for Unix. (commit1)
Separating out the common code and windows specific code in InternalNegoState and NegoState.Windows.cs respectiviely.
Adding NegotiateInfoClass.cs to be used by Unix impl.

Tested locally both AuthenticateAsServer/Client on Windows.
/cc:@stephentoub @bartonjs @CIPop @vijaykota

@shrutigarg
Copy link
Copy Markdown
Contributor Author

following up #5581

@vijaykota
Copy link
Copy Markdown
Contributor

Adding a reference to #2483

@shrutigarg
Copy link
Copy Markdown
Contributor Author

ping .. This is on top of the PR in dotnet/Master #5581 and has all the comments incorporated.

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.

Why is this changing from Win32Exception to Exception? I realize the naming is poor, but it is actually supported on Unix, and it has the key features of both storing the error code and mapping it to a string supplied by the OS. Changing it to be Exception as is done here would seem to lose both of those things.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ohk ,, in that case , will revert back.

@CIPop
Copy link
Copy Markdown

CIPop commented Feb 4, 2016

@shrutigarg The extracted new methods in NegoState.* look good, except for cases where more refactoring was made (e.g. splitting existing methods or morphing two or more methods in a single method).
While optimizations can be made, I'd prefer if these are made in separate PR after we're done with the XPlat implementation. Any optimization will require a pinning test that maximizes code coverage over the affected areas. We should avoid this extra work in the earliest phases.

To be consistent between SslStream/NegotiateStream as well as make support between Desktop and CoreFX, could we extract the PAL in a separate class? We could create NegotiateStreamPal.Windows/Unix.cs similarly to how SslStreamPal is implemented. That way, we can create a very clear PAL internal contract between platforms.

@shrutigarg shrutigarg force-pushed the devNegoFirst branch 2 times, most recently from 594d6b1 to a06eea7 Compare February 8, 2016 17:18
@shrutigarg
Copy link
Copy Markdown
Contributor Author

@CIPop I fixed the refactoring by moving pal implementation to NegotiateStreamPal and created adapter class in last commit iteration. and other nit-pick comments in second last iteration. PTAL.

@vijaykota
Copy link
Copy Markdown
Contributor

@stephentoub , @bartonjs. please take a look at the last 2 commits. Thanks!

@shrutigarg shrutigarg force-pushed the devNegoFirst branch 2 times, most recently from c28a4f1 to b108787 Compare February 12, 2016 10:02
@vijaykota
Copy link
Copy Markdown
Contributor

@dotnet-bot test this please

1 similar comment
@stephentoub
Copy link
Copy Markdown
Member

@dotnet-bot test this please

else if (string.Compare(name, WDigest, StringComparison.OrdinalIgnoreCase) == 0)
{
AuthenticationPackage = WDigest;
}
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.

Why was this case deleted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Ok, good, thanks.

@stephentoub
Copy link
Copy Markdown
Member

A few more comments, but as long as they're tracked to be addressed subsequently, I think it's ok to merge this. We'll want to re-review everything before the dev branch gets merged to master.

@shrutigarg
Copy link
Copy Markdown
Contributor Author

Thanks. Will track them in an issue.

@shrutigarg
Copy link
Copy Markdown
Contributor Author

fixed a few comments and test issue. A few remaining comments are listed in issue #6063.

Other failures in build seems unrelated to me

@stephentoub
Copy link
Copy Markdown
Member

@dotnet-bot test this please

stephentoub added a commit that referenced this pull request Feb 15, 2016
Refactoring the Windows specific code in NegotiateStream
@stephentoub stephentoub merged commit f6ba2ae into dotnet:dev/negotiatestream Feb 15, 2016
@karelz karelz added this to the 1.0.0-rtm milestone Jan 21, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants