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

[Feature Request] Rename AddSignIn to AddMicrosoft etc ... #193

Closed
jmprieur opened this issue Jun 10, 2020 · 6 comments
Closed

[Feature Request] Rename AddSignIn to AddMicrosoft etc ... #193

jmprieur opened this issue Jun 10, 2020 · 6 comments

Comments

@jmprieur
Copy link
Collaborator

jmprieur commented Jun 10, 2020

Is your feature request related to a problem? Please describe.

ASP.NET core ships with several authentication provides (Google, Twitter, MicrosoftAcount (meaning MSA), Facebook).
https://docs.microsoft.com/en-us/aspnet/core/security/authentication/social/google-logins?view=aspnetcore-3.1#multiple-authentication-providers

As Microsoft.Identity.Web is going to be available with the default ASP.NET Core 5.0 templates, the request from the ASP.NET Core team is to rename AddSignIn, AddProtectedWebApi, etc ... to something containing Microsoft, for instance AddSignInWithMicrosoft or AddMicrosoftSignIn etc ...

Describe the solution you'd like

Go with option 3 (below): use Microsoft

Current method Proposed renamed method
services.AddSignIn services.AddMicrosoft
services.AddAuthentication().AddSignIn(); services.AddAuthentication().AddMicrosoft();
AddProtectedWebApi AddMicrosoftProtectedWebApi
AddWebAppCallsProtectedWebApi don't change
AddProtectedWebApiCallsWebApi don't change

Describe alternatives you've considered

Option 1: use MicrosoftIdentityPlatform

Ideally we'd want to mention the Microsoft identity platform, but this is too long.

Option 2: use AzureAD

But:

  • this cause cause confusion with the old AzureAD.UI nuget package
  • Microsoft.Identity.Web also supports Azure AD B2C (and obsolete AzureADB2C.UI)

Option 3: use Microsoft

AddSignIn on the services => AddMicrosoftSignIn
AddSignIn on the AuthenticationBuilder could become just AddMicrosoft (would be like the other identity providers, AddGoogle as explained here)
AddProtectedWebApi => AddMicrosoftProtectedWebApi or AddProtectedWebApiWithMicrosoft

For the methods involving incrementally adding the ability to call downstream APIs, the question is to keep their current name, or also add Microsoft.
AddWebAppCallProtectedWebApi => AddMicrosoftWebAppCallProtectedWebApi
AddProtectedWebApiCallsWebApi => AddMicrosoftWebApiCallsWebApi

Work to do

  • choose the right option, or confirm.
  • Create a new file where we put the old method names with the obsolete attribute as a warning, and which which call the new method names. This is to have them all at one location, and not pollute our files. For instance WebAppObsolete.cs and WebApiObsolete.cs
  • the obsolete attributes will be warnings only, and will redirect to the right method to use. This way, this is not a complete breaking change. We don't want customer using the NuGet package to be pissed off.
  • the wiki pages need to be updated
  • the scenario landing pages need to be changed.
  • the tests and the 3 samples need to be changed.
@jmprieur jmprieur added enhancement New feature or request API-breaking-change labels Jun 10, 2020
@jmprieur jmprieur added this to the 0.1.6-preview milestone Jun 10, 2020
@jmprieur jmprieur changed the title [Feature Request] Rename AddSignIn to AddMicrosoftSignIn [Feature Request] Rename AddSignIn to AddMicrosoftSignIn etc ... Jun 10, 2020
@jmprieur jmprieur modified the milestones: 0.1.6-preview, 0.2.0-preview Jun 10, 2020
@jmprieur jmprieur changed the title [Feature Request] Rename AddSignIn to AddMicrosoftSignIn etc ... [Feature Request] Rename AddSignIn to AddMicrosoft etc ... Jun 10, 2020
@pmaytak
Copy link
Contributor

pmaytak commented Jun 10, 2020

I agree with your opinions on all options. I like AddMicrosoftSignIn and AddMicrosoftProtectedWebApi; I think it's the right amount of specificity. I'm not a huge fan of AddMicrosoft, AddGoogle, etc. I think it's too ambiguous - AddMicrosoftWhat?. I'd rather have AddMicrosoftSignIn or AddMicrosoftAuthentication but for the sake of consistency AddMicrosoft is fine.

@blowdart
Copy link

AddGoogle. AddFacebook et al are the built in ones.

Because it hangs off services.AddAuthentication().AddProvider it makes some sense

services.AddSignIn is weird, we don't generally have that, we hang them off .AddAuthentication

@pmaytak
Copy link
Contributor

pmaytak commented Jun 10, 2020

Because it hangs off services.AddAuthentication().AddProvider it makes some sense

Yep, chaining off AddAuthentication makes it clearer.

services.AddSignIn is weird, we don't generally have that, we hang them off .AddAuthentication

I mentioned this pattern in #90 - if we should use AuthenticationBuilder instead of IServiceCollection.

@Tratcher
Copy link

On #90 I proposed removing services.AddSignIn and only using services.AddAuthentication().Add*();. The same goes for each of the other methods like AddProtectedWebApi.

How about services.AddAuthentication().AddMicrosoftWeb()? It clues the name of the library, Microsoft, and the cloud component without being specific about AAD, B2C, consumer, common, etc..

Before After
AddSignIn AddAuthentication().AddMicrosoftWeb()
AddProtectedWebApi AddAuthentication().AddMicrosoftWebApi()
AddWebAppCallsProtectedWebApi AddAuthentication().AddMicrosoftWebAppToApi()
AddProtectedWebApiCallsWebApi AddAuthentication().AddMicrosoftWebApiToApi()

@jennyf19
Copy link
Collaborator

jennyf19 commented Jun 11, 2020

@Tratcher nice suggestions. I think just starting w/ .AddMicrosoft() would work for sign-in, and I could include "calls" instead of "to" to avoid possible confusion on the others.
AddAuthentication().AddMicrosoftWebApi()
AddAuthentication().AddMicrosoftWebAppCallsApi()
AddAuthentication().AddMicrosoftWebApiCallsApi()

fyi: @kalyankrishna1 @TiagoBrenck

@jennyf19
Copy link
Collaborator

Included in 0.2.0-preview release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants