bug: add app_id to error message #303

jrconlin · 2022-04-20T20:01:55Z

This patch does two things in order to determine the spike in InvalidAppId messages:

Add the sent app_id to the InvalidAppId error message
strip quotes from the app_id (in case this is a json string
transposition error.)

Closes #302

This patch does two things in order to determine the spike in `InvalidAppId` messages: 1) Add the sent app_id to the InvalidAppId error message 2) strip quotes from the app_id (in case this is a json string transposition error.) Closes #302

autoendpoint/src/routers/fcm/router.rs

autoendpoint/src/routers/apns/router.rs

Add minimal handling of GCM data sends. This needs to be tested on stage (if possible. It's no longer possible to create GCM tokens to validate). Note that this overloads "FCM" settings. See router::fcm::settings::FcmSettings for details. Closes: CONSVC-1765

autoendpoint/src/routers/common.rs

pjenvey · 2022-05-02T22:49:49Z

autoendpoint/src/routers/fcm/router.rs

+            Some(v) => v.to_owned(),
+            None => {
+                if creds.is_none() {
+                    return Err(FcmError::NoRegistrationToken.into());


Suggested change

return Err(FcmError::NoRegistrationToken.into());

return Err(FcmError::NoAppId.into());

And the same for line # 114

argh. thanks!

autoendpoint/src/routers/fcm/client.rs

autoendpoint/src/routers/fcm/settings.rs

pjenvey · 2022-05-02T23:12:24Z

autoendpoint/src/routers/fcm/settings.rs

@@ -11,6 +11,24 @@ pub struct FcmSettings {
    pub min_ttl: usize,
    /// A JSON dict of `FcmCredential`s. This must be a `String` because
    /// environment variables cannot encode a `HashMap<String, FcmCredential>`
+    /// This contains both GCM and FCM credentials.


We should really mention how they're distinguished here as well (existence of "{" vs base64)) with the examples below reflecting that, but I'm fine deferring this to a separate issue though.

~~I thought I did that at line 27, where I have credentials for both, or do you mean something else?~~

Ah! I see what you meant. Editing.

... and I found out that endpoint isn't optionally treating the credential value as a path identifier, like what the python version was doing.

pjenvey · 2022-05-02T23:23:12Z

autoendpoint/src/routers/fcm/router.rs

+                if creds.is_none() {
+                    return Err(FcmError::NoRegistrationToken.into());
+                }
+                match creds


Is pulling from creds.get("auth") even necessary? The Python gcm.py router seems to always use router_data.get("token").

GCM stores the server credential auth token as auth. This is different from the registration id value stored in token

Right, it's stored as auth in router_data, but autopush python never actually reads from it. Instead the router reads auth from its own copy supplied from settings at startup (as we do in rust, reading FCMClient's credential.credential).

Whereas here we seem to be reading it as a fallback for router_data.get("token") which is registration_id, not auth (plus I don't think router_data.get("token") should ever be returning None)

Also this value's called auth in send_gcm, maybe call it token or registration_id instead?.

So, I think a BIG part of this is that the variables have terrible names. I'm going to try and fix that.

(The following is also for my benefit...)
What gets stored into the routing information is the project_id and the users routing_token. The project_id identifies which project to use, and is later matched with the project_id in the settings to get the corresponding server_access_token (which may be a value, a JSON block, or a file descriptor). FCM uses an authenticator function, and GCM needs to have access to the server_credentials, so both of those are stored into the FcmClient when it's initialized.

* Realized that autoendpoint was not reading credentials from files. This is different behavior from the python version and SRE would appreciate the option.

autoendpoint/src/routers/fcm/settings.rs

pjenvey · 2022-05-03T19:29:53Z

autoendpoint/src/routers/fcm/router.rs

+                if creds.is_none() {
+                    return Err(FcmError::NoRegistrationToken.into());
+                }
+                match creds


Right, it's stored as auth in router_data, but autopush python never actually reads from it. Instead the router reads auth from its own copy supplied from settings at startup (as we do in rust, reading FCMClient's credential.credential).

Whereas here we seem to be reading it as a fallback for router_data.get("token") which is registration_id, not auth (plus I don't think router_data.get("token") should ever be returning None)

Also this value's called auth in send_gcm, maybe call it token or registration_id instead?.

* clean up generic GCM/FCM auth terms to be clearer about what's going on.

pjenvey

Rest of it looks good

autoendpoint/src/routers/fcm/client.rs

jrconlin · 2022-05-04T22:41:01Z

Fuuuuuuuuuuuu...

bug: add app_id to error message

fa09096

This patch does two things in order to determine the spike in `InvalidAppId` messages: 1) Add the sent app_id to the InvalidAppId error message 2) strip quotes from the app_id (in case this is a json string transposition error.) Closes #302

jrconlin requested review from pjenvey and a team April 20, 2022 20:01

jrconlin added 2 commits April 20, 2022 14:57

f clippy

5ae16f9

f fall back to gcm values

2c84669

pjenvey requested changes Apr 25, 2022

View reviewed changes

autoendpoint/src/routers/fcm/router.rs Outdated Show resolved Hide resolved

autoendpoint/src/routers/fcm/router.rs Outdated Show resolved Hide resolved

autoendpoint/src/routers/apns/router.rs Outdated Show resolved Hide resolved

jrconlin added 2 commits April 25, 2022 18:29

f read the credentials from the sub hash

8916472

f Add minimal GCM handling.

dd01040

Add minimal handling of GCM data sends. This needs to be tested on stage (if possible. It's no longer possible to create GCM tokens to validate). Note that this overloads "FCM" settings. See router::fcm::settings::FcmSettings for details. Closes: CONSVC-1765

jrconlin requested a review from pjenvey April 27, 2022 23:42

jrconlin added 5 commits April 27, 2022 16:45

f remove quote trimming

03a5ca9

f comment out ureq and add mockito debug doc pointer

36376c3

f add nonce comment to the fake auth_key values in testing

bbceaba

f add comments

ae77ccc

f fmt

f094e45

pjenvey requested changes May 2, 2022

View reviewed changes

f r's

da6802c

* Realized that autoendpoint was not reading credentials from files. This is different behavior from the python version and SRE would appreciate the option.

jrconlin requested a review from pjenvey May 3, 2022 17:25

pjenvey reviewed May 3, 2022

View reviewed changes

f r's

b76cab5

* clean up generic GCM/FCM auth terms to be clearer about what's going on.

jrconlin requested a review from pjenvey May 4, 2022 21:22

f remove fallback for token

3036809

pjenvey reviewed May 4, 2022

View reviewed changes

autoendpoint/src/routers/fcm/client.rs Outdated Show resolved Hide resolved

f read_to_string

7f1a856

pjenvey previously approved these changes May 4, 2022

View reviewed changes

f fix test

bd3009a

jrconlin dismissed pjenvey’s stale review via bd3009a May 4, 2022 22:47

jrconlin requested a review from pjenvey May 4, 2022 22:47

pjenvey approved these changes May 4, 2022

View reviewed changes

jrconlin merged commit d2cd2ee into master May 4, 2022

jrconlin deleted the bug/302-appid branch May 4, 2022 23:14

jrconlin mentioned this pull request May 5, 2022

chore: tag 1.62.0 #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bug: add app_id to error message #303

bug: add app_id to error message #303

jrconlin commented Apr 20, 2022

pjenvey May 2, 2022

jrconlin May 3, 2022

pjenvey May 2, 2022

jrconlin May 3, 2022 •

edited

Loading

jrconlin May 3, 2022

pjenvey May 2, 2022

jrconlin May 3, 2022

pjenvey May 3, 2022

jrconlin May 4, 2022

pjenvey May 3, 2022

pjenvey left a comment

jrconlin commented May 4, 2022

	return Err(FcmError::NoRegistrationToken.into());
	return Err(FcmError::NoAppId.into());

bug: add app_id to error message #303

bug: add app_id to error message #303

Conversation

jrconlin commented Apr 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrconlin May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey left a comment

Choose a reason for hiding this comment

jrconlin commented May 4, 2022

jrconlin May 3, 2022 •

edited

Loading