Skip to content

[PM-35785] Add strongly typed policy data - alternative implementation - #1239

Closed
eliykat wants to merge 13 commits into
mainfrom
policy-data-v2
Closed

[PM-35785] Add strongly typed policy data - alternative implementation#1239
eliykat wants to merge 13 commits into
mainfrom
policy-data-v2

Conversation

@eliykat

@eliykat eliykat commented Jul 10, 2026

Copy link
Copy Markdown
Member

🎟️ Tracking

📔 Objective

An alternative approach to #1219 based on @quexten 's feedback.

The approach in main, extended in the previous PR, is to have a registry struct where policy rules can be registered and dynamically looked up at runtime. This was intended to allow other teams to write their own rules and register them in our engine. However, this dynamic runtime behavior was complex and used generics which don't travel over the FFI boundary (an unsolved problem for that approach).

This PR takes a step back and tries to reimplement this behavior in a more simple and idiomatic way. It has breaking changes and is missing tests, so it is NOT intended for merge as-is. It demonstrates the end state as if it replaced the current implementation entirely.

Notes:

  • the registry and dynamic lookup are deleted entirely
  • added a new EnrichedPolicyType enum, which also represents each PolicyType, but carries its own strongly-typed data. This is effectively the "registry", as it has internal match lookups for:
    • converting from a PolicyType + json blob to the enriched type
    • getting the PolicyDefinition enforcement rules
  • naming tweak for clarity: the Policy trait is now PolicyDefinition, because it's metadata that 'defines' how a policy works (open to other naming, naming is hard etc.)
  • policy_definitions.rs contains both the per-policy data structs and the PolicyDefinition trait implementations. I think it's nice that we can use the data struct to also implement the trait - it keeps per-policy configuration together. (It's this file that could be split up and distributed among other feature teams.)
  • overall seems simpler and easier to understand to me.

Remaining challenges:

  • the cross-team interface issue: this effectively abandons the idea of an interface for other teams - they have to edit the internals of our implementation to add their policy. Maybe this is worth the tradeoff, but then we still need to make it as easy and as simple as possible for them, minimizing what they have to edit. I think we can iterate on this with macros if needed to minimize the boilerplate in these match statements.
  • callers will expect to get back a certain policy type. e.g. if they call filter(policies, contexts, PolicyType.MasterPassword), they will expect back all MasterPassword-type policies with corresponding data; not policies containing enums of unknown values that they then have to filter/match on. I'm not sure how to reconcile this. It may be a type cast that native code has to do.

Todos:

  • determine the best serde option for exposing the enriched enum in TS - maybe internal tagging.
  • if this is the best way forward, it needs to be made non-breaking. It should be able to sit behind the current public interface and just map back to a PolicyView.
  • add tests to verify no change in behavior.

🚨 Breaking Changes

@eliykat eliykat changed the title Policy data v2 [PM-35785] Add strongly typed policy data - alternative implementation Jul 10, 2026
@eliykat
eliykat requested a review from quexten July 10, 2026 05:58
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

🔍 SDK Breaking Change Detection

SDK Version: policy-data-v2 (8b1062e)

⚠️ 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 ✅ No breaking changes detected Compilation passed with new SDK version - View Details
android ❌ Breaking changes detected Compilation failed with new SDK version. A corresponding pull request addressing the breaking changes must be ready for merge in bitwarden/android. - 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 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 52.34899% with 71 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.90%. Comparing base (b154bdb) to head (669acde).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
...tes/bitwarden-policies/src/enriched_policy_type.rs 28.57% 30 Missing ⚠️
...rates/bitwarden-policies/src/policy_definitions.rs 36.84% 24 Missing ⚠️
crates/bitwarden-policies/src/policy_client.rs 75.86% 7 Missing ⚠️
crates/bitwarden-policies/src/policy_definition.rs 50.00% 6 Missing ⚠️
crates/bitwarden-uniffi/src/policies.rs 0.00% 3 Missing ⚠️
crates/bitwarden-policies/src/enriched_policy.rs 96.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1239      +/-   ##
==========================================
- Coverage   85.10%   84.90%   -0.21%     
==========================================
  Files         476      477       +1     
  Lines       65928    65520     -408     
==========================================
- Hits        56109    55630     -479     
- Misses       9819     9890      +71     

☔ 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.

Comment on lines +11 to +12
use bitwarden_send::SendType;
use bitwarden_vault::UriMatchType;

@eliykat eliykat Jul 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unclear whether our feature crate can import other feature crates like this. Their crate will almost certainly depend on this crate (to enforce policies in their domain), so it would be a circular dep. Not sure how this could be avoided.

This also causes the uniffi issues.

@eliykat

eliykat commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

Closed in favour of #1364.

@eliykat eliykat closed this Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant