dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.44k stars 10.02k forks source link

AddDefaultIdentity and AddDefaultUI's future #7527

Closed Ponant closed 5 years ago

Ponant commented 5 years ago

Burn them alive 🥇 🥇 🥇

-too buggy scaffolding IdentityUI to extract the library code and a lot of post deletion and re-framing required (extra js validation files etc), too long to detail, sorry! -Docs on the matter are incomplete (different flows: MVC without identity with identity, RP without identity, with identity, etc), none gives a consistent result without reformatting -Does not allow one to use Identity without roles AND without the default UI -Has bugs with overriding (see issues on this repo)

Instead -Create AddIdentity<TUser> to avoid roles (including roles related generation on the EF side). The only option so far seems to be to use IdentityUserContext<TRUser> and use a homemade AddIdentity to just avoid roles. And if the http flow (siginManager, cookies) is an issue as part of an identity extension then why not just add them separately in startup. -If a guy is masochistic enough to accept an identity in a library with predefined bootstrap styling then give it as an option when creating the template, otherwise just give out the full code from the start.

Eilon commented 5 years ago

cc @blowdart

blowdart commented 5 years ago

So we're looking at improving scaffolding, so honestly the "too long to detail" would help a lot in trying to figure out what needs fixed.

Thing is we don't force you to use the defaults. They're just sugar. If you don't like them, if you don't want roles, then use something other than them.

@HaoK I though we had a way to avoid roles? That was a specific request for 2.0 wasn't it?

As for adding things separately in setup the UseDefault is meant to avoid knowing magic incantations to get up and running, adding separately is what it's not supported to be.

And finally, reverting back to code in templates. It's been considered. We've done user surveys, but when the results are tallied, reverting back doesn't come anywhere near up the priority list of customer requirements. Yes, you can total up the number of people saying it's awful on twitter and github, but ultimately that's rather self selecting, you always see more negative coverage than positive.

Ponant commented 5 years ago

If the majority of votes for keeping it (if I understand you well) then be it. It will be interesting which area in the world these devs are covering and if your survey covers enough different views. I find it weird and I do not see it holding on the long term, but ok.

Yes sorry for not having given details but many of them are in the issues raised in the docs, and some of them are closed but not fixed. One example is here https://github.com/aspnet/Docs/issues/8435 where you need to delete the file IdentityHostingStartup. Scaffolding also gave me the needed js files each of them pointing to src="/Identity/something...validation.js". You would just delete them in case you started with a project with account selected. Then this https://github.com/aspnet/AspNetCore/issues/6613 as a side issue but that is more about dynamics.

Regarding roles, you get an ApplicationUser that inherits from IdentityDbContext when scaffolding IdentityDbContext : IdentityDbContext<IdentityUser, IdentityRole, string> which means you get role related tables that are never populated and that you cannot populate without AddRoles. This is a hybrid solution which probably is the EF team to work on. IdentityUserContext would be more natural for the defaultui stuff, roles are absent by construction and will generate tables without roles.

The defaultui story is convoluted and the entropy is high in my opinion, you have different paths for scaffolding, so I think it will be difficult to maintain. Furthermore I do find it unnatural to mix ui with identity.

All in all I am in favor of having an extension for "no roles" and no UI complexity. AddDefaultIdentity is hard to read. All of this was an ironic but sincere advice, not a requirement because as you said you can role your own. AnAddIdentity<TUser> would parallelAddIdentity<TUser,Trole> and the rest for barbecue.

Ponant commented 5 years ago

There is even the email service injected in this default :).

Another thing, I can understand that you want to compress stuff in an extension so that people do not need to addextensions one after the other. But then somewhere in the docs one should say how the framework works, for instance: "If you want to be up and running using roles and claims you would do this" services.AddIdentity<IdentityUser, IdentityRole>() .AddEntityFrameworkStores<ApplicationDbContext>() .AddDefaultTokenProviders(); "but you could also get more granular and use " services.AddIdentityCore<IdentityUser>() .AddEntityFrameworkStores<ApplicationDbContext>() .AddDefaultTokenProviders(); "where we have no roles but where you would need to add SignInManager and cookies handlers". etc, This is not obvious from intellisense and going through the source code takes some time to grasp as you compare the different extensions. That is perhaps a lot of docs but it should help.

blowdart commented 5 years ago

Ah perfect. Yea, there's a documentation gap, as well as lag into scaffolding, plus problems with scaffolding itself, and the API. Some of these are actionable (the docs are frankly my fault), The files coming back again is known, and came up in a meeting today, the rest I'll forward around. Thank you for the details, I do appreciate it.

Ponant commented 5 years ago

Then why not asking for help from the community to fill in the docs and one from you reviews? I am thinking that a good pedagogical strategy is to step through a registration process, basically copy pasting register.cshtml.cs and commenting. Then one would include email confirmation here where you would remove the signin prior to sending email link. No need ot put sendgrid at this point. And follow up with the other flows, pwd reset etc. With this, I think the user will have a good idea on how the framework works and the intent of each api component.

But probably the biggest problem ahead for now is the username vs email uniqueness https://github.com/aspnet/AspNetCore/issues/5767 . It is hard to help for an external person in my opinion because you have all the internal requirements of MS, for instance backward compatibility.

blowdart commented 5 years ago

Closed as discussion, rather than issue