Deleted user is not fully removed from client database after a sync

[eloquence]

Environment information

• Server: SecureDrop 1.5.0 (Qubes staging environment) populated with sources
• Client: SecureDrop 0.2.1-dev-202000819-060226 (Qubes staging)

Steps to reproduce

1. Log into the SecureDrop Client as user A, an admin user
2. Log into the Journalist Interface as user A
3. Create a new user account B via the Journalist Interface
4. Log out of the Journalist Interface
5. Log into the Journalist Interface as user B
6. Send a reply to an existing source as user B
7. Wait for the client to sync. Inspect the client users table and verify that user B has been added.
8. Log out of the Journalist Interface
9. Log into the Journalist Interface as user A
10. Delete user B
11. Wait for the client to sync. Inspect the client users table.

Expected result

No information about user B (name, etc.) is present in the SecureDrop Client database.

Actual result

User B still exists, and a new row deleted|deleted|| has been added.

[emkll]

Went through the STR and can reproduce the findings of this ticket, however it seems like this is expected behavior:

In freedomofpress/securedrop#5176 we introduced changes to return a journalist with uuid and username set to deleted as a special case, to handle replies which were written by a deleted journalist.

This is a special "placeholder" user, and we should expect a unique entry in the database for this user, should the server contain at least one deleted user.

This should be in the database to support offline mode when a reply is attributed to a deleted user (especially given #76).

Am I missing something?

[eloquence]

The problem is the

User B still exists,

i.e. the original user record with the username is still in the database, in addition to the deleted user.

[sssoleileraaa]

if a user is deleted from the server, we need a way to inform the client during a sync. without knowing whether or not a user account has been deleted, we cannot remove an account from the users table. i suggest that we add a new endpoint (i know we take a hit for every api call we make over tor during our sync, but i don't see an existing endpoint that makes sense for us to update to include this information, and it also is more organized to have a separate endpoint - we can provide benchmarks with this change so we have an idea of the impact). once we know that an account has been deleted, we can update our local users table and do things such as the code below every sync to make sure that our failed replies show the deleted user icon instead of the initials of the existing record in our users table.

# Update journalist_id of each failed draft reply that is associated with a deleted user
deleted_user = session.query(User).filter_by(uuid="deleted").one_or_none()
if deleted_user:
    failed_status = (
        session.query(ReplySendStatus).filter_by(name=ReplySendStatusCodes.FAILED.value).one()
    )
    failed_draft_replies = session.query(DraftReply).filter_by(send_status=failed_status).all()
    for failed_reply in failed_draft_replies:
        user = session.query(User).filter_by(id=failed_reply.journalist_id).one_or_none()
        if not user:
            lazy_setattr(failed_reply, "journalist_id", deleted_user.id)

the code above will not work unless our users table is updated so that we know which accounts have been deleted on the server. also, since we need to update the journalist api and sdk, i think we could also prioritize freedomofpress/securedrop#5467 since it's related to this area of the codebase. it could definitely be done separately though.

[sssoleileraaa]

(eventually maybe we could combine some of these endpoints into one endpoint like api/v2/sync that only sends back all changes that were made to the server since the last sync, see freedomofpress/securedrop#5104)

[sssoleileraaa]

we can still push forward with journalist badges in #1142, but it'll mean that we have an edge case where failed replies show up with the user's initials instead of the deleted account icon since we won't know whether or not that user still exists on the server.

note: since we provide journalist_uuid for remote replies we can update the journalist badges to the deleted account icon for successful replies

[eloquence]

@creviera and I chatted a bit, if we want to reliably sync the users database we'll need a new API endpoint for that, which may need to go into 1.6.0. Filed freedomofpress/securedrop#5490 for discussion.

We can potentially rely on reply metadata alone, but it'll leave us with edge cases such as users associated with failed replies (never sent to the server), or users who have logged in but never sent a reply, remaining in the client database. We'd likely accumulate more such edge cases as we add features to the client&server.

[eloquence]

(Note that we are expecting that #1157 will resolve this, keeping open for tracking purposes.)

[sssoleileraaa]

Linking to freedomofpress/securedrop#5467 to make it easier to find, but basically the idea is that we should stop deleting user records that are referenced by other records in the database, and instead create deleted shell accounts, which will resolve this issue. Remember that in order to get rid of a user reply or seen-by record, the entire conversation will need to be deleted, which may include other user replies and metadata since this is a shared inbox. An extra privacy step would be to recreate the UUID associated with the deleted user, but this has minimal gains (threat-modeling and review is still needed).

Here's what I propose for solving this issue:

0. To set ourselves up for the seen-by feature at the per-message level, we need to add users outside of the replies endpoint. To do this, implement part of Synchronize user data via /users endpoint #1157 by only syncing on the creation of new users. Deletion will be resolved in the following steps.
1. Resolve Preserve deleted journalist UUIDs in database to attribute replies securedrop#5467 by keeping shell journalist entries for deleted journalists with associated data in other tables (no data migration necessary because the seen-by feature will continue working and the replies can check if the username of the shell journalist is the reserved word: "DELETED" (usernames are currently not nullable).
2. Create a global "deleted" user and change NULL journalist_ids in the replies and seen_* tables to the id of the "deleted" user entry. This will require that we aggregate seen_* records with NULL journalist IDs for the same submission/reply to a single record for a "deleted" user. Step 1 ensures that we will no longer have to aggregate seen_* records.
3. Remove SDK code that special cases NULL journalist_ids in replies table and tells the client that the replies came from user with UUID "deleted" .
4. Finish what step 0 started by syncing users to match the server, which will fully resolve Synchronize user data via /users endpoint #1157
5. Remove client code that creates users during the replies sync.

Also, tangential to server db cleanup, we should also look into making the journalist_id column across all tables non-null and enable foreign key support: freedomofpress/securedrop#5503.