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

Move all of DataProtection and Extensions.Identity to netstandard2.0 #11008

Merged
merged 83 commits into from
Jun 21, 2019

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Jun 7, 2019

First part of #3774 this will allow Extensions.Identity.Core Stores, and a few parts of data protection to target netstandard 2.0

@HaoK
Copy link
Member Author

HaoK commented Jun 7, 2019

@natemcmaster @blowdart this seems a bit scary as the NetCorePbkdf2Provider provider is using a netcore DeriveBytes API, I only started with a few of the DP.abstractions packages and KeyDerivation which is the main thing that Identity needs for the password hasher.

I also reverted the Apr 26 PR that switched the password hasher to use CryptographicOperations.FixedTimeEquals (2b2aeee)

@Eilon Eilon added the area-dataprotection Includes: DataProtection label Jun 7, 2019
Copy link
Contributor

@natemcmaster natemcmaster left a comment

Choose a reason for hiding this comment

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

Good start, but let's use cross compiling to keep existing improvements.

@HaoK
Copy link
Member Author

HaoK commented Jun 8, 2019

Okay so I took a stab at switching DP to netstandard and the issue is the IStartupFilter (this comes from AspNetCore.Hosting.Abstractions and not Extensions.Hosting.Abstractions) that gets added as part of AddDataProtection()

           services.TryAddEnumerable(ServiceDescriptor.Singleton<IStartupFilter, DataProtectionStartupFilter>());

Was there any prior discussion about how this should be resolved?

@HaoK
Copy link
Member Author

HaoK commented Jun 11, 2019

@natemcmaster @ajcvickers what do you guys think about merging the identity + some of dataprotection changes since this at least enables extensions identity + dp abstractions / keyderivations to be used on netstandard 2.0. I can open another PR addressing the gnarly hosting + DP when we have agreed on an approach

@natemcmaster
Copy link
Contributor

Discussed briefly with @ajcvickers.

I'm okay merging the changes in this PR as long as you update the description to remove the "fixes #3774" description. The goal of #3774 was to support packages which are both .NET Standard and do not have a dependency on the .NET Core shared framework, and this PR doesn't yet satisfy that criteria. To complete that issue, you'll need to address the DP -> Hosting dependency. Here are my thoughts on ways you might solve this: #3774 (comment).

Also, to make the packages available as packages, you need to set <IsShippingPackage> in the .csproj file, otherwise the assemblies will still be shared-framework only.

@HaoK HaoK marked this pull request as ready for review June 12, 2019 05:12
@HaoK HaoK requested a review from a team as a code owner June 12, 2019 05:12
@HaoK
Copy link
Member Author

HaoK commented Jun 12, 2019

Removed fix comment and added <IsShippingPackage>true</IsShippingPackage

@@ -2,7 +2,9 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
#if NETCOREAPP3_0
Copy link
Member Author

Choose a reason for hiding this comment

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

FYi @BrennanConroy I had to add this guard around the using buffers introduced from the spanify web encoders change last week since it isn't available in netstandard 2.0

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense 👍

@HaoK HaoK changed the title Move pieces of DataProtection and Identity to netstandard2.0 Move all of DataProtection and Extensions.Identity to netstandard2.0 Jun 18, 2019
@HaoK
Copy link
Member Author

HaoK commented Jun 18, 2019

This should now fix #3774 pending the resolution of IHostedService not running before the server starts :(

@Tratcher
Copy link
Member

This should now fix #3774 pending the resolution of IHostedService not running before the server starts :(

In WebHost IHostedServices are started just after the server. In generic Host the web app is an IHostedService and they're started in registration order. Try testing with generic host to make sure you can get the ordering right. For WebHost it looks like IHostedServices is better than first-request, but not quite as good as IStartupFilter.

@HaoK
Copy link
Member Author

HaoK commented Jun 21, 2019

Figured out the diff was trailing whitespace in the sharedframework.local...grrr

@HaoK
Copy link
Member Author

HaoK commented Jun 21, 2019

Per offline thread with @Tratcher and @davidfowl this seems good enough for preview 7, will tweak in subsequent PRs as needed

@HaoK HaoK merged commit f35564b into master Jun 21, 2019
@ghost ghost deleted the data-netstd branch June 21, 2019 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-dataprotection Includes: DataProtection
Projects
None yet
Development

Successfully merging this pull request may close these issues.