Code+PKCE flow does not seem to support iframe-based silent refresh yet

[jeroenheijmans]

Issue #600 was closed via PR #735 but I've not been able to get this to work. I personally think it's still broken?

To reproduce my issue:

1. check out this commit in my sample repo
2. run npm ci
3. run ng serve
4. click the "login" button and log in on the demo identityserver
5. you get redirected back to the app, and are logged in
6. clear storage (to simulate you land freshly on the app in a second) - Note: you're still logged into the IDS with a session!
7. reload the page

• Result: the app "hangs" for a while as it's trying to do the iframe silent refresh, but that never properly returns a succeeded promise (as it does do on the master branch of my repository for implicit flow). You see a silent_refresh_timeout on the console as an error.
• Expected: the iframe refresh should return a promise of a successful login event.

FWIW: Here's a screenshot of the current buggy result:

[manfredsteyer]

Hi,

do you still request the offline_access scope?

Best Wishes,
Manfred

[manfredsteyer]

Just looked into your example. It seems like, you still request offline_access. I guess, you are using Identity Server, right? Identity Server always asks for a consent when this scope is requested.

I think we should mention this here [1].

Btw: Please also set useSilentRefresh to true. It's only needed in code flow b/c we have to tell the lib if we want to use code flow + silent refresh or code flow + refresh tokens.

[1] https://manfredsteyer.github.io/angular-oauth2-oidc/docs/additional-documentation/silent-refresh.html

[jeroenheijmans]

Removing offline_access did not seem to change things for me, it seems? So for me, no need to update the docs. And for my repro (where you manually initiate the silent refresh) the useSilentRefresh didn't matter either.

I now see though that indeed the Silent Refresh documentation was completely rewritten. I had missed that (and presume people migrating from 8.x and implicit flow will have too), but will update my sample accordingly.

I will admit, I don't like it personally that the silent-refresh.html now has to be so complex. If there's ever a bug with the documented code, folks are not getting a fix if they update the library as they'll have a fork of that code in their repo. I would've preferred that the silent-refresh.html would pass along as much as possible to the parent window, and have it handle the complex logic.

But this ship might've sailed? If so, this issue is not a "bug" anymore to me.

Either way, whether and how to change the "docs" is not yet clear to me, since offline_access didn't seem to affect anything for me.

Thanks for the quick response though ❤️, I will update my own sample repository shortly!

[manfredsteyer]

Thanks, @jeroenheijmans. What if we do separate commits for docs? Currently, the release notes are created out of the commit messages (e. g. feature(Xyz): abc, or: docs(Xyz): abc).

Regarding the silent-refresh.html: I don't like this either. It's needed b/c Identity Server sends an unneeded hash alongside URL parameters when doing session checks, sth like

#_=_?state=changed

If we find a better way of dealing with it, I'd be really happy to implement it.

[jeroenheijmans]

Ah yeah, that makes sense.

I didn't know the release notes got generated like that. We could slightly change the approach, but also consider this a one-off and just close the issue.

I just finalized updating my sample's master to show Code Flow and everything now works as expected. So I'm happy to just close this issue?

[jeroenheijmans]

As mentioned, the original issue I raised is now solved, You can check above-linked sample to see how it might work.