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 all 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
5 changes: 4 additions & 1 deletion apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,10 @@
this.configService,
);

this.devicesService = new DevicesServiceImplementation(this.devicesApiService);
this.devicesService = new DevicesServiceImplementation(

Check warning on line 773 in apps/browser/src/background/main.background.ts

View check run for this annotation

Codecov / codecov/patch

apps/browser/src/background/main.background.ts#L773

Added line #L773 was not covered by tests
this.devicesApiService,
this.appIdService,
);

this.authRequestService = new AuthRequestService(
this.appIdService,
Expand Down
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,79 @@
<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

<div class="tw-flex tw-items-center tw-gap-2">
<h2 class="tw-m-0">{{ "devices" | i18n }}</h2>
<i
class="bwi bwi-question-circle tw-flex tw-items-center tw-h-4 tw-w-4 bwi-color-primary"
aria-hidden="true"
></i>
Comment on lines +4 to +7
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<i
*ngIf="asyncActionLoading"
class="bwi bwi-spinner bwi-spin tw-flex tw-items-center tw-h-4 tw-w-4"
aria-hidden="true"
></i>
</div>

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

<div *ngIf="loading" class="tw-flex tw-justify-center tw-items-center tw-p-4">
<i class="bwi bwi-spinner bwi-spin tw-text-2xl" aria-hidden="true"></i>
</div>

<bit-table-scroll *ngIf="!loading" [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>
{{ "removeDevice" | i18n }}
</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,214 @@
import { CommonModule } from "@angular/common";
import { Component } from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { firstValueFrom } from "rxjs";
import { switchMap } from "rxjs/operators";

Check warning on line 5 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-L5

Added lines #L1 - L5 were not covered by tests

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 { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service";
import { DialogService, ToastService, TableDataSource, TableModule } from "@bitwarden/components";

Check warning on line 12 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#L7-L12

Added lines #L7 - L12 were not covered by tests

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

Check warning on line 14 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#L14

Added line #L14 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 37 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#L35-L37

Added lines #L35 - L37 were not covered by tests
protected currentDevice: DeviceView | undefined;
protected loading = true;
protected asyncActionLoading = false;

Check warning on line 40 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#L39-L40

Added lines #L39 - L40 were not covered by tests

constructor(
private i18nService: I18nService,
private devicesService: DevicesServiceAbstraction,
private dialogService: DialogService,
private toastService: ToastService,
private validationService: ValidationService,

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
) {
this.devicesService

Check warning on line 49 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#L49

Added line #L49 was not covered by tests
.getCurrentDevice$()
.pipe(
takeUntilDestroyed(),
switchMap((currentDevice) => {
this.currentDevice = new DeviceView(currentDevice);
return this.devicesService.getDevices$();

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#L54-L55

Added lines #L54 - L55 were not covered by tests
}),
)
.subscribe({
next: (devices) => {
this.dataSource.data = devices.map((device) => {
return {

Check warning on line 61 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#L60-L61

Added lines #L60 - L61 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,
};
});
this.loading = false;

Check warning on line 71 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#L71

Added line #L71 was not covered by tests
},
error: () => {
this.loading = false;

Check warning on line 74 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#L74

Added line #L74 was not covered by tests
},
});
}

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

Check warning on line 82 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#L82

Added line #L82 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 110 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#L109-L110

Added lines #L109 - L110 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 119 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#L119

Added line #L119 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 132 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#L132

Added line #L132 was not covered by tests
}

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

Check warning on line 136 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#L136

Added line #L136 was not covered by tests
}

return "";

Check warning on line 139 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#L139

Added line #L139 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 148 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#L148

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

Check warning on line 150 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#L150

Added line #L150 was not covered by tests
}

// If the platform is "Unknown" translate it since it is not a proper noun
const platform =
metadata.platform === "Unknown" ? this.i18nService.t("unknown") : metadata.platform;
const category = this.i18nService.t(metadata.category);

Check warning on line 156 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#L156

Added line #L156 was not covered by tests
return platform ? `${category} - ${platform}` : category;
}

/**
* 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 177 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#L177

Added line #L177 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 187 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#L187

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

if (!confirmed) {
return;

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
}

try {
this.asyncActionLoading = true;
await firstValueFrom(this.devicesService.deactivateDevice$(device.id));
this.asyncActionLoading = false;

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#L197-L200

Added lines #L197 - L200 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 203 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#L203

Added line #L203 was not covered by tests

this.toastService.showToast({

Check warning on line 205 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#L205

Added line #L205 was not covered by tests
title: "",
message: this.i18nService.t("deviceRemoved"),
variant: "success",
});
} catch (error) {
this.validationService.showError(error);

Check warning on line 211 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#L211

Added line #L211 was not covered by tests
}
}
}
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import { NgModule } from "@angular/core";
import { RouterModule, Routes } from "@angular/router";

import { canAccessFeature } from "@bitwarden/angular/platform/guard/feature-flag.guard";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";

Check warning on line 5 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#L4-L5

Added lines #L4 - L5 were not covered by tests

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 10 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#L10

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

Expand All @@ -29,6 +33,12 @@
component: SecurityKeysComponent,
data: { titleId: "keys" },
},
{
path: "device-management",
component: DeviceManagementComponent,
data: { titleId: "devices" },
canActivate: [canAccessFeature(FeatureFlag.DeviceManagement)],
},
],
},
];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
<bit-tab-link route="change-password">{{ "masterPassword" | i18n }}</bit-tab-link>
</ng-container>
<bit-tab-link route="two-factor">{{ "twoStepLogin" | i18n }}</bit-tab-link>
<ng-container *ngIf="deviceManagementAvailable$ | async">
<bit-tab-link route="device-management">{{ "devices" | i18n }}</bit-tab-link>
</ng-container>
<bit-tab-link route="security-keys">{{ "keys" | i18n }}</bit-tab-link>
</bit-tab-nav-bar>
</app-header>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,21 @@
import { Component, OnInit } from "@angular/core";

import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction";
import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum";
import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";

@Component({
selector: "app-security",
templateUrl: "security.component.html",
})
export class SecurityComponent implements OnInit {
showChangePassword = true;
deviceManagementAvailable$ = this.configService.getFeatureFlag$(FeatureFlag.DeviceManagement);

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

View check run for this annotation

Codecov / codecov/patch

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

Added line #L13 was not covered by tests

constructor(private userVerificationService: UserVerificationService) {}
constructor(
private userVerificationService: UserVerificationService,
private configService: ConfigService,

Check warning on line 17 in apps/web/src/app/auth/settings/security/security.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/security/security.component.ts#L17

Added line #L17 was not covered by tests
) {}

async ngOnInit() {
this.showChangePassword = await this.userVerificationService.hasMasterPassword();
Expand Down
Loading
Loading