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

Prevent IntegrityError on repeated reset of journalist password #5618

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

rmol
Copy link
Contributor

@rmol rmol commented Nov 5, 2020

Status

Ready for review

Description of Changes

Fixes #5617.

Instead of breaking down and making everyone uncomfortable when encountering IntegrityError while revoking a journalist's two-factor token during a password reset, roll with it, take it in stride, demonstrating resilience, class, even aplomb.

Testing

Follow the STR in #5617 on develop. Then git checkout -b fix-5617 origin/fix-5617 and repeat. You should now be able to reset a password for the rest of your days error-free, if that seems a good alternative to whatever else 2020 is handing you.

Deployment

It's probably fine. It's better than it was, anyway. With the new error handling in place, the admin doesn't encounter an error, the journalist gets a new password, everyone's happy.

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

Choose one of the following:

  • I have opened a PR in the docs repo for these changes, or will do so later
  • I would appreciate help with the documentation
  • These changes do not require documentation

@emkll emkll moved this from In Development to Ready for Review in SecureDrop Team Board Nov 5, 2020
Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

The code diff looks okay, but I could not reproduce the original error.

I kept clicking reset password again and again.

password_chage_sd_no_error

@rmol
Copy link
Contributor Author

rmol commented Nov 6, 2020

And you were on current develop when you tried, and had logged in at least once as dellsberg?

@kushaldas
Copy link
Contributor

And you were on current develop when you tried, and had logged in at least once as dellsberg?

Yes, and I was still logged on as dellsberg from another browser window.

@eloquence
Copy link
Member

eloquence commented Nov 9, 2020

As I noted on the issue, I was able to reproduce. Can confirm that this PR resolves the issue; code changes make sense to me.

Copy link
Contributor

@kushaldas kushaldas left a comment

Choose a reason for hiding this comment

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

@eloquence confirmed that the patch solves the problem. I did a visual inspection and the code looks okay. CI is green. Approved.

@kushaldas kushaldas merged commit 5c0bf70 into develop Nov 11, 2020
SecureDrop Team Board automation moved this from Ready for Review to Done Nov 11, 2020
@kushaldas kushaldas deleted the fix-5617 branch November 11, 2020 12:06
@eloquence eloquence added this to the 1.7.0 milestone Jan 5, 2021
@kushaldas kushaldas mentioned this pull request Jan 18, 2021
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Resetting journalist password more than once can raise IntegrityError
3 participants