bitwarden
[PM-17562] Add integration filter support
🎟️ Tracking
📔 Objective
This is a POC of an idea for adding arbitrary filters to our existing event integrations. The idea would be to give organization admins the ability to add a filter to a configuration that would filter out events (e.g. I want to listen for these key events, but only when it happens in a specific set of high priority collections). I'm new to expression trees and this is a first draft POC, so feel free to provide as much feedback as you want.
Here's the basic structure of the idea:
- We add a Filters column in the
OrganizationIntegrationConfigurationtable - alongside where we store Template. (Note: I haven't added this piece yet since I wanted to get feedback first on this approach. The property I pull out isstring?so for now it's fine and I'm just doing a bunch in unit tests) - That Filters column is a JSON structure that stores a filter group - basically a collection of Rules and whether it's
alloranyin evaluating them. Rules are just the property, the operation and the value. So you can say "CollectionId, In, a list of values". Or "CollectionId, Equals, exact value". Right now I have in, equals and before/after for dates. - When the
EventIntegrationHandlerpulls the DB configuration, this would just be another column in the object that comes back (and would get cached as well when we get to that). So no extra load on the DB or new call. - The
EventIntegrationHandlerserializes the Filters property and hands it to the filter service. The filter service has a set of compiled expression trees that it can use to quickly run the filter calculation. Since we have a limited set of properties onEventMessagewe can build a handful of useful expressions to evaluate. If we get a better idea of what admins might like to be filtering, we could probably adjust that as well. - At runtime, we just run the compiled expression, passing in the dynamic set of values. So for example, CollectionId, In, set of values - the CollectionId property is already part of the compiled expressions - we build that ahead of time. But the list of values is provided by the Filters at runtime from the DB. So we can efficiently run the query without needing to reflect.
- The outcome of that expression tree evaluation is a bool that comes back to
EventIntegrationHandler- if it's false, we simply move on and never publish the message that would kick off the integration. If it's true, we proceed as we would have previously.
So net-net, this should give us somewhat arbitrary filters without really any performance change to what we're doing now. We don't add any more load on the DB and we hopefully don't add much performance load to the handler due to the expression trees.
⏰ Reminders before review
- Contributor guidelines followed
- All formatters and local linters executed and passed
- Written new unit and / or integration tests where applicable
- Protected functional changes with optionality (feature flags)
- Used internationalization (i18n) for all UI strings
- CI builds passed
- Communicated to DevOps any deployment requirements
- Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team
🦮 Reviewer guidelines
- 👍 (
:+1:) or similar for great changes - 📝 (
:memo:) or ℹ️ (:information_source:) for notes or general info - ❓ (
:question:) for questions - 🤔 (
:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (
:art:) for suggestions / improvements - ❌ (
:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention - 🌱 (
:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt - ⛏ (
:pick:) for minor or nitpick changes
Checkmarx One – Scan Summary & Details – a81fc988-79ab-4553-972e-014d3d9c22d6
New Issues (5)
Checkmarx found the following issues in this Pull Request
| Severity | Issue | Source File / Package | Checkmarx Insight |
|---|---|---|---|
 |
Path_Traversal | /src/Api/Tools/Controllers/SendsController.cs: 211 | detailsMethod PostFile at line 211 of /src/Api/Tools/Controllers/SendsController.cs gets dynamic data from the model element. This element’s value then ...ID: keY0wh5knH3DnvbREmfJIGGig2k%3D |
|
Path_Traversal | /src/Api/Tools/Controllers/SendsController.cs: 211 | detailsMethod PostFile at line 211 of /src/Api/Tools/Controllers/SendsController.cs gets dynamic data from the model element. This element’s value then ...ID: 4QIsZ8agHxuL%2BFjdf49NpZJHsvU%3D |
|
Path_Traversal | /src/Api/Tools/Controllers/SendsController.cs: 211 | detailsMethod PostFile at line 211 of /src/Api/Tools/Controllers/SendsController.cs gets dynamic data from the model element. This element’s value then ...ID: Uwek2GQ3fAsDgtQkgYKs2Vkocog%3D |
|
Path_Traversal | /src/Api/Tools/Controllers/SendsController.cs: 211 | detailsMethod PostFile at line 211 of /src/Api/Tools/Controllers/SendsController.cs gets dynamic data from the model element. This element’s value then ...ID: OxfkMdVQRO3StOFifYOfb36o2%2Bw%3D |
|
Path_Traversal | /src/Api/Tools/Controllers/SendsController.cs: 211 | detailsMethod PostFile at line 211 of /src/Api/Tools/Controllers/SendsController.cs gets dynamic data from the model element. This element’s value then ...ID: HvNxhQLOT4P0U2Lbp6HqePXg%2FNQ%3D |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Codecov Report
Attention: Patch coverage is 86.95652% with 24 lines in your changes missing coverage. Please review.
Project coverage is 51.11%. Comparing base (
b951b38) to head (995efa0). Report is 1 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #5971 +/- ##
==========================================
+ Coverage 47.68% 51.11% +3.42%
==========================================
Files 1693 1697 +4
Lines 75080 75243 +163
Branches 6759 6781 +22
==========================================
+ Hits 35801 38457 +2656
+ Misses 37823 35261 -2562
- Partials 1456 1525 +69
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
I think we should add a feature flag for this.
Yep, and we do. None of this is on anywhere but dev and it's not running in our cloud instance since no Service Bus is configured.
I may still be several days away from getting to this, so I've requested a review from @r-tome and @jrmccannon in case they have the capacity to look at it before me. I definitely think it's worth the additional review though.
At a glance, is there a UI for configuring integrations (including the new filter capability), or are you just editing the db directly at this stage?
@eliykat
At a glance, is there a UI for configuring integrations (including the new filter capability), or are you just editing the db directly at this stage?
Sorry I missed this comment earlier. There is no UI for adding these yet, but there is an API - OrganizationIntegrationConfigurationController.
Quality Gate passed
Issues
3 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
1.2% Duplication on New Code
See analysis details on SonarQube Cloud
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}