Update AccountInfo construction to include tenant-specific claims #7949
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…date new loginHint account property
There was a problem hiding this comment.
Pull Request Overview
This PR updates AccountInfo construction to be tenant-profile sensitive by adding a loginHint property and making the username property reflect tenant-specific values rather than home tenant values.
- Adds
loginHintas a direct property onAccountInfoobjects, promoting it from being accessed throughidTokenClaims - Updates
usernameto be tenant-profile specific, reflecting thepreferred_usernameof the tenant profile - Updates
TenantProfiletype to includeusernameandloginHintproperties
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/msal-common/src/account/AccountInfo.ts | Adds loginHint property to AccountInfo type and updates TenantProfile to include username and loginHint |
| lib/msal-common/src/cache/entities/AccountEntity.ts | Updates AccountEntity to store and handle loginHint property |
| lib/msal-common/src/protocol/Authorize.ts | Updates login hint extraction to check account property first before falling back to token claims |
| lib/msal-browser/src/naa/AccountInfo.ts | Adds loginHint property to NAA-specific AccountInfo type |
| lib/msal-browser/src/naa/mapping/NestedAppAuthAdapter.ts | Updates adapter to include loginHint when constructing account info |
| Test files | Updates test constants and mocks to include loginHint property in account objects |
| API review files | Updates API definitions to reflect new loginHint property |
| Change files | Adds changelog entries for the feature across packages |
| tenantId: "3338040d-6c67-4c5b-b112-36a304b66dad", | ||
| username: "AbeLi@microsoft.com", | ||
| }; | ||
| const testAccount = BASIC_TEST_ACCOUNT_INFO; |
There was a problem hiding this comment.
This is a great bit of refactoring/cleanup here. Thanks!
There was a problem hiding this comment.
Frankly if I had more time I'd dig in and refactor test account creation with the level of simplicity I did for multi-tenant support. I think we should standardize the way we set up our tests so it's more maintainable.
There was a problem hiding this comment.
Account Info should be generated from a proper test ID token basically.
There was a problem hiding this comment.
Try asking copilot to do it for you.
| isHomeTenant: tenantIdMatchesHomeTenant(tenantId, homeAccountId), | ||
| }; | ||
| } else { | ||
| return { | ||
| tenantId, | ||
| localAccountId, | ||
| username: "", |
There was a problem hiding this comment.
I am curious, when do we expect to be building a TenantProfile without any idTokenClaims? Isn't an idToken a pre-requisite?
There was a problem hiding this comment.
I don't expect this code will be reached. This is a side-effect of the calling pattern, idTokenClaims are optional because of how the ResponseHandler checks for an ID token in the server response. I do think our flow will build an account as long as client_info is included in the response, which is why it doesn't fail when the server response doesn't include ID token
Co-authored-by: Thomas Norling <thomas.norling@microsoft.com>
Added a few doc updates @tnorling |
| - As the account object's `upn` ID token claim (not recommended) | ||
|
|
||
| > [!NOTE] | ||
| > The `username` and `upn` properties are partially supported in place of the actual `login_hint` claim, but they are not recommended. Please use the `loginHint` or `idTokenClaims.login_hint` account properties if they are available. |
There was a problem hiding this comment.
Do we call out that login_hint is an optional claim that the app has to opt-in to? We should be really clear that we want folks to go enable this and use it in their application.
This PR:
usernameAccountInfo property to be tenant-profile sensitive. Any AccountInfo object returned will now reflect thepreferred_usernameof the tenant profile it represents instead of keeping the home tenantusernamecomputed value.loginHintto anAccountInfoproperty so client applications can directly reference it in the account instead of throughaccount.idTokenClaims.login_hint.