Skip to content

[PoC] [PM-27720] Device Management - #1224

Draft
dereknance wants to merge 4 commits into
mainfrom
pm-27720-device-management
Draft

[PoC] [PM-27720] Device Management#1224
dereknance wants to merge 4 commits into
mainfrom
pm-27720-device-management

Conversation

@dereknance

@dereknance dereknance commented Jun 30, 2026 •
edited by atlassian Bot
Loading

Copy link
Copy Markdown
Contributor

🎟️ Tracking

PM-27720

📔 Objective

This PR is a proof of concept of the expanded device management features, including some example usages.

🚨 Breaking Changes

@sonarqubecloud

sonarqubecloud Bot commented Jun 30, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@dereknance
[PM-27720] feat: Add bitwarden-managed-settings-types crate
ff956ec 
Introduce the ManagementProfile snapshot and ManagedSettingsError read
error for the device-management (Managed Settings) framework. These are
client configuration forced by an OS Unified Endpoint Management channel,
not Vault Data, and involve no cryptography.

The crate deliberately does not depend on bitwarden-core in order to break
a dependency cycle: bitwarden-core must name the shared profile type while
the higher-level handle and override API depend on bitwarden-core.
@dereknance
[PM-27720] feat: Add managed-settings profile cell to bitwarden-core
4aa2840 
Adds a shared in-memory managed-settings profile cell to InternalClient
with a managed_profile_handle() accessor, and a ClientBuilder::with_managed_profile
hook that defaults to a fresh empty cell. This is the low-level plumbing the
bitwarden-managed-settings crate builds on to share one profile cell between the
host and the SDK.
@dereknance
[PM-27720] feat: Add bitwarden-managed-settings crate
a5a7ea1 
Adds ManagedSettingsClient, a cloneable host-facing handle over the shared
in-memory management profile cell, with new/update_profile/is_managed/get plus
cell/current_profile/from_profile accessors and a feature-gated wasm_bindgen
surface. Adds the ManagedSettingsClientExt trait so a constructed bitwarden-core
Client can read its managed-settings handle back.
@dereknance
[PM-27720] feat: Wire managed-settings into SDK binding constructors
da23e98 
Threads a ManagedSettingsClient through the client-managed-token constructor so
the host shares one managed-settings profile cell with the SDK.

- bitwarden-pm: new_with_client_tokens gains a &ManagedSettingsClient parameter
  and injects its cell via ClientBuilder::with_managed_profile; adds a
  managed_settings() accessor. new/builder/new_with_sync keep their default
  empty cell.
- bitwarden-wasm-internal: the wasm_bindgen constructor gains a
  &ManagedSettingsClient parameter, re-exports the wasm ManagedSettingsClient,
  and adds a managed_settings() accessor.
- bitwarden-uniffi: adds ManagedSettingsBindingClient (uniffi::Object) and gives
  Client::new an Arc<ManagedSettingsBindingClient> parameter.
@dereknance
dereknance force-pushed the pm-27720-device-management branch from 44f97e0 to da23e98 Compare July 24, 2026 20:39
@github-actions

github-actions Bot commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: pm-27720-device-management (da23e98)

⚠️ If breaking changes are detected, a corresponding pull request addressing them must be ready for merge in the affected client repository.

Client Status Details
typescript ❌ Breaking changes detected Compilation failed with new SDK version. A corresponding pull request addressing the breaking changes must be ready for merge in bitwarden/clients. - View Details

Breaking change detection uses the build of the SDK from this branch, including any incompatibities pre-existing on or merged into this branch. Check the workflow logs to confirm.
Results update as workflows complete.

@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.42510% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.82%. Comparing base (e504886) to head (da23e98).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
crates/bitwarden-uniffi/src/managed_settings.rs 20.00% 12 Missing ⚠️
crates/bitwarden-wasm-internal/src/client.rs 0.00% 11 Missing ⚠️
...en-managed-settings/src/managed_settings_client.rs 89.28% 9 Missing ⚠️
crates/bitwarden-pm/src/lib.rs 71.42% 4 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1224      +/-   ##
==========================================
+ Coverage   85.74%   85.82%   +0.07%     
==========================================
  Files         483      490       +7     
  Lines       68364    70158    +1794     
==========================================
+ Hits        58622    60211    +1589     
- Misses       9742     9947     +205

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@addisonbeck addisonbeck added the ai-review Request a Claude code review label Jul 26, 2026
@github-actions

github-actions Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed this PoC that introduces the bitwarden-managed-settings-types and bitwarden-managed-settings crates, wires a shared Arc<RwLock<Option<ManagementProfile>>> cell through ClientBuilder/InternalClient, and threads a new managed_settings argument through the WASM and UniFFI client constructors. The design cleanly breaks the bitwarden-core dependency cycle via the split types crate, the shared-cell semantics are well documented, and the change carries strong unit test coverage across profile decoding, shared-cell observation, and builder injection. Lock-poisoning .expect(...) usage matches the established convention across the codebase, and the inlined ClientBuilder in bitwarden-pm preserves the prior ReqwestTracingMiddleware behavior. No net-new external dependencies are introduced — the two added crates are internal-only.

Code Review Details

No blocking findings.

PR Metadata Assessment

  • ❓ QUESTION: The PR carries the breaking-change label and the WASM/UniFFI constructors gain a required managed_settings argument (a client-facing breaking change), but the "Breaking Changes" description section is left empty. Consider documenting the migration path for the paired clients change.

addisonbeck
addisonbeck reviewed Jul 28, 2026
View reviewed changes
Comment thread crates/bitwarden-managed-settings/src/managed_settings_client.rs
*self
.profile
.write()
.expect("managed-settings cell poisoned") = profile;

@addisonbeck addisonbeck Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What behavior does this cause downstream? Managed settings just stop working and user settings are applied?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addisonbeck addisonbeck addisonbeck left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

ai-review Request a Claude code review breaking-change

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dereknance @addisonbeck