[PM-35785] Strongly typed policy enforcement - #1364
Conversation
🔍 SDK Breaking Change DetectionSDK Version:
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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1364 +/- ##
==========================================
- Coverage 86.28% 86.15% -0.13%
==========================================
Files 500 501 +1
Lines 73417 73852 +435
==========================================
+ Hits 63349 63629 +280
- Misses 10068 10223 +155 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8f16df9 to
fbaf458
Compare
Remove PolicyRegistry and its builder in favor of PolicyType::resolve_policy, a match over all variants. Add concrete Policy impls for every policy type and move the filtering trait into policy.rs. Behavior-preserving.
Introduce the typed enforcement model (EnforcedPolicy, EnforcedPolicyErased, ResolvedPolicyView, PolicyDataType) and expose get_enforced / get_many_enforced plus their FFI-erased variants on PolicyClient and the uniffi client. Reimplement filter_by_type on top of the enforced path.
fbaf458 to
a49f26b
Compare
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the Code Review DetailsNo findings at or above the reporting threshold. All findings from the earlier review passes are confirmed addressed on this head: the disabled-policy/missing-context gate in Dependency Changes
Both are existing workspace dependencies added via |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
djsmith85
left a comment
djsmith85
left a comment
There was a problem hiding this comment.
Platform-owned changes are limited to the added integration tests.
Please extend the CODEOWNERS file with this line
crates/bitwarden-wasm-internal/integration-tests/tests/policies/** @bitwarden/team-admin-console-dev
djsmith85
left a comment
djsmith85
left a comment
There was a problem hiding this comment.
Approving for platform, as there's no longer any changes platform owns.
@sven-bitwarden Please ensure to also review crates/bitwarden-wasm-internal/integration-tests/tests/policies/policy-client.test.ts as the CODEOWNERS changes are only enforced after this has merged.
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
One design note: there are separate client interfaces for native rust vs. FFI consumers. This is to accommodate the per-policy data. As the native rust interface shows, we can resolve this at compile time (using generics) for consumers, and I want to keep that as the first-class interface rather than forcing everyone to use the enum and have redundant match statements everywhere.
IMO I'm not sure whether that's worth the complexity of maintaining two different APIs. It's easy enough to just match with a default catch arm. In rust this should be expected / reasonable to work with.
Anyways, interested to see how this feels when it is more widely used by other consumers, maybe we'll double down on it, or revise.
| }; | ||
| } | ||
|
|
||
| describe("PolicyClient", () => { |
There was a problem hiding this comment.
Nice! Testing the FFI boundary!
| /// | ||
| /// Untyped FFI path: native/WASM callers pass a runtime `policy_type` integer. | ||
| /// Delegates to the registry, falling back to default rules for unknown types. | ||
| /// Not yet intended for consumer use: exposed across the FFI boundary for |
There was a problem hiding this comment.
I wonder whether we should have a separate feature we compile the wasm build with that exposes these (and other test only) methods, but that is not enabled in prod builds.
| /// | ||
| /// Defaults to [`Owner`](OrganizationUserType::Owner) and | ||
| /// [`Admin`](OrganizationUserType::Admin). | ||
| fn exempt_roles(&self) -> &[OrganizationUserType] { |
There was a problem hiding this comment.
I would argue that having a default makes this harder to reason about. It's a general problem with OOP in TS where subclasses make it very hard to reason about the full behavior. We usually do not have that problem in rust because we don't have inheritance.
IMO each type should explicitly state the excempt roles.
(However feel free to disagree and ignore).
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-35785
📔 Objective
Iterate on the policies crate to provide a better policy enforcement interface. This is done in 2 ways:
EnforcedPolicy, rather than simply filteringPolicyViews out of an array (which is whatfilter_by_typedoes today)This PR is fairly large, but the commits are ordered to try to separate interesting vs. uninteresting changes. The commits, in order:
filter_by_typeinterface before refactoring - to ensure we don't cause a regression in existing consumers.PolicyTypeenum, which looks up the correspondingPolicyimplementation. Add implementations for each policy we have today.See the expanded README for an overview of the crate and how to add and consume policies.
One design note: there are separate client interfaces for native rust vs. FFI consumers. This is to accommodate the per-policy data. As the native rust interface shows, we can resolve this at compile time (using generics) for consumers, and I want to keep that as the first-class interface rather than forcing everyone to use the enum and have redundant match statements everywhere.
The next step after this PR would be to introduce state so that consumers are not passing in the policyViews and organization contexts. The new interfaces are not intended to be used yet, as that will be a breaking change.
🚨 Breaking Changes
clients- missing anasUuidcall. Will be fixed by [PM-35785] Fix SDK breaking change: missing asUuid clients#22392 after merge.