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

[All hosts](security) Update NAA article to support a walkthrough #4519

Merged
merged 19 commits into from May 10, 2024

Conversation

davidchesnut
Copy link
Member

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.

Copy link
Contributor

Learn Build status updates of commit f870a4e:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
❌Error Details

  • [Error: CannotMergeCommit] Cannot merge commit f870a4e121cbf686c70ef3f526a4d16afaa5a1ed in branch davech-naa2 of repository https://github.com/OfficeDev/office-js-docs-pr into branch main (commit 7d2c9f3f316d349fa4fac9812c52370573330979). Please follow this documentation: https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/ to use git.exe to resolve you content conflicts locally and then push to remote.

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:

@davidchesnut davidchesnut reopened this Apr 15, 2024
Copy link
Contributor

Learn Build status updates of commit d81e991:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
❌Error Details

  • [Error: CannotMergeCommit] Cannot merge commit d81e991df8f36e90e4b805de9cdb1ecf1f028a20 in branch davech-naa2 of repository https://github.com/OfficeDev/office-js-docs-pr into branch main (commit 7d2c9f3f316d349fa4fac9812c52370573330979). Please follow this documentation: https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/ to use git.exe to resolve you content conflicts locally and then push to remote.

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:

Copy link
Contributor

Learn Build status updates of commit d81e991:

❌ Validation status: errors

Please follow instructions here which may help to resolve issue.

File Status Preview URL Details
❌Error Details

  • [Error: CannotMergeCommit] Cannot merge commit d81e991df8f36e90e4b805de9cdb1ecf1f028a20 in branch davech-naa2 of repository https://github.com/OfficeDev/office-js-docs-pr into branch main (commit 7d2c9f3f316d349fa4fac9812c52370573330979). Please follow this documentation: https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/ to use git.exe to resolve you content conflicts locally and then push to remote.

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:

Copy link
Contributor

Learn Build status updates of commit 757df1c:

💡 Validation status: suggestions

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md 💡Suggestion View Details

docs/develop/enable-nested-app-authentication-in-your-add-in.md

  • Line 222, Column 3: [Suggestion: docs-link-absolute - See documentation] Absolute link 'https://learn.microsoft.com/entra/identity-platform/sample-v2-code?tabs=apptype' will be broken in isolated environments. Replace with a relative link.

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:

Copy link
Contributor

Learn Build status updates of commit 228e4a0:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

@Rick-Kirkham Rick-Kirkham left a 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.

Copy link
Contributor

Learn Build status updates of commit 8042db1:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 5eea1b6:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 8b409a7:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 3dfc70f:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

@Rick-Kirkham Rick-Kirkham left a 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.
Copy link
Contributor

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.

Copy link
Member Author

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.

import { PublicClientNext } from "@azure/msal-browser";

// Configuration for NAA.
const msalConfig = {
Copy link
Contributor

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.
Copy link
Contributor

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>
Copy link
Contributor

Learn Build status updates of commit 744e8a5:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 14337b6:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 24f0145:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

@Rick-Kirkham Rick-Kirkham left a 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.

Copy link
Contributor

Learn Build status updates of commit 2510a1f:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

Learn Build status updates of commit 5b55663:

✅ Validation status: passed

File Status Preview URL Details
docs/develop/enable-nested-app-authentication-in-your-add-in.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

@davidchesnut davidchesnut merged commit 4791fc0 into main May 10, 2024
2 checks passed
@davidchesnut davidchesnut deleted the davech-naa2 branch May 10, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants