Orchardcore: Consider adding Entity Framework Core stores for the OrchardCore.Users module

[kevinchalet]

@alexbocharov I'm pretty sure @sebastienros told me once that you were using a custom Users-inspired module using EF Core instead of YesSql. Do you confirm? :sweat_smile:

[sebastienros]

Not him, I think it was @rserj

[rserj]

Yeh after refactoring with Users I could easy override asp.net identity staff by using Entity framework.

Override UserStore by implementing

IUserStore<IUser>,
IUserRoleStore<IUser>,
IUserPasswordStore<IUser>,
IUserEmailStore<IUser>,
IUserSecurityStampStore<IUser>

RoleStore by implementing
IRoleClaimStore<IRole>,
IRoleProvider

+ I implemented
OrchardCore.Users.Services.IUserService

But I have another layer of abstraction, with IRepository<T>
I'm not using Entity Framework's DBContext directly on my implementations.
@sebastienros @PinpointTownes
Do you think having EFRepository<T>: IRepository<T> in Orchard is OK? or it might be application specific?

[sebastienros]

We could have an EF module with features that provide implementations for different other core modules.

[rserj]

If you think IRepository is Ok, then we better create
OrchardCore.Data.Abstraction project and
OrchardCore.Data.EntityFramework module

[kevinchalet]

But I have another layer of abstraction, with IRepository
I'm not using Entity Framework's DBContext directly on my implementations.

Oh, so you had to re-implement all the UserStore/RoleStore methods yourself?

Identity's user and role stores represent a large codebase so if we opt for this approach, we'll have to maintain a similar "copy" in Orchard (and manually port all the bug fixes added in the official stores). It's a lot of work.

Instead, I think we should use the default Identity EF Core stores and simply add the "proxy methods" we need to make the generic (Identity)/non-generic (Orchard) worlds interoperate, similarly to what's been done for the OpenID module: https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.OpenId.EntityFrameworkCore/Services/OpenIdAuthorizationStore.cs#L70

[rserj]

thanks @PinpointTownes, I see what do you mean
It seems you want to reuse
Microsoft.AspNetCore.Identity.EntityFrameworkCore

https://github.com/aspnet/Identity/tree/d4d105d5b529c8e1701010cb49bc115f0aa23ed0/src/Microsoft.AspNetCore.Identity.EntityFrameworkCore
I take a look their implementation, they use their own models like IdentityUser,
I'm not sure, probably we need to write sort of adapters/make casting with our's Orchard IUser.

[kevinchalet]

I'm not sure, probably we need to write sort of adapters/make casting with our's Orchard IUser.

We'd need to create custom entities derived from the Identity entities and make them implement the IUser/IRole interfaces. E.g: https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.OpenId.EntityFrameworkCore/Models/OpenIdAuthorization.cs

[rserj]

I'll try to make a PR next week
Thanks

[infofromca]

I prefer IRepository. in this way, orchard can be loose couple , and more widely used by user's choice . EF, LiteDB, ..., even in other projects other than CMS.
nowdays, search yessql for the issues, many are related to this IRepository layer.

[kevinchalet]

@rserj I briefly talked with @sebastienros about Orchard's marker interfaces (i.e IUser/IRole in Users and IOpenIdApplication/IOpenIdAuthorization/IOpenIdScope/IOpenIdToken in OpenId).

In a nutshell, I suggested removing these interfaces and replacing them by object so we don't have to create Orchard-specific entities for the Users/OpenID (by returning/taking an object, we can directly use the default OpenIddict and Identity entities in the EF stores).

Do you have any concern about this change?

[rserj]

@PinpointTownes @sebastienros
I agree it will simplify some parts significantly, I checked code, we use IUser as marker interface in OpenId module too, so it should just work.
But I found Admin->Users (OrchardCore.Users.AdminController)we use _session.Query<User, UserIndex>() we better use IUserService instead. Even so, I afraid If any module introduces their own User type they need to override OrchardCore.Users.AdminController and UserDisplayDriver.

As I remember the whole idea was. Each module can just introduce CustomUsers:IUser type, create their own Drivers<IUser> and CustomUser's attributes will just appear in Admin tool UI, like it works with ISite, I'm not sure how it will work if we get rid of from IUser

Can you please create a branch for it? Probably I better make my PR regarding "Entity Framework" to this branch and we may start experimenting by using Demo module as a proof of concepts (this is what I'm currently doing).

[kevinchalet]

But I found Admin->Users (OrchardCore.Users.AdminController)we use _session.Query() we better use IUserService instead.

Yup, that's something we should fix. Same thing for the is User checks, that prevent using a different entity in AdminController: https://github.com/OrchardCMS/OrchardCore/blob/dev/src/OrchardCore.Modules/OrchardCore.Users/Controllers/AdminController.cs#L172-L176

Even so, I afraid If any module introduces their own User type they need to override OrchardCore.Users.AdminController and UserDisplayDriver.

I'm not familiar enough with the display driver thingy but the controller shouldn't be a problem. I made a prototype yesterday for the OpenID module and replacing IOpenIdApplication by object in the controller didn't make any difference (it's just a pointer anyway, all the operations are done by the managers).

@sebastienros is there anything that would prevent us from making UserDisplayDriver derive from DisplayDriver<object> instead of DisplayDriver<User>?

[kevinchalet]

Hum, looks like the display manager won't like that since it's a generic thing that resolves the drivers based on the model type (TModel). If we have drivers for different models that all implement DisplayDriver<object>, they'll likely be mixed up.

[rserj]

Personally, I think this is not good having DisplayDriver<object>, cause it may conflict with some future implementations.
We may just leave IUser for Drivers, and do not use it in Identity

[sebastienros]

This will be fixed as soon as I have refactored some components to be removed from DI and use options instead. https://github.com/OrchardCMS/OrchardCore/issues/1061

[rserj]

@sebastienros @PinpointTownes I almost finished it but I faced one issue.
I'm enabling new Users.EntityFrameworkCore module during setup
But it fails cause there is a RoleUpdaterevent which tries to add roles and permissions after each module gets installed.
But at the moment of installation Migration schema didn't execute yet, so there are no Identity tables in DB, and it fails.
In order to overcome this issue, Application need a way to register their "Features" which will be initialized before installation, like it was implemented in
"OrchardCore.Application.Cms.Targets.ServiceExtensions"

services.AddModules(modules => 
            {
                modules.WithDefaultFeatures(
                    "OrchardCore.Mvc", "OrchardCore.Settings", "OrchardCore.Setup",
                    "OrchardCore.Recipes", "OrchardCore.Commons", "OrchardCore.Demo.Users.EntityFrameworkCore.SqlServer");
            });

In the code above "OrchardCore.Recipes.Migrations" is executing first, and we have DB schema which is prepared for Setup. I wish other Apps can do the same, they can just add own "Features" which must be initialized at first. I tried to add my custom module Users.EntityFrameworkCore and it didn't work because of hard-coded modules list in `OrchardCore.Setup.Services.SetupService

// Features to enable for Setup
            string[] hardcoded =
            {
                "OrchardCore.Commons",
                "OrchardCore.Features",
                "OrchardCore.Recipes",
                "OrchardCore.Scripting"
            };

I guess we should remove this hardcode

[jtkech]

Normally the feature ordering depends on features priority and dependencies in manifest files, so maybe some missing ones.