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: msal upgrade #883

Merged
merged 39 commits into from
Apr 22, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
3bc16fc
replace msal with azure/msal-browser
thewahome Mar 5, 2021
d434316
implement the authentication module
thewahome Mar 5, 2021
c9e0117
add session id to maintain session from try-it experience
thewahome Mar 8, 2021
735b038
remove variable introductions for return statements
thewahome Mar 9, 2021
26cbb09
add try-catch block when getting the token
thewahome Mar 9, 2021
dffb722
change file to util
thewahome Mar 9, 2021
ca13997
move auth files to auth folder
thewahome Mar 9, 2021
522bca6
Update src/app/services/graph-client/auth/custom-authentication-provi…
thewahome Mar 9, 2021
8e013d1
create authmodule as singleton class
thewahome Mar 9, 2021
bd5da7c
Merge branch 'feature/msal-upgrade' of https://github.com/microsoftgr…
thewahome Mar 9, 2021
80983a9
Use already exported instance of msal application
thewahome Mar 9, 2021
d765db6
move msal-service to auth module folder
thewahome Mar 9, 2021
ba89fe8
move msal-service functions to authentication module file
thewahome Mar 10, 2021
d8a47ef
rename file to AuthenticationWrapper
thewahome Mar 10, 2021
5845455
move auth to authentication module folder
thewahome Mar 10, 2021
6c6938b
move graph authentication provider closer to source
thewahome Mar 10, 2021
cc6765e
create authentication wrapper interface
thewahome Mar 10, 2021
68a210a
store home account id to identify logged in account
thewahome Mar 11, 2021
178a841
change key to guid
thewahome Mar 12, 2021
b6eee86
Update src/modules/authentication/AuthenticationWrapper.ts
thewahome Mar 12, 2021
51b96bf
trigger log in if there is no homeId
thewahome Mar 12, 2021
cbf7ff2
Merge branch 'feature/msal-upgrade' of https://github.com/microsoftgr…
thewahome Mar 12, 2021
b230e4d
clear cache to support logoutPopUp
thewahome Mar 12, 2021
d0f7ae0
add a homeaccountId to the filter
thewahome Mar 15, 2021
1f2d81a
Experiment: add environment variable
thewahome Mar 16, 2021
f844bd3
show error when consent fails
thewahome Apr 6, 2021
602958b
Merge branch 'dev' into feature/msal-upgrade
thewahome Apr 6, 2021
c8dab2b
fix lock file
thewahome Apr 6, 2021
e14ecda
fix failing msal crypto related test
thewahome Apr 7, 2021
c0a0fe4
reference correct local storage key in telemetry
thewahome Apr 7, 2021
f02ac9e
log in with interaction when consenting to new permissions
thewahome Apr 8, 2021
c209706
Merge branch 'dev' into feature/msal-upgrade
thewahome Apr 14, 2021
886d42c
bypass selecting the account when consenting to scopes
thewahome Apr 14, 2021
f269d2d
add flag to ensure we are not stuck in infinite loop
thewahome Apr 14, 2021
5884ca8
rearrange statements in function
thewahome Apr 15, 2021
57a32f8
defend against null accounts
thewahome Apr 15, 2021
467ad75
change function to show expected meaning
thewahome Apr 19, 2021
9a26ad4
specify return types and directly use constant
thewahome Apr 20, 2021
eef484b
change isConsentFlow variable name
thewahome Apr 22, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 65 additions & 11 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"version": "4.4.0",
"private": true,
"dependencies": {
"@azure/msal-browser": "2.12.0",
"@babel/core": "7.12.13",
"@babel/eslint-parser": "7.12.13",
"@microsoft/applicationinsights-react-js": "2.3.1",
Expand Down Expand Up @@ -48,7 +49,6 @@
"mini-css-extract-plugin": "0.5.0",
"monaco-editor": "0.15.6",
"monaco-editor-webpack-plugin": "1.7.0",
"msal": "1.2.1",
"node-sass": "4.14.1",
"office-ui-fabric-react": "7.121.2",
"optimize-css-assets-webpack-plugin": "5.0.1",
Expand Down
102 changes: 102 additions & 0 deletions src/app/services/graph-client/auth/authentication-module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import {
AccountInfo,
AuthenticationResult, InteractionRequiredAuthError,
PopupRequest, PublicClientApplication, SilentRequest
} from '@azure/msal-browser';

import { geLocale } from '../../../../appLocale';
import { AUTH_URL, DEFAULT_USER_SCOPES } from '../../graph-constants';

const defaultScopes = DEFAULT_USER_SCOPES.split(' ');

export class AuthenticationModule {
Copy link

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.

Copy link
Collaborator Author

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.

Copy link
Contributor

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.

https://docs.microsoft.com/en-us/azure/active-directory/develop/v2-oauth2-auth-code-flow#request-an-authorization-code

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


private msalApplication: PublicClientApplication;

constructor(msalApplication: PublicClientApplication) {
thewahome marked this conversation as resolved.
Show resolved Hide resolved
this.msalApplication = msalApplication;
}

public getAccount(): AccountInfo | undefined {
if (this.msalApplication) {
const allAccounts = this.msalApplication.getAllAccounts();
if (allAccounts && allAccounts.length > 0) {
return allAccounts[0];

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?

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?

Copy link
Collaborator Author

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

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.

Copy link
Collaborator Author

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

}
}
return undefined;
}

public async getToken() {
const silentRequest = {
thewahome marked this conversation as resolved.
Show resolved Hide resolved
scopes: defaultScopes,
authority: this.getAuthority(),
account: this.getAccount()
};
const authResponse = await this.msalApplication.acquireTokenSilent(silentRequest);
thewahome marked this conversation as resolved.
Show resolved Hide resolved
return authResponse;
thewahome marked this conversation as resolved.
Show resolved Hide resolved
}

public async getAuthResult(scopes: string[] = [], sessionId?: string): Promise<AuthenticationResult> {
const userScopes = (scopes.length > 0) ? scopes : defaultScopes;
const silentRequest: SilentRequest = {
scopes: userScopes,
authority: this.getAuthority(),
account: this.getAccount()
};

try {
const authResponse: AuthenticationResult = await this.msalApplication.acquireTokenSilent(silentRequest);
thewahome marked this conversation as resolved.
Show resolved Hide resolved
return authResponse;
} catch (error) {
if (error instanceof InteractionRequiredAuthError || this.getAccount() === undefined) {
return this.loginWithInteraction(userScopes, sessionId);
} else {
throw error;
}
}
}

private getAuthority(): string {
thewahome marked this conversation as resolved.
Show resolved Hide resolved
// support for tenanted endpoint
const urlParams = new URLSearchParams(location.search);
let tenant = urlParams.get('tenant');

if (tenant === null) {
thewahome marked this conversation as resolved.
Show resolved Hide resolved
tenant = 'common';
}

return `${AUTH_URL}/${tenant}/`;
}

private async loginWithInteraction(userScopes: string[], sessionId?: string) {
const popUpRequest: PopupRequest = {
scopes: userScopes,
authority: this.getAuthority(),
prompt: 'select_account',
redirectUri: this.getCurrentUri(),
extraQueryParameters: { mkt: geLocale }
};

if (sessionId) {
popUpRequest.sid = sessionId;
}

try {
const authResponse: AuthenticationResult = await this.msalApplication.loginPopup(popUpRequest);
return authResponse;
} catch (error) {
throw error;
}
}

/**
* get current uri for redirect uri purpose
* ref - https://github.com/AzureAD/microsoft-authentication-library-for
* -js/blob/9274fac6d100a6300eb2faa4c94aa2431b1ca4b0/lib/msal-browser/src/utils/BrowserUtils.ts#L49
*/
private getCurrentUri(): string {
const currentUrl = window.location.href.split('?')[0].split('#')[0];
return currentUrl.toLowerCase();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { PublicClientApplication } from '@azure/msal-browser';
import { AuthenticationProvider } from '@microsoft/microsoft-graph-client';

import { AuthenticationModule } from './authentication-module';

export class CustomAuthenticationProvider implements AuthenticationProvider {

private msalApplication: PublicClientApplication;
thewahome marked this conversation as resolved.
Show resolved Hide resolved

constructor(msalApplication: PublicClientApplication) {
this.msalApplication = msalApplication;
}

/**
* getAccessToken
*/
public async getAccessToken(): Promise<string> {
try {
const authModule = new AuthenticationModule(this.msalApplication);
const authResult = await authModule.getAuthResult();
return authResult.accessToken;
} catch (error) {
throw error;
}
}
}
16 changes: 16 additions & 0 deletions src/app/services/graph-client/auth/login-type.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { LoginType } from '../../../../types/enums';
thewahome marked this conversation as resolved.
Show resolved Hide resolved

/**
* Returns whether to load the POPUP/REDIRECT interaction
* @returns string
*/

export function getLoginType() {
const userAgent = window.navigator.userAgent;
const msie = userAgent.indexOf('MSIE ');
const msie11 = userAgent.indexOf('Trident/');
const msedge = userAgent.indexOf('Edge/');
const isIE = msie > 0 || msie11 > 0;
const isEdge = msedge > 0;
return isIE || isEdge ? LoginType.Redirect : LoginType.Popup;
}
18 changes: 6 additions & 12 deletions src/app/services/graph-client/msal-agent.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
import { ImplicitMSALAuthenticationProvider } from
'@microsoft/microsoft-graph-client/lib/src/ImplicitMSALAuthenticationProvider';
import { MSALAuthenticationProviderOptions } from
'@microsoft/microsoft-graph-client/lib/src/MSALAuthenticationProviderOptions';
import { UserAgentApplication } from 'msal';
import { Configuration } from 'msal/lib-commonjs/Configuration';
import { DEFAULT_USER_SCOPES } from '../graph-constants';
import { Configuration, PublicClientApplication } from '@azure/msal-browser';

import { CustomAuthenticationProvider } from './auth/custom-authentication-provider';

function getClientIdFromWindow() {
return (window as any).ClientId;
Expand All @@ -16,17 +12,15 @@ function getClientIdFromEnv() {

const windowHasClientId = getClientIdFromWindow();
const clientId = windowHasClientId ? getClientIdFromWindow() : getClientIdFromEnv();

const configuration: Configuration = {
auth: {
clientId
},
cache: {
cacheLocation: 'localStorage',
storeAuthStateInCookie: true,
},
}
};

const options = new MSALAuthenticationProviderOptions(DEFAULT_USER_SCOPES.split(' '));
export const msalApplication = new UserAgentApplication(configuration);
export const authProvider = new ImplicitMSALAuthenticationProvider(msalApplication, options);
export const msalApplication = new PublicClientApplication(configuration);
export const authProvider = new CustomAuthenticationProvider(msalApplication);
Loading