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

@azure/msal-angular@1.0.0-alpha.0 can't log in #1184

Closed
1 of 6 tasks
jakehockey10 opened this issue Jan 2, 2020 · 20 comments
Closed
1 of 6 tasks

@azure/msal-angular@1.0.0-alpha.0 can't log in #1184

jakehockey10 opened this issue Jan 2, 2020 · 20 comments
Assignees
Labels
msal-angular Related to @azure/msal-angular package question Customer is asking for a clarification, use case or information.

Comments

@jakehockey10
Copy link

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Other... Please describe:

Browser:

  • Chrome version XX
  • Firefox version XX
  • IE version XX
  • Edge version XX
  • Safari version XX
  • All

Library version


Library version: 1.0.0-alpha.0

Current behavior

Using loginRedirect to make login request results in the following error in the console, and not able to login.

No redirect callbacks have been set. Please call handleRedirectCallback() with the appropriate function arguments before continuing.

Expected behavior

Using loginRedirect should work by redirectCallbacksSet being set to true on the UserAgentApplication / MsalService instance when the redirect callback is defined in the config object.

Minimal reproduction of the problem with instructions

  • Update to @azure/msal-angular@1.0.0-alpha.0
  • Follow steps in README.
  • Call loginRedirect
@jasonnutter jasonnutter self-assigned this Jan 2, 2020
@jasonnutter jasonnutter added the msal-angular Related to @azure/msal-angular package label Jan 2, 2020
@jasonnutter
Copy link
Contributor

jasonnutter commented Jan 2, 2020

@jakehockey10 As noted in the message, if you use the redirect functions, you need to set handleRedirectCallback to a callback function (execute on page load, everytime) which your application uses to process the result of the operation (i.e. similar to the .then on the popup operations).

Here's an example from the sample app: https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev-angular-1.0-msal-1/samples/MSALAngularDemoApp/src/app/app.component.ts#L63

Try adding that and let me know if it works.

Looks like I missed explaining this in the readme, so I'll make sure it gets added. Thanks!

@jasonnutter jasonnutter added the question Customer is asking for a clarification, use case or information. label Jan 2, 2020
@markmblc
Copy link

markmblc commented Jan 3, 2020

There seems to be issues with redirect flow in masl-angular/1.0.0-alpha, even in the provided sample code.

When disabling useHash and using loginRedirect, the id_token in the URL is not processed after redirect.

@jakehockey10
Copy link
Author

I added the call to handleRedirectCallback, and I never login. But if I click the login button again, it says login is already in progress. My issue might be related to @marqit8 's issue

@jasonnutter
Copy link
Contributor

@jakehockey10 @marqit8 I think I have an idea of what's going on, I'll work on putting a new build together and let you know when its ready, thanks!

@markmblc
Copy link

markmblc commented Jan 3, 2020

For more information, I can get redirect to work if I add handleAuthenticationResponse after handleRedirectCallback like below. But I have to have navigateToLoginRequestUrl set to false because the state is wrong after it caches the hash and redirects.

this.msal.handleRedirectCallback((authError: AuthError, authResponse: AuthResponse) => {
      if (this.msal.getAccount()) {
        console.log('Authenticated user', authResponse);
        this.router.navigateByUrl('/home');
      } else if (authError) {
        console.error('Unable to authenticate user', authError);
      }
    });
    if (window.location.hash) {
      (<any>this.msal).handleAuthenticationResponse();
    }
    if (this.msal.getAccount()) {
      this.router.navigateByUrl('/home');
    }

@jasonnutter
Copy link
Contributor

@marqit8 @jakehockey10 Please try msal@1.2.1-beta.1 and @azure/msal-angular@1.0.0-alpha.1 and let me know if it fixes redirect.

@marqit8 Once you upgrade you can remove the specific logic you added above that called handleAuthenticationResponse, as the library should call that correctly for you.

@markmblc
Copy link

markmblc commented Jan 8, 2020

Login functionality is working great but I have two other potential issues, or I am misunderstanding what is supposed to happen.

First, it doesn't seem like handleRedirectCallback is necessary as the hash is processed in the UserAgentApplication constructor. I am wondering, should the isAngular check stay in the UserAgentApplication constructor so that handleRedirectCallback processes the hash?

Second, the documentation on navigateToLoginRequestUrl is a little vague and the source is a bit confusing. I believe this is supposed to have Angular redirect back to the page that originally triggered the loginRedirect call. This works fine, but the problem is when it is being called inside MsalGuard. The canActivate function returns false so Angular never actually changes the location.href to the route before calling loginRedirect, so it always ends up back at the redirectUri.

On a related note, I have a business need to force the prompt=login query parameter in the authentication request. I started using this alpha version because msal core now has that ability. However, this doesn't help much when using MsalGuard since it handles the call to loginRedirect.

I have no problem with writing my own guard, but there is so much happening with the silent refresh and such, that creating my own could cause future updates to break it. I suggest moving some of the extra code in MsalGuard into MsalService so that it can be called from a custom guard, or allow more configuration options to control the authentication request.

I feel like getAccount() can handle the acquireTokenSilent call, or there should be a wrapper function that does both and returns a promise or something. It doesn't feel like the guard should be emitting broadcast events.

@jasonnutter
Copy link
Contributor

@marqit8 Good questions. Some thoughts:

  1. handleLoginRedirectCallback: The purpose of this function is to provide your app access to the result and errors of the redirect operations. I have noticed that it isn't useful when navigateToLoginRequestUrl is enabled (as your app gets redirect before the redirect callback can be invoked), which is potentially a bug we should fix.
  2. navigateToLoginRequestUrl: I've noticed this as well, and I'm pretty sure this is a bug that we should fix (returning from the login screen should send you to the page you were trying to access, not the redirect uri). I'll see if we can have that fixed in an upcoming version.
  3. prompt=login: You should be able to have this set on the login calls made by the guard by setting{ prompt: "login" } as extraQueryParameters.
  4. MsalGuard: Can you clarify what code you think should be moved out of the guard, and why it shouldn't emit events?

Thanks for the feedback!

@markmblc
Copy link

markmblc commented Jan 9, 2020

@jasonnutter Thanks for your response. I will just toss out my wishlist, but I understand if the changes are too significant since msal-angular is a small part of the big picture. I am sure I missed a consideration so hopefully my suggestions will at least serve as inspiration.

Instead of a broadcast service (which a comment in UserAgentApplication says is pretty much only for angular), msal.service should use an Angular EventEmitter that emits something like an AuthState object that looks like this:

   interface AuthState {
      account?: Account;
      status?: AuthStatusEnum;
      error?: string;
   }

Anyone implementing msal-angular in their app is almost certainly creating their own auth.service that pretty much does this (most likely using RxJS's BehaviorSubject instead since it is hot).

The nice thing about this approach is that the same AuthState object can be emitted over and over every time a token is silently refreshed or something. The subscribers can decide if it should take action on each emission.

handleRedirectCallback would not really be needed with this event driven approach. The msal.service or UserAgentApplication constructor can handle parsing the hash and the first call to the below refreshAccount() function will build and emit the first AuthState.

So for MsalGuard, here is the code that I suggest exist in MsalService:

  authState = new EventEmitter<AuthState>();

  // Refresh the current token and emit a new AuthState
  // Could probably do more, but keeping it brief.
  refreshAccount(): Promise<AuthState> {
    return new Promise<AuthState>((resolve) => {
      if (!super.getAccount()) {
        return resolve({
          status: AuthStatusEnum.LoginRequired,
        });
      }

      super.acquireTokenSilent({
        scopes: [this.msalConfig.auth.clientId]
      })
        .then((account: Account) => resolve({
          account: account,
          status: AuthStatusEnum.LoginSuccessful,
        }))
        .catch((error: string) => resolve({
          status: AuthStatusEnum.LoginFailed,
          error: error,
        }));
    })
      .then((authState: AuthState) => {
        this.authState.emit(authState);
        return authState;
      });
  }

MsalGuard would then just do this.

  canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): boolean | Promise<boolean> {
    this.authService.getLogger().verbose("location change event from old url to new url");
    return this.authService.refreshAccount().then((authState: AuthState) => {
      if (authState.account) {
        return true;
      }

      // Login redirect/popup code here. 

      // To address the navigateToLoginRequestUrl bug, maybe AuthenticationParameters 
      // should have a loginRequestUrl property which we can set to origin + state.url here.
      // If null, acquireTokenHelper will just do what it does now.

      return false;
});

So now I can easily write my own guard in case I want to do something different than login. My specific business need is that instead of logging in, my app should redirect to a page with a login button, then call loginRedirect() from there.

Phew, sorry for the length. Hope some of it is useful.

@jasonnutter
Copy link
Contributor

@marqit8 Thanks for that insight, definitely helpful. You're right in that I don't think this is something we would address right, in order to focus on getting the new MSAL Angular version released, but is an improvement that could be made incrementally later. I'll definitely keep it in mind, thanks!

@jasonnutter
Copy link
Contributor

Closing, as the main issued has been addressed.

@allen-huynh
Copy link

1. `handleLoginRedirectCallback`: The purpose of this function is to provide your app access to the result and errors of the redirect operations. I have noticed that it isn't useful when `navigateToLoginRequestUrl`  is enabled (as your app gets redirect before the redirect callback can be invoked), which is potentially a bug we should fix.

2. `navigateToLoginRequestUrl`: I've noticed this as well, and I'm pretty sure this is a bug that we should fix (returning from the login screen should send you to the page you were trying to access, not the redirect uri). I'll see if we can have that fixed in an upcoming version.

Hi @jasonnutter ,
I spent some time troubleshooting why having popUp: false, navigateToLoginRequestUrl: true, and in setting canActivate: [MsalGuard] on a route were redirecting me to the redirectUri instead of the protected component.
Figured it had to be a bug and tried to look for an issue for this, and I stumbled on this side conversation.
Do you have any updates on when the bug on returning to the page the user would be trying to access? Hoping it'll be fixed soon.

My current package versions:
azure/msal-angular version 1.0.0-beta.3
msal version 1.2.2-beta.0

@jasonnutter
Copy link
Contributor

@allen-huynh-at-dow I've actually been looking at this exact issue, planning to have a fix in the next version.

@kaeh
Copy link

kaeh commented Mar 23, 2020

Looking forward for a fix too, for now we are using the popup but we prefer the redirect

@jasonnutter
Copy link
Contributor

@kaeh
Copy link

kaeh commented Mar 26, 2020

I still have an error :/

my package.json have

"@azure/msal-angular": "1.0.0-beta.4",
"msal": "^1.2.2-beta2",

My app.module have

MsalModule.forRoot({
            auth: {
                clientId: "XXX-XXX-XXX",
                redirectUri: window.location.origin + "/home",
                navigateToLoginRequestUrl: true
            },
            cache: {
                cacheLocation: "localStorage"
            },
            framework: {
                isAngular: true
            }
        })

And i have this issue when i should be redirected

ERROR Error: "Uncaught (in promise): ClientConfigurationError: No redirect callbacks have been set. Please call handleRedirectCallback() with the appropriate function arguments before continuing. More information is available here: https://github.com/AzureAD/microsoft-authentication-library-for-js/wiki/MSAL-basics.

@jakehockey10
Copy link
Author

@kaeh can you add the snippet of code where you are calling handleRedirectCallback()?

@kaeh
Copy link

kaeh commented Mar 26, 2020

That's probably where i have made a mistake, because i don't call it.

By the past, when @azure/msal-angular was still in 0.0.0-alpha, i just had to declare the clientId, the redirectUri and the MsalGuard and that's all.

Now it looks like i'm missing something ?

But then why would the popup works and not the redirect ?

@jasonnutter
Copy link
Contributor

@kaeh Currently, if you use one of the redirect methods, you are required to implement this.authService.handleRedirectCallback. Thanks to #1358, this will no longer be required.

For now, you should implement that function, and the error will go away. Example: https://github.com/AzureAD/microsoft-authentication-library-for-js/blob/dev/samples/angular8-sample-app/src/app/app.component.ts#L27

@kaeh
Copy link

kaeh commented Mar 27, 2020

@jasonnutter it works, thank you :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
msal-angular Related to @azure/msal-angular package question Customer is asking for a clarification, use case or information.
Projects
None yet
Development

No branches or pull requests

5 participants