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.1k stars 414 forks source link

[Bug]: handleRefreshRetry potentially not working under certain circumstances #1930

Closed jhpetersen closed 2 weeks ago

jhpetersen commented 3 weeks ago

Version

17.0.0

Please provide a link to a minimal reproduction of the bug

No response

Please provide the exception or error you saw

In our hybrid mobile App (Ionic 6, Angular 15, Cordova Android 12) the silent refresh breaks on first occuring http error if no network is available (disconnected manually via Chrome Dev Tools after successful initial login flow). No further retries are done.

"silent renew failed!" on "net::ERR_INTERNET_DISCONNECTED" http error gets logged.

Steps to reproduce the behavior

As i don't understand/know the conditions/circumstances under which this specific issue happens (see additional description below for further details), i can not provide any reproduction steps - just used a standard code flow + PKCE.

A clear and concise description of what you expected to happen.

Looking at `handleRefreshRetry()` function within code-flow-callback-handler.service.ts and refresh-token-callback-handler.service.ts, i would expect the refresh gets retried after a network http error.

Additional context

Debugging into handleRefreshRetry() function reveals that error.error instanceof ProgressEvent evaluates to false (!), although error.error.constructor.name === 'ProgressEvent' evaluates to true. So the restart timer doesn't start and the error gets rethrown.

I am using the following config:

    {
        // sensitive options left out...
        scope: 'openid profile offline_access',
        responseType: 'code',
        silentRenew: true,
        useRefreshToken: true,
        renewTimeBeforeTokenExpiresInSeconds: 30,
        tokenRefreshInSeconds: 8,
        refreshTokenRetryInSeconds: 8,
        triggerAuthorizationResultEvent: true,
        ignoreNonceAfterRefresh: true,
        autoUserInfo: true,
        logLevel: LogLevel.Debug
    }

Perhaps this could be addressed by modifying handleRefreshRetry() function according to Angulars documentation on how to check for network errors? Something like:

handleRefreshRetry(errors, config) {
        return errors.pipe(mergeMap((error) => {
            // retry token refresh if there is no internet connection
            if (error &&
                error instanceof HttpErrorResponse &&
                (
                    error.status === 0 ||
                    (error.error instanceof ProgressEvent &&
                     error.error.type === 'error')
                ) {
                const { authority, refreshTokenRetryInSeconds } = config;
                const errorMessage = `OidcService code request ${authority} - no internet connection`;
                this.loggerService.logWarning(config, errorMessage, error);
                return timer(refreshTokenRetryInSeconds * 1000);
            }
            return throwError(() => error);
        }));
    }
jhpetersen commented 2 weeks ago

Ok, i am closing this issue as it's apparently not really a bug of this library. Turns out cordova-plugin-file is causing this by doing bad bad bad monkey patching

Nevertheless i would suppose to think about my proposal to loosen or extend the error check to status === 0..