JudahGabriel / RavenDB.Identity

RavenDB Identity provider for ASP.NET Core. Let RavenDB manage your users and logins.
https://www.nuget.org/packages/RavenDB.Identity/1.0.0
MIT License
61 stars 29 forks source link

.NET Core 3.0 and IdentityServer #18

Closed JoshClose closed 4 years ago

JoshClose commented 4 years ago

Update the library to work with .NET Core 3.0 and IdentityServer. Lots of things have changed and moved around.

JoshClose commented 4 years ago

It looks like for AddDefaultTokenProviders to be used, the project SDK needs to be changed to <Project Sdk="Microsoft.NET.Sdk.Razor">, the target framework to netcoreapp3.0 and item group <FrameworkReference Include="Microsoft.AspNetCore.App" /> needs to be added.

If you don't include those and let the user do it instead, then the project can stay as netstandard2.0.

There may be something I'm missing there.

JoshClose commented 4 years ago

Based on how ASP.NET configures the default setup:

public void ConfigureServices(IServiceCollection services)
{
    services.AddDbContext<ApplicationDbContext>(options =>
        options.UseSqlite(
            Configuration.GetConnectionString("DefaultConnection")));

    services.AddDefaultIdentity<ApplicationUser>()
        .AddEntityFrameworkStores<ApplicationDbContext>();

    services.AddIdentityServer()
        .AddApiAuthorization<ApplicationUser, ApplicationDbContext>();

    services.AddAuthentication()
        .AddIdentityServerJwt();

    services.AddControllersWithViews();
    services.AddRazorPages();

    // In production, the React files will be served from this directory
    services.AddSpaStaticFiles(configuration =>
    {
        configuration.RootPath = "ClientApp/build";
    });
}

maybe builder extensions would be more appropriate.

public static class IdentityBuilderExtensions
{
    public static IdentityBuilder AddRavenDbStores<TUser>(this IdentityBuilder builder)
        where TUser : IdentityUser
    {
        return builder.AddRavenDbStores<TUser, IdentityRole>();
    }

    public static IdentityBuilder AddRavenDbStores<TUser, TRole>(this IdentityBuilder builder)
        where TUser : IdentityUser
        where TRole : IdentityRole, new()
    {
        builder.Services.AddScoped<IUserStore<TUser>, UserStore<TUser, TRole>>();
        builder.Services.AddScoped<IRoleStore<TRole>, RoleStore<TRole>>();

        return builder;
    }
}

Then the user could set up their own identity and hook it up to the RavenDB.Identity store instead.

services.AddRavenDbAsyncSession(CreateDocumentStore());

services
    .AddDefaultIdentity<User>(options =>
    {
        options.SignIn.RequireConfirmedEmail = true;
    })
    .AddRoles<Role>()
    .AddRavenDbStores<User, Role>();
JoshClose commented 4 years ago

I have a branch with ongoing changes. https://github.com/InfinitTechnologies/RavenDB.Identity/commit/80c6d7245aef7a875e20b527e282115a3a5f7f5c

JudahGabriel commented 4 years ago

There's already an opened PR for this: https://github.com/JudahGabriel/RavenDB.Identity/pull/17

What do you think about it?

JoshClose commented 4 years ago

From my investigation, I believe the project can stay as netstandard2.0 and doesn't need to be changed to netcoreapp3.0. The changes I suggested would need to be made for it to stay as netstandard2.0 though.

I don't think you would want this enabled. <GeneratePackageOnBuild>.

I don't agree with the library changes, but all of the samples might be useful. I will have to look into the RavenDB.DependencyInjection changes, but I would assume that could probably stay as netsandard2.0 also.

What I don't know is if there is a way build for both .NET Core 2.2 and .NET Core 3.0 when your project is always netstandard2.0. The package references are different between the two.

adam8797 commented 4 years ago

Hi, sorry I've been meaning to get back to that PR but I lost track for a while.

I believe that you need to move the target framework up. There are changes in RavenDB.DependencyInjection that are not compatible with the netcore3 libraries. If you're running on .net core, you'll need to have a dependency on the netcore3 version of the RavenDB.DependencyInjection library.

You'll see that my proposed changes multi-target netstandard2.0 and netcoreapp3.0

JudahGabriel commented 4 years ago

I'm getting conflicting information here. @JoshClose thinks we can stay as netstandard2.0, while @adam8797 says we need to bump it (and RavenDB.DependencyInjection) to netcoreapp3.0.

JoshClose commented 4 years ago

Check out my changes here: https://github.com/InfinitTechnologies/RavenDB.Identity/commit/80c6d7245aef7a875e20b527e282115a3a5f7f5c?w=1

I haven't tried to update RavenDb.DependencyInjection yet though. If that requires it, then it won't really matter for RavenDB.Identity. I'll see if I can give that a try soon.

adam8797 commented 4 years ago

Part of the problem here does stem from RavenDB.DependencyInjection, and I believe that it does require a change in RavenDB.Identity as well.

There are a few different reasons.

First, and most importantly: there are dependencies on Asp.Net core itself, in both DI and Identity. See the Nuget Packages installed in DI (before my changes):

<PackageReference Include="Microsoft.AspNetCore.Hosting.Abstractions" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.Options.ConfigurationExtensions" Version="2.2.0" />

And the packages installed in Identity (before my changes):

<PackageReference Include="Microsoft.AspNetCore.Identity" Version="2.2.0" />
<PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="2.2.0" />

Any Asp.Net Core 3.0 app will have newer versions of all of these dependencies already installed, so installing this package will create version conflicts.

"No problem, It'll just use the higher version." you may think. Well besides being a dangerous idea with a package that has changed its major version, there is a conflict that keeps us from moving.

The problem stems from the fact that Microsoft moved and deprecated one interface. See here for my commit that works around it.

Microsoft.AspNetCore.Hosting.IHostingEnvironment has been deprecated and replaced with Microsoft.Extensions.Hosting.IHostEnvironment, which now exists in a different NuGet package

In order to accommodate this, we can multi-target netstandard2.0 and netcoreapp3.0. NuGet natively supports multi-targeting with different dependencies. In this way, we can support both Core 2.0 and Core 3.0, by changing which package gets installed depending on which type of app you're developing.

If you want to maintain compatibility with both both core2 and core3, you have to multi-target.

JoshClose commented 4 years ago

netstandard2.1 could be used for netcoreapp3.0 and netstandard2.0 for <= netcoreapp2.2 as netstandard2.1 is the updated version that is compatible with netcoreapp3.0.

I started looking at RavenDB.DependencyInjection is I believe that can still be netstandard too. I haven't finished going through it completely yet though.

adam8797 commented 4 years ago

I can totally buy using netstandard2.1 and netstandard2.0, if you dont want to target netcoreapp directly.

The only thing I would ask, is if there is any scenario where someone running Asp.Net Core 3.0 will not be running on netcoreapp3.0. AFAIK, they've dropped support for the full .Net Framwork, so you'll only ever run on netcoreapp3.0. And because this is an Asp.Net centered library, there will never be a reason to run it on anything but netcoreapp3.0+

One of the big reasons to target netstandard in the past (in the context of ASP.Net) was to support both Full Framework and .Net Core. That doesn't appear to be an issue anymore with Asp.Net Core 3

JudahGabriel commented 4 years ago

Thanks for the explanations, guys.

I'm OK with targeting netcoreapp; if folks want something that works on other platforms, they can reference older versions of this package.

So, should we just bump RavenDB.Identity and RavenDB.DependencyInjection to netcoreapp3.0? I'm leaning this way. If you guys don't object, I can pull the trigger and do that.

adam8797 commented 4 years ago

Yeah, if you don't mind breaking AspNetCore2.0, you can just straight bump the target framework. I think using an old version will be fine, I dont see this lib changing much. Same goes for DI, but those changes are just slightly more involved.

If you want to do it yourself, use my PR as a model for what needs to change, I tried to keep it minimal.

JoshClose commented 4 years ago

Microsoft.Extensions.DependencyInjection itself uses netcoreapp3.0, so go for it.

I would still suggest using IdentityBuilderExtensions I put above and removing ServiceCollectionExtensions. https://github.com/JudahGabriel/RavenDB.Identity/issues/18#issuecomment-540143049

I think you can do a netstandard2.0 and a netcoreapp3.0 like @adam8797 has in his pull request. You just need target conditions for the different references for each.

JudahGabriel commented 4 years ago

I've bumped Raven.DependencyInjection to use netstandard2.1, which is implemented by .NET Core 3.

Next step is updating this project to use that.

JoshClose commented 4 years ago

Sweet.

JudahGabriel commented 4 years ago

OK guys, I've migrated the package to use netstandard2.1, which supports .NET Core 3 apps.

I've also migrated all the samples to use netcoreapp3.0. I've tested them all and got them working as .NET Core 3 apps.

I've published an update to NuGet. The new package is RavenDB.Identity 6.2.0, referencing the newly upgraded RavenDB.DependencyInjection 3.1.

Closing this issue. Let me know if there's anything I'm missing.