Fix auth MS Graph React sample #750
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 4, 2024
AlexJerabek
reviewed
Apr 4, 2024
AlexJerabek
requested changes
Apr 5, 2024
Samples/auth/Office-Add-in-Microsoft-Graph-React/login/login.ts
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/login/login.ts
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/login/login.ts
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/login/login.ts
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/logout/logout.ts
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/logout/logout.ts
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/src/components/App.tsx
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/src/components/App.tsx
Outdated
Show resolved
Hide resolved
Samples/auth/Office-Add-in-Microsoft-Graph-React/src/components/App.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Alex Jerabek <[email protected]>
Co-authored-by: Alex Jerabek <[email protected]>
AlexJerabek
approved these changes
Apr 16, 2024
codexeon
reviewed
Apr 25, 2024
| server: { | ||
| type: 'https', | ||
| options: { | ||
| cert: certPath + 'localhost.crt', |
There was a problem hiding this comment.
usually use path.resolve for paths
codexeon
reviewed
Apr 25, 2024
| auth: { | ||
| clientId: 'YOUR APP ID HERE', | ||
| authority: 'https://login.microsoftonline.com/common', | ||
| redirectUri: 'https://localhost:3000/login/login.html' // Must be registered as "spa" type. |
There was a problem hiding this comment.
${window.location.origin}/login/login.html will still work after publish to server
codexeon
reviewed
Apr 25, 2024
| }, | ||
| cache: { | ||
| cacheLocation: 'localStorage', // Needed to avoid a "login required" error. | ||
| storeAuthStateInCookie: true // Recommended to avoid certain IE/Edge issues. |
There was a problem hiding this comment.
don't think this is still necessary
codexeon
reviewed
Apr 25, 2024
| Office.context.ui.messageParent(JSON.stringify({ status: 'success', token: response.accessToken, userName: response.account.username})); | ||
| } else { | ||
| // A problem occurred, so invoke login. | ||
| pca.loginRedirect({ |
There was a problem hiding this comment.
this is a Promise, may want to await it so that the catch block could work properly if error.
codexeon
reviewed
Apr 25, 2024
| }, | ||
| cache: { | ||
| cacheLocation: 'localStorage', // Needed to avoid a "login required" error. | ||
| storeAuthStateInCookie: true // Recommended to avoid certain IE/Edge issues. |
There was a problem hiding this comment.
I think you can remove this setting
codexeon
reviewed
Apr 25, 2024
| displayError: (x: string) => void) => { | ||
| setToken: (x: string) => void, | ||
| setUserName: (x: string) => void, | ||
| displayError: (x: string) => void) => { | ||
|
|
||
| setState({ authStatus: 'loginInProcess' }); | ||
|
|
||
| await Office.context.ui.displayDialogAsync( |
There was a problem hiding this comment.
displayDialogAsync does not return a promise, await here does not do anything.
codexeon
reviewed
Apr 25, 2024
|
|
||
| const processLoginMessage = (arg: {message: string, origin: string}) => { | ||
| const processLoginMessage = (arg: { message: string, origin: string }) => { | ||
|
|
||
| let messageFromDialog = JSON.parse(arg.message); |
There was a problem hiding this comment.
best to check origin == window.location.origin before trusting the message
There was a problem hiding this comment.
added check and throws error if not correct.
codexeon
reviewed
Apr 25, 2024
|
|
||
| // You can select which account application should sign out. | ||
| const logoutRequest = { | ||
| account: messageFromParent.userName, |
There was a problem hiding this comment.
account property is AccountInfo, not a string. I don't think this will be used, it may even cause issues.
codexeon
approved these changes
Apr 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's in this Pull Request?
MSAL was old and sample wasn't completely working. I upgraded to MSAL browser 3.7.1 and fixed several issues.