Add support for code flow silent-refresh and popup #609
Conversation
|
Fantastic! This looks like a great PR to me, I have no comments based on looking at the code. Would be good if someone can find a moment to do a functional test with these changes, for both implicit and pkce flows. I'll try to find some time, but no promises, unfortunately. |
|
These changes are working for me for a PKCE based code flow, but it would certainly be good to have somebody else verify that these work for them. I have absolutely not tested implicit mode with these, so while I'm relatively confident they still work, I'm not certain about that. It would be nice to be able to hide the deprecated options properties from intellisense, so that people are not trying to use them with code flow, and getting burned by that. Unfortunately I've found no clean way of doing that which would not break existing |
|
@manfredsteyer do you have of when this can be merged? Per @jeroenheijmans are we waiting on unit tests? |
|
@manfredsteyer when do you think this will be merged? We really need this PR. Thanks! |
KevinCathcart
force-pushed
the
code_flow_improvements
branch
from
308998c to
e5e1f7d
Compare
|
@manfredsteyer @jeroenheijmans Sorry to keep pinging about this. This is definitely a needed feature by our team. |
|
It's quiet about this :( Would really have this merged soon since it's critical in the move from implicit to code with pkce. |
| * pass the iframes hash fragment to this method. | ||
| * pass the iframes hash fragment to this method, and | ||
| * is also used by popup flows in the same manner. | ||
| * This can be used with code flow, where is must be set |
|
Any update on this? We would love to use this library but really need this feature. |
In the meantime, you can also use this library (auth code+PKCE + refresh tokens) if your authorization server follows the recommended attack mitigations for refresh tokens mentioned in https://tools.ietf.org/html/draft-ietf-oauth-browser-based-apps-04#section-8 and https://tools.ietf.org/html/draft-ietf-oauth-security-topics-13#section-4.12 |
|
@andifalk thanks for your suggestion! We're using Okta and unfortunately their UI doesn't even allow you to enable refreh_tokens if you're trying to authorize with a SPA |
@daltonwallace you could try to configure a native client instead of SPA in okta. This gives the option of authcode+PKCE and refresh tokens as well. |
|
@andifalk also a good suggestion. I actually tried that and Okta gave me an error saying the request can't come from a browser 🤦♂️ |
@daltonwallace oh that is bad. Just read that okta does Not want to support refresh tokens for spa instead of offering settings to limit the lifetime of refresh tokens |
|
I'm also very interested in this PR. Is there any reason why this isn't merged yet? |
|
Please merge this!! 💯 |
|
I hope this will be merged soon. Really need this |
|
Please merge this , we really need this as well to integrate authorisation code+PKCE with our wso2 identity server which is also not supporting refresh token grant for SPA |
| @@ -195,7 +195,7 @@ export class OAuthService extends AuthConfig implements OnDestroy { | |||
| } | |||
|
|
|||
| protected refreshInternal(params, noPrompt) { | |||
| if (this.responseType === 'code') { | |||
| if (!this.silentRefreshRedirectUri && this.responseType === 'code') { | |||
There was a problem hiding this comment.
@KevinCathcart wouldn't it also be possible to check if there is a refreshToken available, instead of checking if the silentRefreshRedirectUri ist configured?
This adds support for "prompt=none" style silent-refresh for the code flow, and popup support too since it was only a a tiny bit more work. This fixes #600 and fixes #602.
The most significant change here was altering the silent refresh process to be fully based on events rather than the legacy callbacks. After receiving an original event or the timeout signal, it will emit the correct silent_refresh event, just like the existing code did. These changes should be entirely backwards compatible, since the same events will be emitted.
LoginOptionshas been added to the code flow, since being able to pass specific values to it is required for either silent refresh or popup to work for code flow. I've only added support for thenon-deprecated properties, and not added support for
disableOAuth2StateChecksince I'm not sure when that is actually useful. (The nonce is always added to the state, and only a terribly non-conformant OIDC server would fail to return the state parameter. )The names of some of the properties are a bit misleading, since they say "Hash", but can apply to query-strings in code flow. I've not renamed them, since that would be a breaking change. I have, however, updated the JSDoc comments.