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

[Identity] update fix multiple accounts logic in IBC #32134

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

Conversation

KarishmaGhiya
Copy link
Member

@KarishmaGhiya KarishmaGhiya commented Dec 10, 2024

Packages impacted by this PR

@azure/identity

Issues associated with this PR

Fixes #28896

Describe the problem that is addressed by this PR

This PR attempts to solve two problems:

  1. During silent authentication, the silent auth should only happen if there's either an authentication record provided or if the account is saved in the token cache. Currently, the silent auth also happens if there's an account found in msal token cache, but there's no verification done to ensure that the account actually belongs to the current user.
  2. When a login hint is passed and the user has already logged in the past, currently we notice the next time the user logs in, there is no prompt for the user to log in again and silent authentication happens. If there's a prompt instead for the user to select account along with the login hint at the top of the prompt options, it becomes easier for the user to verify and understand that they have logged in with the correct email they intended to.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

  • For point 1 and 2, the other languages already follow the design.

Are there test cases added in this PR? (If not, why?)

  • This flow was verified with samples

Checklists

  • Added impacted package name to the issue description
  • Added a changelog (if necessary)

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

@KarishmaGhiya KarishmaGhiya marked this pull request as ready for review December 20, 2024 00:30

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • sdk/identity/identity/samples/v4/javascript/package.json: Language not supported
@@ -465,6 +446,7 @@ To work with multiple accounts for the same Client ID and Tenant ID, please prov
silentRequest.resourceRequestMethod = options.proofOfPossessionOptions.resourceRequestMethod;
silentRequest.resourceRequestUri = options.proofOfPossessionOptions.resourceRequestUrl;
}
await app.getTokenCache().getAllAccounts();
Copy link
Member Author

@KarishmaGhiya KarishmaGhiya Jan 3, 2025

Choose a reason for hiding this comment

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

Apparently I need to add this because of a bug in msal, until that's fixed -AzureAD/microsoft-authentication-library-for-js#7303 (comment)
@xirzec

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

Successfully merging this pull request may close these issues.

Identity: Unable to log into multiple accounts using the Token Cache
3 participants