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

loginPopup doesn't properly close itself #174

Closed
dfederm opened this issue Nov 4, 2017 · 28 comments
Closed

loginPopup doesn't properly close itself #174

dfederm opened this issue Nov 4, 2017 · 28 comments

Comments

@dfederm
Copy link

@dfederm dfederm commented Nov 4, 2017

Using version 0.1.3

Code:

let microsoftApp = new UserAgentApplication("my-app-id", null, null);
microsoftApp.loginPopup(["openid", "email"])
    .then(idtoken => /* do something with the id token */)
    .catch(error => /* do something with the error */);

Expected behavior:
A login popup pops up and after the user logs in the pop up closes itself and the loginPopup promise resolves (or rejects).

Actual behavior:
A login popup pops up and after the user logs in the pop up redirects back to my site in the pop up.

image

@dfederm
Copy link
Author

@dfederm dfederm commented Nov 4, 2017

I think I found a workaround. If I create a UserAgentApplication on that redirected page, it closes the popup and works as I expect it to.

It seems like a weird requirement to require the redirect. This is an SPA and I only need the id token (to auth with my application). Both the Google and Facebook JS libraries can handle this scenario without requiring a redirect in the pop up.

Loading

@rohitnarula7176
Copy link
Contributor

@rohitnarula7176 rohitnarula7176 commented Nov 6, 2017

@dfederm This is by design. All the tokens we receive are in the form of a redirect response on the redirectURI page and you need an instance of userAgentApplication on that page for MSAL to process that response and close the popup. Closing this issue as this is by design.

Loading

@dfederm
Copy link
Author

@dfederm dfederm commented Nov 7, 2017

@rohitnarula7176 that's extremely inconventient. An SPA has to stand up a new endpoint simply to include the msal lib again so it can close itself?

The Google and Facebook JS libraries can do it, why can't msal?

Loading

@volosovich
Copy link

@volosovich volosovich commented Jul 25, 2018

I agree with @dfederm it's extremely inconventient.
What problem do you fix with this design?
All oAuth providers sent token to promise resolve and close popup. It's default behavior for all SPA's

Loading

@eugeniymatkovskiy
Copy link

@eugeniymatkovskiy eugeniymatkovskiy commented Jul 27, 2018

I encountered the problem to implement this library to my application because of inappropriate behaviour.
It's time consuming due to its inflexibility. I couldn't initialize the library after AJAX request - wtf???
This behaviour is inconsistence with usual flow (like in Google and Facebook), so I have to add some hacks to my application

Loading

@chetanku
Copy link

@chetanku chetanku commented Aug 1, 2018

@dfederm how did you fix this problem? new to this, why do we have to create UserAgentApplication again in the redirecte URI? how do we do it?

Loading

@dfederm
Copy link
Author

@dfederm dfederm commented Aug 1, 2018

@chetanku my workaround was just to load the entire SPA in the redirect, and the SPA creates a UserAgentApplication at app startup if the hash include "id_token". It's pretty terrible and I only have to do this workaround for MSA log in support; Facebook and Google's libraries work perfectly.

    // This is weird but Microsoft login redirects back to the site and expects it to create the object before it closes the popup.
    // See https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/174
    if (location.hash.includes("id_token")) {
      new UserAgentApplication("4ecf3d26-e844-4855-9158-b8f6c0121b50", null, null);
    }

Loading

@chetanku
Copy link

@chetanku chetanku commented Aug 1, 2018

@dfederm Thank you so much, this is very weird. Hope Microsoft fixes this soon.

Loading

@volosovich
Copy link

@volosovich volosovich commented Aug 7, 2018

wow, it's undocumented behavior.
I need to have id_token in URL and after create UserAgentApplication I can get access token, save it and close modal.
I think, this information need to be added to MSAL wiki pages.

Loading

@neural22
Copy link

@neural22 neural22 commented Aug 30, 2018

This behaviour is absolutely incomprehensible, by definition the authentication process is completed when the token is returned by the service, so why should we need to instantiate an other UserAgentApplication just to avoid cancel operation error closing the popup? It does not make sense at all...

Loading

@mrlubos
Copy link

@mrlubos mrlubos commented Sep 6, 2018

Hi @rohitnarula7176, I am trying to understand why the popup would not get closed after authentication on its own. Is there an application that uses this kind of workflow you could reference? I cannot imagine where this design would be more convenient than the traditional implementation. Thanks!

Loading

@jcabrito
Copy link

@jcabrito jcabrito commented Oct 25, 2018

// This is weird but Microsoft login redirects back to the site and expects it to create the object before it closes the popup.
// See #174
if (location.hash.includes("id_token")) {
new UserAgentApplication("4ecf3d26-e844-4855-9158-b8f6c0121b50", null, null);
}

Where exactly do i need to put this code in reactjs?

Loading

@Ethan-Arrowood
Copy link

@Ethan-Arrowood Ethan-Arrowood commented Nov 13, 2018

@jcabrito i added it to my componentWillMount on the root react component

Loading

@blindfish3
Copy link

@blindfish3 blindfish3 commented Feb 1, 2019

Wish I'd found this a few days ago: now the incomprehensible and undocumented behaviour is explained.
+1 to at least document this.

Loading

@russellr922
Copy link

@russellr922 russellr922 commented Feb 7, 2019

instantiating UserAgentApplication twice results in the cookie length issue:

#534

Loading

@kroska
Copy link

@kroska kroska commented Mar 8, 2019

@chetanku my workaround was just to load the entire SPA in the redirect, and the SPA creates a UserAgentApplication at app startup if the hash include "id_token". It's pretty terrible and I only have to do this workaround for MSA log in support; Facebook and Google's libraries work perfectly.

    // This is weird but Microsoft login redirects back to the site and expects it to create the object before it closes the popup.
    // See https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/174
    if (location.hash.includes("id_token")) {
      new UserAgentApplication("4ecf3d26-e844-4855-9158-b8f6c0121b50", null, null);
    }

tks!!

Loading

@Danieliverant
Copy link

@Danieliverant Danieliverant commented May 5, 2019

Using Angular 7 - the Angular wrapper not compatible so I have to use the vanilla JS library.
And I'm facing the same issue.

I've created a service guard that do all the authentication process and on success redirect the app (using Angular Router) to the main page.

How can I use @chetanku workaround to close the popup?
where should I insert the code snippet?

edit: I tried to subscribe to the router events in the component the renders after the redirect and creating a new instance of Msal.UserAgentApplication as suggested but it just opens new windows.

Loading

@Apollinaire
Copy link

@Apollinaire Apollinaire commented May 13, 2019

Not working for me either, on React + msal 1.0.0. I tried the fix suggested above, it did not work. Adding a listener on the popup isn't that hard, and all the other libs do it so it's an expected behaviour and there are plenty of open source examples for the implementation.

Loading

@sameerag
Copy link
Member

@sameerag sameerag commented May 14, 2019

@Apollinaire Can you please send us the sample code that you have used with your react application? For a native JS application, I have tested and the popup closes by itself. Repro steps can help us understand your issue.

@Danieliverant Is your issue with Angular 7? For angular, we have not yet upgraded our current wrapper with the latest msal 1.0.0, our roadmap will be soon updated with potential dates. Having said that, if you believe the core msal is causing the issue, please share the sample code and steps to repro,

Loading

@Apollinaire
Copy link

@Apollinaire Apollinaire commented May 14, 2019

I found the issue: so the workaround is actually to create the AuthProvider as a global, and not in React. If you do this it won't work:

class Msal extends React.Component {
  componentWillMount() {
    const authProvider = new UserAgentApplication(msalConfig);
    this.setState({ authProvider })
  }
  render() {
    // ...
  }
}

But this setup works:

const authProvider = new UserAgentApplication(msalConfig);
class Msal extends React.Component {
  componentWillMount() {
    this.setState({ authProvider })
  }
  render() {
    // ...
  }
}

This still feels like a hack and should not be 'per design'. The good design would be to track the popup's location and resolve the promise when the token appears in the hash.
But anyway, the lib does not support response_type=code so it's not suitable for me.

Loading

@DarylThayil
Copy link
Contributor

@DarylThayil DarylThayil commented May 14, 2019

@Apollinaire thanks for sharing, I think that the setup code you provided that works is not necessarily a hack, but understand the frustration around no set pattern for react.
https://github.com/AzureAD/microsoft-authentication-library-for-js/wiki#roadmap
React is on our roadmap, and we hope to have a wrapper that abstracts all of this away.

We are also working on ideas for cleaner ways to resolve popups, but need to do some work to decouple the flows first. Right now the code handling redirect, popup and silent all share some code paths and for us to change one, we first need to separate those code paths.

I know its not a great answer today, but we definitely understand your pain and are working to make things easier moving forward.
Any thoughts or contributions are appreciated.

Loading

@Apollinaire
Copy link

@Apollinaire Apollinaire commented May 15, 2019

@DarylThayil Thanks for your answer. It's good to know that the maintainers intends to improve for SPAs too.
Do you ever plan on supporting the authorization_code too? I think the endpoint V2 supports it: https://docs.microsoft.com/en-us/graph/auth-v2-user#authorization-request

Loading

@DarylThayil
Copy link
Contributor

@DarylThayil DarylThayil commented May 15, 2019

@Apollinaire we absolutely do. Currently, our plan is that our node.js lib will support it, although that does not help SPA. We have used the implict flow on the web because of previous recommendations by the Oauth spec , but from what I understand those winds have shifted and auth_code is now safe to use on the web. This requires some investigation on our end and I don't have a solid answer yet, but do check our roadmap from time to time and we will put it there.

Loading

@oehm-smith
Copy link

@oehm-smith oehm-smith commented May 24, 2019

For the record, in Angular7 app using MSAL I have:

import {Component, OnInit} from '@angular/core';
import {UserAgentApplication} from 'msal';

@Component({
    selector: 'app-login',
    templateUrl: './login.component.html',
    styleUrls: ['./login.component.scss']
})
export class LoginComponent implements OnInit {
    ngOnInit() {
        console.log(`login component`);
        // necessary - https://github.com/AzureAD/microsoft-authentication-library-for-js/issues/174
        new UserAgentApplication(
            {
                auth: {
                    clientId: 'xxx',
                }
            });
    }
}

This component is hit as part of the redirectUri.

Loading

@NateJBeck
Copy link

@NateJBeck NateJBeck commented Sep 9, 2019

For those using Angular 6+ looking to implement the workaround from @dfederm, my suggestion would be to subscribe to NavigationStart Router events in a root level component like AppComponent.

This should eliminate the need to create a separate route just to handle the popup close.

const MSAL_CONFIG = {
  auth: {
    clientId: 'xxx-xxx-xxx...'
  }
}

export class AppComponent {
  constructor(
    private router: Router,
  ) {
    this.router.events.subscribe(event => {
      if (event instanceof NavigationStart) {
        if (location.hash.includes("id_token")) {
          // this will close MS login popup
          new UserAgentApplication(MSAL_CONFIG);
        }
      }
   }
}

Note: I had timeout issues with the fetch() implementation seen in this library's documentation - after I started using Angular's native HttpClient (which I prefer anyway) I did not see any more timeouts.

Loading

@theloserbm
Copy link

@theloserbm theloserbm commented Oct 16, 2019

If you need to use access token at some point you'll need to add that as well.

if (location.hash.includes("id_token") || location.hash.includes("access_token"))

Loading

@germain-italic
Copy link

@germain-italic germain-italic commented Nov 7, 2019

I am closing the popup like this:

if (location.hash.includes("id_token")) {
    open(location, '_self').close()
    window.close()
}

Not sure if it's legal, but it works.

Loading

@gauriz
Copy link

@gauriz gauriz commented Jul 30, 2020

There's the same issue I'm facing with 'Login with Github' option too!

Loading

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet