Re-associate draft replies with "deleted" user account

[sssoleileraaa]

Description

Fixes #1414
Fixes #1416
Fixes #1411

This PR fixes all the issues above by ensuring that a pending or failed reply (aka "draft reply", because it exists in the draftreplies table) cannot have a null sender. One of the issues that came up during our testing was that we were allowing null journalist_id in the draftreplies table. Once the data was in this state, there would be issues whenever the client would have to update badges (on authentication change, when a user's name changed, and when badge needed to be updated to show the "deleted" sparkles icon).

Test Plan

In case you are a QA tester, ensure that your local database is cleaned up from running QA tests during previous release candidate testing by deleting all draft replies in the draftreplies table where journalist_id is null.

Follow STR in #1414:

• Confirm that you can no longer repro the issue

Follow STR in #1416

• Confirm that you can no longer repro the issue

Follow STR in #1411

• Confirm that you can no longer repro the issue

Checklist

If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:

• I have tested these changes in the appropriate Qubes environment
• I do not have an appropriate Qubes OS workstation set up (the reviewer will need to test these changes)
• These changes should not need testing in Qubes

If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:

• I have updated the AppArmor profile
• No update to the AppArmor profile is required for these changes
• I don't know and would appreciate guidance

If these changes modify the database schema, you should include a database migration. Please check as applicable:

• I have written a migration and upgraded a test database based on main and confirmed that the migration applies cleanly
• I have written a migration but have not upgraded a test database based on main and would like the reviewer to do so
• I need help writing a database migration
• No database schema changes are needed

[rocodes]

I'll test this change out now (partly since I feel bad that I didn't catch this during QA yesterday night...)

[eloquence]

Follow STR in #1414

• Confirm that you can no longer repro the issue

Follow STR in #1416

• Confirm that you can no longer repro the issue

❌ However, upon the STR in #1416, the user is not actually deleted from the client database, and the client repeatedly logs the following error:

2022-02-03 18:18:56,943 - securedrop_client.api_jobs.sync:99(_update_users) ERROR: Cannot delete user with uuid='a4e05eb6-c7f2-454c-88fa-cefd51b6fca1' without a reserved deleted user account

This is against a 2.1.0 prod server that doesn't yet have the changes in freedomofpress/securedrop#6225, so may be expected behavior pre-2.2.0 (if it represents no regression over current state and is quietly ignored, that may be OK during the brief time between SD Client 0.6.0 and SD Server 2.2.0). I've not tested with a 2.2.0 RC yet.

[rocodes]

I have the same experience--the journalists table on the server and the users table on the client become out-of-sync, with the pending reply keeping the user from being deleted on the client. (Screenshot of the outcome--I tried twice, hence the two different accounts with pending replies--both those users have been deleted from the journalists table on the server but persist in the users table.)

2022-02-03 12:22:30,743 - securedrop_client.api_jobs.sync:99(_update_users) ERROR: Cannot delete user with uuid='f173298a-f944-418d-94eb-38db85f492ae' without a reserved deleted user account

[sssoleileraaa]

Super clear. Thanks for the report, both.

The log message, Cannot delete user with uuid='a4e05eb6-c7f2-454c-88fa-cefd51b6fca1' without a reserved deleted user account, mostly explains what's going on. The additional piece of information is that this could only happen for a draft reply.

The server, even what is currently planned to go into 2.2.0, does not provide a "deleted' user account by default- both create the account on demand. I see a couple solutions:

1. A "deleted" user account is created by default, so that it can exist before any actual user is deleted on the server.
2. We create a draft "deleted" user account that only exists locally (so there would be two accounts that would return True when we check the deleted property of a user- True if username == deleted and True if uuid == deleted (remember that username is unique and the server currently disallows a "deleted" username to be used).
3. We allow null journalist_ids for draft replies, and add conditional logic in the client around any code that needs to know who the sender of a draftreply is, and treat a null sender to mean the same thing as an existing sender with the deleted property returning True.

@eloquence and @rocodes, since you're both familiar with the problem, which solution do you prefer? In order of preference, I'm leaning towards 1, 3, and then 2.

Actually, even if the server did this in 2.2.0, it still wouldn't solve the backwards compatibility issue. So we need to rule out 1. New order of preference is 3, and then 2, but I keep going back and forth on which one I prefer. 2 would make it clearer that there are multiple "deleted" users, and it would also be clearer than treating null as a deleted user. It would also allow us to make journalist_id non null in the future. And, it's late so I can't remember why I originally preferred 3. It might have had to do with how we wouldn't have to use a non uuid of "deleted" as the uuid for the local "deleted" user.

[rocodes]

Thanks for the explanation, @creviera. I need to think it over and look at the code a little more (and obviously I defer to whatever your preference is since I haven't touched this part of the code), but of the options you've listed, I think I slightly prefer 2. Reasons: a) drafts only exist on the client, and this in-between state is only possible on the client, so a local solution seems preferable; and b) and it seems like keeping the option open to enforce non-null journalist IDs is a good idea, and if go with option 3 and then and want to enforce constraints on journalist_id later on, we may be setting ourselves up for more bugs/complexity.

I'm a little confused about the on-demand account piece that you mentioned, since I thought that the deleted account persisted once the one or more users had been deleted from the server (so that their old messages etc could be associated with an account), and I have definitely deleted journalist users from this server before. Like I said, please take my comments with a grain of salt cause clearly I don't know this part of the code well at all 🙃

I'll spend some more time with it before we talk again on Monday.

[eloquence]

Without looking at the code, option 3 intuitively makes sense to me -- as a slight variation/alternative, could it make sense to create a deleted-internal user for such cases (which I guess is similar to option 2)?

[sssoleileraaa]

Thanks for the explanation, @creviera. I need to think it over and look at the code a little more (and obviously I defer to whatever your preference is since I haven't touched this part of the code), but of the options you've listed, I think I slightly prefer 2. Reasons: a) drafts only exist on the client, and this in-between state is only possible on the client, so a local solution seems preferable; and b) and it seems like keeping the option open to enforce non-null journalist IDs is a good idea, and if go with option 3 and then and want to enforce constraints on journalist_id later on, we may be setting ourselves up for more bugs/complexity.

It's a good point. It is a lot easier to reason about the code if we can ensure that there is always a real account association for the sender of replies, including draft replies.

I'm a little confused about the on-demand account piece that you mentioned, since I thought that the deleted account persisted once the one or more users had been deleted from the server (so that their old messages etc could be associated with an account), and I have definitely deleted journalist users from this server before. Like I said, please take my comments with a grain of salt cause clearly I don't know this part of the code well at all 🙃

Ah, yes, the "on-demand" comment is referring to how the "deleted" user account is created by the 2.2.0 server only when a user is deleted (see https://github.com/freedomofpress/securedrop/pull/6225/files#diff-a2d9e7b21fb79b5ba176e62769dd01d43dd7c6dd248dd12d717463d37523afcaR816), so it's not guaranteed to be there for us to use for draft replies. Also, the pre-2.2.0 server only tells the client about the "deleted" user when the client retrieves replies; the pre-2.2.0 server does not tell the client about the "deleted" user when the client retrieves users (the 2.2.0 server does). So we have to support both cases.

Without looking at the code, option 3 intuitively makes sense to me -- as a slight variation of that, could it make sense to create a deleted-internal user for such cases?

Yes, the deleted-internal user is option 2. I'll start writing this up and maybe it'll become obvious that option 2 is the way to go. We wouldn't want to use a reserved name, such as deleted-internal, if the server doesn't disallow it from being used when an admin creates a new account. The safest thing to do if we create an internal "deleted" user is to use the deleted UUID that the pre-2.2.0 server has been using for a long time to represent a deleted user.

[sssoleileraaa]

Just discussed this with @eloquence and we agree that the internal/draft "deleted" user should have a reserved spot on the server so that it can be disallowed: https://github.com/freedomofpress/securedrop/blob/82ec17bfc062c2e3a89c8de7c83c8b8088c5a3e8/securedrop/models.py#L445

@zenmonkeykstop this would require a data migration to handle the case where an admin has created an account with the name "deleted-internal" (or whatever name we decide to reserve). Let's discuss in person so that we can potentially add this to the rc2 for 2.2.0 release.

[sssoleileraaa]

@legoktm offered a good suggestion for our option 2 solution: just use the username deleted since it's already a reserved username on the server, and then update the uuid of that account to the real "deleted" user account if one is created in 2.2.0 or later.

[sssoleileraaa]

I'll keep this in draft mode until I can clean up my commits and do more testing, but the implementation for re-associated draft replies is finished.

[sssoleileraaa]

This probably isn't necessary anymore. I'll look into removing this Monday after more testing. It doesn't hurt but it's adding code complexity that isn't needed if we ensure that a Reply must always have an associated Sender account.

[gonzalo-bulnes]

I'm quite late to this thread, but would love further context on why we're opting to re-associate draft replies that were written by deleted users with some sort of user account. (No urgency; I wouldn't treat my question as a blocker.) Since drafts are typically personal, and haven't been seen by the source, my intuition would have been to treat them as state that's local to the client and delete them when the author is deleted.

[sssoleileraaa]

After a code walkthrough with @rocodes today, I squashed some commits. The only thing that has changed since our walkthrough is the code in storage.py around how we used to create accounts based on the replies endpoint. I updated the logic there to only create the DeletedUser locally to ensure that that will be the only difference between the server and workstation when it comes to accounts.

[sssoleileraaa]

I'm quite late to this thread, but would love further context on why we're opting to re-associate draft replies that were written by deleted users with some sort of user account. (No urgency; I wouldn't treat my question as a blocker.) Since drafts are typically personal, and haven't been seen by the source, my intuition would have been to treat them as state that's local to the client and delete them when the author is deleted.

@gonzalo-bulnes, once things slow down a bit with QA and the release is unblocked, I think we should have a SDW group discussion about this. It leads to at least a few other interesting topics around the v2 endpoint, server version support, features like saving draft replies or resending failed replies. But to try to answer your question before we have this group discussion, drafts, at this point, are just replies that are pending or failed to send. So no draft will exist unless the user pressed the Send button. And, it helps when you can see your failed reply show up in the conversation scrollarea, so that you know you what didn't make it to the server.

[gonzalo-bulnes]

[D]rafts, at this point, are just replies that are pending or failed to send. So no draft will exist unless the user pressed the Send button.

Gotcha, that makes a lot more sense that the "writing in progress" drafts I was imagining. 👍

And, it helps when you can see your failed reply show up in the conversation scroll area, so that you know you what didn't make it to the server.

Yes, of course. ✔️

[sssoleileraaa]

Also relevant to your question @gonzalo-bulnes: #1190

[sssoleileraaa]

looks like this needs a rebase...

[rocodes]

Environment: staging servers @ 2.1.0

Test Plan

Follow STR in #1414:

• Confirm that you can no longer repro the issue

Follow STR in #1416

• Confirm that you can no longer repro the issue

Good news, bad news.
If I follow the STR, on 2 occasions the new replies were associated with the new user that logged in, instead of the "deleted" user, and this persisted after multiple syncs. (For this test, I named journalists 1delete, 2delete, etc, and send a draft reply containing their username. You can see in the screenshot that the 6delete comment, written by the since-deleted 6delete user account, is associated with the 7delete user account instead of the sparkles.)
However, on 2 other occasions, the changes worked as expected. I do think this is the edgiest of edge-cases though, and I'm not sure it makes sense to hold things up trying to fix it

.

Follow STR in #1411

• Confirm that you can no longer repro the issue

[conorsch]

Tested these changes against a 2.1.0 prod server, due to freedomofpress/securedrop-sdk#171. Results were:

• ✔️ [0.6.0-rc1 QA] Client crash when name of user not logged into client (but logged into JI) changed #1411
• ✔️ [0.6.0-rc2 QA] Client crashes on login if user was logged out due to deletion #1414
• ⚠️ [0.6.0-rc2 QA] Client does not show deletd badge (sparkles) for draft replies + freezing #1416

For 1416, there were no problems with rendering the conversation pane. The specific problem I observed was a violation of this step in the "expected behavior" section of the report:

The reply badge for the pending reply from "deleteme" user fails to send with the "deleted" icon (sparkles)

That's not what I observed. After deleting the user whose draft reply had failed to send due to broken network, I logged in as a different user, and observed that the draft reply was sent to the source, and credited to the newly logged in user. Screenshots to document:

Draft reply in failed state

Sent reply initially attributed to deleted user, by name

Sent reply ultimately attributed to currently logged in user, who was logged in when draft sent

We should inspect the draft reply handling a bit more closely.

[eloquence]

Per discussion, I'll attempt to repro #1416, but I'll fully restart the client before logging back in again as a different user.

[eloquence]

Can't reproduce when fully restarting the app. The transition is as expected: initially still attributed and "failed to send", then the app syncs & the user is replaced with the star icon, but the reply stays "failed to send".

[eloquence]

Based on testing so far, I believe that the issue Conor and Ro hit appears to be a side effect of #1420, which appears to be a longstanding issue with jobs being retried when we don't expect them to be. If we can confirm that, it's IMO reasonable to consider it out of scope for the 0.6.0 release.

[sssoleileraaa]

Since this is not a 0.6.0 regression, I 100% agree that we should release the current changes as planned, on schedule, so that we don't hold up the server release.

[sssoleileraaa]

Based on testing so far, I believe that the issue Conor and Ro hit appears to be a side effect of #1420, which appears to be a longstanding issue with jobs being retried when we don't expect them to be. If we can confirm that, it's IMO reasonable to consider it out of scope for the 0.6.0 release.

@rocodes - if you're in before me tomorrow, could you confirm that #1420 is not a regression. If it is not, let's prioritize merging this PR and cutting an rc3 release. I don't think we should hold up the 0.6.0 release if the bug currently exists in production and since we are in edge case territory. I also don't want to delay the server 2.2.0 release. Once we develop a fix for #1420, we should make another client release immediately after.

Let me know if you think that's a good plan or if you think it's worth delaying both releases of the server and client (and remember we haven't announced a date for the release of the server yet, which we plan to do Thursday).

[rocodes]

Ok, so I've left a comment in the Retries Failed Jobs issue, and we'll definitely need to spend some more time figuring out some of these edge cases around network disconnection, but the tl;dr is, as Erik indicates, this behaviour was present pre-0.6.0.

I think we can do as you suggested, @creviera, and go ahead and merge these changes, then tackle the retry job issues separately afterwards.

[conorsch]

Thanks for the vigorous testing, all! Quoth @creviera:

Since this is not a 0.6.0 regression, I 100% agree that we should release the current changes as planned, on schedule, so that we don't hold up the server release.

Couldn't agree more. I'll start on the version bump to rc3.

[rocodes]

LVGTM; we've discussed mitigating the retry issue separately, so approving. Thanks for all the incredibly helpful commenting throughout here.

[rocodes]

Small note to our future selves that we may want to log before we run the database transactions, to pinpoint exactly where the errors are