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 Show Logged in account email or client ID for azd auth login and azd auth login --check-status #2856

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

john0isaac
Copy link
Contributor

@john0isaac john0isaac commented Oct 11, 2023

This pull request updates to the login functionality in the Azure CLI. The main changes involve improving the login status messaging and adding detailed account information after a successful login.

closes #1745

azd auth login and azd auth login --check-status with normal account

image

image

azd auth login --check-status with service principal

image

cli/azd/cmd/auth_login.go Outdated Show resolved Hide resolved
@john0isaac john0isaac requested a review from ellismg October 11, 2023 17:31
@john0isaac

This comment was marked as outdated.

Copy link
Member

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

Still a little confused on how this ends up working. From your screenshot, it looks like you did the right thing, but I'm confused on how it works with the current diff.

My belief is that when you are logged in with a service principal, the call to readUserProperties in auth/manager is going to return a user properties struct that has a nil HomeAccountID and so GetSignedInAccount will return (nil, nil). I think that will cause a panic when we try to dereference the PerferredUsername.

Were there additional commits that fixed this up that you just missed pushing? Or is my mental model on how this code works incorrect?

cli/azd/cmd/auth_login.go Outdated Show resolved Hide resolved
@john0isaac

This comment was marked as outdated.

@john0isaac john0isaac changed the title Feature Show Logged in account email azd auth login --check-status Feature Show Logged in account email azd auth login and azd auth login --check-status Oct 12, 2023
@john0isaac john0isaac changed the title Feature Show Logged in account email azd auth login and azd auth login --check-status Feature Show Logged in account email or client ID for azd auth login and azd auth login --check-status Oct 12, 2023
@john0isaac john0isaac requested a review from ellismg October 12, 2023 11:05
@microsoft-github-policy-service microsoft-github-policy-service bot added no-recent-activity identity issues with no activity and removed no-recent-activity identity issues with no activity labels May 17, 2024
@rajeshkamal5050 rajeshkamal5050 added the no-recent-activity identity issues with no activity label Aug 20, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the no-recent-activity identity issues with no activity label Aug 24, 2024
@rajeshkamal5050
Copy link
Contributor

rajeshkamal5050 commented Sep 3, 2024

@ellismg @vhvb1989 are we still looking into this enhancement and is the PR still valid?

@john0isaac can we close this and open a new one incorporating feedback?

@rajeshkamal5050 rajeshkamal5050 added no-recent-activity identity issues with no activity issue-addressed labels Sep 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the no-recent-activity identity issues with no activity label Sep 3, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added no-recent-activity identity issues with no activity and removed no-recent-activity identity issues with no activity labels Nov 8, 2024
@john0isaac john0isaac requested a review from JeffreyCA as a code owner January 8, 2025 16:48
@weikanglim
Copy link
Contributor

@john0isaac Thanks for keeping this PR alive, our maintainers will push on this to get this merged.

@SophCarp Thoughts on this from a UX perspective? I have some suggestions below:

Logged in to Azure as user@address.

For service principals, I feel like we should display name and the ID:

Logged in to Azure as Cluster Deployer (8f7d3b2e-9c5a-4f16-b4d2-1e9a3f8c5d7b).

^ For the App ID, we might benefit from grey / less emphasized text -- the italics is meant to demonstrate this.

@john0isaac
Copy link
Contributor Author

john0isaac commented Jan 11, 2025

@weikanglim can you help me with the service principle display name? as i'm not sure where to obtain it from.
I tried to gray it out like you suggested for the service principal.
image

And I changed it for the email to make it bold and use the same wording you suggested.
image

image

@vhvb1989 I applied your suggestion as it's way much better that what i did i think i was being lazy when i said to wait for Matt 😆 but internally I just wanted to avoid rework as he may have wanted something different.
Can you please re review it? Thank you so much.

@john0isaac john0isaac force-pushed the feat-show-logged-account branch from 0020bd3 to 5ccd731 Compare January 11, 2025 23:36
@john0isaac john0isaac force-pushed the feat-show-logged-account branch 2 times, most recently from 1282f52 to a9b3ae3 Compare January 11, 2025 23:52
@john0isaac john0isaac force-pushed the feat-show-logged-account branch from a9b3ae3 to 1507cc2 Compare January 11, 2025 23:59
@john0isaac john0isaac requested a review from vhvb1989 January 12, 2025 00:00
@weikanglim
Copy link
Contributor

weikanglim commented Jan 15, 2025

@john0isaac

Can you try using the claims from the access token returned by verifyLoggedIn? We currently call this in both --check-status and normal login flow.

I'd recommend not making any changes to auth -- I believe just extracting information from the claims should be enough to provide the display name. LMK if you run into issues.

I was able to print out what's available from the claims using the code below:

	claims, err := auth.GetClaimsFromAccessToken(token.Token)
	log.Printf("claims: %+v", claims)

I think claims.UniqueName is what we want -- feel free to look the relevant entra docs.

Comment on lines 381 to 404
details, err := la.authManager.LogInDetails(ctx)

// error getting user account, successful log in
if err != nil {
log.Printf("error: getting signed in account: %v", err)
la.console.Message(ctx, cLoginSuccessMessage+".")
return nil, nil
}

switch details.LoginType {
case auth.EmailLoginType:
fmt.Fprintf(la.console.Handles().Stdout, "%s as %s.", cLoginSuccessMessage, output.WithBold("%s", details.Account))
return nil, nil
case auth.ClientIdLoginType:
fmt.Fprintf(
la.console.Handles().Stdout,
"%s as (%s).",
cLoginSuccessMessage,
output.WithGrayFormat("%s", details.Account))
return nil, nil
default:
la.console.Message(ctx, cLoginSuccessMessage+".")
return nil, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this code the same as the one above?

Here's a PR with how you can use UX items to describe and re-use output in azd: john0isaac#2

Ux items let us keep track of indentation and have some single source to define the output elements that can be used from different packages.

Copy link
Contributor Author

@john0isaac john0isaac Jan 18, 2025

Choose a reason for hiding this comment

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

kinda the difference is that this only triggers on successful login from auth login while the above added lines are for auth login --check-status which have to outputs either logged in or not logged in.

Thanks for the PR i will work on it.

@john0isaac
Copy link
Contributor Author

@john0isaac

Can you try using the claims from the access token returned by verifyLoggedIn? We currently call this in both --check-status and normal login flow.

I'd recommend not making any changes to auth -- I believe just extracting information from the claims should be enough to provide the display name. LMK if you run into issues.

I was able to print out what's available from the claims using the code below:

	claims, err := auth.GetClaimsFromAccessToken(token.Token)
	log.Printf("claims: %+v", claims)

I think claims.UniqueName is what we want -- feel free to look the relevant entra docs.

For my personal account these are the claims:

{PreferredUsername: UniqueName:live.com#[email protected] GivenName:John FamilyName:Aziz MiddleName: Name: Oid: TenantId:f8cdef31-a31e-****-****-**** Subject:************ Upn: Email:[email protected] AlternativeId: Issuer:https://sts.windows.net/f8cdef31-****-****-****/ Audience:https://management.azure.com/ ExpirationTime:1737228383 IssuedAt:1737223568 NotBefore:1737223568}

For my Ambassadors (Consider it a work account.. kinda) these are the claims:

{PreferredUsername: UniqueName:[email protected] GivenName:John FamilyName:Aziz MiddleName: Name:John Aziz Oid:d27c9994-*****-*****-***** TenantId:84c31ca0-*****-*****-***** Subject:************* Upn:[email protected] Email: AlternativeId: Issuer:https://sts.windows.net/84c31ca0-************/ Audience:https://management.azure.com/ ExpirationTime:1737229048 IssuedAt:1737223774 NotBefore:1737223774}

For service principal login these are the claims:

{PreferredUsername: UniqueName: GivenName: FamilyName: MiddleName: Name: Oid:81b42351-f2f7-******-****-***** TenantId:80c273e6-*****-*****-**** Subject:******-******-*****-***** Upn: Email: AlternativeId: Issuer:https://sts.windows.net/80c273e6-*****-*****-******/ Audience:https://management.azure.com/ ExpirationTime:1737227895 IssuedAt:1737223995 NotBefore:1737223995}

There isn't a single property for the claims we can use as for the personal account it's Email and for the work account it's UniqueName and for the service principal you don't receive the client id or the service principal account id in the response nor you receive a name to identify it from the claims. i think it's safer the way we have it now I will apply victor's changes.

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

Successfully merging this pull request may close these issues.

Need a way to display current authenticated user
6 participants