-
Notifications
You must be signed in to change notification settings - Fork 88
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: msal upgrade #883
Conversation
src/app/services/graph-client/auth/custom-authentication-provider.ts
Outdated
Show resolved
Hide resolved
|
||
const defaultScopes = DEFAULT_USER_SCOPES.split(' '); | ||
|
||
export class AuthenticationModule { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how did you test that this is now doing the right thing as far as PKCE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ddyett
Frankly, I have trusted that the msal team has done the right thing. My tests were geared towards whether GE was able to sign in and out effectively into all the accounts. I may need guidance on ways to test this the PKCE way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thewahome Turn on Fiddler, you'll see a call to /authorize with &code_challenge parameter. That indicates that it is using PKCE.
You'll also see the refresh token supplied in the response to /token.
Graph Explorer and MSAL are using the auth code flow as expected. The one thing that we likely aren't doing but should do is set the state token in the initial request to /authorize and then checking on the response from /token that we are getting the same state token. This helps prevent CSRF
Please add doc for all the functions and comments for improving readability. https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html. |
if (this.msalApplication) { | ||
const allAccounts = this.msalApplication.getAllAccounts(); | ||
if (allAccounts && allAccounts.length > 0) { | ||
return allAccounts[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that the first account that is the account[0] will always be the valid or intended account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AzureAD/microsoft-authentication-library-for-js#2196
AzureAD/microsoft-authentication-library-for-js#1843
The array is not ordered. How is the current logged in account tracked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I have seen that the MSAL.js team recommends that we track this user ourselves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it guaranteed that the first account that is the account[0] will always be the valid or intended account?
Any idea about this? My concern is about picking up the wrong account. It would be a major issue if that happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah...
I can use either:
- msalApplication.getAccountByHomeId
- msalApplication.getAccountByLocalId
- msalApplication.getAccountByUsername
This means I'd have to create a way to track either of the identifiers
if (this.msalApplication) { | ||
const allAccounts = this.msalApplication.getAllAccounts(); | ||
if (allAccounts && allAccounts.length > 0) { | ||
return allAccounts[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AzureAD/microsoft-authentication-library-for-js#2196
AzureAD/microsoft-authentication-library-for-js#1843
The array is not ordered. How is the current logged in account tracked?
Good work on adding a class structure and organizing the code :) |
…der.ts Co-authored-by: DeVere Dyett <ddyett@microsoft.com>
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
export const DEFAULT_USER_SCOPES = 'openid profile User.Read'; | ||
export const DEVX_API_URL = 'https://graphexplorerapi.azurewebsites.net'; | ||
export const GRAPH_API_SANDBOX_URL = 'https://proxy.apisandbox.msdn.microsoft.com'; | ||
export const HOME_ACCOUNT_KEY = 'fbf1ecbe-27ab-42d7-96d4-3e6b03682ee4'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if this can be stored in the .env file instead
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
|
||
if (allAccounts.length > 1) { | ||
const homeAccountId = this.getHomeAccountId(); | ||
return (homeAccountId) ? msalApplication.getAccountByHomeId(homeAccountId) || undefined : undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why use msalApplication.getAccountByHomeId(homeAccountId) || undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function msalApplication.getAccountByHomeId(homeAccountId)
returns AccountInfo | null
, however the references of the getAccount()
function expects AccountInfo | undefined
. So if the msalApp returns null
I default it to undefined
so that nothing breaks
export class AuthenticationWrapper implements IAuthenticationWrapper { | ||
|
||
private static instance: AuthenticationWrapper; | ||
private isConsentFlow: boolean = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a summary explaining the role of the properties and functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nikithauc I tried to limit using comments by instead using variable and function names that could be self-documenting; Unless when documenting a major decision eg the clearCache()
function. Does this mean the properties and functions do not convey the meaning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I didn't understand what isConsentFlow
means?
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Azure Static Web Apps: Your stage site is ready! Visit it here: https://jolly-sand-0ac78c710-883.centralus.azurestaticapps.net |
Overview
Upgrade to msal 2.0
Closes #688
Notes
LogutPopUp is still in design. the default logout implementation opens a redirect flow to log someone out.
We now use a work-around to initiate a popup flow when logging out of the try-it experience. And clear the cache so that the account is logged out of the browser
Testing Instructions
Local
Online
Steps
logging in and out of:
-- MSA account
-- MS account
-- Demo tenant account
Permissions:
-- Consent to 1 scope related to a specific url (MSA/MS/Demo)
-- Consent to many scopes through the panel (MSA/MS/Demo)
Send authenticated requests to the graph