From 644b9b40c9a08e39a370d282cb52bfabce34911a Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 25 Apr 2024 15:45:48 -0700 Subject: [PATCH 1/6] feat: log bad client state to stderr & metrics Closes: SYNC-4244 --- syncserver/src/tokenserver/extractors.rs | 1 + tokenserver-common/src/error.rs | 12 +++++++++++- tokenserver-db/src/lib.rs | 2 ++ tokenserver-db/src/models.rs | 16 ++++++++++++++-- 4 files changed, 28 insertions(+), 3 deletions(-) diff --git a/syncserver/src/tokenserver/extractors.rs b/syncserver/src/tokenserver/extractors.rs index a250812aff..385f5f3828 100644 --- a/syncserver/src/tokenserver/extractors.rs +++ b/syncserver/src/tokenserver/extractors.rs @@ -113,6 +113,7 @@ impl TokenserverRequest { .contains(&self.auth_data.client_state) { let error_message = "Unacceptable client-state value stale value".to_owned(); + warn!("Client attempted stale value"; "uid"=> self.user.uid, "client_state"=> self.user.client_state.clone()); return Err(TokenserverError::invalid_client_state(error_message)); } diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 1f7dcf31ff..2e62141bda 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -272,7 +272,17 @@ impl ReportableError for TokenserverError { TokenType::Oauth => Some("request.error.oauth".to_owned()), } } else { - None + if matches!( + self, + TokenserverError { + status: "invalid-client-state", + .. + } + ) { + Some("request.error.invalid_client_state".to_owned()) + } else { + None + } } } } diff --git a/tokenserver-db/src/lib.rs b/tokenserver-db/src/lib.rs index 1b9f86c623..34447c3d59 100644 --- a/tokenserver-db/src/lib.rs +++ b/tokenserver-db/src/lib.rs @@ -1,6 +1,8 @@ extern crate diesel; #[macro_use] extern crate diesel_migrations; +#[macro_use] +extern crate slog_scope; mod error; pub mod mock; diff --git a/tokenserver-db/src/models.rs b/tokenserver-db/src/models.rs index e78328319a..3a2ed04d56 100644 --- a/tokenserver-db/src/models.rs +++ b/tokenserver-db/src/models.rs @@ -389,7 +389,7 @@ impl TokenserverDb { let raw_user = raw_users[0].clone(); // Collect any old client states that differ from the current client state - let old_client_states = { + let old_client_states: Vec = { raw_users[1..] .iter() .map(|user| user.client_state.clone()) @@ -397,6 +397,14 @@ impl TokenserverDb { .collect() }; + if !old_client_states.is_empty() { + info!( + "Tokenserver user has old client states"; + "uid" => raw_user.uid, + "count" => old_client_states.len() + ) + } + // Make sure every old row is marked as replaced. They might not be, due to races in row // creation. for old_user in &raw_users[1..] { @@ -463,7 +471,11 @@ impl TokenserverDb { // The most up-to-date user doesn't have a node and is retired. This is an internal // service error for compatibility reasons (the legacy Tokenserver returned an // internal service error in this situation). - (_, None) => Err(DbError::internal("Tokenserver user retired".to_owned())), + (_, None) => { + let uid = raw_user.uid.clone(); + warn!("Tokenserver user retired"; "uid" => &uid); + Err(DbError::internal("Tokenserver user retired".to_owned())) + } } } } From 823378336a16698b050731d42a862d7e3ff97b68 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Thu, 25 Apr 2024 15:57:01 -0700 Subject: [PATCH 2/6] f clippy --- tokenserver-common/src/error.rs | 20 +++++++++----------- tokenserver-db/src/models.rs | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/tokenserver-common/src/error.rs b/tokenserver-common/src/error.rs index 2e62141bda..67c0586252 100644 --- a/tokenserver-common/src/error.rs +++ b/tokenserver-common/src/error.rs @@ -271,18 +271,16 @@ impl ReportableError for TokenserverError { TokenType::BrowserId => Some("request.error.browser_id".to_owned()), TokenType::Oauth => Some("request.error.oauth".to_owned()), } - } else { - if matches!( - self, - TokenserverError { - status: "invalid-client-state", - .. - } - ) { - Some("request.error.invalid_client_state".to_owned()) - } else { - None + } else if matches!( + self, + TokenserverError { + status: "invalid-client-state", + .. } + ) { + Some("request.error.invalid_client_state".to_owned()) + } else { + None } } } diff --git a/tokenserver-db/src/models.rs b/tokenserver-db/src/models.rs index 3a2ed04d56..500570ef39 100644 --- a/tokenserver-db/src/models.rs +++ b/tokenserver-db/src/models.rs @@ -472,7 +472,7 @@ impl TokenserverDb { // service error for compatibility reasons (the legacy Tokenserver returned an // internal service error in this situation). (_, None) => { - let uid = raw_user.uid.clone(); + let uid = raw_user.uid; warn!("Tokenserver user retired"; "uid" => &uid); Err(DbError::internal("Tokenserver user retired".to_owned())) } From 65b7805cf0adcb7ac6074701913ff69e0e8cf078 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 26 Apr 2024 14:37:17 -0700 Subject: [PATCH 3/6] feat: allow range specification for purge_old_records --- tools/tokenserver/database.py | 48 ++++++++++++++++++++++++-- tools/tokenserver/purge_old_records.py | 18 ++++++++++ tools/tokenserver/test_database.py | 24 +++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-) diff --git a/tools/tokenserver/database.py b/tools/tokenserver/database.py index d1aa711b79..0dbee59d9a 100644 --- a/tools/tokenserver/database.py +++ b/tools/tokenserver/database.py @@ -101,6 +101,26 @@ :offset """) +_GET_OLD_USER_RECORDS_FOR_SERVICE_RANGE = """\ +select + uid, email, generation, keys_changed_at, client_state, + nodes.node, nodes.downed, created_at, replaced_at +from + users left outer join nodes on users.nodeid = nodes.id +where + users.service = :service +and + replaced_at is not null and replaced_at < :timestamp +and + ::RANGE:: +order by + replaced_at desc, uid desc +limit + :limit +offset + :offset +""" + _GET_ALL_USER_RECORDS_FOR_SERVICE = sqltext("""\ select @@ -420,8 +440,29 @@ def get_user_records(self, email): finally: res.close() + def _build_old_user_query(self, range, **params): + if range: + # construct the range from the passed arguments + rstr = [] + try: + if range[0]: + rstr.append("uid > :start") + params["start"] = range[0] + if range[1]: + rstr.append("uid < :end") + params["end"] = range[1] + except IndexError: + pass + rrep = " and ".join(rstr) + sql = sqltext( + _GET_OLD_USER_RECORDS_FOR_SERVICE_RANGE.replace( + "::RANGE::", rrep)) + else: + sql = _GET_OLD_USER_RECORDS_FOR_SERVICE + return sql + def get_old_user_records(self, grace_period=-1, limit=100, - offset=0): + offset=0, range=None): """Get user records that were replaced outside the grace period.""" if grace_period < 0: grace_period = 60 * 60 * 24 * 7 # one week, in seconds @@ -432,7 +473,10 @@ def get_old_user_records(self, grace_period=-1, limit=100, "limit": limit, "offset": offset } - res = self._execute_sql(_GET_OLD_USER_RECORDS_FOR_SERVICE, **params) + + sql = self._build_old_user_query(range, **params) + + res = self._execute_sql(sql, **params) try: for row in res: yield row diff --git a/tools/tokenserver/purge_old_records.py b/tools/tokenserver/purge_old_records.py index e7abe84426..623f5141c4 100644 --- a/tools/tokenserver/purge_old_records.py +++ b/tools/tokenserver/purge_old_records.py @@ -45,6 +45,7 @@ def purge_old_records( dryrun=False, force=False, override_node=None, + uid_range=None, ): """Purge old records from the database. @@ -299,6 +300,18 @@ def main(args=None): "", "--override_node", help="Use this node when deleting (if data was copied)" ) + parser.add_option( + "", + "--range_start", + default=None, + help="Start of UID range to purge" + ) + parser.add_option( + "", + "--range_end", + default=None, + help="End of UID range to purge (exclusive)" + ) opts, args = parser.parse_args(args) if len(args) != 2: @@ -309,6 +322,10 @@ def main(args=None): util.configure_script_logging(opts) + uid_range = None + if opts.start_range or opts.end_range: + uid_range = (opts.start_range, opts.end_range) + purge_old_records( secret, grace_period=opts.grace_period, @@ -319,6 +336,7 @@ def main(args=None): dryrun=opts.dryrun, force=opts.force, override_node=opts.override_node, + range=uid_range ) if not opts.oneshot: while True: diff --git a/tools/tokenserver/test_database.py b/tools/tokenserver/test_database.py index 065b7a8d4d..eadb309267 100644 --- a/tools/tokenserver/test_database.py +++ b/tools/tokenserver/test_database.py @@ -463,3 +463,27 @@ def test_first_seen_at(self): user3 = self.database.get_user(EMAIL) self.assertEqual(user3['uid'], user2['uid']) self.assertNotEqual(user3['first_seen_at'], user2['first_seen_at']) + + def test_build_old_range(self): + params = dict() + sql = self.database._build_old_user_query(None, **params) + self.assert_(not sql.text.contains("uid > :start")) + self.assert_(not sql.text.contains("uid < :end")) + self.assertIsNone(params.get("start")) + self.assertIsNone(params.get("end")) + + params = dict() + rrange = (None, "abcd") + sql = self.database._build_old_user_query(rrange, **params) + self.assert_(not sql.text.contains("uid > :start")) + self.assert_(sql.text.contains("uid < :end")) + self.assertIsNone(params.get("start")) + self.assertEqual(params.get("end"), rrange[1]) + + params = dict() + rrange = ("1234", "abcd") + sql = self.database._build_old_user_query(rrange, **params) + self.assert_(sql.text.contains("uid > :start")) + self.assert_(sql.text.contains("uid < :end")) + self.assertEqual(params.get("start"), rrange[0]) + self.assertEqual(params.get("end"), rrange[1]) From 4e1b56305f215ff5bba1ab2ed65fee0e46ac6b61 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 26 Apr 2024 14:58:03 -0700 Subject: [PATCH 4/6] f fix test --- tools/tokenserver/database.py | 4 ++-- tools/tokenserver/test_database.py | 19 ++++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/tools/tokenserver/database.py b/tools/tokenserver/database.py index 0dbee59d9a..f04d64e018 100644 --- a/tools/tokenserver/database.py +++ b/tools/tokenserver/database.py @@ -440,7 +440,7 @@ def get_user_records(self, email): finally: res.close() - def _build_old_user_query(self, range, **params): + def _build_old_user_query(self, range, params, **kwargs): if range: # construct the range from the passed arguments rstr = [] @@ -474,7 +474,7 @@ def get_old_user_records(self, grace_period=-1, limit=100, "offset": offset } - sql = self._build_old_user_query(range, **params) + sql = self._build_old_user_query(range, params) res = self._execute_sql(sql, **params) try: diff --git a/tools/tokenserver/test_database.py b/tools/tokenserver/test_database.py index eadb309267..709f5aab28 100644 --- a/tools/tokenserver/test_database.py +++ b/tools/tokenserver/test_database.py @@ -466,24 +466,25 @@ def test_first_seen_at(self): def test_build_old_range(self): params = dict() - sql = self.database._build_old_user_query(None, **params) - self.assert_(not sql.text.contains("uid > :start")) - self.assert_(not sql.text.contains("uid < :end")) + sql = self.database._build_old_user_query(None, params) + self.assert_(sql.text.find("uid > :start") < 0) + self.assert_(sql.text.find("uid < :end") < 0) self.assertIsNone(params.get("start")) self.assertIsNone(params.get("end")) params = dict() rrange = (None, "abcd") - sql = self.database._build_old_user_query(rrange, **params) - self.assert_(not sql.text.contains("uid > :start")) - self.assert_(sql.text.contains("uid < :end")) + sql = self.database._build_old_user_query(rrange, params) + self.assert_(sql.text.find("uid > :start") < 0) + self.assert_(sql.text.find("uid < :end") > 0) self.assertIsNone(params.get("start")) self.assertEqual(params.get("end"), rrange[1]) params = dict() rrange = ("1234", "abcd") - sql = self.database._build_old_user_query(rrange, **params) - self.assert_(sql.text.contains("uid > :start")) - self.assert_(sql.text.contains("uid < :end")) + sql = self.database._build_old_user_query(rrange, params) + self.assert_(sql.text.find("uid > :start") > 0) + self.assert_(sql.text.find("uid < :end") > 0) self.assertEqual(params.get("start"), rrange[0]) self.assertEqual(params.get("end"), rrange[1]) + From 4dd1aa0be65925882bfe960f7767bd419c4689f2 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Fri, 26 Apr 2024 14:58:40 -0700 Subject: [PATCH 5/6] f flake8 --- tools/tokenserver/test_database.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tools/tokenserver/test_database.py b/tools/tokenserver/test_database.py index 709f5aab28..fac5980a6d 100644 --- a/tools/tokenserver/test_database.py +++ b/tools/tokenserver/test_database.py @@ -487,4 +487,3 @@ def test_build_old_range(self): self.assert_(sql.text.find("uid < :end") > 0) self.assertEqual(params.get("start"), rrange[0]) self.assertEqual(params.get("end"), rrange[1]) - From 21e5a3d18368e86bb4c7857e667fd798be403bc9 Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 7 May 2024 16:46:53 -0700 Subject: [PATCH 6/6] f r's --- tokenserver-db/src/models.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/tokenserver-db/src/models.rs b/tokenserver-db/src/models.rs index 500570ef39..44817f1031 100644 --- a/tokenserver-db/src/models.rs +++ b/tokenserver-db/src/models.rs @@ -397,14 +397,6 @@ impl TokenserverDb { .collect() }; - if !old_client_states.is_empty() { - info!( - "Tokenserver user has old client states"; - "uid" => raw_user.uid, - "count" => old_client_states.len() - ) - } - // Make sure every old row is marked as replaced. They might not be, due to races in row // creation. for old_user in &raw_users[1..] {