abpframework / abp

Open-source web application framework for ASP.NET Core! Offers an opinionated architecture to build enterprise software solutions with best practices on top of the .NET. Provides the fundamental infrastructure, cross-cutting-concern implementations, startup templates, application modules, UI themes, tooling and documentation.
https://abp.io
GNU Lesser General Public License v3.0
12.87k stars 3.44k forks source link

Angular UI: Refresh token error should be handled #8580

Closed mehmet-erim closed 3 years ago

mehmet-erim commented 3 years ago

When occurs an error while refreshing the token, the OAuth config should be cleared from local storage to prevent the app initialization problem.

davidzwa commented 3 years ago

Yeah can confirm that this results in pretty harsh app init, requiring to catch in a global exception handler.

Please also consider that when tenant is not found (f.e. when it is deleted, or just invalid), ABP's application-configuration (as it is the first for my app) endpoint and any subsequent API calls throw a 500 server exception. Pretty harsh and hard to recover from, albeit not impossible. Quite similar to refresh token: requires a reset in the AbpSession. Ive caught it in my app, but took quite some effort + a page reload (APP_INITIALIZER providers cannot be sequential or rebooted easily)

Can make a separate issue if you prefer, but thought it might fit better in the same solution: auth or tenant wrong? Clear localStorage and trigger NGXS/Store action to login again.

My idea: Can we work towards an NGXS Action for this? That way we can call onAction or other NGXS subscription goodies, and we can also fix any subsequent errors in our app or show a message to the user.

mehmet-erim commented 3 years ago

Hi @davidzwa

In the production, the unavailability of a tenant is a big problem and this error will not be encountered very often. Actually, you're right, any errors may occur on the initialization. We've recently added an injection token to handle the first application configuration request errors. You can see the PR.

See the usage:

// app.module.ts

providers: [
 {
      provide: APP_INIT_ERROR_HANDLERS,
      multi: true,
      useValue: () => {
        // handle error here
      },
    },
]

Is this help with your case?

davidzwa commented 3 years ago

That's brilliant! Thanks @mehmet-erim

I found this when I reseeded the tenancy table and kept the name the same, but the Guid logically changed... pretty stupid. Happens a lot in Development environment though!

davidzwa commented 2 years ago

@mehmet-erim I've tried to apply your previous APP_INIT_ERROR_HANDLERS tip and back then I couldnt get it to work in most errors/cases. I think I have good information now on why HTTP errors are not properly handled/thrown (@abp/ng-core initializer). Btw I can also make another issue if you prefer.

This is not a small problem. I've been dealing with it for ... years? now.

Problem reproduction: 1) 👍🏼 stop your .NET server (on purpose) 2) 👍🏼 run the app ensuring getInitialData is called as APP_INITIALIZER https://github.com/abpframework/abp/blob/20bde1b1fbfa78c5f6e658ed91c755e549566613/npm/ng-packs/packages/core/src/lib/core.module.ts#L156 3) 👍🏼 verify the application-configuration 504 timeout error with your network devtools (on purpose!) 4) 👍🏼 see error nicely rethrown in RestService https://github.com/abpframework/abp/blob/20bde1b1fbfa78c5f6e658ed91c755e549566613/npm/ng-packs/packages/core/src/lib/services/rest.service.ts#L50 5) 👍🏼 see error is nicely passed on in AbpApplicationConfiguration Service https://github.com/abpframework/abp/blob/20bde1b1fbfa78c5f6e658ed91c755e549566613/npm/ng-packs/packages/core/src/lib/proxy/volo/abp/asp-net-core/mvc/application-configurations/abp-application-configuration.service.ts#L12 6) ❗ see the error is not caught/thrown in ConfigStateService https://github.com/abpframework/abp/blob/20bde1b1fbfa78c5f6e658ed91c755e549566613/npm/ng-packs/packages/core/src/lib/services/config-state.service.ts#L24 7) ❗ It immediately ends up in HttpErrorInterceptor and/or GlobalErrorHandler, which is not correct in my opinion.

Conclusion: the error does not end up in the main.ts error catch, which means we cannot deal with it in the bootstrapping phase of Angular (or in other words startup hangs).

This means that the getInitialData function cannot catch the error, so it can never loop and call the APP_INIT_ERROR_HANDLERS provider array, nor can it throw the error to main.ts. https://github.com/abpframework/abp/blob/20bde1b1fbfa78c5f6e658ed91c755e549566613/npm/ng-packs/packages/core/src/lib/utils/initial-utils.ts#L41 Do you see what Im heading at? Its complex, but you should be able to see my point.

Impact I think this problem creates many possible startup problems hard to fix. These happen when they follow the same store/subject design pattern as in ConfigStateService. I didnt check as my knowledge is limited.

Possible problematic startup scenario's:

Solution Ensure errors are thrown in @abp/ng.core services like ConfigStateService called due to providers by token APP_INITIALIZER.

For example, replacing the configState.refreshAppState() of type ConfigStateService with abpConfigService.get() of type AbpApplicationConfigurationService fixes the case above concerning the error being properly thrown for me. Ofcourse, its not the best solution as now the config is not stored. https://github.com/abpframework/abp/blob/20bde1b1fbfa78c5f6e658ed91c755e549566613/npm/ng-packs/packages/core/src/lib/utils/initial-utils.ts#L32