Journalist Interface and API return different subsets of `Source`s #6667

cfm · 2022-11-02T17:50:33Z

Description

The Journalist Interface and API return different subsets of Sources. Under certain circumstances, the same journalist user will see more sources listed in a SecureDrop Client session than they will see in a simultaneous Journalist Interface session.

Recommended approach

Reconcile (or factor out?) the filters used by the Journalist Interface's index() and the Journalist API's get_all_sources()
Bonus: Check whether loaddata.py::record_source_interaction() can use session.commit() as usual
Bonus: Tests!

Steps to Reproduce

On the Server: Not an exact reproduction; this is my long-running test instance.

sdadmin@app:~$ sudo apt-cache policy securedrop-app-code
securedrop-app-code:
  Installed: 2.5.0+focal
  Candidate: 2.5.0+focal
  Version table:
 *** 2.5.0+focal 500
        500 https://apt.freedom.press/ focal/main amd64 Packages
        100 /var/lib/dpkg/status

Previously:

www-data@app:~/securedrop$ ./loaddata.py --seed 1547 \
    --journalist-count 0 \
    --source-count 3500 \
    --messages-per-source 1 \
    --replies-per-source 1

Then I used the Journalist Interface to Select All sources and Delete Selected.

Expected Behavior

On the Client: No sources are listed.

Actual Behavior

On the Client:

user@sd-app:~/securedrop-client$ sudo apt-cache policy securedrop-client
securedrop-client:
  Installed: 0.8.1+bullseye
  Candidate: 0.8.1+bullseye
  Version table:
 *** 0.8.1+bullseye 500
        500 https://apt.freedom.press/ bullseye/main amd64 Packages
        100 /var/lib/dpkg/status

Comments

On the Server:

sdadmin@app:~$ sudo -u www-data sqlite3 /var/lib/securedrop/db.sqlite "SELECT * FROM sources;"
1|eb2df246-c2d6-46dd-af81-83608411870f|3IGB6CI2DKHC2OGLJTSZYND6SOFGAQNMV2D5BCMXNR52PPV5UKXED5BCSMLKNN5M5UQTZPTB5VPJQBZMJYN34DXGE5GYIHMMOXZT7EQ=|politic paperweight||0|0|
255|db09f881-3b8f-4dec-bdf6-ab0bb715ea86|4PBQ3ZMPOJTKRRZZHGBCAX74GHOBINT6LHET3YOSO4Q77XGMMF3M5RZZVNYTO7DSEDUAWBUT36DOYCHM6O3QDMV7NVLYBPTJEO7HZEQ=|blissful obverse||0|0|
3756|b6280f51-ad5f-448c-8697-45e70cd417c5|IJNT2TRBQNT4ZHXSKRHCA44EAEPBOY76I3IXGQNBAGQVL2R7TZ5UHLAFLPUDM2A6UNTH2IUXWVMZ5WQHO7V3DX6BHZBROD2D5XWSTEA=|odds-on interruption||1|0|

And sure enough, compare:

The Journalist Interface filters on last_updated != None:

securedrop/securedrop/journalist_app/main.py

Lines 101 to 106 in 6170370

    
           # Query for sources without stars, along with their unread 
        
           # submission counts. 
        
           unstarred = ( 
        
               db.session.query(Source, unread_stmt.c.num_unread) 
        
               .filter_by(pending=False, deleted_at=None) 
        
               .filter(Source.last_updated.isnot(None))

The Journalist API does not:

securedrop/securedrop/journalist_app/api.py

Lines 124 to 126 in 6170370

    
           @api.route("/sources", methods=["GET"]) 
        
           def get_all_sources() -> Tuple[flask.Response, int]: 
        
               sources = Source.query.filter_by(pending=False, deleted_at=None).all()

The text was updated successfully, but these errors were encountered:

cfm · 2022-11-02T17:52:32Z

This is the kind of bug that motivates me to argue that the Journalist Interface should be a consumer of the Journalist API, whereas they're currently independent implementations of overlapping functionality.

eloquence · 2022-11-03T00:03:22Z

Thanks @cfm, and agree that consuming the API in the JI would be a best practice (with my standard caveat about investing in an implementation that I hope we'll be able to deprecate with the SecureDrop Workstation). In your estimation, could this issue plausibly arise outside the development environment / loaddata use case, or is last_updated always set in those cases?

cfm · 2022-11-03T16:36:06Z

That's a fair question to raise, @eloquence, and in practice I think you're right (to imply :-) that this is a rare edge-case. In theory, however, I don't see how it's meaningfully more rare via loaddata.py...

securedrop/securedrop/loaddata.py

Lines 172 to 174 in 87d84cf

    
           source.pending = False 
        
           source.last_updated = datetime.datetime.utcnow() 
        
           db.session.flush()

...than via the Source Interface's /submit:

securedrop/securedrop/source_app/main.py

Lines 307 to 309 in 542b59d

    
           logged_in_source_in_db.pending = False 
        
           logged_in_source_in_db.last_updated = datetime.now(timezone.utc) 
        
           db.session.commit()

My concern here, however, is not really that this state is a possibility, since it's really just an edge case of the normal pending = False case. I'm more interested in the asymmetry between the Journalist Interface and the Journalist API, which—much as I'd love to take this as our cue to do a bit of defensive refactoring—I estimate to be a one-line fix. :-)

eloquence · 2022-11-03T22:25:35Z

Thanks - I was mostly trying to understand the risk of production-level data integrity issues or user-facing impact.

In theory, however, I don't see how it's meaningfully more rare

If this summary is an accurate description of the flush/commit behavior, then it seems like loaddata may sometimes not persist the last_updated information if it doesn't reach a commit call. It's not clear to me why this particular method doesn't commit and only flushes.

But yes, completely agree they should also behave the same way in terms of the query.

cfm · 2022-11-04T03:07:46Z

On Thu, Nov 03, 2022 at 03:25:45PM -0700, Erik Moeller wrote: If [this summary](https://stackoverflow.com/a/4202016) is an accurate description of the flush/commit behavior, then it seems like `loaddata` may sometimes not persist the `last_updated` information if it doesn't reach a `commit` call. It's not clear to me why this particular method doesn't commit and only flushes.

Yes, I meant to look into that question next. Curiously, this is our *only* use of session.flush() *anywhere*[1], which makes me question it in the absence of any accompanying justification. I'll start a checklist at the top of this ticket: two tasks, and maybe a *two*-line fix. :-) [1]: https://github.com/freedomofpress/securedrop/search?q=session.flush&type=

audiodude · 2022-11-13T20:38:19Z

I'll work on this

audiodude · 2022-11-14T02:26:02Z

Newbie question here, but how do I get to the application that is marked as "On the client" with this interface:

audiodude · 2022-11-15T17:22:47Z

Sorry to post so many questions on such a simple task, but here goes. What is the preferred way of reconciling the Journalist Interface and the Journalist API? That is, should they both filter by last_updated is None or neither?

eloquence · 2022-11-15T19:35:39Z

I think the preferred behavior would be for the API to also filter with .last_updated.isnot(None). These are not valid sources and should never be returned.

The interface in the screenshot is the SecureDrop Client, which you see at https://github.com/freedomofpress/securedrop-client - let us now if you have any difficulty setting it up, and thanks for working on this :)

…idate-sources-search

cfm added bug api labels Nov 2, 2022

cfm added good first issue hackathon journalist interface (JI) labels Nov 2, 2022

cfm mentioned this issue Nov 2, 2022

GpgKeyNotFoundError can block deletion of a Source #6669

Open

legoktm assigned audiodude Nov 13, 2022

nabla-c0d3 linked a pull request Mar 5, 2023 that will close this issue

Consolidate logic for searching sources and provide pagination framework #6764

Open

nabla-c0d3 added a commit to nabla-c0d3/securedrop that referenced this issue Apr 21, 2023

Merge branch 'freedomofpress:develop' into freedomofpress#6667-consol…

37b401f

…idate-sources-search

audiodude removed their assignment Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Journalist Interface and API return different subsets of `Source`s #6667

Journalist Interface and API return different subsets of `Source`s #6667

cfm commented Nov 2, 2022 •

edited

Loading

cfm commented Nov 2, 2022 •

edited

Loading

eloquence commented Nov 3, 2022 •

edited

Loading

cfm commented Nov 3, 2022

eloquence commented Nov 3, 2022

cfm commented Nov 4, 2022 via email •

edited

Loading

audiodude commented Nov 13, 2022

audiodude commented Nov 14, 2022

audiodude commented Nov 15, 2022

eloquence commented Nov 15, 2022

Journalist Interface and API return different subsets of Sources #6667

Journalist Interface and API return different subsets of Sources #6667

Comments

cfm commented Nov 2, 2022 • edited Loading

Description

Recommended approach

Steps to Reproduce

Expected Behavior

Actual Behavior

Comments

cfm commented Nov 2, 2022 • edited Loading

eloquence commented Nov 3, 2022 • edited Loading

cfm commented Nov 3, 2022

eloquence commented Nov 3, 2022

cfm commented Nov 4, 2022 via email • edited Loading

audiodude commented Nov 13, 2022

audiodude commented Nov 14, 2022

audiodude commented Nov 15, 2022

eloquence commented Nov 15, 2022

Journalist Interface and API return different subsets of `Source`s #6667

Journalist Interface and API return different subsets of `Source`s #6667

cfm commented Nov 2, 2022 •

edited

Loading

cfm commented Nov 2, 2022 •

edited

Loading

eloquence commented Nov 3, 2022 •

edited

Loading

cfm commented Nov 4, 2022 via email •

edited

Loading