Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Sign out functionality #91

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

ibersanoMS
Copy link
Collaborator

No description provided.

@ibersanoMS ibersanoMS temporarily deployed to e2e February 13, 2023 19:57 — with GitHub Actions Inactive
@ibersanoMS ibersanoMS temporarily deployed to e2e February 13, 2023 20:39 — with GitHub Actions Inactive
@ibersanoMS ibersanoMS temporarily deployed to e2e February 14, 2023 20:01 — with GitHub Actions Inactive
@ibersanoMS ibersanoMS temporarily deployed to e2e February 22, 2023 22:38 — with GitHub Actions Inactive
@ibersanoMS ibersanoMS requested a review from jonlester February 24, 2023 18:05
@@ -32,7 +32,7 @@
<a class="nav-link" href="/Graph">Graph Query</a>
</li>
<li class="nav-item">
<a class="nav-link" href="/Anonymous" onclick="alert('Please open a GitHub issue if you need a sign out example.')">Sign Out</a>
Copy link
Member

Choose a reason for hiding this comment

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

/Anonymous is a valid path that we should keep. I'm not sure why it was labeled as "Sign-Out", but we should keep it as <a class="nav-link" href="/Anonymous">Anonymous</a> and Sign-out would be a separate nav link.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a button on line 26 for Anonymous. I've got one for signout and one for Anonymous

@@ -236,6 +242,19 @@ public async Task HandleAuth(HttpContext context)

}

public async Task HandleLogout(HttpContext context)
{
EasyAuthState state = context.EasyAuthStateFromHttpContext();
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary. Can remove

context.Response.Cookies.Delete(Constants.CookieName);

// Re route the user to Azure AD to logout
await context.SignOutAsync(OpenIdConnectDefaults.AuthenticationScheme, new AuthenticationProperties { RedirectUri = _configureOptions.DefaultRedirectAfterSignin }) ;
Copy link
Member

Choose a reason for hiding this comment

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

We should allow the default redirect uri to be overridden by reading the Contstants.RedirectParameterName query param. We should also allow the literal value of "_blank" to be provided, which would not do a redirect at all, and just leave you at the AAD signout page. This would be the case when there is no path that allows anonymous access. I'm also wondering if we should use a value other than the DefaultRedirectAfterSignin, and render our own razor page after signing out. I need to think about that.

Copy link
Member

@jonlester jonlester left a comment

Choose a reason for hiding this comment

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

See inline comments

-Moved signout out of the middleware to allow the OIDC handler to do it's job
- added configuration option for default page to arrive at after sign-out, ability to override with a redirect url parameter, and added a basic page that renders if the user is signed out but there's no option for an anonymous access page to send them back to
- preserve the login_hint claim in the easyauth cookie so that we can use the "logout_hint" protocol extension to log the user out of a specific account.  Added the optional claim to the app manifest creation.
- updated docs.
- TODO: unit tests
@jonlester jonlester temporarily deployed to e2e February 28, 2023 22:57 — with GitHub Actions Inactive
- Test cookie signout and oidc redirect
- some doc updates
@jonlester jonlester temporarily deployed to e2e March 1, 2023 21:25 — with GitHub Actions Inactive
- moved login_hint claim out of the userInfoPayload so that it wouldn't be sent to the backend application
- updated signout tests to include parameters for logout_hint and post-logout redirect
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants