Repository navigation
Mirror provider EIDs to a cookie and run pull sync as an identity provider #1489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5c0b5ea
f2b5d4c
de1877b
42d5dd0
96a248f
f0dc376
7850c89
53e829f
c671688
62f83c3
39309ff
9f1e764
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,7 +14,7 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; | |||||||||||||||||||||||||||||||||||||||||||
| use validator::{Validate, ValidationError, ValidationErrors}; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| use crate::ec::provider::validate_enabled_configs; | ||||||||||||||||||||||||||||||||||||||||||||
| use crate::ec::registry::PartnerRegistry; | ||||||||||||||||||||||||||||||||||||||||||||
| use crate::ec::registry::{PULL_SYNC_KIND, PartnerRegistry}; | ||||||||||||||||||||||||||||||||||||||||||||
| use crate::error::TrustedServerError; | ||||||||||||||||||||||||||||||||||||||||||||
| use crate::integrations::{ | ||||||||||||||||||||||||||||||||||||||||||||
| adserver_mock::AdServerMockConfig, | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -352,6 +352,20 @@ fn validate_identity_providers(ec: &Ec) -> Result<(), Report<TrustedServerError> | |||||||||||||||||||||||||||||||||||||||||||
| if ec.identity_providers.is_empty() { | ||||||||||||||||||||||||||||||||||||||||||||
| return Ok(()); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| // Pull-sync providers come only from partner config, which carries the | ||||||||||||||||||||||||||||||||||||||||||||
| // pull token and rate limit; configuring the kind directly bypasses both. | ||||||||||||||||||||||||||||||||||||||||||||
| if let Some(config) = ec | ||||||||||||||||||||||||||||||||||||||||||||
| .identity_providers | ||||||||||||||||||||||||||||||||||||||||||||
| .iter() | ||||||||||||||||||||||||||||||||||||||||||||
| .find(|config| config.kind() == PULL_SYNC_KIND) | ||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||
| return Err(Report::new(TrustedServerError::Configuration { | ||||||||||||||||||||||||||||||||||||||||||||
| message: format!( | ||||||||||||||||||||||||||||||||||||||||||||
| "ec.identity_providers: provider `{}` uses kind `{PULL_SYNC_KIND}`; configure pull sync on its [[ec.partners]] entry instead", | ||||||||||||||||||||||||||||||||||||||||||||
| config.id | ||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||
| })); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| let partners = PartnerRegistry::from_ec(ec)?; | ||||||||||||||||||||||||||||||||||||||||||||
| validate_enabled_configs(&ec.identity_providers, &partners).map_err(|report| { | ||||||||||||||||||||||||||||||||||||||||||||
| let detail = report | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -363,7 +377,24 @@ fn validate_identity_providers(ec: &Ec) -> Result<(), Report<TrustedServerError> | |||||||||||||||||||||||||||||||||||||||||||
| report.change_context(TrustedServerError::Configuration { | ||||||||||||||||||||||||||||||||||||||||||||
| message: format!("ec.identity_providers: {detail}"), | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||
| })?; | ||||||||||||||||||||||||||||||||||||||||||||
| // Pull-sync partners run as providers too; a source claimed by both would | ||||||||||||||||||||||||||||||||||||||||||||
| // fail the coordinator build on every request. | ||||||||||||||||||||||||||||||||||||||||||||
| for config in ec.identity_providers.iter().filter(|config| config.enabled) { | ||||||||||||||||||||||||||||||||||||||||||||
| if let Some(source) = config.sources.iter().find(|source| { | ||||||||||||||||||||||||||||||||||||||||||||
| partners | ||||||||||||||||||||||||||||||||||||||||||||
| .find_by_source_domain(source) | ||||||||||||||||||||||||||||||||||||||||||||
| .is_some_and(|partner| partner.pull_sync_enabled && partner.pull_sync_url.is_some()) | ||||||||||||||||||||||||||||||||||||||||||||
| }) { | ||||||||||||||||||||||||||||||||||||||||||||
| return Err(Report::new(TrustedServerError::Configuration { | ||||||||||||||||||||||||||||||||||||||||||||
| message: format!( | ||||||||||||||||||||||||||||||||||||||||||||
| "ec.identity_providers: source `{source}` of provider `{}` is also claimed by a pull-sync partner", | ||||||||||||||||||||||||||||||||||||||||||||
| config.id | ||||||||||||||||||||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||||||||||||||||||||
| })); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| fn validate_js_asset_proxy_config(settings: &Settings) -> Result<(), Report<TrustedServerError>> { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1399,6 +1430,45 @@ gam_network_id = "99999" | |||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||
| fn runtime_validation_rejects_source_claimed_by_provider_and_pull_sync() { | ||||||||||||||||||||||||||||||||||||||||||||
| let mut settings = settings_with_identity_provider(serde_json::json!({})); | ||||||||||||||||||||||||||||||||||||||||||||
| settings.ec.partners = vec![ | ||||||||||||||||||||||||||||||||||||||||||||
| serde_json::from_value(serde_json::json!({ | ||||||||||||||||||||||||||||||||||||||||||||
| "name": "ids", | ||||||||||||||||||||||||||||||||||||||||||||
| "source_domain": "ids.example.com", | ||||||||||||||||||||||||||||||||||||||||||||
| "pull_sync_enabled": true, | ||||||||||||||||||||||||||||||||||||||||||||
| "pull_sync_url": "https://sync.example.com/pull", | ||||||||||||||||||||||||||||||||||||||||||||
| "pull_sync_allowed_domains": ["sync.example.com"], | ||||||||||||||||||||||||||||||||||||||||||||
| "ts_pull_token": "example-pull-token-32-bytes-minimum", | ||||||||||||||||||||||||||||||||||||||||||||
| })) | ||||||||||||||||||||||||||||||||||||||||||||
| .expect("should build partner"), | ||||||||||||||||||||||||||||||||||||||||||||
| ]; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| let err = validate_settings_for_runtime(&settings) | ||||||||||||||||||||||||||||||||||||||||||||
| .expect_err("should reject a source claimed twice"); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||||||||||||||||||||
| err.to_string().contains("pull-sync partner"), | ||||||||||||||||||||||||||||||||||||||||||||
| "should explain the double claim: {err:?}" | ||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔧 Cover the The new check in
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||
| fn runtime_validation_rejects_a_directly_configured_pull_sync_provider() { | ||||||||||||||||||||||||||||||||||||||||||||
| let settings = settings_with_identity_provider(serde_json::json!({ | ||||||||||||||||||||||||||||||||||||||||||||
| "kind": PULL_SYNC_KIND, | ||||||||||||||||||||||||||||||||||||||||||||
| })); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| let err = validate_settings_for_runtime(&settings) | ||||||||||||||||||||||||||||||||||||||||||||
| .expect_err("should reject the pull-sync kind in identity providers"); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| assert!( | ||||||||||||||||||||||||||||||||||||||||||||
| err.to_string().contains("[[ec.partners]]"), | ||||||||||||||||||||||||||||||||||||||||||||
| "should point to partner config: {err:?}" | ||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| #[test] | ||||||||||||||||||||||||||||||||||||||||||||
| fn runtime_validation_rejects_provider_source_without_partner() { | ||||||||||||||||||||||||||||||||||||||||||||
| let settings = settings_with_identity_provider(serde_json::json!({ | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔧 Skip the after-response row read when nothing is due
This line sends every pull-sync deployment through
run_after_response_lookups(main.rs:730). That function callsgraph.load_snapshot(main.rs:761) before it dispatches. Finalize issues the follow-up permit when the page-load write "succeeded or had nothing to change" (finalize.rs:221-224), so a steady-state returning visitor reaches that read on every navigation.The legacy dispatcher avoided this read on purpose. It filtered partners against the request snapshot first ("a dispatch with nothing to pull must not add a KV operation", base
ec/pull_sync.rs:158-160), so a row that held every pull UID cost no KV operation. Now a returning visitor whose pull sources are all held costs two reads and no partner request: this one, plus thecommit_after_response_lookupsre-read from the open thread onpull_sync.rs:60.Proposed fix (apply manually: touches
coordinator/mod.rsandmain.rs, outside this hunk; not compiled). Check the request's snapshot, which finalize leaves holding the committed row, and return before the live read when no after-response provider has a due source. A source held in that snapshot is still held in the live row unless consent was withdrawn, and a withdrawal blocks the lookup anyway.A
NotReadorFailedrequest snapshot yields no entry, so every source counts as due and the live read still runs.