[PM-15063] Show banner in web app when user has at least one pending authrequest #13147
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13147 +/- ##
==========================================
+ Coverage 35.03% 35.08% +0.05%
==========================================
Files 3034 3036 +2
Lines 92636 92776 +140
Branches 16858 16905 +47
==========================================
+ Hits 32452 32549 +97
- Misses 57724 57753 +29
- Partials 2460 2474 +14 ☔ View full report in Codecov by Sentry. |
alec-livefront
changed the title
alec-livefront
changed the title
alec-livefront
changed the title
| dataSource = new TableDataSource<DeviceTableData>(); | ||
| currentDevice: DeviceView | undefined; | ||
| loading = true; | ||
| asyncActionLoading = false; |
There was a problem hiding this comment.
Vault files look gewd. Just curious why the protected was removed.
There was a problem hiding this comment.
I'm guessing I was working with some finicky testing situation here - it's not necessary to remove though so I've added it back: 271376d
There was a problem hiding this comment.
The same device will show up 2x when there is a pending auth request and after it is approved. The duplicate only goes away when you refresh the screen. Here are three screenshots showing each state.
Pending auth request (device shown 2x)
After auth request has been approved (device shown 2x)
After refreshing the device management screen (device shown 1x)
| @@ -44,7 +49,7 @@ interface DeviceTableData { | |||
| imports: [CommonModule, SharedModule, TableModule, PopoverModule], | |||
| }) | |||
| export class DeviceManagementComponent { | |||
| protected readonly tableId = "device-management-table"; | |||
| protected readonly retableId = "device-management-table"; | |||
| // Load devices | ||
| this.loadDevices().catch((error) => this.validationService.showError(error)); | ||
|
|
||
| // Listen for auth request messages | ||
| this.messageListener.allMessages$.pipe(takeUntilDestroyed()).subscribe((message) => { | ||
| if (message.command !== "openLoginApproval") { | ||
| return; | ||
| } | ||
| // Handle inserting a new device when an auth request is received | ||
| this.handleAuthRequest(message as { command: string; notificationId: string }).catch( | ||
| (error) => this.validationService.showError(error), | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Since we aren't awaiting loadDevices(), I think this creates a race condition. If handleAuthRequest() were to finish first, then I believe loadDevices() could potentially overwrite the table data and remove the newDevice that was added in handleAuthRequest().
| * Handle inserting a new device when an auth request is received | ||
| * @param message - The auth request message | ||
| */ | ||
| private async handleAuthRequest(message: { |
There was a problem hiding this comment.
I'd rename this to upsertDeviceWithPendingAuthRequest or something similar. Otherwise the name seems like it's actually handling/managing the auth request, which we do in managePendingAuthRequest()
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15063?atlOrigin=eyJpIjoiOGY5MzdhNDMyZDllNDc0ZWFkMzk2NjI1NjRhMTU5MGIiLCJwIjoiaiJ9
📔 Objective
Adds a new banner to the vault that will display when a device auth request is made. The banner contains a link to the device management screen (
/#/settings/security/device-management). This PR also modifies the device management screen to listen for theopenLoginApprovalmessage and upsert the new request data into the table.📸 Screenshots
GMT20250129-223543_Clip_Alec.Rippberger.s.Clip.01_29_2025.mp4
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes