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
[All hosts](security) Update NAA article to support a walkthrough #4519
Conversation
Learn Build status updates of commit f870a4e: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
Learn Build status updates of commit d81e991: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
Learn Build status updates of commit d81e991: ❌ Validation status: errorsPlease follow instructions here which may help to resolve issue.
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
Learn Build status updates of commit 757df1c: 💡 Validation status: suggestions
docs/develop/enable-nested-app-authentication-in-your-add-in.md
For more details, please refer to the build report. Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them. For any questions, please:
|
Learn Build status updates of commit 228e4a0: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
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.
I haven't actually gone through the steps yet. I'll try to today or tomorrow.
docs/develop/enable-nested-app-authentication-in-your-add-in.md
Outdated
Show resolved
Hide resolved
docs/develop/enable-nested-app-authentication-in-your-add-in.md
Outdated
Show resolved
Hide resolved
docs/develop/enable-nested-app-authentication-in-your-add-in.md
Outdated
Show resolved
Hide resolved
Learn Build status updates of commit 8042db1: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Co-authored-by: Rick Kirkham <Rick-Kirkham@users.noreply.github.com>
Learn Build status updates of commit 5eea1b6: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Learn Build status updates of commit 8b409a7: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Learn Build status updates of commit 3dfc70f: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
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.
A few comments. One big point: having a walkthrough is a good idea, but I think it should be a separate article from the overview. If you keep it in this article, I recommend that you move it to the end of the article. As the article is structured now, the overview information comes after the walkthrough, which I think is confusing and some readers who are just looking for overview info, might think that there isn't any more information beyond the small bit that precedes the walkthrough.
@@ -41,30 +46,52 @@ Trusted broker groups are dynamic by design and can be updated in the future to | |||
|
|||
Configure your add-in to use NAA by setting the `supportsNestedAppAuth` property to true in your MSAL configuration. This enables MSAL to use APIs on its native application host (for example, Outlook) to acquire tokens for your application. If you don't set this property, MSAL uses the default JavaScript-based implementation to acquire tokens for your application, which may lead to unexpected auth prompts and unsatisfiable conditional access policies when running inside of a webview. | |||
|
|||
```JavaScript | |||
// Configuration for NAA. | |||
The following steps show how to enable NAA in the `taskpane.js` or `taskpane.ts` file in a project built with `yo office`. The code can be adapted to any project. |
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.
Can it really be adapted to any project? Would it apply to the SSO project? To the React project? The latter doesn't have a taskpane.js or taskpane.ts file.
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.
I'll work to reword that better.
docs/develop/enable-nested-app-authentication-in-your-add-in.md
Outdated
Show resolved
Hide resolved
import { PublicClientNext } from "@azure/msal-browser"; | ||
|
||
// Configuration for NAA. | ||
const msalConfig = { |
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 msalConfig const is only called in the onReady method, so why not declare it there too?
}; | ||
let accessToken = null; | ||
|
||
// TODO 2: Call acquireTokenSilent. |
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.
Let's put all the TODOs here, so they provide a kind of overview of what is going to be done. That's how we do it in other walkthroughs.
Also, there was no TODO 1, so it looks like the numbering is off.
Co-authored-by: Rick Kirkham <Rick-Kirkham@users.noreply.github.com>
Learn Build status updates of commit 744e8a5: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Learn Build status updates of commit 14337b6: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Learn Build status updates of commit 24f0145: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
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.
Approving, although I still recommend splitting off the walkthrough to a different article.
Learn Build status updates of commit 2510a1f: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Learn Build status updates of commit 5b55663: ✅ Validation status: passed
For more details, please refer to the build report. For any questions, please:
|
Reworked the code and steps so that you could follow this as a walkthrough and paste code into a yo office project and see it work.