This repository has been archived by the owner. It is now read-only.

[OIDC] Investigate Music store failure with SignOut #313

Closed
HaoK opened this Issue Jun 30, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@HaoK
Copy link
Member

HaoK commented Jun 30, 2015

Something regressed with OIDC sign out, we are seeing a redirect to "/" instead of the session end from the music store test

@HaoK HaoK added this to the 1.0.0-beta6 milestone Jun 30, 2015

@HaoK HaoK self-assigned this Jun 30, 2015

@guardrex

This comment has been minimized.

Copy link

guardrex commented Jul 11, 2015

Might be related: #356

At some point, there was a breaking change, right? ... The method went from Context.Response.SignOut() to Context.Authentication.SignOutAsync. I'm also getting a redirect to "/" ... but not so much "instead of session end" as instead of the normal AAD IDP logout redirect that used to occur with .SignOut(). The user remains signed in with the IDP now, but that's a regression, right? ... or I haven't implemented something properly.

Update: I found the annoucement #283, but it doesn't help me understand if this is by design, a regression, or what to do about it either way.

@HaoK

This comment has been minimized.

Copy link
Member

HaoK commented Jul 11, 2015

@guardrex

This comment has been minimized.

Copy link

guardrex commented Jul 11, 2015

@HaoK Cool - Thanks.

The behavior can be seen in my test apps:

dnx451 version that uses the old SignOut() method: https://www.........com:8003/
Use login: ...@....onmicrosoft.com
Password:

You'll notice the redirect to AAD on SignOut() that then redirects back to the app. On the next Log In, the redirect to ADD does require fresh credentials.

dnxcore50 version that uses the new SignOutAsync() method: https://www.........com:8000/
Use login: ...@....onmicrosoft.com
Password:

You'll notice that SignOutAsync() clears the cookie/claims but there is no redirect to AAD. When you sign in again, the redirect to AAD only redirects back to the app (no credentials required) ... the user was never signed out at AAD.

Let me know if you want a repo of the app.

@HaoK

This comment has been minimized.

Copy link
Member

HaoK commented Jul 11, 2015

@guardrex

This comment has been minimized.

Copy link

guardrex commented Jul 11, 2015

I can put the full app up, but if you just wanted to see the controller, it just goes like this. I think I copied the format from the ADAL for .NET 5 sample project:

// GET: /Account/LogOut
[HttpGet]
public IActionResult LogOut(){
    if (Context.User.Identity.IsAuthenticated) {
        Context.Authentication.SignOutAsync(OpenIdConnectAuthenticationDefaults.AuthenticationScheme);
        Context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
    }
    return RedirectToAction("Index", "Home");
}

I also tried a version that went like this, but I had the same outcome:

// GET: /Account/LogOut
[HttpGet]
public async Task<ActionResult> LogOut(){
    if (Context.User.Identity.IsAuthenticated) {
        await Context.Authentication.SignOutAsync(OpenIdConnectAuthenticationDefaults.AuthenticationScheme);
        await Context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
    }
    return RedirectToAction("Index", "Home");
}
@HaoK

This comment has been minimized.

Copy link
Member

HaoK commented Jul 11, 2015

Yeah so try retuning empty result instead. That is the issue I think. You are overwriting the sign out

On Jul 11, 2015, at 4:00 PM, Luke Latham <notifications@github.commailto:notifications@github.com> wrote:

I can put the full app up, but if you just wanted to see the controller, it just goes like this. I think I copied the format from the ADAL for .NET 5 sample project:

// GET: /Account/LogOut
[HttpGet]
public IActionResult LogOut(){
if (Context.User.Identity.IsAuthenticated) {
Context.Authentication.SignOutAsync(OpenIdConnectAuthenticationDefaults.AuthenticationScheme);
Context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
}
return RedirectToAction("Index", "Home");
}

I also tried a version that went like this, but I had the same outcome:

// GET: /Account/LogOut
[HttpGet]
public async Task LogOut(){
if (Context.User.Identity.IsAuthenticated) {
await Context.Authentication.SignOutAsync(OpenIdConnectAuthenticationDefaults.AuthenticationScheme);
await Context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
}
return RedirectToAction("Index", "Home");
}

Reply to this email directly or view it on GitHubhttps://github.com//issues/313#issuecomment-120667723.

@HaoK

This comment has been minimized.

Copy link
Member

HaoK commented Jul 11, 2015

@guardrex

This comment has been minimized.

Copy link

guardrex commented Jul 11, 2015

Ok ... I'll give that a shot ... testing now ... brb

@guardrex

This comment has been minimized.

Copy link

guardrex commented Jul 11, 2015

Got IT! Thanks ... that did the trick. I'll put the answer over on the issue I opened at #356 and close that out.

Have a great weekend! ... and I'll raise a Beck's to you tonight! 🍺

@guardrex

This comment has been minimized.

Copy link

guardrex commented Jul 13, 2015

@PinpointTownes recommends in #356 (comment) to go with an await version, like this:

// GET: /Account/LogOut
[HttpGet]
public async Task LogOut(){
    await Context.Authentication.SignOutAsync(OpenIdConnectAuthenticationDefaults.AuthenticationScheme);
    await Context.Authentication.SignOutAsync(CookieAuthenticationDefaults.AuthenticationScheme);
}
@HaoK

This comment has been minimized.

Copy link
Member

HaoK commented Jul 15, 2015

Moving to beta 7 as this is just a test update

@HaoK HaoK modified the milestones: 1.0.0-beta7, 1.0.0-beta6 Jul 15, 2015

@HaoK

This comment has been minimized.

Copy link
Member

HaoK commented Jul 16, 2015

Opened issue in music store to track this, there doesn't appear to be any regression on the functional side.

aspnet/MusicStore#511

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.