damienbod / angular-auth-oidc-client

npm package for OpenID Connect, OAuth Code Flow with PKCE, Refresh tokens, Implicit Flow
https://www.npmjs.com/package/angular-auth-oidc-client
MIT License
1.15k stars 434 forks source link

In 15.0.4 authorizeWithPopUp acts totally different then in 15.0.3 #1737

Open majkers opened 1 year ago

majkers commented 1 year ago

Version

15.0.4

Additional context

In 15.0.3 version of library when I used authorizeWithPopUp method my application opened a popUp and after a successful login it closed it. It was ideal for me beacuse I am using two configuration and need to log in to one of them when being logged to other without any redirections. I had a problem with logout in popup but I managed to do that https://github.com/damienbod/angular-auth-oidc-client/issues/1727 But from 15.0.4 the behaviour of authorizeWithPopUp changed. Id doesn't close the popup but statys in it and acts more or less like a new window with the same application. And this is not the behavior I expect. Can You please add a method that acted like authorizeWithPopUp in 15.0.3 version?

majkers commented 1 year ago

I think there is an error in logic. Previously there was or between conditions and now it is and so for me isCurrentlyInPopup always returs false; and I can open a lot of popups

image

majkers commented 1 year ago

One more clue

image

adrian-heath commented 1 year ago

We are also currently experiencing the same issue with authorizeWithPopPp never completing and emitting from the Observable it returns. Is there any update on a fix to this issue?

I t is currently holding us back from upgrading to the latest version.

FabianGosebrink commented 1 year ago

Wasn't that fixed with the PR merged above?

adrian-heath commented 1 year ago

Ah maybe. I'll try it. For some reason 'npm outdated' was still showing 15.0.4 as the latest version for me.

Sorry for any confusion

FabianGosebrink commented 1 year ago

Easy, no problem. Let us know if you have any issues still :)

adrian-heath commented 1 year ago

@FabianGosebrink I tried v15.0.5 this morning and can unfortunately confirm that authorizeWithPopup is never emitting it's observable in 15.0.4 or 15.0.5. 15.0.3 works fine.

We are invoking the method from a popup dialog and the subscribe that closes the dialog no longer fires. Neither does it error either, but the oAuth dialog is opening correctly and closing once the user enters their login details

this._oidcSecurityService.authorizeWithPopUp(authOptionsOrNull, popupOptions).subscribe({
  next: ({ isAuthenticated, userData, accessToken, errorMessage }) => {
    this._dialogRef.close(isAuthenticated);
FabianGosebrink commented 1 year ago

So you never run into the next in your provided code?

adrian-heath commented 1 year ago

No. Neither a value or error gets emitted from the observable returned by authorizeWithPopup

FabianGosebrink commented 1 year ago

That is not good. Is it reproducible with one of our demos?

adrian-heath commented 1 year ago

I believe so, yes. I just built and ran sample-code-flow-popup and it doesn't appear loginWithPopup is causing the expected console output. I added the first console output below for clarity and it never appears in the console.

  loginWithPopup() {
    this.oidcSecurityService.authorizeWithPopUp().subscribe(({ isAuthenticated, userData, accessToken, errorMessage }) => {
      console.log('authorizeWithPopUp subscribe');
      console.log(isAuthenticated);
      console.log(userData);
      console.log(accessToken);
      console.log(errorMessage);
    });
  }

I did see there was a unit test but I suspect that it isn't waiting for the subscribe due to the absence of done in the test so it never fails

  describe('authorizeWithPopUp', () => {
    it('calls login service loginWithPopUp', waitForAsync((**done**) => {
      const config = { configId: 'configId1' };

      spyOn(configurationService, 'getOpenIDConfigurations').and.returnValue(of({ allConfigs: [config], currentConfig: config }));
      const spy = spyOn(loginService, 'loginWithPopUp').and.callFake(() => of(null));

      oidcSecurityService.authorizeWithPopUp().subscribe(() => {
        expect(spy).toHaveBeenCalledOnceWith(config, [config], undefined, undefined);
        **done**();
      });
    }));
  });
adrian-heath commented 1 year ago

Actually I rebuilt and the subscribe does fire in the sample however the trace is quite different. I am use Code flow with PKCE and this is the logging I see

login-dialog.component.ts:61 [DEBUG] 0-MarketingOps - BEGIN Authorize OIDC Flow with popup, no auth data login-dialog.component.ts:61 [DEBUG] 0-MarketingOps - Nonce created. nonce:f2e0bf20b3a50084e0a5caae38f936a371BQLpKxW login-dialog.component.ts:61 [DEBUG] 0-MarketingOps - Authorize created. adding myautostate: a77070a32f7cc85b4763fa8d2eccd46ca4GGr7trT

The sample shows this output

console-logger.service.ts:15 [DEBUG] 0-angularCodeRefreshTokens - Working with config '0-angularCodeRefreshTokens' using https://offeringsolutions-sts.azurewebsites.net console-logger.service.ts:15 [DEBUG] 0-angularCodeRefreshTokens - currentUrl to check auth with: https://localhost:4204/ console-logger.service.ts:15 [DEBUG] 0-angularCodeRefreshTokens - checkAuth completed - firing events now. isAuthenticated: false

So this may be config dependent.

FabianGosebrink commented 1 year ago

This is what I feared as well. Because we had issues with that, and they were solved. But with a specific config, the issue seems to appear again.

adrian-heath commented 1 year ago

If you are free I can show you what I am seeing in a teams call if that makes it a bit easier.

adrian-heath commented 1 year ago

@FabianGosebrink Ok. so it is happening in the sample-code-flow-popup sample for me. What I am now seeing is that it opens the popup and then redirects back to the return url inside the popup and the popup remains open showing a second instance of the sample app.

image

majkers commented 1 year ago

@FabianGosebrink I have already explained above what the problem is. You can check it. Logical or changed to logical and. Mentioned PR actually broked it. It all started after it was merged

adrian-heath commented 1 year ago

I am on the main branch which should be 15.0.5 and be working correctly? I see this for isCurrentlyInPopup and the dialog opens as expected but doesn't seem to behave correctly after logging in. Even on 15.0.5 I still see the above behavior and it never fires the observable for me. image I've got to finish for today so I'll dig in a bit more tomorrow

FabianGosebrink commented 1 year ago

@FabianGosebrink I have already explained above what the problem is. You can check it. Logical or changed to logical and. Mentioned PR actually broked it. It all started after it was merged

Yes, will take a look at the PR and revert it. But we have to make sure that we do not introduce the same issue as the PR fixed.

adrian-heath commented 1 year ago

@FabianGosebrink I spent several hours debugging the code today and I think I've found the issue. This is based on 15.0.5 code. Basically it boils down to the code in the isCurrentlyInPopup and openPopup. This is the existing code in iscurrentlyInPopup with some extra logging image When this runs inside the popup in response the the redirect it propduces the following output image popup is undefined as can be seen in the storage image The fact popup is undefined means isCurrentlyInPopup is returning fals and so this code in CheckAuthService never sends a message back to the opening window image

So I looked at openPopUp and found it was setting that storage AFTER the popup window is opened. Once I changed openPopup to set the storage before opening the popup everything started working as expected image As you can see I have set the property early and cleared it if the popup fails to open.

FabianGosebrink commented 1 year ago

First: Thank you for this. Would you like to do a PR? Then we can test this.

adrian-heath commented 1 year ago

Sure. I can do that. Do you have a branch naming policy?

FabianGosebrink commented 1 year ago

Please use your initials and the forward slash and then the name of your branch ah/<name-of-branch>. Thanks!

adrian-heath commented 1 year ago

I'm currently having issues pushing a branch up. Not yet figured out why. In the meantime here is a patch for the change fix-popup.patch

FabianGosebrink commented 1 year ago

You have to fork the repo in your GitHub Account, create the branch, and then do the PR from there.

adrian-heath commented 1 year ago

Thank you PR is created here https://github.com/damienbod/angular-auth-oidc-client/pull/1776