Skip to content

Templates => Auth 2.0 - #624

Merged
mlorbetske merged 8 commits into
dotnet:rel/vs2017/post-rtwfrom
HaoK:haok/auth2
Apr 21, 2017
Merged

mlorbetske merged 8 commits into
dotnet:rel/vs2017/post-rtwfrom
HaoK:haok/auth2

Conversation

@HaoK

@HaoK HaoK commented Apr 20, 2017 •

Copy link
Copy Markdown
Member

Initial PR for adding Auth 2.0, I also fixed the change to Configure[App]Configuration (cc @glennc)

I only verified that I could compile and run Individual/Single/MultiOrg.

@Tratcher @Eilon @mlorbetske @danroth27 @blowdart @ajcvickers

@mlorbetske
mlorbetske requested a review from danroth27 April 20, 2017 07:41
@HaoK HaoK mentioned this pull request Apr 20, 2017
13 tasks done
{
// Clear the existing external cookie to ensure a clean login process
await HttpContext.Authentication.SignOutAsync(_externalCookieScheme);
await HttpContext.Authentication.SignOutAsync(IdentityCookieOptions.ExternalScheme);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-Authentication?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, I'll remove

<hr />
@{
var loginProviders = SignInManager.GetExternalAuthenticationSchemes().ToList();
var schemes = await SchemeProvider.GetAllSchemesAsync();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Start by adding a manual filter for the identity cookies?

@HaoK HaoK Apr 20, 2017 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe we can start by just adding a flag to the AuthenticationScheme called IsRemote, and only set that for our Remote handlers (OAuth based + Twitter), then we can filter for Remote schemes?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're baking template logic into the framework.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well we need something, I guess for now, lets just bring back what we had, I'll add DisplayName to AuthenticationScheme and have the Auth's set it like before, and resurrect the SignInManager API for Preview 1, and we can figure out a longer term plan for Preview 2 if need

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's a lot more work than adding a temp filter right here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not really, you underestimate how many places I had to do this exact code pattern last night :)

@using Microsoft.AspNetCore.Http.Authentication
@using Microsoft.AspNetCore.Authentication
@model LoginViewModel
@inject SignInManager<ApplicationUser> SignInManager

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like SignInManager isn't used anymore - should it be removed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, will remove

@HaoK

HaoK commented Apr 20, 2017

Copy link
Copy Markdown
Member Author

Updated with the new iteration, SignInManager.GetExternal... is back, and DisplayName has been restored.

@HaoK

HaoK commented Apr 21, 2017

Copy link
Copy Markdown
Member Author

Okay I think changes are pretty much good to go, @danroth27 and I are doing final runtime verification, look ok to everyone to merge today? cc @DamianEdwards @davidfowl

@mlorbetske
mlorbetske merged commit 77db92e into dotnet:rel/vs2017/post-rtw Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants