explicit rules always - #1354
Conversation
* Connector spec and first test * Files updated * Updated files * Fix link * Rules update * Docs and connector spec * Missing tests * Rules updated * Updated test * Adding more variables * Updated example and marked as beta * Rules update * Rules update * Adding changelog * Lint * Drop versions --------- Co-authored-by: Erik Schultink <eschultink@users.noreply.github.com>
Wire rules_file for all connectors in worklytics-connector-specs (using no-app-ids variants for MSFT) and log a one-time deprecation warning when the proxy falls back to prebuilt Java rules. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR makes connector sanitization rules selection explicit by referencing YAML rule files in the worklytics-connector-specs Terraform module, and adds a runtime warning when falling back to prebuilt Java rules (to discourage implicit/default behavior).
Changes:
- Add
rules_filereferences for Microsoft 365, Google Workspace, and multiple OAuth/bulk connectors so rules can be resolved from YAML rather than inline/defaults. - Update Terraform tests to reflect that
hrisnow usesrules_fileand should producerules_rawwhenbase_diris provided. - Adjust Java rules-loading fallback to emit a warning when falling back to prebuilt Java rules.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| java/core/src/main/java/co/worklytics/psoxy/ConfigRulesModule.java | Adds explicit warning on fallback to prebuilt Java rules and simplifies fallback path. |
| infra/modules/worklytics-connector-specs/tests/rules_validation.tftest.hcl | Updates assertions to match hris switching to rules_file/rules_raw. |
| infra/modules/worklytics-connector-specs/msft-365.tf | Adds rules_file references for Microsoft 365 connectors (defaulting to no-app-ids/no-userIds variants). |
| infra/modules/worklytics-connector-specs/main.tf | Adds rules_file for many connectors and migrates some bulk connectors from inline rules to YAML-backed rules. |
| infra/modules/worklytics-connector-specs/google-workspace.tf | Adds rules_file references for Google Workspace connectors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.warning(String.format( | ||
| "No custom RULES found (env var, parameter store, secret manager, GCS, or S3); " | ||
| + "falling back to prebuilt Java rules for SOURCE=%s. " | ||
| + "This fallback will be removed in v0.7; rules must be explicitly configured.", | ||
| rulesKey)); |
Rename pseudonymizing MSFT rules YAML to the base filenames, move clear-text variants to *_inc-app-ids.yaml, and update Terraform, Java, tests, and CHANGELOG. Co-authored-by: Cursor <cursoragent@cursor.com>
Restore loadAndLog on prebuilt-rules fallback, simplify the deprecation warning, and move 0.6.8 release notes out of the 0.6.9 section. Co-authored-by: Cursor <cursoragent@cursor.com>
…perty. Host modules resolve rules paths locally and accept inline rules for hosted deployments, deprecating rules_raw on api_connectors. Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid coalesce() in the for-expression filter when both rules and rules_raw are unset, which failed gcp-host concurrency tests. Co-authored-by: Cursor <cursoragent@cursor.com>
validateYamlExample compares Java prebuilt rules (with app IDs) to the _inc-app-ids YAML variants after the MSFT canonical rename. Co-authored-by: Cursor <cursoragent@cursor.com>
| - pathTemplate: "/beta/copilot/users/{id}/interactionHistory/getAllEnterpriseInteractions" | ||
| allowedQueryParams: | ||
| - "$filter" | ||
| - pathRegex: "^/beta/copilot/users/(/p~[a-zA-Z0-9_-]+?)?[^/]*/interactionHistory/getAllEnterpriseInteractions(\\\ |
There was a problem hiding this comment.
| - pathRegex: "^/beta/copilot/users/(/p~[a-zA-Z0-9_-]+?)?[^/]*/interactionHistory/getAllEnterpriseInteractions(\\\ | |
| - pathRegex: "^/beta/copilot/users/(p~[a-zA-Z0-9_-]+?)?[^/]*/interactionHistory/getAllEnterpriseInteractions(\\\ |
doesn't this have an extra forward slash for the pseudonymized case? /users//p~..../
| { | ||
| "id": "cdfb873d-61f9-4656-bbbb-a915ef1a255d", | ||
| "businessPhones": [ | ||
| "6175759475" |
There was a problem hiding this comment.
@eschultink @aperez-worklytics again this made it to a PR.
| jsonPaths: | ||
| - "$..user.id" | ||
| - "$..userId" | ||
| - "$..emailAddress" |
There was a problem hiding this comment.
@aperez-worklytics emailAddress was removed in the version with ids. there is no emailAddress in that api response I think
https://github.com/Worklytics/psoxy/blob/main/docs/sources/microsoft-365/msft-teams/example-api-responses/original/Teams_v1.0.json
There was a problem hiding this comment.
True, that comes because in the code we have that as default for all the endpoints:
in this case, emailAddress is not part of the response payload. At some point we should drop the rules built in code with the ones generated based on our request/response model.
| type = map(any) | ||
| description = "Map of configuration settings specifically for MSFT 365 connectors (e.g. test GUIDs, custom paths). Note that provider-controlling parameters (like msft_tenant_id or existing app IDs) remain top-level variables." | ||
| description = "Map of configuration settings specifically for MSFT 365 connectors (e.g. test GUIDs, custom paths). Supported keys: example_msft_user_guid, example_msft_group_guid, msft_teams_example_team_guid, msft_teams_example_channel_guid, msft_teams_example_chat_guid, msft_teams_example_call_guid, msft_teams_example_call_record_guid, msft_teams_example_online_meeting_join_url, msft_onedrive_example_drive_id, msft_onedrive_example_item_id. Note that provider-controlling parameters (like msft_tenant_id or existing app IDs) remain top-level variables." | ||
| default = {} |
There was a problem hiding this comment.
not a big deal, but asked for a quick validation instead of just listing stuff on the description
| default = {} | |
| validation { | |
| condition = length(setsubtract( | |
| toset(keys(var.msft_365_connector_settings)), | |
| toset([ | |
| "example_msft_user_guid", | |
| "example_msft_group_guid", | |
| "msft_teams_example_team_guid", | |
| "msft_teams_example_channel_guid", | |
| "msft_teams_example_chat_guid", | |
| "msft_teams_example_call_guid", | |
| "msft_teams_example_call_record_guid", | |
| "msft_teams_example_online_meeting_join_url", | |
| "msft_onedrive_example_drive_id", | |
| "msft_onedrive_example_item_id", | |
| ]) | |
| )) == 0 | |
| error_message = "msft_365_connector_settings contains unsupported keys. Supported keys are: example_msft_user_guid, example_msft_group_guid, msft_teams_example_team_guid, msft_teams_example_channel_guid, msft_teams_example_chat_guid, msft_teams_example_call_guid, msft_teams_example_call_record_guid, msft_teams_example_online_meeting_join_url, msft_onedrive_example_drive_id, and msft_onedrive_example_item_id." | |
| } |
| .sourceFamily("microsoft-365") | ||
| .defaultScopeId("azure-ad") | ||
| .sourceKind("outlook-cal") | ||
| .rulesFile("outlook-cal_inc-app-ids") |
There was a problem hiding this comment.
ok, this feels that the test names should be twisted. Not really in this PR scope, but:
CalendarTests == regular tests pseudonymizing APP IDs. (no suffix, default behavior of the rules of all connectors)
Calendar_NoAppIdTests should be renamed to Calendar_IncludingAppIdsTests.
Same applies to all MSFT test classes
| --- | ||
| endpoints: | ||
| - pathRegex: "^/v1.0/users(/p~[a-zA-Z0-9_-]+?)?[^/]*" | ||
| - pathRegex: "^/v1.0/users/?[^/]*" |
There was a problem hiding this comment.
that this shows in renames is confusing; actual change is renaming plain rules -- inc-app-ids, and no-app-ids --> plain;
but is this correct? change in endpoint pattern of this PR correct?? at least ensure covered by tests.
There was a problem hiding this comment.
I think so, these are now with plain ids. We can go with a more accurate version that in case of supporting anything in the path we go for just a GUID
| - "$..['@odata.context']" | ||
| - "$..['@odata.id']" | ||
| - "$..['@odata.type']" | ||
| - "$..['@odata.count']" |
There was a problem hiding this comment.
why do we nee to redact type/count? doesn't seem like this should be senstive or identifying?
There was a problem hiding this comment.
It was redacted in the past as it wasn't required in the model
Integrate upstream 0.6.9/0.6.10 changes while preserving explicit rules_file wiring, MSFT YAML canonical rename (pseudonymizing default, *_inc-app-ids variants), and host module rules property. Resolve OneDrive/Teams/Outlook merge conflicts, adopt rc fake PII example data, add $filter support to OneDrive rules, fix copilot pathRegex slash, improve ConfigRulesModule fallback log, and add msft_365_connector_settings validation. Co-authored-by: Erik Schultink <eschultink@users.noreply.github.com>
Add ConsistencyLevel headers and $filter query params to embedded Entra/Teams/Mail YAML endpoints so validateYamlExample matches PrebuiltSanitizerRules after the merge. Fix copilot pathRegex in Java, and restore OneDrive get_drive_delta sanitized fixture. Co-authored-by: Erik Schultink <eschultink@users.noreply.github.com>
ENTRA_ID_GROUP_MEMBERS in PrebuiltSanitizerRules allows ConsistencyLevel but not $filter; YAML must match for validateYamlExample tests. Co-authored-by: Erik Schultink <eschultink@users.noreply.github.com>
Combine 0.6.10 CHANGELOG entries from both branches; connector-specs and other files auto-merged cleanly. Co-authored-by: Cursor <cursoragent@cursor.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
| --- | ||
| endpoints: | ||
| - pathRegex: "^/v1.0/users(/p~[a-zA-Z0-9_-]+?)?[^/]*" | ||
| - pathRegex: "^/v1.0/users/?[^/]*" |
There was a problem hiding this comment.
I think so, these are now with plain ids. We can go with a more accurate version that in case of supporting anything in the path we go for just a GUID
| - "$..['@odata.context']" | ||
| - "$..['@odata.id']" | ||
| - "$..['@odata.type']" | ||
| - "$..['@odata.count']" |
There was a problem hiding this comment.
It was redacted in the past as it wasn't required in the model
| "msft_teams_example_call_guid", | ||
| "msft_teams_example_call_record_guid", | ||
| "msft_teams_example_online_meeting_join_url", | ||
| "msft_onedrive_example_drive_id", | ||
| "msft_onedrive_example_item_id", |
Summary
Prepares Psoxy for hosted proxy deployments where connector specifications live outside the proxy repo—eventually provided by a Worklytics Terraform module (sourced from the main Worklytics codebase) rather than being embedded in
worklytics-connector-specshere.This PR makes rules explicit and externally configurable end-to-end, deprecating reliance on Java prebuilt defaults.
Connector specs → host modules
worklytics-connector-specsnow declares arules_filepointing at its YAML rules (underdocs/sources/).rules_filepaths through toaws-host/gcp-host; the host modules resolve paths (viapsoxy_base_dir) and provisionRULESto SSM/Secret Manager/env. This is preferred over pre-resolving file content in connector-specs.*.yamlfiles; variants that include app/user IDs in the clear are renamed*_inc-app-ids.yaml.Rules configuration surface (
api_connectors)Two mechanisms, both supported:
rules_filerulesrules_rawis deprecated onapi_connectors(TODO: remove in v0.7); userulesinstead.Java fallback deprecation
Proxy logs a warning when falling back to prebuilt Java rules (no explicit
RULESconfigured). This fallback will be removed in v0.7.Test plan
terraform testinworklytics-connector-specsterraform validateininfra/examples-dev/awsEntraID_NoAppIds_Tests, etc.)ConfigRulesModuleTestRelease notes (0.6.9)
See
CHANGELOG.md. MSFT YAML variant filenames changed (*_inc-app-ids.yamlfor clear-text app ID variants); default pseudonymizing rules are now the canonical*.yamlfiles.PSEUDONYMIZE_APP_IDSdefaults totrue.