Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Jul 16, 2020

Status

Ready for review.

Description of Changes

Fixes #5378

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.

Testing

  • make build-debs
  • make staging
  • Run create-dev-data.py in the app-staging vm for 2 journalists accounts.
  • Try to add a new user called deleted via manage.py, this should fail.
  • Login as journalist and then try to change the username dellsberg to deleted, this should flash an error about "Invalid username"

invalid_username_error_sd

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:

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

@kushaldas kushaldas requested a review from redshiftzero as a code owner July 16, 2020 10:17
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.
@kushaldas kushaldas force-pushed the more_checks_invalid_username branch from 32e6ead to 90e19ea Compare July 16, 2020 10:34
Copy link
Contributor

@rmol rmol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

  • make build-debs
  • make staging
  • Run create-dev-data.py in the app-staging vm for 2 journalists accounts.
  • Try to add a new user called deleted via manage.py, this should fail.
  • Login as journalist and then try to change the username dellsberg to deleted, this should flash an error about "Invalid username"

@rmol rmol merged commit bbf76b7 into develop Jul 16, 2020
@rmol rmol deleted the more_checks_invalid_username branch July 16, 2020 14:45
@rmol rmol added this to the 1.5.0 milestone Jul 16, 2020
kushaldas added a commit that referenced this pull request Jul 16, 2020
@rmol rmol mentioned this pull request Jul 16, 2020
@rmol rmol mentioned this pull request Jul 22, 2020
18 tasks
sssoleileraaa pushed a commit that referenced this pull request Aug 31, 2020
…name

Fixes #5378, adds more checks for invalid username
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A couple of ideas for better handling of deleted journalists
2 participants