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

Update authentication.notifySuccess function documentation with secure usage guidance #2685

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

AE-MS
Copy link
Contributor

@AE-MS AE-MS commented Jan 17, 2025

Description

The authentication.notifySuccess function did not explicitly discuss secure usage guidance and this PR adds it.

Separately, I am working with the docs team to get the prose documentation pages (here and here) updated to be more explicit about this.

The final docs now look like this, after feedback from @JoshuaPartlow:
{6F05D66E-FA63-42CD-B16D-DCB67BDC35E5}
This is obviously the local generation of the docs -- they will be formatted slightly differently on the official site.

@AE-MS AE-MS marked this pull request as ready for review January 17, 2025 22:59
@AE-MS AE-MS requested a review from a team as a code owner January 17, 2025 22:59
@AE-MS AE-MS requested a review from JoshuaPartlow January 17, 2025 22:59
@JoshuaPartlow
Copy link
Contributor

@AE-MS - Given the size of the info, our thought is that it might go better with the remarks section. Since result is the only parameter it doesn't seem like it would get lost there or anything. For the content itself, no major concerns, but our suggestion with a few edits would be the following:

Note: The result parameter should never contain the token that was received from the identity provider, because a malicious app (rather than your own app) might have opened the authentication window. If that was the case, passing the token in this parameter would leak it to them. More secure methods for completing the authentication flow include:

  • For a purely browser-based experience (e.g., a personal app/tab app), you could store the token in browser local storage and then have your personal app retrieve it once this notifySuccess call is received.
  • For a server-based experience (e.g., a message extension), your authentication window could store the token on your service and then generate a unique code passed via this result parameter. The caller can then use the unique code to retrieve the token from your service.

@erikadoyle as an fyi

JoshuaPartlow
JoshuaPartlow previously approved these changes Jan 23, 2025
Copy link
Contributor

@JoshuaPartlow JoshuaPartlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and provided feedback in comments.

@AE-MS AE-MS enabled auto-merge (squash) January 23, 2025 16:57
@AE-MS AE-MS merged commit 784f4db into main Jan 23, 2025
39 checks passed
@AE-MS AE-MS deleted the AE_MS/notifySuccess branch January 23, 2025 18:44
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.

4 participants