Skip to content
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

Remove old 1.x auth stack #4485

Merged
merged 11 commits into from
Jan 9, 2019
Merged

Remove old 1.x auth stack #4485

merged 11 commits into from
Jan 9, 2019

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Dec 7, 2018

@HaoK HaoK requested a review from Tratcher December 7, 2018 00:03
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

All of the servers implemented IHttpAuthenticationFeature, you'll need to remove the property there as well.

@HaoK
Copy link
Member Author

HaoK commented Dec 7, 2018

Updated HttpSys and IIS to remove the Handler property

@Tratcher
Copy link
Member

Tratcher commented Dec 7, 2018

And kestrel?

@HaoK
Copy link
Member Author

HaoK commented Dec 7, 2018

I couldn't see anything obvious that needed updating in kestrel and it didn't fail in building... PR validation looks like it failed just due to breaking changes for the removal, fixing those now

@Tratcher
Copy link
Member

Tratcher commented Dec 7, 2018

I guess in Kestrel it's only stored in the collection feature collection, it's not actually implemented. https://github.com/aspnet/KestrelHttpServer/search?q=IHttpAuthenticationFeature&unscoped_q=IHttpAuthenticationFeature

@kevinchalet
Copy link
Contributor

👍 that was an incredible source of pain for users not familiar with the Auth 2.0 "grand refactoring".

@Tratcher
Copy link
Member

2018-12-12T00:52:00.4744658Z   D:\a\1\s\src\Security\src\Microsoft.AspNetCore.Authentication.JwtBearer\JwtBearerOptions.cs(103,24): error CS1574: XML comment has cref attribute 'AuthenticationProperties' that could not be resolved [D:\a\1\s\src\Security\src\Microsoft.AspNetCore.Authentication.JwtBearer\Microsoft.AspNetCore.Authentication.JwtBearer.csproj]
2018-12-12T00:52:00.4745720Z   D:\a\1\s\src\Security\src\Microsoft.AspNetCore.Authentication.Twitter\Messages\RequestToken.cs(4,33): error CS0234: The type or namespace name 'Authentication' does not exist in the namespace 'Microsoft.AspNetCore.Http' (are you missing an assembly reference?) [D:\a\1\s\src\Security\src\Microsoft.AspNetCore.Authentication.Twitter\Microsoft.AspNetCore.Authentication.Twitter.csproj]
2018-12-12T00:52:00.4905085Z   D:\a\1\s\src\Security\src\Microsoft.AspNetCore.Authentication.Twitter\Messages\RequestTokenSerializer.cs(6,33): error CS0234: The type or namespace name 'Authentication' does not exist in the namespace 'Microsoft.AspNetCore.Http' (are you missing an assembly reference?) [D:\a\1\s\src\Security\src\Microsoft.AspNetCore.Authentication.Twitter\Microsoft.AspNetCore.Authentication.Twitter.csproj]
2

@HaoK
Copy link
Member Author

HaoK commented Dec 13, 2018

@natemcmaster are the test failures expected at this point, pr-validation-temp passed, is this PR ok to merge?

@natemcmaster
Copy link
Contributor

Depends on which tests failed and why. I recommend comparing your PR with https://github.com/aspnet/AspNetCore-Internal/labels/test-failure to see if failures are known or not.

@HaoK
Copy link
Member Author

HaoK commented Dec 13, 2018

This PR is just removing obsolete apis so its either a compilation break or no change, the failures mostly appear to be in kestrel so should be fine

@Tratcher
Copy link
Member

The IIS failures look related.
https://dnceng.visualstudio.com/public/_build/results?buildId=57950&view=logs&jobId=60ad5fa3-9ee1-561c-fc1d-1349a7b472eb&taskId=71e748b8-0ded-5502-f62f-08e9d9d9dfd7&lineStart=697&lineEnd=698&colStart=1&colEnd=1

IIS is still using package references. @natemcmaster would he need to check in and then update the IIS packages in the next build?

@natemcmaster
Copy link
Contributor

We are close to getting IIS onto project ref, so I would be okay ignoring the IIS test failures for a day. Okay with you @pakrym ?

@pakrym
Copy link
Contributor

pakrym commented Dec 13, 2018

As long as we don't forget about them I'm fine.

@HaoK
Copy link
Member Author

HaoK commented Dec 13, 2018

I'm happy to wait to merge next week as well, whatever is easiest for you guys is fine, this PR is done at this point so I can wait to merge when convenient

@davidfowl
Copy link
Member

@HaoK my bad bro, I broke all of your stuff

@HaoK
Copy link
Member Author

HaoK commented Jan 9, 2019

All good, rebased against master, will try to merge today if the checks pass

@HaoK
Copy link
Member Author

HaoK commented Jan 9, 2019

Wow checks all passed after rebasing and resolving conflicts, so safe to click the green button here @natemcmaster @Tratcher ?

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Wow green! LGTM

@Tratcher
Copy link
Member

Tratcher commented Jan 9, 2019

Yup, go for it.

@HaoK HaoK merged commit d7a7c65 into master Jan 9, 2019
@natemcmaster natemcmaster deleted the haok/ripauth branch January 18, 2019 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants