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

Add User Profile Menu for OpenID Connect login #3443

Merged
merged 10 commits into from
Apr 10, 2024

Conversation

tlmii
Copy link
Member

@tlmii tlmii commented Apr 5, 2024

Resolves #3194
Resolves #3197

  • Introduces the FluentProfileMenu to the upper-right of the site header when the user is authenticated with OpenID Connect.
  • Displays the user's full name and user id (often, but not always, an email) as well as their initials.
  • Adds two config settings - Dashboard:Frontend:OpenIdConnect:NameClaimType and Dashboard:Frontend:OpenIdConnect:UsernameClaimType to allow for customization of which claims determine the values displayed in the profile menu
  • Provides a sign out button on the profile menu allowing the user to sign out - this clears the cookie and redirects them to the IdP where they can also sign out if they wish.

A couple screenshots:

image
image

TODO:

  • Figure out what the header of the profile menu should be. Currently says OpenID Connect since that's the auth mode we're in. Could maybe find a way to tie it to the name of the IdP?
  • Test with another IdP (so far: Entra ID and Keycloak)
  • Figure out if there's anything we can do (another config setting? more docs?) to make the OpenIdConnect code we use interoperate with Keycloak's logout better (right now it gets an error because post_logout_redirect_uri is passed but id_token_hint is not, which they consider an error even though the spec doesn't appear to).

Note: Testing this PR will require you to use your own Idp since this will only appear with OpenID Connect auth enabled. If you want to test it with TestShop (or other playground apps) you'll also need to disable the automatic setting of the authmode. Easiest way I could find to do that was to comment out this line:

context.EnvironmentVariables[DashboardConfigNames.DashboardFrontendAuthModeName.EnvVarName] = "BrowserToken";

But maybe there's a better way.

Microsoft Reviewers: Open in CodeFlow

/// </summary>
/// <param name="text">The string to shorten</param>
/// <param name="maxLength">The max length of the result</param>
public static string TrimMiddle(this string text, int maxLength)
Copy link
Member Author

Choose a reason for hiding this comment

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

This hasn't been used for a while, so I deleted it and the unit test cases.

FooterLink="">
<HeaderTemplate>
<FluentStack VerticalAlignment="VerticalAlignment.Center">
<FluentLabel>OpenID Connect</FluentLabel>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what I referenced in one of the TODOs in the description. We can:

  1. Leave it as-is
  2. Set it to something else constant
  3. Set it to something else constant and localize it
  4. Figure out some way to get an appropriate value from an IdP
  5. Remove it altogether

Copy link
Member

Choose a reason for hiding this comment

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

Another option is just to remove the text. I think it's pretty clear what the popup means in context:

image

::deep.layout > header fluent-button[appearance=stealth]:not(:hover)::part(control),
::deep.layout > header fluent-anchor[appearance=stealth]:not(:hover)::part(control),
::deep.layout > header fluent-anchor[appearance=stealth].logo::part(control),
::deep.layout > header > .header-gutters > fluent-button[appearance=stealth]:not(:hover)::part(control),
Copy link
Member Author

Choose a reason for hiding this comment

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

Made a bunch of these header styles more specific to the elements we were trying to impact so they wouldn't change the style of things within the FluentProfileMenu

if (string.IsNullOrWhiteSpace(_name))
{
// Make sure there's always a name, even if that name is a placeholder
_name = Loc[nameof(Login.AuthorizedUser)];
Copy link
Member Author

Choose a reason for hiding this comment

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

Open to suggestions on what, if anything, we should display in this case. I've seen this type of placeholder before so I went with it for now.

@kvenkatrajan
Copy link
Member

kvenkatrajan commented Apr 5, 2024

f igure out what the header of the profile menu should be. Currently says OpenID Connect since that's the auth mode we're in.

Can we update to "Signed in as:" Or "Profile" for now? @leslierichardson95 thoughts?

@kvenkatrajan
Copy link
Member

Looks wonderful by the way - great job 👍

@leslierichardson95
Copy link

f igure out what the header of the profile menu should be. Currently says OpenID Connect since that's the auth mode we're in.

Can we update to "Signed in as:" Or "Profile" for now? @leslierichardson95 thoughts?

@kvenkatrajan I like "Signed in as:".


namespace Aspire.Dashboard.Extensions;

public static class ClaimsIdentityExtensions
Copy link
Member

Choose a reason for hiding this comment

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

Not public

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops. Fixed.

@tlmii
Copy link
Member Author

tlmii commented Apr 8, 2024

f igure out what the header of the profile menu should be. Currently says OpenID Connect since that's the auth mode we're in.

Can we update to "Signed in as:" Or "Profile" for now? @leslierichardson95 thoughts?

@kvenkatrajan I like "Signed in as:".

Updated:

image

Sign Out being on the same line as "Signed in as:" feels a little off to me now. But it still makes more sense than what was there before I think.

@JamesNK
Copy link
Member

JamesNK commented Apr 8, 2024

FYI the browser token auth uses Logged in and Login in its UI. Consider changing text here to be consistent.

@mitchdenny
Copy link
Member

What are we doing around authorization?

@kvenkatrajan
Copy link
Member

kvenkatrajan commented Apr 8, 2024

What are we doing around authorization?

We have #808 tracking this.

@tlmii
Copy link
Member Author

tlmii commented Apr 8, 2024

FYI the browser token auth uses Logged in and Login in its UI. Consider changing text here to be consistent.

Switched to "Logged in as:"

If we can't find a way to pull the name of the IdP from somewhere, maybe we can make it an option. I'd like that better than this (and it'd be consistent with Teams, Azure Portal, etc). But I don't want to hold this PR up for that.

@mitchdenny
Copy link
Member

808 seemed related to project templates?

@kvenkatrajan
Copy link
Member

Ah its 808 on the other board: https://github.com/dotnet/aspire-private-planning/issues/808

Copy link
Member

@drewnoakes drewnoakes left a comment

Choose a reason for hiding this comment

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

Looks great.

Approving, though as I mentioned above I think the popup could be simplified:

image

src/Aspire.Dashboard/README.md Show resolved Hide resolved
@tlmii tlmii enabled auto-merge (squash) April 10, 2024 07:10
@tlmii tlmii merged commit 2a8886a into dotnet:main Apr 10, 2024
8 checks passed
@tlmii
Copy link
Member Author

tlmii commented Apr 10, 2024

/backport to release/8.0

Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8632467352

@tlmii tlmii deleted the dev/login-details branch April 11, 2024 03:18
drewnoakes added a commit to drewnoakes/aspire that referenced this pull request Apr 12, 2024
At some point during development of dotnet#3443 the configuration key was changed and these error messages were missed.
drewnoakes added a commit that referenced this pull request Apr 12, 2024
At some point during development of #3443 the configuration key was changed and these error messages were missed.
@github-actions github-actions bot locked and limited conversation to collaborators May 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add user info on dashboard views once logged in Display logout link for authenticated user
7 participants