Fixes #5176 Adds code+test to return replies without a journalist

[kushaldas]

Status

Ready for review.

Description of Changes

Fixes #5176
Fixes #5177

We can safely return a reply by a journalist who is deleted from
the system. This patch also includes a test case, and dev-data
generation script also now adds a reply and then deletes the
journalist.

Testing

• create-dev-data.py will add the first source with a journalist and then deletes the journalist
• https://github.com/freedomofpress/securedrop-sdk/blob/master/sdclientapi/__init__.py#L675 the list of replies for the first source can be fetched without any error

Deployment

Any special considerations for deployment? Consider both:

1. Upgrading existing production instances.
2. New installs.

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 changes to securedrop-admin:

• Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

• Configuration tests pass

If you made non-trivial code changes:

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

If you made changes to documentation:

• Doc linting (make docs-lint) passed locally

If you added or updated a code dependency:

Choose one of the following:

• I have performed a diff review and pasted the contents to the packaging wiki
• I would like someone else to do the diff review

[redshiftzero]

Looks good! CI is failing due to #5180 so we can resolve that first and then rebase this change

[redshiftzero]

I think one change that does make sense is setting both the default username and uuid for journalist replies when the journalist does not exist to deleted from the API (and leaving first/last names to their empty values, which is what we do elsewhere in the API for absent first/last name fields). Thus:

• when we sync the client, we'll have a single row for deleted journalists with UUID deleted (what we use as a unique identifier for journalists). Then when we log (generally done with the UUID except for login), we'll see clearly in the logs that this is a deleted journalist.
• if there is an existing journalist that just happens to have username deleted, they will be distinct in the users table in the client-side database as they will have different UUIDs. No unique constraint will be hit server-side as only one entry will exist with username deleted in the server database.

I've tested this locally, so I'll push a commit with this change. If others disagree with this approach please comment.

[eloquence]

Seems reasonable to me, I have a soft preference for using strings like _deleted_ that are more unambiguously searchable and traceable to our code, but of course defer to y'all.

[redshiftzero]

I'm approving @kushaldas' changes but would like someone to review the commit I added here. I've tested by applying this change to the dev env and syncing with the client, all works as expected (I went with deleted just so we can use as is in the UI without needing to strip off the _). There is an improvement we can make to preserve more conversation history as sources are deleted in the client (instead of associating replies with the deleted row when users are deleted), I'll file that over there, but the latest release of the client should work with this server-side change for the deleted journalist case.

[emkll]

Thanks @kushaldas and @redshiftzero , both your changes look good to me visually. Also functionally tested this in the dev env by replying with dellsberg and then deleting dellsberg. On develop, I can reproduce the error:

user@dev:~/src/securedrop$ curl -X GET   http://127.0.0.1:8081/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207/replies   -H 'Authorization: Token $TOKEN'   -H 'Content-Type: application/json' 

Traceback (most recent call last):
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 2309, in __call__
    return self.wsgi_app(environ, start_response)
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 2295, in wsgi_app
    response = self.handle_exception(e)
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 1741, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/_compat.py", line 35, in reraise
    raise value
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 2292, in wsgi_app
    response = self.full_dispatch_request()
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 1815, in full_dispatch_request
    rv = self.handle_user_exception(e)
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 1718, in handle_user_exception
    reraise(exc_type, exc_value, tb)
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/_compat.py", line 35, in reraise
    raise value
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 1813, in full_dispatch_request
    rv = self.dispatch_request()
  File "/opt/venvs/securedrop-app-code/lib/python3.5/site-packages/flask/app.py", line 1799, in dispatch_request
    return self.view_functions[rule.endpoint](**req.view_args)
  File "/home/user/src/securedrop/securedrop/journalist_app/api.py", line 48, in decorated_function
    return f(*args, **kwargs)
  File "/home/user/src/securedrop/securedrop/journalist_app/api.py", line 230, in all_source_replies
    reply in source.replies]}), 200
  File "/home/user/src/securedrop/securedrop/journalist_app/api.py", line 230, in <listcomp>
    reply in source.replies]}), 200
  File "/home/user/src/securedrop/securedrop/models.py", line 289, in to_json
    'journalist_username': self.journalist.username,
AttributeError: 'NoneType' object has no attribute 'username'

Switched to this branch, API call completes successfully, no errors 🎉

user@dev:~/src/securedrop$ git checkout fix_journalist_api 
Branch 'fix_journalist_api' set up to track remote branch 'fix_journalist_api' from 'origin'.
Switched to a new branch 'fix_journalist_api'
user@dev:~/src/securedrop$ curl -X GET   http://127.0.0.1:8081/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207/replies   -H 'Authorization: Token $TOKEN'   -H 'Content-Type: application/json' 
{
  "replies": [
    {
      "filename": "3-inimitable_assessment-reply.gpg", 
      "is_deleted_by_source": false, 
      "journalist_first_name": null, 
      "journalist_last_name": null, 
      "journalist_username": "journalist", 
      "journalist_uuid": "4a6bf90f-62fe-478e-8f37-cdeb1968fee7", 
      "reply_url": "/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207/replies/dea0d542-9acd-4a87-b590-9faddd1fe0e1", 
      "size": 1150, 
      "source_url": "/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207", 
      "uuid": "dea0d542-9acd-4a87-b590-9faddd1fe0e1"
    }, 
    {
      "filename": "4-inimitable_assessment-reply.gpg", 
      "is_deleted_by_source": false, 
      "journalist_first_name": null, 
      "journalist_last_name": null, 
      "journalist_username": "journalist", 
      "journalist_uuid": "4a6bf90f-62fe-478e-8f37-cdeb1968fee7", 
      "reply_url": "/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207/replies/f27b2b5f-72e4-47be-a6db-08ea0eb3af00", 
      "size": 1219, 
      "source_url": "/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207", 
      "uuid": "f27b2b5f-72e4-47be-a6db-08ea0eb3af00"
    }, 
    {
      "filename": "5-inimitable_assessment-reply.gpg", 
      "is_deleted_by_source": false, 
      "journalist_first_name": "", 
      "journalist_last_name": "", 
      "journalist_username": "deleted", 
      "journalist_uuid": "deleted", 
      "reply_url": "/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207/replies/efc0507c-92ed-4818-a4c5-9b44e61381b0", 
      "size": 1137, 
      "source_url": "/api/v1/sources/a0450224-1a73-4937-9080-604dc7943207", 
      "uuid": "efc0507c-92ed-4818-a4c5-9b44e61381b0"
    }
  ]
}

[emkll]

👍 setting the uuid is a great idea

[redshiftzero]

Thanks for review! I'll merge this now and create a patch for applying to our shared test instance.