Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-1214] device management screen #12455

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
79327d8
Add basic device management screen
alec-livefront Dec 7, 2024
d4c0091
Add table styling
alec-livefront Dec 9, 2024
efda9ae
Add options button
alec-livefront Dec 9, 2024
a4d1740
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 16, 2024
dc2596e
Fix bad merge
alec-livefront Dec 16, 2024
4c49954
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 17, 2024
5eb9955
Fix icons, trusted status, login status and last login
alec-livefront Dec 17, 2024
99f6af0
Add sorting and table description.
alec-livefront Dec 17, 2024
f1a59e9
Add table button functionality
alec-livefront Dec 17, 2024
ebddea0
Add device deactivation
alec-livefront Dec 17, 2024
b078c74
Add strict typing and other cleanup
alec-livefront Dec 17, 2024
660aba2
Type fixes and message update
alec-livefront Dec 17, 2024
80760c8
Update login status column with pending auth request
alec-livefront Dec 18, 2024
278fa9b
Update table styling
alec-livefront Dec 18, 2024
eef346a
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 18, 2024
74363fb
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 18, 2024
bcd847d
Add virtual scroll to table
alec-livefront Dec 19, 2024
0561cd8
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 19, 2024
f336afc
Remove approve device and log out menu options
alec-livefront Dec 19, 2024
82dbc98
Merge branch 'auth/pm-1214/device-management-screen' of https://githuโ€ฆ
alec-livefront Dec 19, 2024
d995fe5
Remove device when successfully deactivated
alec-livefront Dec 19, 2024
3eb0ef7
Update row size
alec-livefront Dec 19, 2024
c3c73cc
Add service tests
alec-livefront Dec 19, 2024
6ca912a
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 20, 2024
99a647c
Move getCurrentDevice$ to DevicesService
alec-livefront Dec 20, 2024
d01ca4d
Update "Remove" to "Remove device"
alec-livefront Dec 20, 2024
0932e18
Add loading spinners
alec-livefront Dec 20, 2024
3934101
Use ValidationService to handle error
alec-livefront Dec 20, 2024
4bb0459
Fix platform translation in device management component to handle "Unโ€ฆ
alec-livefront Dec 23, 2024
c8dc459
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 23, 2024
66e497c
Add DeviceManagement feature flag
alec-livefront Dec 26, 2024
59cae5b
Merge branch 'main' into auth/pm-1214/device-management-screen
alec-livefront Dec 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

๐Ÿ’ก : 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<div class="tabbed-content">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2024-12-27 at 2 26 06โ€ฏPM

Current Implementation

Screenshot 2024-12-27 at 2 26 16โ€ฏPM

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<p>{{ "deviceListDescription" | i18n }}</p>

<bit-table-scroll [dataSource]="dataSource" [rowSize]="50">
<ng-container header>
<th
*ngFor="let col of columnConfig"
[class]="col.headerClass"
bitCell
[bitSortable]="col.sortable ? col.name : null"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Screenshot 2024-12-27 at 2 32 06โ€ฏPM

[default]="col.name === 'displayName' ? 'desc' : null"
scope="col"
role="columnheader"
>
{{ col.title }}
</th>
<th bitCell scope="col" role="columnheader"></th>
</ng-container>
<ng-template bitRowDef let-row>
<td bitCell class="tw-flex tw-gap-2">
<div class="tw-flex tw-items-center tw-justify-center tw-w-10">
<i [class]="getDeviceIcon(row.type)" class="bwi-lg" aria-hidden="true"></i>
</div>
<div>
{{ row.displayName }}
<span *ngIf="row.trusted" class="tw-text-sm tw-text-muted tw-block">
{{ "trusted" | i18n }}
</span>
</div>
</td>
<td bitCell>
<span *ngIf="isCurrentDevice(row)" bitBadge variant="primary">{{
"currentSession" | i18n
}}</span>
<span *ngIf="hasPendingAuthRequest(row)" bitBadge variant="warning">{{
"requestPending" | i18n
}}</span>
</td>
<td bitCell>{{ row.firstLogin | date: "medium" }}</td>
<td bitCell>
<button
type="button"
bitIconButton="bwi-ellipsis-v"
[bitMenuTriggerFor]="optionsMenu"
></button>
<bit-menu #optionsMenu>
<button
type="button"
bitMenuItem
(click)="removeDevice(row)"
[disabled]="isCurrentDevice(row)"
>
<span [class]="isCurrentDevice(row) ? 'tw-text-muted' : 'tw-text-danger'">
<i class="bwi bwi-trash" aria-hidden="true"></i>
{{ "remove" | i18n }}
JaredSnider-Bitwarden marked this conversation as resolved.
Show resolved Hide resolved
</span>
</button>
</bit-menu>
</td>
</ng-template>
</bit-table-scroll>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent docs throughout. Thank you!

Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
import { CommonModule } from "@angular/common";
import { Component } from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { firstValueFrom } from "rxjs";

Check warning on line 4 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L1 - L4 were not covered by tests

import { DeviceTrustServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust.service.abstraction";
import { DevicesServiceAbstraction } from "@bitwarden/common/auth/abstractions/devices/devices.service.abstraction";
import { DeviceView } from "@bitwarden/common/auth/abstractions/devices/views/device.view";
import { DeviceType, DeviceTypeMetadata } from "@bitwarden/common/enums";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { DialogService, ToastService, TableDataSource, TableModule } from "@bitwarden/components";

Check warning on line 11 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L6 - L11 were not covered by tests

import { SharedModule } from "../../../shared";

Check warning on line 13 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L13 was not covered by tests

interface DeviceTableData {
id: string;
type: DeviceType;
displayName: string;
loginStatus: string;
firstLogin: Date;
trusted: boolean;
devicePendingAuthRequest: object | null;
}

/**
* Provides a table of devices and allows the user to log out, approve or remove a device
*/
@Component({
selector: "app-device-management",
templateUrl: "./device-management.component.html",
standalone: true,
imports: [CommonModule, SharedModule, TableModule],
})
export class DeviceManagementComponent {
protected readonly tableId = "device-management-table";
protected dataSource = new TableDataSource<DeviceTableData>();

Check warning on line 36 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L34 - L36 were not covered by tests
protected currentDevice: DeviceView | undefined;

constructor(
private i18nService: I18nService,
private devicesService: DevicesServiceAbstraction,
private deviceTrustService: DeviceTrustServiceAbstraction,
private dialogService: DialogService,
private toastService: ToastService,

Check warning on line 44 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L44 was not covered by tests
) {
// Get current device
this.deviceTrustService

Check warning on line 47 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L47 was not covered by tests
.getCurrentDevice$()
.pipe(takeUntilDestroyed())
.subscribe((device) => {
this.currentDevice = new DeviceView(device);

Check warning on line 51 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L51 was not covered by tests
});

// Get all devices and map them for the table
this.devicesService

Check warning on line 55 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L55 was not covered by tests
.getDevices$()
.pipe(takeUntilDestroyed())
.subscribe((devices) => {
this.dataSource.data = devices.map((device) => {
return {

Check warning on line 60 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L59 - L60 were not covered by tests
id: device.id,
type: device.type,
displayName: this.getHumanReadableDeviceType(device.type),
loginStatus: this.getLoginStatus(device),
devicePendingAuthRequest: device.response.devicePendingAuthRequest,
firstLogin: new Date(device.creationDate),
trusted: device.response.isTrusted,
};
});
});
}

/**
* Column configuration for the table
*/
protected readonly columnConfig = [

Check warning on line 76 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L76 was not covered by tests
{
name: "displayName",
title: this.i18nService.t("device"),
headerClass: "tw-w-1/3",
sortable: true,
},
{
name: "loginStatus",
title: this.i18nService.t("loginStatus"),
headerClass: "tw-w-1/3",
sortable: true,
},
{
name: "firstLogin",
title: this.i18nService.t("firstLogin"),
headerClass: "tw-w-1/3",
sortable: true,
},
];

/**
* Get the icon for a device type
* @param type - The device type
* @returns The icon for the device type
*/
getDeviceIcon(type: DeviceType): string {
const defaultIcon = "bwi bwi-desktop";
const categoryIconMap: Record<string, string> = {

Check warning on line 104 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L103 - L104 were not covered by tests
webVault: "bwi bwi-browser",
desktop: "bwi bwi-desktop",
mobile: "bwi bwi-mobile",
cli: "bwi bwi-cli",
extension: "bwi bwi-puzzle",
sdk: "bwi bwi-desktop",
};

const metadata = DeviceTypeMetadata[type];

Check warning on line 113 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L113 was not covered by tests
return metadata ? (categoryIconMap[metadata.category] ?? defaultIcon) : defaultIcon;
}

/**
* Get the login status of a device
* It will return the current session if the device is the current device
* It will return the date of the pending auth request when available
* @param device - The device
* @returns The login status
*/
private getLoginStatus(device: DeviceView): string {
if (this.isCurrentDevice(device)) {
return this.i18nService.t("currentSession");

Check warning on line 126 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L126 was not covered by tests
}

if (device.response.devicePendingAuthRequest?.creationDate) {
return this.i18nService.t("requestPending");

Check warning on line 130 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L130 was not covered by tests
}

return "";

Check warning on line 133 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L133 was not covered by tests
}

/**
* Get a human readable device type from the DeviceType enum
* @param type - The device type
* @returns The human readable device type
*/
private getHumanReadableDeviceType(type: DeviceType): string {
const metadata = DeviceTypeMetadata[type];

Check warning on line 142 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L142 was not covered by tests
if (!metadata) {
return this.i18nService.t("unknownDevice");

Check warning on line 144 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L144 was not covered by tests
}

const category = this.i18nService.t(metadata.category);

Check warning on line 147 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L147 was not covered by tests
return metadata.platform ? `${category} - ${metadata.platform}` : category;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok that makes sense! 4bb0459

}

/**
* Check if a device is the current device
* @param device - The device or device table data
* @returns True if the device is the current device, false otherwise
*/
protected isCurrentDevice(device: DeviceView | DeviceTableData): boolean {
return "response" in device
? device.id === this.currentDevice?.id
: device.id === this.currentDevice?.id;
}

/**
* Check if a device has a pending auth request
* @param device - The device
* @returns True if the device has a pending auth request, false otherwise
*/
protected hasPendingAuthRequest(device: DeviceTableData): boolean {
return (

Check warning on line 168 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L168 was not covered by tests
device.devicePendingAuthRequest !== undefined && device.devicePendingAuthRequest !== null
);
}

/**
* Remove a device
* @param device - The device
*/
protected async removeDevice(device: DeviceTableData) {
const confirmed = await this.dialogService.openSimpleDialog({

Check warning on line 178 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L178 was not covered by tests
title: { key: "removeDevice" },
content: { key: "removeDeviceConfirmation" },
type: "warning",
});

if (!confirmed) {
return;

Check warning on line 185 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L185 was not covered by tests
}

try {
await firstValueFrom(this.devicesService.deactivateDevice$(device.id));

Check warning on line 189 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added lines #L188 - L189 were not covered by tests

// Remove the device from the data source
this.dataSource.data = this.dataSource.data.filter((d) => d.id !== device.id);

Check warning on line 192 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L192 was not covered by tests

this.toastService.showToast({

Check warning on line 194 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L194 was not covered by tests
title: "",
message: this.i18nService.t("deviceRemoved"),
variant: "success",
});
} catch (e) {
this.toastService.showToast({

Check warning on line 200 in apps/web/src/app/auth/settings/security/device-management.component.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L200 was not covered by tests
title: "",
message: this.i18nService.t("errorOccurred"),
variant: "warning",
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โ“ : 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import { ChangePasswordComponent } from "../change-password.component";
import { TwoFactorSetupComponent } from "../two-factor/two-factor-setup.component";

import { DeviceManagementComponent } from "./device-management.component";

Check warning on line 7 in apps/web/src/app/auth/settings/security/security-routing.module.ts

View check run for this annotation

Codecov / codecov/patch

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

Added line #L7 was not covered by tests
import { SecurityKeysComponent } from "./security-keys.component";
import { SecurityComponent } from "./security.component";

Expand All @@ -29,6 +30,11 @@
component: SecurityKeysComponent,
data: { titleId: "keys" },
},
{
path: "device-management",
component: DeviceManagementComponent,
data: { titleId: "devices" },
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โš ๏ธ : I believe this should be gated behind the feature flag guard for web app approvals, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

],
},
];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
<bit-tab-link route="change-password">{{ "masterPassword" | i18n }}</bit-tab-link>
</ng-container>
<bit-tab-link route="two-factor">{{ "twoStepLogin" | i18n }}</bit-tab-link>
<bit-tab-link route="device-management">{{ "devices" | i18n }}</bit-tab-link>
<bit-tab-link route="security-keys">{{ "keys" | i18n }}</bit-tab-link>
</bit-tab-nav-bar>
</app-header>
Expand Down
30 changes: 30 additions & 0 deletions apps/web/src/locales/en/messages.json
Original file line number Diff line number Diff line change
Expand Up @@ -1715,6 +1715,12 @@
"logBackIn": {
"message": "Please log back in."
},
"currentSession": {
"message": "Current session"
},
"requestPending": {
"message": "Request pending"
},
"logBackInOthersToo": {
"message": "Please log back in. If you are using other Bitwarden applications log out and back in to those as well."
},
Expand Down Expand Up @@ -3765,6 +3771,15 @@
"device": {
"message": "Device"
},
"loginStatus": {
"message": "Login status"
},
"firstLogin": {
"message": "First login"
},
"trusted": {
"message": "Trusted"
},
"creatingAccountOn": {
"message": "Creating account on"
},
Expand Down Expand Up @@ -8236,6 +8251,15 @@
"approveRequest": {
"message": "Approve request"
},
"deviceApproved": {
"message": "Device approved"
},
"deviceRemoved": {
"message": "Device removed"
},
"removeDeviceConfirmation": {
"message": "Are you sure you want to remove this device?"
},
"noDeviceRequests": {
"message": "No device requests"
},
Expand Down Expand Up @@ -9927,6 +9951,12 @@
"removeMembers": {
"message": "Remove members"
},
"devices": {
"message": "Devices"
},
"deviceListDescription": {
"message": "Your account was logged in to each of the devices below. If you do not recognize a device, remove it now."
},
"claimedDomains": {
"message": "Claimed domains"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,8 @@ export abstract class DeviceTrustServiceAbstraction {
* Note: For debugging purposes only.
*/
recordDeviceTrustLoss: () => Promise<void>;
/**
* Retrieves the current device
*/
getCurrentDevice$: () => Observable<DeviceResponse>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,10 @@ export abstract class DevicesApiServiceAbstraction {
* @param deviceIdentifier - current device identifier
*/
postDeviceTrustLoss: (deviceIdentifier: string) => Promise<void>;

/**
* Deactivates a device
* @param deviceId - The device ID
*/
deactivateDevice: (deviceId: string) => Promise<void>;
}
Loading
Loading