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

Add onRedirectNavigate to redirect operations in browser #2669

Merged
merged 9 commits into from
Dec 7, 2020

Conversation

jasonnutter
Copy link
Contributor

@jasonnutter jasonnutter commented Nov 30, 2020

Adds onRedirectNavigate callback from v1 (#1691).

This adds a new callback for loginRedirect, acquireTokenRedirect, and logout to allow applications to get the URL that would be navigated to, and to stop navigation. This is to support scenarios where applications want to perform the redirect themselves, instead of relying on MSAL.

Usage:

msal.loginRedirect({
  onRedirectNavigate: url => {
    // url that would be navigated to
    console.log(url);
   
     // Applications must explicitly return false to stop navigation
     return false;
  }
})

Also includes a sample Chromium extension demonstrating the usage.

TODO:

  • Test with Chrome extension

@jasonnutter jasonnutter linked an issue Nov 30, 2020 that may be closed by this pull request
8 tasks
@github-actions github-actions bot added the msal-browser Related to msal-browser package label Nov 30, 2020
@coveralls
Copy link

coveralls commented Nov 30, 2020

Coverage Status

Coverage decreased (-2.8%) to 78.766% when pulling 4f595e8 on browser-onredirectnavigate into 182cbc4 on dev.

Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Could you update the request docs with this new param + maybe a usage example?

@jasonnutter
Copy link
Contributor Author

Could you update the request docs with this new param + maybe a usage example?

I will definitely add a usage example. Do we still need the standalone docs for request/response? Would be nice to have those docs fully automated (typedoc, in this case).

@tnorling
Copy link
Collaborator

tnorling commented Dec 1, 2020

Could you update the request docs with this new param + maybe a usage example?

I will definitely add a usage example. Do we still need the standalone docs for request/response? Would be nice to have those docs fully automated (typedoc, in this case).

I think the request docs are out of date anyway. I'm all for automating it if they're still easy to find (I don't know where the typedocs live today)

@github-actions github-actions bot added the samples Related to the samples apps for the library. label Dec 4, 2020
@jasonnutter
Copy link
Contributor Author

jasonnutter commented Dec 4, 2020

Could you update the request docs with this new param + maybe a usage example?

I will definitely add a usage example. Do we still need the standalone docs for request/response? Would be nice to have those docs fully automated (typedoc, in this case).

I think the request docs are out of date anyway. I'm all for automating it if they're still easy to find (I don't know where the typedocs live today)

Yeah, the request/response docs are very much out of date, and so I would propose deleting them, since the typedocs will always be update to date in theory. cc: @pkanher617

Or, just have the existing markdown files just link to the typedocs.

https://azuread.github.io/microsoft-authentication-library-for-js/ref/msal-browser/

onRedirectNavigate: (url) => {
expect(url).to.be.eq(testLogoutUrl);
done();
return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @pkanher617 this can be how apps stop the redirect from happening during logout

@tnorling
Copy link
Collaborator

tnorling commented Dec 4, 2020

Yeah, the request/response docs are very much out of date, and so I would propose deleting them, since the typedocs will always be update to date in theory. cc: @pkanher617

Or, just have the existing markdown files just link to the typedocs.

https://azuread.github.io/microsoft-authentication-library-for-js/ref/msal-browser/

Let's delete the docs and update the links to those docs to point to the typedocs?

@jasonnutter
Copy link
Contributor Author

Yeah, the request/response docs are very much out of date, and so I would propose deleting them, since the typedocs will always be update to date in theory. cc: @pkanher617
Or, just have the existing markdown files just link to the typedocs.
https://azuread.github.io/microsoft-authentication-library-for-js/ref/msal-browser/

Let's delete the docs and update the links to those docs to point to the typedocs?

Done.

@jasonnutter jasonnutter merged commit a3e1f45 into dev Dec 7, 2020
@jasonnutter jasonnutter deleted the browser-onredirectnavigate branch December 7, 2020 21:30
@bryik
Copy link

bryik commented Dec 16, 2020

This PR closed issue #2127, but I do not see how it solves the problem described by that issue.

I guess msalInstance.logout() could be provided with an onRedirectNavigate() callback that clears local storage and prevents the logout redirect from occurring e.g.

msalInstance.logout({
  onRedirectNavigate: () => {
    window.localStorage.clear();
    window.sessionStorage.clear();
    return false;
  }
});

Is that the recommended way to logout of a specific application without logging out of all apps tied to an AAD session?

imo it would be great to have this mentioned in the logout doc. I was surprised that calling logout() in a little test app signed me out of Outlook.

@jasonnutter
Copy link
Contributor Author

jasonnutter commented Dec 16, 2020

Is that the recommended way to logout of a specific application without logging out of all apps tied to an AAD session?

Yes, using onRedirectNavigate and returning false is how you can stop the logout navigation (which is what will sign you out of other applications). Note, you do not need to manually clear localStorage or sessionStorage (unless there are other non-MSAL.js things you want to remove), as MSAL.js will still clear MSAL.js entries from browser storage.

imo it would be great to have this mentioned in the logout doc. I was surprised that calling logout() in a little test app signed me out of Outlook.

Yes, I have it on my list to update that document as apart of #2546.

@bryik
Copy link

bryik commented Dec 16, 2020

Perfect, thank you @jasonnutter!

@kireet3213
Copy link

Hey there. I set onRedirectNavigate to false with updated msal-browser and msal-react packages but I fail to authenticate the user with msalInstance.handleRedirectPromise(redirectUrl). It says that there is no redirection in progress, returning null. Is there some other way to get it authenticated in this library?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-browser Related to msal-browser package samples Related to the samples apps for the library.
Projects
None yet
7 participants