Skip to content

Commit

Permalink
fix: remember keys_changed_at when reallocating after node replacement.
Browse files Browse the repository at this point in the history
If a user's current storage node is decomissioned, they may end up
without an active `users` record in the db, and there is code to
handle this case by lazily recreating an active record on demand.

Previously, this code would only copy `generation` and `client_state`
fields, essentially forgotten any previously-seen value for the new
`keys_changed_at` field. This commit fixes things so that value is
also remembered.

I do not expect this bug to have caused any issues in practice, since
we already have a bunch of code to handle users for which we have
not yet seen a value of `keys_changed_at`. But we should remember it
when we can because it helps the server enforce that clients are
behaving correctly.
  • Loading branch information
rfk committed Feb 11, 2020
1 parent d8c71d0 commit ee6c065
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 2 deletions.
3 changes: 2 additions & 1 deletion tokenserver/assignment/sqlnode/sql.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,7 +316,8 @@ def get_user(self, service, email):
if cur_row.generation < MAX_GENERATION:
user = self.allocate_user(service, email,
cur_row.generation,
cur_row.client_state)
cur_row.client_state,
cur_row.keys_changed_at)
for old_row in old_rows:
# Collect any previously-seen client-state values.
if old_row.client_state != user['client_state']:
Expand Down
7 changes: 6 additions & 1 deletion tokenserver/tests/assignment/test_sqlnode.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,17 @@ def test_cleanup_of_old_records(self):

def test_node_reassignment_when_records_are_replaced(self):
self.backend.allocate_user("sync-1.0", "[email protected]",
generation=42, client_state="aaa")
generation=42,
keys_changed_at=12,
client_state="aaa")
user1 = self.backend.get_user("sync-1.0", "[email protected]")
self.backend.replace_user_records("sync-1.0", "[email protected]")
user2 = self.backend.get_user("sync-1.0", "[email protected]")
# They should have got a new uid.
self.assertNotEqual(user2["uid"], user1["uid"])
# But their account metadata should have been preserved.
self.assertEqual(user2["generation"], user1["generation"])
self.assertEqual(user2["keys_changed_at"], user1["keys_changed_at"])
self.assertEqual(user2["client_state"], user1["client_state"])

def test_node_reassignment_not_done_for_retired_users(self):
Expand Down

0 comments on commit ee6c065

Please sign in to comment.