bug: fix Tokenserver generation and keys_changed_at handling (#1397)

syncstorage/src/settings.rs

@@ -289,6 +289,7 @@ impl Settings {
         s.set_default("tokenserver.node_type", "spanner")?;
         s.set_default("tokenserver.statsd_label", "syncstorage.tokenserver")?;
         s.set_default("tokenserver.run_migrations", cfg!(test))?;
+        s.set_default("tokenserver.token_duration", 3600)?;
 
         // Set Cors defaults
         s.set_default(

syncstorage/src/tokenserver/db/models.rs

@@ -164,7 +164,7 @@ impl TokenserverDb {
              WHERE service = ?
                AND email = ?
                AND generation <= ?
-               AND COALESCE(keys_changed_at, 0) <= COALESCE(?, 0)
+               AND COALESCE(keys_changed_at, 0) <= COALESCE(?, keys_changed_at, 0)
                AND replaced_at IS NULL
         "#;
 

syncstorage/src/tokenserver/extractors.rs

@@ -31,7 +31,6 @@ lazy_static! {
     static ref CLIENT_STATE_REGEX: Regex = Regex::new("^[a-zA-Z0-9._-]{1,32}$").unwrap();
 }
 
-const DEFAULT_TOKEN_DURATION: u64 = 5 * 60;
 const SYNC_SERVICE_NAME: &str = "sync-1.5";
 
 /// Information from the request needed to process a Tokenserver request.
@@ -98,6 +97,14 @@ impl TokenserverRequest {
             });
         }
 
+        // If the client previously reported a client state, every subsequent request must include
+        // one. Note that this is only relevant for BrowserID requests, since OAuth requests must
+        // always include a client state.
+        if !self.user.client_state.is_empty() && self.auth_data.client_state.is_empty() {
+            let error_message = "Unacceptable client-state value empty string".to_owned();
+            return Err(TokenserverError::invalid_client_state(error_message));
+        }
+
         // The client state on the request must not have been used in the past.
         if self
             .user
@@ -147,6 +154,26 @@ impl TokenserverRequest {
             });
         }
 
+        // If there's no keys_changed_at on the request, there must be no value stored on the user
+        // record. Note that this is only relevant for BrowserID requests, since OAuth requests
+        // must always include a keys_changed_at header. The Python Tokenserver converts a NULL
+        // keys_changed_at on the user record to 0 in memory, which means that NULL
+        // keys_changed_ats are treated equivalently to 0 keys_changed_ats. This would allow users
+        // with a 0 keys_changed_at on their user record to hold off on sending a keys_changed_at
+        // in requests even though the value in the database is non-NULL. To be thorough, we
+        // handle this case here.
+        if auth_keys_changed_at.is_none()
+            && matches!(user_keys_changed_at, Some(inner) if inner != 0)
+        {
+            let context =
+                "No keys_changed_at sent for a user for whom we've already seen a keys_changed_at"
+                    .to_owned();
+            return Err(TokenserverError {
+                context,
+                ..TokenserverError::invalid_keys_changed_at()
+            });
+        }
+
         Ok(())
     }
 }
@@ -252,7 +279,7 @@ impl FromRequest for TokenserverRequest {
                     match duration_string.parse::<u64>() {
                         // The specified token duration should never be greater than the default
                         // token duration set on the server.
-                        Ok(duration) if duration <= DEFAULT_TOKEN_DURATION => Some(duration),
+                        Ok(duration) if duration <= state.token_duration => Some(duration),
                         _ => None,
                     }
                 })
@@ -265,7 +292,7 @@ impl FromRequest for TokenserverRequest {
                 hashed_fxa_uid,
                 hashed_device_id,
                 service_id,
-                duration: duration.unwrap_or(DEFAULT_TOKEN_DURATION),
+                duration: duration.unwrap_or(state.token_duration),
                 node_type: state.node_type,
             };
 
@@ -413,6 +440,15 @@ impl FromRequest for AuthData {
 
             let TokenserverMetrics(mut metrics) = TokenserverMetrics::extract(&req).await?;
 
+            // The Python Tokenserver treats zero values and null values both as being
+            // null, so for consistency, we need to convert a `Some(0)` value to `None`
+            fn convert_zero_to_none(generation_or_keys_changed_at: Option<i64>) -> Option<i64> {
+                match generation_or_keys_changed_at {
+                    Some(0) => None,
+                    _ => generation_or_keys_changed_at,
+                }
+            }
+
             match token {
                 Token::BrowserIdAssertion(assertion) => {
                     let mut tags = Tags::default();
@@ -436,8 +472,8 @@ impl FromRequest for AuthData {
                         device_id: verify_output.device_id,
                         email: verify_output.email.clone(),
                         fxa_uid: fxa_uid.to_owned(),
-                        generation: verify_output.generation,
-                        keys_changed_at: verify_output.keys_changed_at,
+                        generation: convert_zero_to_none(verify_output.generation),
+                        keys_changed_at: convert_zero_to_none(verify_output.keys_changed_at),
                     })
                 }
                 Token::OAuthToken(token) => {
@@ -458,8 +494,8 @@ impl FromRequest for AuthData {
                         email,
                         device_id: None,
                         fxa_uid,
-                        generation: verify_output.generation,
-                        keys_changed_at: Some(key_id.keys_changed_at),
+                        generation: convert_zero_to_none(verify_output.generation),
+                        keys_changed_at: convert_zero_to_none(Some(key_id.keys_changed_at)),
                     })
                 }
             }
@@ -683,6 +719,8 @@ mod tests {
         static ref SERVER_LIMITS: Arc<ServerLimits> = Arc::new(ServerLimits::default());
     }
 
+    const TOKEN_DURATION: u64 = 3600;
+
     #[actix_rt::test]
     async fn test_valid_tokenserver_request() {
         let fxa_uid = "test123";
@@ -1064,7 +1102,7 @@ mod tests {
             hashed_fxa_uid: "abcdef".to_owned(),
             hashed_device_id: "abcdef".to_owned(),
             service_id: 1,
-            duration: DEFAULT_TOKEN_DURATION,
+            duration: TOKEN_DURATION,
             node_type: NodeType::default(),
         };
 
@@ -1107,7 +1145,7 @@ mod tests {
             hashed_fxa_uid: "abcdef".to_owned(),
             hashed_device_id: "abcdef".to_owned(),
             service_id: 1,
-            duration: DEFAULT_TOKEN_DURATION,
+            duration: TOKEN_DURATION,
             node_type: NodeType::default(),
         };
 
@@ -1149,7 +1187,7 @@ mod tests {
             hashed_fxa_uid: "abcdef".to_owned(),
             hashed_device_id: "abcdef".to_owned(),
             service_id: 1,
-            duration: DEFAULT_TOKEN_DURATION,
+            duration: TOKEN_DURATION,
             node_type: NodeType::default(),
         };
 
@@ -1192,7 +1230,7 @@ mod tests {
             hashed_fxa_uid: "abcdef".to_owned(),
             hashed_device_id: "abcdef".to_owned(),
             service_id: 1,
-            duration: DEFAULT_TOKEN_DURATION,
+            duration: TOKEN_DURATION,
             node_type: NodeType::default(),
         };
 
@@ -1229,7 +1267,7 @@ mod tests {
             hashed_fxa_uid: "abcdef".to_owned(),
             hashed_device_id: "abcdef".to_owned(),
             service_id: 1,
-            duration: DEFAULT_TOKEN_DURATION,
+            duration: TOKEN_DURATION,
             node_type: NodeType::default(),
         };
 
@@ -1267,7 +1305,7 @@ mod tests {
             hashed_fxa_uid: "abcdef".to_owned(),
             hashed_device_id: "abcdef".to_owned(),
             service_id: 1,
-            duration: DEFAULT_TOKEN_DURATION,
+            duration: TOKEN_DURATION,
             node_type: NodeType::default(),
         };
 
@@ -1303,6 +1341,7 @@ mod tests {
                 )
                 .unwrap(),
             ),
+            token_duration: TOKEN_DURATION,
         }
     }
 }

syncstorage/src/tokenserver/handlers.rs

@@ -1,5 +1,4 @@
 use std::{
-    cmp,
     collections::HashMap,
     time::{Duration, SystemTime, UNIX_EPOCH},
 };
@@ -84,7 +83,12 @@ fn get_token_plaintext(
             })?;
         let client_state_b64 = base64::encode_config(&client_state, base64::URL_SAFE_NO_PAD);
 
-        format!("{:013}-{:}", updates.keys_changed_at, client_state_b64)
+        format!(
+            "{:013}-{:}",
+            // We fall back to using the user's generation here, which matches FxA's behavior
+            updates.keys_changed_at.unwrap_or(updates.generation),
+            client_state_b64
+        )
     };
 
     let expires = {
@@ -108,28 +112,72 @@ fn get_token_plaintext(
 }
 
 struct UserUpdates {
-    keys_changed_at: i64,
+    keys_changed_at: Option<i64>,
+    generation: i64,
     uid: i64,
 }
 
 async fn update_user(
     req: &TokenserverRequest,
     db: Box<dyn Db>,
 ) -> Result<UserUpdates, TokenserverError> {
-    // If the keys_changed_at in the request is larger than that stored on the user record,
-    // update to the value in the request.
-    let keys_changed_at =
-        cmp::max(req.auth_data.keys_changed_at, req.user.keys_changed_at).unwrap_or(0);
-
-    let generation = if let Some(generation) = req.auth_data.generation {
-        // If there's a generation on the request, choose the larger of that and the generation
-        // already stored on the user record.
-        cmp::max(generation, req.user.generation)
-    } else {
-        // If there's not a generation on the request and the keys_changed_at on the request is
-        // larger than the generation stored on the user record, set the user's generation to be
-        // the keys_changed_at on the request.
-        cmp::max(req.auth_data.keys_changed_at, Some(req.user.generation)).unwrap_or(0)
+    let keys_changed_at = match (req.auth_data.keys_changed_at, req.user.keys_changed_at) {
+        // If the keys_changed_at in the request is larger than that stored on the user record,
+        // update to the value in the request.
+        (Some(request_keys_changed_at), Some(user_keys_changed_at))
+            if request_keys_changed_at >= user_keys_changed_at =>
+        {
+            Some(request_keys_changed_at)
+        }
+        // If there is a keys_changed_at in the request and it's smaller than that stored on the
+        // user record, we've already returned an error at this point.
+        (Some(_request_keys_changed_at), Some(_user_keys_changed_at)) => unreachable!(),
+        // If there is a keys_changed_at on the request but not one on the user record, this is the
+        // first time the client reported it, so we assign the new value.
+        (Some(request_keys_changed_at), None) => Some(request_keys_changed_at),
+        // At this point, we've already validated that, if there is a keys_changed_at already
+        // stored on the user record, there must be one in the request. If that isn't the case,
+        // we've already returned an error.
+        (None, Some(user_keys_changed_at)) if user_keys_changed_at != 0 => unreachable!(),
+        // If there's no keys_changed_at in the request and the keys_changed_at on the user record
+        // is 0, keep the value as 0.
+        (None, Some(_user_keys_changed_at)) => Some(0),
+        // If there is no keys_changed_at on the user record or in the request, we want to leave
+        // the value unset.
+        (None, None) => None,
+    };
+
+    let generation = match req.auth_data.generation {
+        // If there's a generation in the request and it's greater than or equal to that stored on
+        // the user record, update to the value in the request.
+        Some(request_generation) if request_generation >= req.user.generation => request_generation,
+        // If there's a generation in the request and it's smaller than that stored on the user
+        // record, we've already returned an error.
+        Some(_request_generation) => unreachable!(),
+        None => match (req.auth_data.keys_changed_at, req.user.keys_changed_at) {
+            // If there's not a generation on the request but the keys_changed_at on the request
+            // is greater than the user's current generation AND the keys_changed_at on the request
+            // is greater than the user's current keys_changed_at, set the user's generation to
+            // the new keys_changed_at.
+            (Some(request_keys_changed_at), Some(user_keys_changed_at))
+                if request_keys_changed_at > user_keys_changed_at
+                    && request_keys_changed_at > req.user.generation =>
+            {
+                request_keys_changed_at
+            }
+            // If there's not a generation on the request but the keys_changed_at on the request
+            // is greater than the user's current generation AND there is a keys_changed_at on the
+            // request but not currently on the user record, set the user's generation to the new
+            // keys_changed_at.
+            (Some(request_keys_changed_at), None)
+                if request_keys_changed_at > req.user.generation =>
+            {
+                request_keys_changed_at
+            }
+            // If the request has a keys_changed_at but the above conditions don't hold OR if the
+            // request doesn't have a keys_changed_at, just keep the same generation.
+            (_, _) => req.user.generation,
+        },
     };
 
     // If the client state changed, we need to mark the current user as "replaced" and create a
@@ -153,7 +201,7 @@ async fn update_user(
                 })
                 .await?
                 .id,
-            keys_changed_at: Some(keys_changed_at),
+            keys_changed_at,
             created_at: timestamp,
         };
         let uid = db.post_user(post_user_params).await?.id;
@@ -169,22 +217,24 @@ async fn update_user(
 
         Ok(UserUpdates {
             keys_changed_at,
+            generation,
             uid,
         })
     } else {
-        if generation != req.user.generation || Some(keys_changed_at) != req.user.keys_changed_at {
+        if generation != req.user.generation || keys_changed_at != req.user.keys_changed_at {
             let params = PutUser {
                 email: req.auth_data.email.clone(),
                 service_id: req.service_id,
                 generation,
-                keys_changed_at: Some(keys_changed_at),
+                keys_changed_at,
             };
 
             db.put_user(params).await?;
         }
 
         Ok(UserUpdates {
             keys_changed_at,
+            generation,
             uid: req.user.uid,
         })
     }

syncstorage/src/tokenserver/mod.rs

@@ -35,6 +35,7 @@ pub struct ServerState {
     pub node_capacity_release_rate: Option<f32>,
     pub node_type: NodeType,
     pub metrics: Box<StatsdClient>,
+    pub token_duration: u64,
 }
 
 impl ServerState {
@@ -73,6 +74,7 @@ impl ServerState {
                     node_capacity_release_rate: settings.node_capacity_release_rate,
                     node_type: settings.node_type,
                     metrics: Box::new(metrics),
+                    token_duration: settings.token_duration,
                 }
             })
             .map_err(Into::into)

syncstorage/src/tokenserver/settings.rs

@@ -63,6 +63,8 @@ pub struct Settings {
     /// verifications do not require requests to FXA if the JWK is set on Tokenserver. The server
     /// will return an error at startup if the JWK is not cached and this setting is `None`.
     pub additional_blocking_threads_for_fxa_requests: Option<u32>,
+    /// The amount of time in seconds before a token provided by Tokenserver expires.
+    pub token_duration: u64,
 }
 
 #[derive(Clone, Debug, Deserialize)]
@@ -102,6 +104,7 @@ impl Default for Settings {
             run_migrations: cfg!(test),
             spanner_node_id: None,
             additional_blocking_threads_for_fxa_requests: None,
+            token_duration: 3600,
         }
     }
 }