Handle case of deleted journalists #5284

prateekj117 · 2020-05-30T16:36:22Z

Status

Ready for review

Description of Changes

Changes proposed in this pull request:

Because of #5178, deleted journalist username and uuid is set to deleted. We should not allow the deleted journalist to login. Also, we can differentiate it from other not deleted journalists whose username is deleted by comparing both the username and uuid rather than just username. Also, we should not allow adding new journalist with username deleted for less confusion.

Testing

How should the reviewer test this PR?

Make a test journalist with username deleted, it would fail (due to changes in form.py file). Undo those changes and make a new journalist with username deleted and try logging in using credentials of that journalist, it should succeed. Change uuid of that journalist to deleted, it should fail to login again for that journalist.

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

kushaldas

We should have a list of usernames which will not be allowed, that will enable us to add more such keywords/names safely to the blocking list in future.

We also need required unit and functional tests and also related explanation documentation for the same in the PR.

securedrop/journalist_app/forms.py

prateekj117 · 2020-06-02T20:35:09Z

@kushaldas Done. Ready for another review. Let me know if I missed something.

kushaldas · 2020-06-04T07:58:43Z

@prateekj117 this PR needs a rebase with develop.

kushaldas · 2020-06-04T08:02:10Z

@prateekj117 Also, can you please add a functional test for the same? As right now the error should propagate to the admin nicely.

prateekj117 · 2020-06-06T13:27:19Z

@kushaldas Done. Ready for another review.

prateekj117 · 2020-06-09T15:57:39Z

@kushaldas One thing that can be missing from here is I think we can also check the exception that is being raised in unit tests. Let me know if that is required.

prateekj117 · 2020-06-10T16:22:30Z

Hey @kushaldas. The thing is, it's an endpoint, I am not sure how I will write a test for checking exception raising. app.post won't be able to check the exception as it happens on the server-side and app.post would just return a response. Can you share an example? It would be of great help. Thanks.

prateekj117 · 2020-06-10T18:24:27Z

Ok, I think I got what you meant. Added the tests. Let me know if anything else is required.

kushaldas · 2020-06-15T08:56:01Z

Hey @kushaldas. The thing is, it's an endpoint, I am not sure how I will write a test for checking exception raising. app.post won't be able to check the exception as it happens on the server-side and app.post would just return a response. Can you share an example?

This whole code base is server side. I don't understand what do you mean by endpoint in this case. You have to test that the right error message is showing in the proper way in the web application, Please have a look at https://github.com/freedomofpress/securedrop/blob/develop/securedrop/tests/functional/test_source_notfound.py or any other file in that directory for examples.

prateekj117 · 2020-06-15T09:04:52Z

@kushaldas Now that I see the examples, I get it. I will correct it. Thanks !!!

prateekj117 · 2020-06-18T04:55:25Z

@kushaldas Done.

rmol

Thanks @prateekj117. This looks pretty good. I requested a pedantic grammar change, would like to keep the list of invalid usernames on the Journalist model, and think we should omit invalid usernames from the exception or flash messages.

docs/admin.rst

securedrop/journalist_app/forms.py

securedrop/models.py

securedrop/tests/functional/journalist_navigation_steps.py

securedrop/tests/test_journalist.py

prateekj117 · 2020-06-25T19:51:51Z

@rmol Did the changes. Ready for another review.

kushaldas

We should get some feedback from @ninavizz about adding/showing the invalid usernames in the web form to create a new user.

securedrop/journalist_app/forms.py

securedrop/models.py

rmol

This looks good. I've run through the test scenario and confirmed behavior and messaging with the latest changes. Thanks again for your patience and perseverance, @prateekj117.

The behavior is as we specified.

In #5284 we made sure that the username `deleted` is not not allowed to be added in the system via the admin section of the journalist web application. But, one could still edit any existing user and change the name to `deleted`. Or, an admin can add a new user via `manage.py` in command line. In PR adds checks to make sure that addiving a new user via `manage.py` will fail if you try to set the username as `deleted`, it also blocks editing any existing username to `deleted`. The PR also includes unit and functional tests.

In #5284 we made sure that the username `deleted` is not not allowed to be added in the system via the admin section of the journalist web application. But, one could still edit any existing user and change the name to `deleted`. Or, an admin can add a new user via `manage.py` in command line. In PR adds checks to make sure that addiving a new user via `manage.py` will fail if you try to set the username as `deleted`, it also blocks editing any existing username to `deleted`. The PR also includes unit and functional tests. (cherry picked from commit 90e19ea)

prateekj117 requested review from kushaldas and redshiftzero as code owners May 30, 2020 16:36

kushaldas suggested changes Jun 1, 2020

View reviewed changes

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

prateekj117 requested review from emkll and zenmonkeykstop as code owners June 2, 2020 20:33

prateekj117 force-pushed the handle-deleted-condition branch 2 times, most recently from f2db3a0 to 997fed7 Compare June 6, 2020 12:39

prateekj117 requested a review from kushaldas June 8, 2020 16:29

prateekj117 force-pushed the handle-deleted-condition branch from 97ef783 to 0b9ecfd Compare June 10, 2020 16:27

prateekj117 force-pushed the handle-deleted-condition branch from a385888 to 9e43e6c Compare June 17, 2020 18:42

prateekj117 force-pushed the handle-deleted-condition branch 2 times, most recently from d0f7373 to 85327b0 Compare June 24, 2020 12:29

rmol suggested changes Jun 25, 2020

View reviewed changes

prateekj117 force-pushed the handle-deleted-condition branch from 85327b0 to bf1d966 Compare June 25, 2020 19:07

rmol previously approved these changes Jun 25, 2020

View reviewed changes

prateekj117 dismissed rmol’s stale review via 4a4818d June 26, 2020 16:48

prateekj117 force-pushed the handle-deleted-condition branch from bf1d966 to 4a4818d Compare June 26, 2020 16:48

kushaldas previously requested changes Jul 1, 2020

View reviewed changes

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

securedrop/models.py Show resolved Hide resolved

prateekj117 requested a review from kushaldas July 6, 2020 15:58

prateekj117 added 12 commits July 8, 2020 15:20

Handle case of deleted journalists

6761ce6

Check usernames from a list

763076a

Add tests to check if admin add user fail with invalid username

a6e53f8

Add documentation for not allowing deleted username

e14201f

Unused variable in tests

4981859

Check invalid username from lists in login

916f8db

Add tests for deleted user cannot login

5a833c6

Check deleted username exception handling using tests

12808d5

Add functional test for admin panel

dae2ab8

Add wait_for in selenium test

95a0b43

Requested changes

ae6ed94

Change error text to be more informative

6fafa9f

prateekj117 force-pushed the handle-deleted-condition branch from 4a4818d to 6fafa9f Compare July 8, 2020 10:22

prateekj117 added 2 commits July 8, 2020 16:00

Correct linting errors

ce9fcca

Correct failing tests

5a0b46f

prateekj117 requested review from rmol and eloquence July 8, 2020 12:53

rmol approved these changes Jul 8, 2020

View reviewed changes

rmol merged commit 1f91c97 into freedomofpress:develop Jul 8, 2020

rmol added this to the 1.5.0 milestone Jul 15, 2020

rmol mentioned this pull request Jul 15, 2020

A couple of ideas for better handling of deleted journalists #5378

Closed

kushaldas mentioned this pull request Jul 16, 2020

Fixes #5378, adds more checks for invalid username #5380

Merged

12 tasks

zenmonkeykstop mentioned this pull request Jul 20, 2020

Release SecureDrop 1.5.0 #5368

Closed

18 tasks

prateekj117 mentioned this pull request Aug 26, 2020

Preserve deleted journalist UUIDs in database to attribute replies #5467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle case of deleted journalists #5284

Handle case of deleted journalists #5284

prateekj117 commented May 30, 2020 •

edited

Loading

kushaldas left a comment

prateekj117 commented Jun 2, 2020

kushaldas commented Jun 4, 2020

kushaldas commented Jun 4, 2020

prateekj117 commented Jun 6, 2020

prateekj117 commented Jun 9, 2020

prateekj117 commented Jun 10, 2020

prateekj117 commented Jun 10, 2020

kushaldas commented Jun 15, 2020

prateekj117 commented Jun 15, 2020

prateekj117 commented Jun 18, 2020

rmol left a comment

prateekj117 commented Jun 25, 2020

kushaldas left a comment

rmol left a comment

Handle case of deleted journalists #5284

Handle case of deleted journalists #5284

Conversation

prateekj117 commented May 30, 2020 • edited Loading

Status

Description of Changes

Testing

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you made non-trivial code changes:

kushaldas left a comment

Choose a reason for hiding this comment

prateekj117 commented Jun 2, 2020

kushaldas commented Jun 4, 2020

kushaldas commented Jun 4, 2020

prateekj117 commented Jun 6, 2020

prateekj117 commented Jun 9, 2020

prateekj117 commented Jun 10, 2020

prateekj117 commented Jun 10, 2020

kushaldas commented Jun 15, 2020

prateekj117 commented Jun 15, 2020

prateekj117 commented Jun 18, 2020

rmol left a comment

Choose a reason for hiding this comment

prateekj117 commented Jun 25, 2020

kushaldas left a comment

Choose a reason for hiding this comment

rmol left a comment

Choose a reason for hiding this comment

prateekj117 commented May 30, 2020 •

edited

Loading

If you made changes to `securedrop-admin`: