Silent renew with refresh tokens - handle no connection use case

[dome4]

As discussed in #985, the periodical silent renew breaks after the first failed request. Here, the no connection use case should be handeled properly. The no connection check is done according to the following instructions:

• Angular - Getting error details (https://angular.io/guide/http#getting-error-details):
• use ProgressEvent instead of ErrorEvent: Wrong error type in httpclient doc angular/angular#34748

TODO: implement retry logic

• RefreshTokenCallbackHandlerService
• CodeFlowCallbackHandlerService
• tests

Looking forward to opinions :)

[FabianGosebrink]

Looks good to me so far. Can you add tests and the open checkboxes? Let us know if you need help. Thanks, you rock!

[dome4]

@FabianGosebrink thanks for the feedback. I am currently trying to implement a fully offline capable pwa (https://github.com/dome4/angular-auth-oidc-client/commits/keycloak-pwa). So besides the retry logic, it would be helpful to be able to skip the service worker, which causes some problems in production. It would be possible to skip the auth-related requests via datagroups (https://angular.io/guide/service-worker-config#datagroups) and maxAge 0, but this comes with massive performance problems. Interceptors are also not possible because there's no way to acces the HttpClient used in the library from the app scope. Therefore, it would make sense to have the service worker skip the auth-related requests. Are there any concerns about this feature?

I am aware that I am somewhat mixing up the original meaning of the pull requests. But the feature is necessary for me to realize the offline capability.

In addition I refactored the return of the mocked ConfigurationProdivder because otherwise the tests are broken:

const { stsServer } = this.configurationProvider.getOpenIDConfiguration();

Currently, the mocked ConfigurationProdivder returns null. Trying to destruct null results in an error. That might also be the reason why the pipeline for main failed.

[dome4]

I added a simple retry logic to the requests executed in the following methods:

• AuthWellKnownDataService -getWellKnownDocument()
• SigninKeyDataService - getSigningKeys()
• LogoffRevocationService - revokeRefreshToken() & revokeAccessToken()
• UserService - getIdentityUserData()
• ParService - postParRequest()
  In my opinion, these requests don't need complex retry logic but should be executed three times in total just because connection problems might occur in production.

Furthermore, I added the same custom retry logic to the CodeFlowCallbackHandlerService like in RefreshTokenCallbackHandlerService. The tests are passing.

I'm looking forward to opinions and will start with the tests if you agree with the changes :)

[FabianGosebrink]

If this is a custom header, we have to prefix it with an x- so the header would be x-ngsw-bypass. As we use this string multiple times it would be nice to have a const in the class or the file like NGSW_CUSTOM_HEDAER = 'x-ngsw-bypass'. What do you think?

[dome4]

Defining a constant for the header / param sounds good to me 👍

The first try of this feature was using the custom header ngsw-byass. The problem is that prefixing the header with x- will not work because of the following check in the service worker nsgw-worker.js:

onFetch(event) {
            ...
            if (req.headers.has('ngsw-bypass') || /[?&]ngsw-bypass(?:[=&]|$)/i.test(requestUrlObj.search)) {
                return;
            }
            ...

So the header must be exactly ngsw-bypass. Otherwise it will be ignored. Furthermore, I experienced the error that Keycloak rejects requests containing the custom header due to a CORS error because the custom header is not in the list of the allowed headers. I tried to add it somehow to keycloak but that was not easily possible and might also cause conflicts with other identity servers.

Besides using a custom header, it's possible to use a custom param , which is what I would suggest. When using this approach a param will be appended to all auth-related requests: https://identity-server.com/example?ngsw-bypass=. and the CORS issues are gone.

I see that the description in the configuration.md is misleading and needs to be adjusted to the new approach. So my suggestion would be to define the constant NGSW_CUSTOM_PARAM = ngsw-bypass and update the description from Adds the ngsw-bypass header to all requests to Adds the ngsw-bypass param to all requests

[FabianGosebrink]

Sounds good to me. Please go with that. @damienbod ?

[dome4]

I created the tests for the new functions and added a test help to test the retry logic. Since I couldn't find any other helper, I created the file under projects/angular-auth-oidc-client/test/create-retriable-stream.helper.ts. There might be better paths so feel free to feedback and I'll update the location :)

[damienbod]

@dome4 Thanks for the PR! Thanks @FabianGosebrink for the reviewing.

LGTM

I'll merge and test today. All good we could release then.

[FabianGosebrink]

add new line after line 55

[FabianGosebrink]

add new line after 118

[FabianGosebrink]

add new line after 7

[FabianGosebrink]

This looks really good to me, three really small new lines missing. Incredible job, really good. Thank you very much. Love the way you tackled down the problem and then documented and tested it in a beautiful manner. Kudos to you!

[FabianGosebrink]

For me this is done, code looks good. @damienbod please test and if good then we can merge imho. @dome4 thanks for this amazing feature and nice code. Thank you.