Skip to content

[PM-33981] feat: Integrate device management into Account Security #2491

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.

Sign up for GitHub

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

Draft
andrebispo5 wants to merge 1 commit into
base: pm-33981/innovation-device-list
Choose a base branch
from
Draft

[PM-33981] feat: Integrate device management into Account Security #2491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityAction.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ enum AccountSecurityAction: Equatable {
/// The logout button was pressed.
case logout

/// The manage devices button was tapped.
case manageDevicesTapped

/// The pending login requests button was tapped.
case pendingLoginRequestsTapped

Expand Down
4 changes: 4 additions & 0 deletions BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityProcessor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ final class AccountSecurityProcessor: StateProcessor<// swiftlint:disable:this t
coordinator.navigate(to: .deleteAccount)
case .logout:
showLogoutConfirmation()
case .manageDevicesTapped:
coordinator.navigate(to: .deviceManagement)
case .pendingLoginRequestsTapped:
Comment on lines +102 to 104
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

New behavior isn’t covered by tests: there’s no test asserting .manageDevicesTapped navigates to .deviceManagement, and no test that loadData() sets isManageDevicesEnabled based on the feature flag (no manageDevices references in AccountSecurityProcessorTests). Please add coverage for both.

Copilot uses AI. Check for mistakes.
coordinator.navigate(to: .pendingLoginRequests)
case let .sessionTimeoutActionChanged(newValue):
Expand Down Expand Up @@ -181,6 +183,8 @@ final class AccountSecurityProcessor: StateProcessor<// swiftlint:disable:this t

state.isAuthenticatorSyncEnabled = try await services.stateService.getSyncToAuthenticator()

state.isManageDevicesEnabled = await services.configService.getFeatureFlag(.manageDevices)
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

FeatureFlag.manageDevices is not defined (the FeatureFlag extension only defines flags like .deviceAuthKey, .cxpExportMobile, etc.), so this won’t compile. Add a static let manageDevices = FeatureFlag(rawValue: ...) and include it in FeatureFlag.allCases (this codebase uses a manually maintained allCases).

Copilot uses AI. Check for mistakes.

if state.biometricUnlockStatus.isEnabled || state.isUnlockWithPINCodeOn {
await completeAccountSetupVaultUnlockIfNeeded()
}
Expand Down
3 changes: 3 additions & 0 deletions BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityState.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ struct AccountSecurityState: Equatable {
/// Whether the user has enabled the sync with the authenticator app..
var isAuthenticatorSyncEnabled = false

/// Whether the manage devices feature is enabled.
var isManageDevicesEnabled = false

/// Whether the timeout action policy is in effect.
var isPolicyTimeoutActionEnabled = false

Expand Down
30 changes: 22 additions & 8 deletions BitwardenShared/UI/Platform/Settings/Settings/AccountSecurity/AccountSecurityView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ struct AccountSecurityView: View {
VStack(spacing: 16) {
setUpUnlockActionCard

pendingLoginRequests
if !store.state.isManageDevicesEnabled {
pendingLoginRequests
}
Comment on lines +24 to +26
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

The new conditional UI paths aren’t covered by AccountSecurityView ViewInspector tests: when isManageDevicesEnabled == true, the pending login requests section should be hidden and the “Manage devices” row should appear and dispatch .manageDevicesTapped on tap. Please add tests for these cases.

Copilot uses AI. Check for mistakes.

if store.state.showUnlockOptions {
unlockOptionsSection
Expand Down Expand Up @@ -80,25 +82,37 @@ struct AccountSecurityView: View {
private var otherSection: some View {
SectionView(Localizations.other) {
ContentBlock(dividerLeadingPadding: 16) {
SettingsListItem(
Localizations.accountFingerprintPhrase,
accessibilityIdentifier: "AccountFingerprintPhraseLabel",
) {
Task {
await store.perform(.accountFingerprintPhrasePressed)
if store.state.isManageDevicesEnabled {
SettingsListItem(
Localizations.manageDevices,
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

Localizations.manageDevices doesn’t appear to be backed by a localization key in en.lproj/Localizable.strings (no "ManageDevices" entry). This will break SwiftGen generation / compilation. Please add the ManageDevices string key (and regenerate if needed) before referencing it here.

Suggested change
Localizations.manageDevices,
"Manage devices",

Copilot uses AI. Check for mistakes.
accessibilityIdentifier: "ManageDevicesLabel"
) {
store.send(.manageDevicesTapped)
} trailingContent: {
Image(asset: SharedAsset.Icons.chevronRight16)
.imageStyle(.accessoryIcon16)
}
}

SettingsListItem(
Localizations.twoStepLogin,
accessibilityIdentifier: "TwoStepLoginLinkItemView",
accessibilityIdentifier: "TwoStepLoginLinkItemView"
) {
store.send(.twoStepLoginPressed)
} trailingContent: {
Image(asset: SharedAsset.Icons.externalLink24)
.imageStyle(.rowIcon)
}

SettingsListItem(
Localizations.accountFingerprintPhrase,
accessibilityIdentifier: "AccountFingerprintPhraseLabel"
) {
Task {
await store.perform(.accountFingerprintPhrasePressed)
}
}

if store.state.isLockNowVisible {
SettingsListItem(
Localizations.lockNow,
Expand Down
15 changes: 15 additions & 0 deletions BitwardenShared/UI/Platform/Settings/SettingsCoordinator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,14 @@ final class SettingsCoordinator: Coordinator, HasStackNavigator { // swiftlint:d

typealias Services = HasASSettingsMediator
& HasAccountAPIService
& HasAppIdService
& HasAppInfoService
& HasAuthRepository
& HasAuthService
& HasAutofillCredentialService
& HasBiometricsRepository
& HasConfigService
& HasDeviceAPIService
& HasEnvironmentService
& HasErrorAlertServices.ErrorAlertServices
& HasErrorReporter
Expand Down Expand Up @@ -167,6 +169,8 @@ final class SettingsCoordinator: Coordinator, HasStackNavigator { // swiftlint:d
showAutoFill()
case .deleteAccount:
showDeleteAccount()
case .deviceManagement:
showDeviceManagement()
Comment on lines 171 to +173
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

There’s no regression test coverage for the new .deviceManagement route in SettingsCoordinatorTests (no references to deviceManagement there). Please add a test that navigating to .deviceManagement presents/pushes the expected device management screen.

Copilot uses AI. Check for mistakes.
case .dismiss:
stackNavigator?.dismiss()
case .exportVault:
Expand Down Expand Up @@ -344,6 +348,17 @@ final class SettingsCoordinator: Coordinator, HasStackNavigator { // swiftlint:d
stackNavigator?.present(DeleteAccountView(store: Store(processor: processor)))
}

/// Shows the device management screen.
///
private func showDeviceManagement() {
let processor = DeviceManagementProcessor(
coordinator: asAnyCoordinator(),
services: services,
state: DeviceManagementState(),
)
stackNavigator?.present(DeviceManagementView(store: Store(processor: processor)))
Comment on lines +354 to +359
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

DeviceManagementProcessor/DeviceManagementState/DeviceManagementView don’t exist anywhere in the repo (search for these symbols only finds this usage), so this route won’t compile. Please add the device management screen implementation (or import the correct module) before wiring up navigation here.

Suggested change
let processor = DeviceManagementProcessor(
coordinator: asAnyCoordinator(),
services: services,
state: DeviceManagementState(),
)
stackNavigator?.present(DeviceManagementView(store: Store(processor: processor)))
// TODO: Implement device management flow and present the appropriate view.
// Currently left as a no-op to avoid referencing undefined types.
assertionFailure("Device management screen is not yet implemented.")

Copilot uses AI. Check for mistakes.
}

/// Shows the share sheet to share one or more items.
///
/// - Parameter items: The items to share.
Expand Down
3 changes: 3 additions & 0 deletions BitwardenShared/UI/Platform/Settings/SettingsRoute.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ public enum SettingsRoute: Equatable, Hashable {
/// A route to the delete account screen.
case deleteAccount

/// A route to the device management screen.
case deviceManagement

/// A route that dismisses the current view.
case dismiss

Expand Down
Loading