Angular - Improve Authentication Token Handling #24050
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances authentication token handling in Angular by implementing a SharedWorker-based token storage system. The changes improve cross-tab token synchronization and move sensitive tokens from localStorage to in-memory storage for better security.
- Introduces a SharedWorker for cross-tab token synchronization
- Implements MemoryTokenStorageService to store sensitive tokens in memory instead of localStorage
- Adds error handling for token refresh operations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| token-storage.worker.ts | New SharedWorker implementation for cross-tab token storage synchronization |
| memory-token-storage.service.ts | New service that stores sensitive OAuth tokens in memory with SharedWorker support |
| storage.factory.ts | Updated to use MemoryTokenStorageService instead of AbpLocalStorageService |
| oauth.service.ts | Added try-catch error handling around token refresh operation |
| index.ts | Added export for the new MemoryTokenStorageService |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| return this.oAuthService.refreshToken(); | ||
| } catch (error) { | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
The catch block silently swallows errors without logging or providing any feedback. Consider logging the error or returning a rejected promise with an appropriate error message to help with debugging authentication issues.
| return Promise.resolve(); | |
| console.error('Error during token refresh:', error); | |
| return Promise.reject(error); |
| } | ||
|
|
||
| if (this.useSharedWorker && this.port) { | ||
| // this.cache.delete(key); |
There was a problem hiding this comment.
Remove commented-out code. If this line is needed, it should be uncommented and executed; otherwise, it should be deleted entirely.
| // this.cache.delete(key); |
| // @ts-ignore | ||
| if (typeof SharedWorker !== 'undefined') { | ||
| try { | ||
| // @ts-ignore | ||
| this.worker = new SharedWorker( |
There was a problem hiding this comment.
Replace @ts-ignore comments with proper TypeScript type declarations or type guards. The repeated use of @ts-ignore suppresses type checking and can hide potential runtime errors.
| } | ||
| }; | ||
|
|
||
| console.log('✅ Using SharedWorker for cross-tab token storage'); |
There was a problem hiding this comment.
Remove or replace console.log statements in production code. Consider using a proper logging service or removing this debug statement entirely.
| console.log('✅ Using SharedWorker for cross-tab token storage'); |
|
|
||
| console.log('✅ Using SharedWorker for cross-tab token storage'); | ||
| } catch (error) { | ||
| console.warn('⚠️ SharedWorker failed:', error); |
There was a problem hiding this comment.
Consider using a proper logging service instead of console.warn for error reporting in production code. This would allow better error tracking and debugging capabilities.
| this.useSharedWorker = false; | ||
| } | ||
| } else { | ||
| console.warn('⚠️ SharedWorker not supported'); |
There was a problem hiding this comment.
Consider using a proper logging service instead of console.warn. This would provide consistent logging throughout the application.
| 'abpOAuthClientId', 'granted_scopes' | ||
| ]; | ||
|
|
||
| private worker?: any; |
There was a problem hiding this comment.
private worker?: SharedWorker;
It would be better to add a proper type for worker instead of leaving it as an optional SharedWorker. This improves clarity and type safety.
|
|
||
| self.onconnect = (event: MessageEvent) => { | ||
| const port = event.ports[0]; | ||
| ports.add(port); |
There was a problem hiding this comment.
It looks like there’s a potential memory leak issue here.
When closing a tab, the number of ports continues to increase, which indicates that the reference to the closed tab is not being properly released. This could negatively impact performance over time.
Steps to reproduce:
- Run the project and open it in 3 different tabs.
- Close 2 of the tabs.
- Open 2 new tabs again.
At this point, you’ll notice that the ports array contains 5 elements instead of 3.
The closed tab’s port should be removed from the ports array when the tab is closed.
To fix this issue, we probably need to introduce a new **disconnect** action type (or a similar mechanism) to properly handle tab disconnections.
When a tab is closed, we should dispatch this action to remove the corresponding port from the ports array, ensuring that stale connections don’t accumulate and cause memory leaks.
Description
Resolves #23930
Improvements