984232 - Add low-level user record management methods to WIMMS

Status: VERIFIED FIXED

(Cloud Services Graveyard :: Server: Token, defect, P2)

Description

[Ryan Kelly [:rfkelly]]

This adds some methods to WIMMS to do the following low-level user record manipulations:

  * find all user records for a given email address
  * mark all records for a given email address as replaced
  * find all user records that were replaced more than X seconds ago
  * delete a specific user record

The idea is to use these in our management scripts for purging old records, and for cleaning up records of users who delete their accounts.

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: wimms-user-record-methods.diff

Attaching the patch.

To do the query for "all records replaced more than X seconds ago" we require a new index on the users table.  It'll run fine without the index, but use a full table scan, which is not awesome.  We'll need to ensure this index gets properly created when we push this to production.

Comment 2

[Ryan Kelly [:rfkelly]]

/cc :mostlygeek as a heads-upon the potential new index on tokenserver "users" table.

Comment 3

[Ryan Kelly [:rfkelly]]

I think I'll need to update this patch based on Bug 971907 Comment 23

Comment 4

[Ryan Kelly [:rfkelly]]

Attachment: wimms-user-record-methods.diff

Updated patch based on the above comment.

This uses a "retire_user()" method rather than "delete_user()".  When retiring a user, we mark their user records as replaced so that GC can find them and clean them up, and we set a humungous generation number so that future login attempts will be blocked.

Comment 5

[Toby Elliott [:telliott]]

Comment on attachment 8392596 [details] [diff] [review]
wimms-user-record-methods.diff

Review of attachment 8392596 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't appear to do what it claims to do, since it's filtering on service a bunch in ways that it's not clear we want to. Is the goal here to be able to purge a user entirely, or just in one service? I guess it's not clear to me when we use these particular methods.

::: wimms/sql.py
@@ +85,5 @@
> +set
> +    replaced_at = :timestamp,
> +    generation = 9223372036854775807
> +where
> +    service = :service and email = :email

If this is marking all records for the user as replaced, we shouldn't filter on service.

@@ +92,5 @@
> +
> +
> +_GET_USER_RECORDS_OLD = sqltext("""\
> +select
> +    uid, node, generation, client_state, created_at, replaced_at

Do we need generation, client_state or created_at here? Those all seem pretty irrelevant to our needs at this point.

@@ +114,5 @@
> +    users
> +where
> +    email = :email
> +and
> +    service = :service

not really RECORDS_ALL if it's only records for a specific service. May just be a nomenclature problem.

Comment 6

[Ryan Kelly [:rfkelly]]

The consuming code is things like Bug 984297 and Bug 971907.  Where you're right, it just loops through all the available services and processes each in turn.  This makes sense for the get_user_records etc, because we want to process them a service at a time.  But retire_user() can probably be simplified by omitting the service parameter, I can't think of a reason to want to do that for one service but not all services.

> Is the goal here to be able to purge a user entirely, or just in one service

There are two related goals.  retire_user() is to be able to purge a user entirely (Bug 971907) but the other new methods are for general cleanup of old records that will probably want to be done on a per-service basis (Bug 984297)

Comment 7

[Toby Elliott [:telliott]]

Right. There's a mismatch between the documentation/comments and what you're actually doing in the library code. That's a recipe for confusion in 6 months :)

Comment 8

[Ryan Kelly [:rfkelly]]

Attachment: wimms-user-record-methods.diff

Updated patch attached.

Without a service name to shard on, we need to re-implement retire_user() for the sharded backed.  It's pretty straightforward though (an dI found a line of dead code in the process)

Comment 9

[Toby Elliott [:telliott]]

Comment on attachment 8393224 [details] [diff] [review]
wimms-user-record-methods.diff

Review of attachment 8393224 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, aside from some misleading comments.

::: wimms/sql.py
@@ +311,5 @@
> +    # Methods for low-level user record management.
> +    #
> +
> +    def get_user_records(self, service, email):
> +        """Get all records for a user, even ones that have been replaced."""

Still have too liberal a use of "all" in the comments (this only returns all of a single service's records for a user)

Comment 10

[Ryan Kelly [:rfkelly]]

Committed in https://github.com/mozilla-services/wimms/commit/25d69a0dfb75d7ef9d61120ee0d3bfa07dd7d3f9

With the comment "Get all the user's records for a service, including the old ones."

Comment 11

[James Bonacci [:jbonacci]]

Verified in code after a local install.