Add Seen/Unseen support at source level

[sssoleileraaa]

Description

Resolves #187

Test Plan

Test global read

Test that a source shows up as read (not bold) when someone other than the logged-in user has read the source.

Test global read with multiple readers

Test that a source shows up as read (not bold) when someone has read a part of a conversation and another person has read the other part of the conversation.

Test read after sync

Test that a source shows up as read (unbold) if someone on the JI downloads all the source conversation items.

Test unread

Test that a source shows up as unread (bold) if there are any source conversation items that are unread.

Test unread after sync

Test that a source shows up as unread (bold) if a new item for a seen source is retrieved from sync.

[eloquence]

In non-Qubes dev env:

• Selecting sources alters via either the SecureDrop Client or the JI alters its source-level seen status (for all users), until the next new source activity comes in
• If source is currently selected, new activity is immediately marked as seen
• Replies don't impact seen status
• Deleting users doesn't impact seen status
• Files/messages behave identically
• Seen status is not shown in offline mode, and is restored when logging back in

I haven't tested network error behavior & performance w/ Tor yet, but at a first quick run-through this is already looking great! 🎉

[sssoleileraaa]

I will remove this after the first review in case there are other changes I need to make.

[emkll]

Tested this PR in qubes, and using a long-running physical instance. I am still investigating, but there seems to be an issue when posting to the /seen endpoint, as well as perhaps the handling of that case (failed seen call in the client logic), which could be due to an sdk issue.

After first sync, I see a number of error 400's for POST to seen endpoint on each sync:

[...]
[12/Nov/2020:09:52:24 -0500] "POST /api/v1/seen HTTP/1.1" 400 637 "-" "-"
[12/Nov/2020:09:52:42 -0500] "POST /api/v1/seen HTTP/1.1" 400 637 "-" "-"
[12/Nov/2020:09:57:44 -0500] "POST /api/v1/seen HTTP/1.1" 400 637 "-" "-"
[...]

In the client logs, the logs look as follows:

2020-11-12 09:39:10,141 - securedrop_client.storage:108(get_remote_data) INFO: Fetched 20 remote sources.
2020-11-12 09:39:10,141 - securedrop_client.storage:109(get_remote_data) INFO: Fetched 75 remote submissions.
2020-11-12 09:39:10,141 - securedrop_client.storage:110(get_remote_data) INFO: Fetched 40 remote replies.
2020-11-12 09:39:10,147 - securedrop_client.storage:89(chronometer) INFO: update_sources duration: 0.0060s
2020-11-12 09:39:10,156 - securedrop_client.storage:89(chronometer) INFO: update_files duration: 0.0083s
2020-11-12 09:39:10,166 - securedrop_client.storage:89(chronometer) INFO: update_messages duration: 0.0099s
2020-11-12 09:39:10,195 - securedrop_client.storage:89(chronometer) INFO: update_replies duration: 0.0291s
2020-11-12 09:39:12,867 - securedrop_client.queue:179(process) ERROR: ReplyError: 'bad request'

This manifests in the client as follows (I installed over an existing, working sd-app with an already-populated database):

• the database migration occurs without issue and the client starts
• When the first sync occurs, the submissions become bolded in the source list, as expected
• However, after the first sync completes, all sources become unbolded (this coincides with the error described above)
• Any subsequent message from an existing source is never again bolded
• New sources are bolded and unbolded automatically as described above

I tried wiping the client database and starting from scratch to rule out any possible local database issues, but the issue persists.

[emkll]

Thanks for the update @creviera . Just tested again on c589dd9 and the problem still persists. The error is only really visible when using sd-proxy in Qubes, but the underlying issue is still present (but there are no adverse effects) when using the development environment. I think I figured out why:

In the dev environment, I see the 400 in the logs (but it doesn't affect functionality, likely because an exception is not thrown when handling the response in the sdk):

172.17.0.1 - - [16/Nov/2020 19:30:38] "POST /api/v1/seen HTTP/1.1" 400 -
172.17.0.1 - - [16/Nov/2020 19:30:39] "POST /api/v1/seen HTTP/1.1" 400 -
172.17.0.1 - - [16/Nov/2020 19:30:49] "GET /api/v1/sources HTTP/1.1" 200 -
172.17.0.1 - - [16/Nov/2020 19:30:49] "GET /api/v1/submissions HTTP/1.1" 200 -
172.17.0.1 - - [16/Nov/2020 19:30:49] "GET /api/v1/replies HTTP/1.1" 200 -
172.17.0.1 - - [16/Nov/2020 19:30:49] "GET /api/v1/user HTTP/1.1" 200 -
172.17.0.1 - - [16/Nov/2020 19:30:49] "POST /api/v1/seen HTTP/1.1" 400 -

In my full Qubes workstation setup, added some debug lines to the client and observed:

2020-11-16 10:58:35,463 - securedrop_client.queue:109(add_job) DEBUG: Added <securedrop_client.api_jobs.seen.SeenJob object at 0x7a2df829d828> to queue
2020-11-16 10:58:35,471 - securedrop_client.gui.widgets:658(on_source_changed) DEBUG: Set conversation to the selected source with uuid: 45fe2022-3998-4c73-afca-1b9072e85f73
2020-11-16 10:58:35,476 - securedrop_client.queue:267(resume_queues) DEBUG: Resuming main queue
2020-11-16 10:58:35,477 - securedrop_client.queue:270(resume_queues) DEBUG: Resuming download queue
2020-11-16 10:58:36,887 - securedrop_client.queue:179(process) ERROR: ReplyError: 'bad request'
2020-11-16 10:58:36,887 - securedrop_client.logic:626(on_seen_failure) DEBUG: 'bad request'
2020-11-16 10:58:36,888 - securedrop_client.queue:180(process) DEBUG: Skipping job

The request is failing with an error 400, and is getting repeated. Adding debug lines to the proxy line where the exception occurred provides more information:

2020-11-16 14:27:29,970 - securedrop_proxy.proxy:260(proxy) ERROR: {'Authorization': 'Token mytokenhere', 'Content-Type': 'application/json', 'Accept': 'application/json', 'Content-Length': '44'}
2020-11-16 14:27:29,970 - securedrop_proxy.proxy:261(proxy) ERROR: {"files": [], "messages": [], "replies": []}
2020-11-16 14:27:29,971 - securedrop_proxy.proxy:262(proxy) ERROR: {"error":"Bad Request","message":"Please specify the resources to mark seen."}

It appears here that the /seen endpoint is being called with no parameters, and this code path is being hit: https://github.com/freedomofpress/securedrop/blob/develop/securedrop/journalist_app/api.py#L332

Do you happen to know under which condition /seen will be called with empty parameters? I think it may be best to address closer to the API call by either:

1. Adding a condition in seen.py to prevent SeenJobs from being created/called without any files, messages and replies specified (https://github.com/freedomofpress/securedrop-client/pull/1165/files#diff-92a7091e5b41adadf9ccbfa9c44e59a91af7f7aec8b954bbf58f48a303be1ce2R22) or
2. Making sure that mark_seen function in logic.py does not create a SeenJob if all the lists in parameter are empty.

Regardless of which approach, we will want to make sure the error percolates up to the client if ever the api call fails. Looks like the proxy returns non-zero to the sdk when an error 400 occurs, and this is not happening in the client. We should wrap the call_api method of seen job try block https://github.com/freedomofpress/securedrop-client/pull/1165/files#diff-92a7091e5b41adadf9ccbfa9c44e59a91af7f7aec8b954bbf58f48a303be1ce2R16 to make sure the error is handled gracefully.

What do you think?

[sssoleileraaa]

Great debugging! I'll be back from vacation tomorrow and will implement option 1, which neatly organizes seen request and request validation code together within SeenJob.

[emkll]

Went though another pass, your fixes here do address the bad request issue, but I was incorrect in linking the bad request to the sources marked as in the UI with no user intervention, the bad request may have been a red herring.

I still see the automatic unbolding behavior in sd-app VMs. Strangely enough, I cannot reproduce this issue in the dev environment. In the client running in an sd-app VM, when I submit a message as a new source, or when I submit a message as an existing source (without this source selected in the client), all messages get unbolded without user intervention, about 5-10 seconds later, right about the following message appears

securedrop_client.gui.widgets:659(on_source_changed) DEBUG: Set conversation to the selected source with uuid: [...]

the seen_* tables in the local database are not updated, which suggests this is perhaps an issue with the UI only and not the underlying seen/unseen logic, nor with the syncing with the server as I initially thought. Investigating

[sssoleileraaa]

Since we ONLY see the issue when running securedrop-client (installed from a deb) rather than running the securedrop_client module from the virtual environment, then perhaps it's an issue when using the system-installed Qt version. If you run apt-cache show python-pyqt5 and apt-cache show python-sip and compare it to requirements/dev-requirements.txt you'll see the difference in package versions. And we can't pip install the exact packages that the sd-app system uses, see #633 and #273 (comment) and #1089 for background on other issues we've seen with different versions of Qt. For now, we can try to debug by adding lots of log lines and re-building the client deb and installing each time we want to test a potential fix.

[sssoleileraaa]

Tip: Instead of re-packaging the client each time we want to test, we can just use the prod virtualenv, just source /opt/venv/securedrop-client/bin/activate.

[sssoleileraaa]

Fix:

1. source /opt/venv/securedrop-client/bin/activate
2. pip install pyqt5==5.15

[emkll]

2. pip install pyqt5==5.15

I suspect also pip install qyt5==5.11.3 will also work, based on the findings above it looks like there's a discreptency between pyqt 5.11.3 as served by PyPI and 5.11.3 as packaged by Debian. Unfortunately, this will work in a development environment (and when sd-app has a network connection) but this will not work due to the fact that sd-app does not have a network connection (and we aren't validating hashes).

However, we could add this to build-requirements.txt and ship it in the virtualenv within the deb. This might have some significant added cost:

• The wheel hosted on PyPI is ~64MB
• This will may complicate the wheel building story (see add more developer docs #1089 for the debug build instructions
• Diff reviews will be complex
• Unclear if there are any further impacts of using a newer version of PyQT than what the system provides

In #1089 , the documentation has been updated to build a debug build of PyQT. We should see if we can find the underlying issue using the debug build. This is probably not the last time we will see discrepancies across versions/builds of PyQT.

[kushaldas]

In #1089 , the documentation has been updated to build a debug build of PyQT. We should see if we can find the underlying issue using the debug build. This is probably not the last time we will see discrepancies across versions/builds of PyQT.

I am wondering if we should spend that time to debug those older version of Qt. I am saying this as @creviera already found so many issues before. And the total time required to build and then debug is a lot. This looks like a constant research project for us.

A few options we have:

• Rebuild the wheel by ourselves and add to our wheel mirror.
• Download the upstream wheel and just add it to our wheel mirror. In this case we are blindly trusting what upstream is providing.
• Spend time trying to build a debug build ofPyQt and then debug the issue. One problem is that debug build most probably will not be built in the same configurations of the upstream wheel.
• (In future): we can test our securedrop-client again PySide2 wheels, and maybe move to start using those. But this also requires research time + a few import statement changes in the code.

[sssoleileraaa]

I suspect also pip install qyt5==5.11.3 will also work, based on the findings above it looks like there's a discreptency between pyqt 5.11.3 as served by PyPI and 5.11.3 as packaged by Debian. Unfortunately, this will work in a development environment (and when sd-app has a network connection) but this will not work due to the fact that sd-app does not have a network connection (and we aren't validating hashes).

Yes, 5.11.3 and on all seem to work fine, so seems like the difference must be in how it was packaged. I still think we should use the latest version of PyQt5 to get a couple years of bug fixes. The "fix" I mentioned above also demonstrates that there is no need to install a different version of Qt. And of course we won't have network access on sd-app and will need to update our build dependencies file, etc. (if we decide to go with this way of fixing the issue).

A few options we have:

• Rebuild the wheel by ourselves and add to our wheel mirror.
• Download the upstream wheel and just add it to our wheel mirror. In this case we are blindly trusting what upstream is providing.

It looks like the source is provided on PyPI, see https://riverbankcomputing.com/software/pyqt/download. On PyPI, you can see that the source tarball and wheels are authored by Riverbank Computing Limited under a GPL v3 license. If we don't want to trust the wheel hosted alongside the source on PyPI (we haven't so far with any of our dependencies, so why start now?), we can download the source tarball and build it ourselves easily (#1089 focuses on building a debug version of pyqt and qt, so below is a quick summary of what needs to happen).

1. download the pyqt source from PyPI
2. pip install https://pypi.org/project/PyQt-builder/
3. download the qt source for the version of qt we have installed on sd-app from debian
4. run pyqt-bundle wheel --qt-dir=<qt path>, see https://www.riverbankcomputing.com//static/Docs/PyQt-builder/pyqtbundle.html

• Spend time trying to build a debug build ofPyQt and then debug the issue. One problem is that debug build most probably will not be built in the same configurations of the upstream wheel.

I will be testing the changes @emkll added to #1089 to get the debug-level logs from qt and I'll also try to find a workaround (this doesn't look like the same issue as #254 (comment)). Let's see how that goes in the next couple days. We may decide that it makes the most sense to use our own wheel of the latest pyqt5 using the steps listed above.

[sssoleileraaa]

One problem is that debug build most probably will not be built in the same configurations of the upstream wheel.

This is a very good point, since 5.11.3 packaged by Riverbank Computing doesn't have the same issue as 5.11.3 packaged by debian. :/

[emkll]

Thanks @creviera for your work on this and @kushaldas for the assist in debugging these css issues . The latest commit does indeed resolve the issues I've been observing in Qubes. I went through another round of testing, and the test plan is passing for me locally. This is now good to merge.

One thing to note, either in the release notes or the release communications: The seen status is inferred from either client activity (with seen support introduced in this PR), or through is_downloaded on the journalist interface, so it's very likely that existing Client users will see some sources marked as unseen, despite having them seen them before in a previous build of the client.