Skip to content
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

feat: Add ability to track messages based on known VAPID keys #739

Merged
merged 17 commits into from
Sep 20, 2024

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented Jul 24, 2024

Note: This introduces a new setting for autoendpoint: tracking_keys
This is a JSON formatted list of the "raw"/x962 formatted VAPID public keys that should be monitored for Push tracking and follows the same format as other key data fields.

The newly added script/convert_pem_to_x962.py script aids by reading a VAPID public key PEM file and outputing a x962 formatted string.

Closes: SYNC-4349

/// Should this subscription update be tracked internally?
/// (This should ONLY be applied for messages that match known
/// Mozilla provided VAPID public keys.)
pub tracking_id: Option<String>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed from a simple bool to an Option<String> since that's most likely the final form of this. For now, we're just using a "filler" UUID4, but I strongly suspect that https://mozilla-hub.atlassian.net/browse/SYNC-3400 will introduce a more formalized tracking_id generator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the comment still reflects the bool -- so it needs an update

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is an option, so it has bool like properties (None vs. Some). I'll clarify the comment.

scripts/convert_pem_to_x962.py Outdated Show resolved Hide resolved
scripts/convert_pem_to_x962.py Outdated Show resolved Hide resolved
autoendpoint/src/extractors/subscription.rs Outdated Show resolved Hide resolved
/// PEM format into appropriate x962 format.
pub tracking_keys: String,
/// Cached, parsed tracking keys.
//TODO: convert to decoded Vec<u8>?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't seem like a necessary TODO?

Suggested change
//TODO: convert to decoded Vec<u8>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It kinda is.
Ideally, the key we track should be the bytearray of the decoded public key. This would prevent things like case confusion or formatting errors. Since we are using a consistently formatted, well known key, we can short cut things for now and just do string matches, but that's very much a cheat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but I think we can still kill this tracking_vapid_pubs field entirely since we're only calling tracking_keys() once at startup now. Then the comment should probably move to that method

pub tracking_keys: String,
/// Cached, parsed tracking keys.
//TODO: convert to decoded Vec<u8>?
pub tracking_vapid_pubs: Vec<String>,
Copy link
Member

@pjenvey pjenvey Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't an actual setting I think it should live in AppState instead. Then fn tracking_keys won't need to do caching.

(the existing fn auth_keys should probably similarly live in AppState too instead of it reparsing the setting every time but I don't think it's that heavily called).

It'll make it more annoying to test -- but can we just wrap it in a quick tuple struct with an is_trackable method to avoid needing an AppState for the test?

struct VapidTracker(Vec<String);
impl VapidTracker {
    pub fn is_trackable(...
}

/// Should this subscription update be tracked internally?
/// (This should ONLY be applied for messages that match known
/// Mozilla provided VAPID public keys.)
pub tracking_id: Option<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the comment still reflects the bool -- so it needs an update

/// PEM format into appropriate x962 format.
pub tracking_keys: String,
/// Cached, parsed tracking keys.
//TODO: convert to decoded Vec<u8>?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but I think we can still kill this tracking_vapid_pubs field entirely since we're only calling tracking_keys() once at startup now. Then the comment should probably move to that method

Comment on lines 217 to 222
.map(|s| {
s.to_str()
.unwrap_or(uuid::Uuid::new_v4().as_simple().to_string().as_str())
.to_string()
})
.unwrap_or(uuid::Uuid::new_v4().as_simple().to_string())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These chains can be a bit much but I'd argue utilzing and_then/ok improves this since we'll only need new_v4 at the very end (also surprised clippy didn't force unwrap_or_else here).

Suggested change
.map(|s| {
s.to_str()
.unwrap_or(uuid::Uuid::new_v4().as_simple().to_string().as_str())
.to_string()
})
.unwrap_or(uuid::Uuid::new_v4().as_simple().to_string())
.and_then(|val| val.to_str().ok())
.map(|s| s.to_owned())
.unwrap_or_else(|| uuid::Uuid::new_v4().as_simple().to_string())

@jrconlin jrconlin merged commit b525601 into master Sep 20, 2024
1 check passed
@jrconlin jrconlin deleted the feat/SYNC-4349_track branch September 20, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants