[PM-35785] [PM-35787] Add strongly typed data and aggregation for policies - #1219
[PM-35785] [PM-35787] Add strongly typed data and aggregation for policies#1219eliykat wants to merge 11 commits into
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 #1219 +/- ##
==========================================
+ Coverage 85.12% 85.19% +0.07%
==========================================
Files 466 469 +3
Lines 64327 64636 +309
==========================================
+ Hits 54759 55068 +309
Misses 9568 9568 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| /// Opt-in extension for policies whose data can be combined across | ||
| /// organizations. | ||
| /// | ||
| /// Implementing this trait unlocks | ||
| /// [`PolicyAggregateFilter::get_enforced_aggregate_policy`] for the policy. | ||
| /// [`NoData`] policies get a trivial implementation for free. | ||
| pub trait PolicyAggregate: PolicyData { | ||
| /// Combines multiple [`Data`](PolicyData::Data) values into a single value. | ||
| /// How those values are combined is determined by the policy. Will only be | ||
| /// called with a non-empty `items`. | ||
| fn aggregate(&self, items: Vec<Self::Data>) -> Self::Data; | ||
| } |
There was a problem hiding this comment.
Should this be implemented by the Policy definition, or by the Self::Data type? It relates more to the Data type than the policy generally.
Alternative example:
pub trait FoldableData {
fn fold_closure(result: Self, next: Self) -> Self;
}
/// if you want aggregate functionality, you implement this _instead of_ `PolicyData` (rather than in addition to)
pub trait AggregatePolicy {
type Data: Default + DeserializeOwned + FoldableData;
}And then fold_closure can be called in the iterator chain which is nice:
filtered
.iter()
.map(|p| self.get_data_or_default(p))
.fold(Self::Data::default(), |result, d| result.fold_closure(d))I'm not sure if this is better or just different. It may be less intuitive for the policy author to implement.
| /// `view.organization_id`. Both invariants should be guaranteed by | ||
| /// upstream filtering and lookup. | ||
| /// | ||
| /// # Panics |
There was a problem hiding this comment.
On panics:
- If it is a public API, avoid panics.
- If it is a non-public API, you MAY panic, if you control all callsites
⚠️ The farther the invariant enforcing code is from the code that relies on the invariant and panics, the greater the risk. Usually you don't want to have more than a handful of callsites or LoC between.
Currently it seems rather easy to get this to panic, so I'd re-design this so that it cannot happen.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-35785
https://bitwarden.atlassian.net/browse/PM-35787
📔 Objective
#964 and associated PRs added basic policy enforcement to the SDK: given a list of policies, it would tell you which should be enforced against the user.
This PR adds additional functionality to the domain:
policyView.dataJSON blob without deserializing it themselvesOverview of changes:
enforcementto house all enforcement logicfilter.rshas been largely moved toenforcement::Policybecause it defines thePolicytraitdata.rsadds trait and logic for strongly typed data handlingaggregate.rsadds trait and logic for policy aggregationTODOs:
data.rsin one PR,aggregate.rsin the next)PolicyFilter.enforcedpanic, or return results?Datatypes across the FFI boundary? I think we need aPolicyTypeenum that is used to wrap data; but we already have one that is a plain discriminator for raw policy structs, and I'm not sure how that interacts. Do we just have to have 2 separate enum definitions: one without strongly typed data and one with?🚨 Breaking Changes