[PM-1214] device management screen #12455

alec-livefront · 2024-12-17T22:27:54Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-1214?atlOrigin=eyJpIjoiOGUyMGIwODJmODQwNDlkMjlhZmFhZDYxNmVjN2EyZGEiLCJwIjoiaiJ9

📔 Objective

Adds a device management tab under settings -> security. Allows a user to manage the devices of their account by approving them, logging them out or deleting them.

📸 Screenshots

GMT20241220-201106_Clip_Alec.Rippberger.s.Clip.12_20_2024.mp4

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

github-actions · 2024-12-17T22:36:16Z

Checkmarx One – Scan Summary & Details – c2367472-8cd9-471f-abdf-8445686944ef

Fixed Issues

Severity	Issue	Source File / Package
	Client_Privacy_Violation	/apps/browser/src/vault/popup/components/vault-v2/vault-list-items-container/vault-list-items-container.component.html: 15

codecov · 2024-12-17T22:38:13Z

Codecov Report

Attention: Patch coverage is 7.50000% with 74 lines in your changes missing coverage. Please review.

Project coverage is 33.77%. Comparing base (6d65ce9) to head (59cae5b).

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...h/settings/security/device-management.component.ts	0.00%	60 Missing ⚠️
...services/devices/devices.service.implementation.ts	0.00%	6 Missing ⚠️
.../auth/settings/security/security-routing.module.ts	0.00%	3 Missing ⚠️
...c/app/auth/settings/security/security.component.ts	40.00%	2 Missing and 1 partial ⚠️
apps/browser/src/background/main.background.ts	0.00%	1 Missing ⚠️
...uth/services/devices-api.service.implementation.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12455      +/-   ##
==========================================
- Coverage   33.78%   33.77%   -0.02%     
==========================================
  Files        2912     2913       +1     
  Lines       90731    90806      +75     
  Branches    17159    17175      +16     
==========================================
+ Hits        30654    30670      +16     
- Misses      57688    57747      +59     
  Partials     2389     2389

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alec-livefront · 2024-12-18T04:13:22Z

I could use help on a few things I'm unsure of on this PR:

The ticket calls for virtual scroll on the table. However, when I try to implement bit-table-scroll I get Unhandled error in angular TypeError: Cannot read properties of undefined (reading 'template'). I looked at the vnext-clients component to see if I could spot what might be different here but came up empty.
I'm not sure how to implement the removeDevice, ~~approveDevice or logOutDevice~~ functions (or if it is currently possible with the backend). I tried to flush out removeDevice using the devices/{id}/deactivate endpoint but I don't believe it is working. I could use some direction on what to do here.
The Figma designs seem to show relative times for the last login (like "This week", "Last month". I'm wondering if there is existing logic to convert a date to a human readable time exists elsewhere in the app? I couldn't find anything. It shouldn't be hard to write but I wanted to be consistent. Another issue is I'm wondering how to get the last login date - it doesn't seem to be included in the device info from the backend.

…b.com/bitwarden/clients into auth/pm-1214/device-management-screen

JaredSnider-Bitwarden · 2024-12-20T17:49:20Z

libs/common/src/auth/services/device-trust.service.implementation.ts

+  getCurrentDevice$(): Observable<DeviceResponse> {
+    return defer(async () => {
+      const deviceIdentifier = await this.appIdService.getAppId();
+      return this.devicesApiService.getDeviceByIdentifier(deviceIdentifier);
+    });
+  }


⚠️ : I believe this new method better belongs on the DevicesService vs this service. This also allows you to undo the strict typing updates here so we don't have to re-test the device trust implementation for this change.

JaredSnider-Bitwarden

This is looking great! I've got a few UI suggestions and some minor clean up items below:

JaredSnider-Bitwarden · 2024-12-20T19:15:52Z

apps/web/src/app/auth/settings/security/device-management.component.html

💡 : upon watching the video, I think we should implement a loading state for the table which doesn't show the table until the data is loaded.

I've added a loading state for when the table is loading, and added the spinner for when async actions are waiting to complete: 0932e18

apps/web/src/app/auth/settings/security/device-management.component.html

JaredSnider-Bitwarden · 2024-12-20T19:21:17Z

apps/web/src/app/auth/settings/security/device-management.component.html

+  <h2>
+    {{ "devices" | i18n }}
+    <i class="bwi bwi-question-circle tw-h-5 tw-w-5 bwi-color-primary" aria-hidden="true"></i>
+  </h2>


The figma calls for a loading icon here when async actions like remove device or log out are being taken

JaredSnider-Bitwarden · 2024-12-20T19:37:50Z

apps/web/src/app/auth/settings/security/device-management.component.ts

+    }
+
+    const category = this.i18nService.t(metadata.category);
+    return metadata.platform ? `${category} - ${metadata.platform}` : category;


❓ : Nice job building out this metadata out. Do you think we should translate "Unknown" as a platform?

Can you elaborate on that? Are you talking about the DeviceType.UnknownBrowser having a platform of "Unknown"?

Yes, I'm sorry that comment was not as clear as I meant for it to be. "Unknown" is the only non proper noun so we could translate it.

Ah ok that makes sense! 4bb0459

JaredSnider-Bitwarden · 2024-12-20T19:39:30Z

apps/web/src/app/auth/settings/security/device-management.component.ts

+      this.toastService.showToast({
+        title: "",
+        message: this.i18nService.t("errorOccurred"),
+        variant: "warning",
+      });


❓ : can we change this to be a call to this.validationService.showError(error); instead? As far as I know, that's the general recommendation for non-specific error handling.

JaredSnider-Bitwarden · 2024-12-20T19:39:58Z

apps/web/src/app/auth/settings/security/security-routing.module.ts

+      {
+        path: "device-management",
+        component: DeviceManagementComponent,
+        data: { titleId: "devices" },
+      },


⚠️ : I believe this should be gated behind the feature flag guard for web app approvals, right?

JaredSnider-Bitwarden · 2024-12-20T19:42:18Z

libs/common/src/auth/abstractions/devices/devices.service.abstraction.ts

🏅 Thank you for refactoring this to use abstract and actual methods instead of arrow functions.

JaredSnider-Bitwarden · 2024-12-20T19:43:14Z

apps/web/src/app/auth/settings/security/device-management.component.ts

Excellent docs throughout. Thank you!

JaredSnider-Bitwarden · 2024-12-20T19:46:07Z

libs/common/src/auth/services/device-trust.service.spec.ts

Same comment as here: https://github.com/bitwarden/clients/pull/12455/files#r1894224023

JaredSnider-Bitwarden · 2024-12-20T19:46:29Z

libs/common/src/auth/services/devices-api.service.implementation.spec.ts

🏅 Thank you for adding this missing test suite!

…known" platform correctly

rr-bw · 2024-12-27T22:30:42Z

apps/web/src/app/auth/settings/security/device-management.component.html

@@ -0,0 +1,79 @@
+<div class="tabbed-content">


Question: Why tabbed-content? Did you mean tabbed-header? And why not use Tailwind?

And a slight UI adjustment: The header for the Devices table isn't quite formatted according to Figma. There should be a bit more spacing between the heading and subheading/paragraph text. (It also doesn't look the same as the headers in the other tabs: Master Password, Two-step login).

Figma

Current Implementation

rr-bw · 2024-12-27T22:31:42Z

apps/web/src/app/auth/settings/security/device-management.component.html

+    <i
+      class="bwi bwi-question-circle tw-flex tw-items-center tw-h-4 tw-w-4 bwi-color-primary"
+      aria-hidden="true"
+    ></i>


Is there a corresponding link/popover text for this question mark icon?

rr-bw · 2024-12-27T22:34:42Z

apps/web/src/app/auth/settings/security/device-management.component.html

+        *ngFor="let col of columnConfig"
+        [class]="col.headerClass"
+        bitCell
+        [bitSortable]="col.sortable ? col.name : null"


For me, the "Current Session" is the second item. It should be the first (according to Jira task).

alec-livefront added 11 commits December 6, 2024 21:33

Add basic device management screen

79327d8

Add table styling

d4c0091

Add options button

efda9ae

Merge branch 'main' into auth/pm-1214/device-management-screen

a4d1740

Fix bad merge

dc2596e

Merge branch 'main' into auth/pm-1214/device-management-screen

4c49954

Fix icons, trusted status, login status and last login

5eb9955

Add sorting and table description.

99f6af0

Add table button functionality

f1a59e9

Add device deactivation

ebddea0

Add strict typing and other cleanup

b078c74

alec-livefront added 3 commits December 17, 2024 16:41

Type fixes and message update

660aba2

Update login status column with pending auth request

80760c8

Update table styling

278fa9b

alec-livefront changed the title ~~Auth/pm 1214/device management screen~~ [PM-1214] device management screen Dec 18, 2024

Merge branch 'main' into auth/pm-1214/device-management-screen

eef346a

alec-livefront requested a review from JaredSnider-Bitwarden December 18, 2024 04:13

alec-livefront and others added 9 commits December 18, 2024 08:39

Merge branch 'main' into auth/pm-1214/device-management-screen

74363fb

Add virtual scroll to table

bcd847d

Merge branch 'main' into auth/pm-1214/device-management-screen

0561cd8

Remove approve device and log out menu options

f336afc

Merge branch 'auth/pm-1214/device-management-screen' of https://githu…

82dbc98

…b.com/bitwarden/clients into auth/pm-1214/device-management-screen

Remove device when successfully deactivated

d995fe5

Update row size

3eb0ef7

Add service tests

c3c73cc

Merge branch 'main' into auth/pm-1214/device-management-screen

6ca912a

alec-livefront marked this pull request as ready for review December 20, 2024 16:09

alec-livefront requested a review from a team as a code owner December 20, 2024 16:09

JaredSnider-Bitwarden reviewed Dec 20, 2024

View reviewed changes

JaredSnider-Bitwarden requested changes Dec 20, 2024

View reviewed changes

rr-bw self-requested a review December 20, 2024 19:56

alec-livefront and others added 8 commits December 20, 2024 14:04

Move getCurrentDevice$ to DevicesService

99a647c

Update "Remove" to "Remove device"

d01ca4d

Add loading spinners

0932e18

Use ValidationService to handle error

3934101

Fix platform translation in device management component to handle "Un…

4bb0459

…known" platform correctly

Merge branch 'main' into auth/pm-1214/device-management-screen

c8dc459

Add DeviceManagement feature flag

66e497c

Merge branch 'main' into auth/pm-1214/device-management-screen

59cae5b

alec-livefront requested a review from JaredSnider-Bitwarden December 27, 2024 20:28

rr-bw requested changes Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-1214] device management screen #12455

[PM-1214] device management screen #12455

alec-livefront commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

alec-livefront commented Dec 18, 2024 •

edited

Loading

JaredSnider-Bitwarden Dec 20, 2024

JaredSnider-Bitwarden left a comment •

edited

Loading

JaredSnider-Bitwarden Dec 20, 2024

alec-livefront Dec 20, 2024

JaredSnider-Bitwarden Dec 20, 2024

alec-livefront Dec 20, 2024

JaredSnider-Bitwarden Dec 20, 2024

alec-livefront Dec 20, 2024

JaredSnider-Bitwarden Dec 21, 2024

alec-livefront Dec 23, 2024

JaredSnider-Bitwarden Dec 20, 2024

alec-livefront Dec 20, 2024

JaredSnider-Bitwarden Dec 20, 2024

alec-livefront Dec 27, 2024

JaredSnider-Bitwarden Dec 20, 2024

JaredSnider-Bitwarden Dec 20, 2024

JaredSnider-Bitwarden Dec 20, 2024

JaredSnider-Bitwarden Dec 20, 2024

rr-bw Dec 27, 2024

rr-bw Dec 27, 2024

rr-bw Dec 27, 2024

[PM-1214] device management screen #12455

Are you sure you want to change the base?

[PM-1214] device management screen #12455

Conversation

alec-livefront commented Dec 17, 2024 • edited Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

github-actions bot commented Dec 17, 2024 • edited Loading

Fixed Issues

codecov bot commented Dec 17, 2024 • edited Loading

Codecov Report

alec-livefront commented Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

JaredSnider-Bitwarden left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alec-livefront commented Dec 17, 2024 •

edited

Loading

github-actions bot commented Dec 17, 2024 •

edited

Loading

codecov bot commented Dec 17, 2024 •

edited

Loading

alec-livefront commented Dec 18, 2024 •

edited

Loading

JaredSnider-Bitwarden left a comment •

edited

Loading