API changes for listing/marking source conversation items that have been seen by journalists #5513

rmol · 2020-09-17T19:53:08Z

Status

Ready for review

Description of Changes

Add new API endpoint for listing or marking source conversation items that have been seen by a journalist.

Add utility method to mark a heterogeneous collection of Submission and Reply objects seen.

Add Submission.is_file and Submission.is_message to encapsulate the characterization based on filename.

Fixes #5475.

Testing

Run the dev server with make dev.

Get an API token as the journalist user:

export TOKEN=$(curl -o- -X POST -H "Content-Type: application/json" --data '{"username":"journalist","passphrase":"correct horse battery staple profanity oil chewy","one_time_code":"047889"}' http://localhost:8081/api/v1/token | jq -r .token)

List current submissions:

curl -X GET -H "Content-Type: application/json" -H "Authorization: Token $TOKEN" http://localhost:8081/api/v1/submissions

All should have a seen_by property, currently an empty list.

Mark a message seen:

curl -X POST -H "Content-Type: application/json" -H "Authorization: Token $TOKEN" --data '{"messages": ["e711d29d-d0a1-440f-80e4-6642b77ec3ea"]}' http://localhost:8081/api/v1/seen

Replace the UUID with one from one of the messages in the response from the /submissions endpoint, of course.

Mark the same message seen again. There should be no error.
Retrieve /submissions again. The message's seen_by list should now contain your journalist account's UUID.
Visit the source of that message in the journalist interface. The message you marked should not be listed in bold text. Clicking "Select unread" should not select that message.
Visit the source list in the journalist interface. Select the source of the message you marked and click "Download Unread". The zip file you receive should not contain the seen message.

Via the source interface, upload a file, then retrieve /submissions again, and mark the file seen:

curl -X POST -H "Content-Type: application/json" -H "Authorization: Token $TOKEN" --data '{"files": ["4fe4b7f5-3031-4651-9092-888951681cf7"]}' http://localhost:8081/api/v1/seen

Replace the UUID with that of the file you just uploaded, of course.

Repeat the checks in the journalist interface that you performed for a seen message, confirming that the file you marked seen is always considered read.
Get another API token, this time as the dellsberg journalist account.

Retrieve the list of all replies:

curl -X GET -H "Content-Type: application/json" -H "Authorization: Token $TOKEN" http://localhost:8081/api/v1/replies

Mark one of them read:

curl -X POST -H "Content-Type: application/json" -H "Authorization: Token $TOKEN" --data '{"replies": ["8924a691-3b0f-45c1-bfed-b0b91e264855"]}' http://localhost:8081/api/v1/seen

Hit /replies again and confirm that the dellsberg account UUID appears in the reply's seen_by list.

Deployment

Depends on the database changes in #5505.

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

sssoleileraaa

did a first pass

securedrop/create-dev-data.py

securedrop/journalist_app/api.py

sssoleileraaa · 2020-09-23T00:28:30Z

Not sure how the API is going to look once you're done, but just as a reminder, we were also considering updating the current replies and submissions endpoints as an option:

[Option 2] Update current endpoints

"replies": [
        {
            "filename": <filename>,
            ...
            "seen_by": [<journalist_uuid>, ]
        },
        {
            "filename": <filename>,
            ...
            "seen_by": [<journalist_uuid>, ]
        },
]

"submissions": [
        {
            "is_read": <is_read>,
            "filename": <filename>,
            ...
            "seen_by": [<journalist_uuid>, ]
            "opened_by": [<journalist_uuid>, ]
        },
        {
            "is_read": <is_read>,
            "filename": <filename>,
            ...
            "seen_by": [<journalist_uuid>, ]
            "opened_by": [<journalist_uuid>, ]
        },
]

sssoleileraaa · 2020-09-23T00:40:17Z

just a thought, we could rename "is_read" to "downloaded_from_web" ~~or "seen_from_web"~~

rmol · 2020-09-23T11:50:03Z

Not sure how the API is going to look once you're done, but just as a reminder, we were also considering updating the current replies and submissions endpoints as an option

That is already present, as suggested in the test plan. Look at the to_json methods of Submission and Reply in models.py.

rmol · 2020-09-23T11:58:47Z

just a thought, we could rename "is_read" to "downloaded_from_web"

I know we've been playing pretty fast and loose with the API and versioning (at least it's mostly been additions 😐 ), but is_read is in production. It's possible that someone is using the API for something other than the workstation, so we should at least pretend we care about API stability.

lgtm-com · 2020-09-23T17:54:16Z

This pull request introduces 2 alerts when merging ecfe91f into 3d13faf - view on LGTM.com

new alerts:

1 for Unused import
1 for Variable defined multiple times

lgtm-com · 2020-09-23T18:07:08Z

This pull request introduces 2 alerts when merging b16332e into 3d13faf - view on LGTM.com

new alerts:

1 for Unused import
1 for Variable defined multiple times

lgtm-com · 2020-09-23T18:15:03Z

This pull request introduces 1 alert when merging 5bdc7e0 into 3d13faf - view on LGTM.com

new alerts:

1 for Variable defined multiple times

lgtm-com · 2020-09-23T22:24:16Z

This pull request introduces 3 alerts when merging c98374b into 3d13faf - view on LGTM.com

new alerts:

3 for Unused import

Add new API endpoint for listing or marking source conversation items that have been seen by a journalist. Add utility method to mark a heterogeneous collection of Submission and Reply objects seen. Add Submission.is_file and Submission.is_message to encapsulate the characterization based on filename.

With the addition of journalist_app.utils.mark_seen, it can replace the helper methods in tests/utils/db_helper.py, and the ad hoc logic to mark things seen in journalist_app/col.py.

sssoleileraaa

first pass through the code - going to start manually testing

sssoleileraaa · 2020-09-24T00:39:43Z

securedrop/models.py

@@ -212,7 +211,23 @@ def __init__(self, source: Source, filename: str) -> None:
    def __repr__(self) -> str:
        return '<Submission %r>' % (self.filename)

-    def to_json(self) -> 'Dict[str, Union[str, int, bool]]':
+    @property
+    def is_file(self) -> bool:


thought about doing this myself in #5505 but the pr was already so large. nice change.

sssoleileraaa · 2020-09-24T00:43:46Z

securedrop/models.py

+    def to_json(self) -> "Dict[str, Union[str, int, bool]]":
+        seen_by = {
+            f.journalist.uuid for f in SeenFile.query.filter(SeenFile.file_id == self.id)
+            if f.journalist


i see, so this won't include the fact that there is a deleted journalist who saw a file. gonna keep reviewing and will cycle back to this. could be an issue because we still send "deleted" uuids for replies when journalists are deleted. this takes a different course.

It does not. I didn't see any way for sensible inclusion of those records in seen_by here. We do have Submission.seen, which will reflect those, and until we start keeping deleted journalists around, we can't distinguish between the records for the submission in the seen tables anyway, so since the only purpose of those rows at this point is establishing the global seen state, I thought Submission.seen the right answer.

There are also replies we have to worry about which don't have a global seen state, but I definitely see why you made this choice. Agreed that implementing #5467 + global user account for data migration will make this work fine.

Replies do have a global seen state: if one exists, its creator has seen it. SeenReply can tell us who else has seen it, but only if journalist_id is not null, so again, I didn't see any point in including records where it is.

conorsch

Detailed test plan checks out, functionality matches expectations. Visual review of the diff was smooth, as well. As discussion during review indicates, we should queue up some "v2" refinements for the API.

rmol requested review from kushaldas and redshiftzero as code owners September 17, 2020 19:53

eloquence mentioned this pull request Sep 17, 2020

Add API endpoints for seen_by #5475

Closed

rmol marked this pull request as draft September 18, 2020 13:57

sssoleileraaa force-pushed the 5474-seen-tables branch 16 times, most recently from 1c3e733 to ee10a74 Compare September 22, 2020 23:17

sssoleileraaa mentioned this pull request Sep 22, 2020

Add seen tables and update seen status based on downloads in JI #5505

Merged

29 tasks

sssoleileraaa reviewed Sep 23, 2020

View reviewed changes

securedrop/create-dev-data.py Show resolved Hide resolved

securedrop/journalist_app/api.py Outdated Show resolved Hide resolved

sssoleileraaa force-pushed the 5474-seen-tables branch from ee10a74 to 3d13faf Compare September 23, 2020 00:42

rmol force-pushed the 5475-unseen-academicals branch from 078a022 to ecfe91f Compare September 23, 2020 17:47

rmol force-pushed the 5475-unseen-academicals branch from ecfe91f to b16332e Compare September 23, 2020 18:00

rmol force-pushed the 5475-unseen-academicals branch from b16332e to 5bdc7e0 Compare September 23, 2020 18:08

rmol force-pushed the 5475-unseen-academicals branch 3 times, most recently from d24f4dd to c98374b Compare September 23, 2020 22:17

rmol marked this pull request as ready for review September 23, 2020 22:19

rmol force-pushed the 5475-unseen-academicals branch from c98374b to 38e866c Compare September 23, 2020 22:31

conorsch mentioned this pull request Sep 23, 2020

Add /users endpoint #5506

Merged

5 tasks

Allie Crevier and others added 4 commits September 23, 2020 20:11

store seen-by when files are downloaded

c1a9282

Drop GET handling at /seen API endpoint

7ca095f

Mark submissions and replies seen the same way in more places

3542b67

With the addition of journalist_app.utils.mark_seen, it can replace the helper methods in tests/utils/db_helper.py, and the ad hoc logic to mark things seen in journalist_app/col.py.

rmol force-pushed the 5475-unseen-academicals branch from 38e866c to 3542b67 Compare September 24, 2020 00:11

rmol requested review from conorsch and emkll as code owners September 24, 2020 00:11

rmol changed the base branch from 5474-seen-tables to develop September 24, 2020 00:12

rmol changed the title ~~[WIP] API changes for listing/marking source conversation items that have been seen by journalists~~ API changes for listing/marking source conversation items that have been seen by journalists Sep 24, 2020

sssoleileraaa reviewed Sep 24, 2020

View reviewed changes

conorsch approved these changes Sep 24, 2020

View reviewed changes

conorsch merged commit 1f5b5fd into develop Sep 24, 2020

emkll mentioned this pull request Sep 28, 2020

Release SecureDrop 1.6.0 #5501

Closed

22 tasks

This was referenced Sep 29, 2020

Add API documentation for "seen" API freedomofpress/securedrop-docs#11

Closed

Add support for /seen endpoint and seen data freedomofpress/securedrop-sdk#132

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API changes for listing/marking source conversation items that have been seen by journalists #5513

API changes for listing/marking source conversation items that have been seen by journalists #5513

rmol commented Sep 17, 2020 •

edited

Loading

sssoleileraaa left a comment

sssoleileraaa commented Sep 23, 2020 •

edited

Loading

sssoleileraaa commented Sep 23, 2020 •

edited

Loading

rmol commented Sep 23, 2020

rmol commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

sssoleileraaa left a comment

sssoleileraaa Sep 24, 2020

sssoleileraaa Sep 24, 2020

rmol Sep 24, 2020

sssoleileraaa Sep 24, 2020

rmol Sep 24, 2020

conorsch left a comment

API changes for listing/marking source conversation items that have been seen by journalists #5513

API changes for listing/marking source conversation items that have been seen by journalists #5513

Conversation

rmol commented Sep 17, 2020 • edited Loading

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made non-trivial code changes:

sssoleileraaa left a comment

Choose a reason for hiding this comment

sssoleileraaa commented Sep 23, 2020 • edited Loading

sssoleileraaa commented Sep 23, 2020 • edited Loading

rmol commented Sep 23, 2020

rmol commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

lgtm-com bot commented Sep 23, 2020

sssoleileraaa left a comment

Choose a reason for hiding this comment

sssoleileraaa Sep 24, 2020

Choose a reason for hiding this comment

sssoleileraaa Sep 24, 2020

Choose a reason for hiding this comment

rmol Sep 24, 2020

Choose a reason for hiding this comment

sssoleileraaa Sep 24, 2020

Choose a reason for hiding this comment

rmol Sep 24, 2020

Choose a reason for hiding this comment

conorsch left a comment

Choose a reason for hiding this comment

rmol commented Sep 17, 2020 •

edited

Loading

sssoleileraaa commented Sep 23, 2020 •

edited

Loading

sssoleileraaa commented Sep 23, 2020 •

edited

Loading