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

Avoid rehashing user password hashes in {add,update}_global_user() #398

Merged
merged 1 commit into from
Aug 23, 2018

Conversation

mpkut
Copy link
Contributor

@mpkut mpkut commented Aug 22, 2018

In 6.2.32 and 6.2.34, our testing shows that using the GUI to add an existing address to a list causes the user's current password hash to be rehashed and stored. This renders the user's password unusable and effectively forces a password reset sequence. The behavior seems to correspond with the spontaneous password reinitialization problem discussed in #269.

This targeted fix uses hash_type() to detect password hashes and avoid rehashing them. For the sake of consistent behavior, both update_global_user() and add_global_user() are updated.

The change does not address all of the design concerns of #269 but in our testing it does prevent the seemingly spontaneous password resets caused by being added to a list using the GUI.

@ikedas
Copy link
Member

ikedas commented Aug 23, 2018

This looks a workaround for issue #167. We would be better to add it to the next beta (6.2.35b.1) and to see what will occur. So I'll merge.

@ikedas ikedas merged commit 3fb13ff into sympa-community:sympa-6.2 Aug 23, 2018
@ikedas
Copy link
Member

ikedas commented Aug 23, 2018

I made additional changes to track the bug: e02945f

@mpkut mpkut deleted the 6.2.34_fix_rehash_password branch August 23, 2018 04:56
@mpkut
Copy link
Contributor Author

mpkut commented Aug 23, 2018

Thank you!

BTW I would not see this as a workaround for #167; that seems to be a password reset loop. We have seen those too, in two varieties: 1) stored form data in browsers, and 2) people remaining on the password reset page even when they have successfully chosen a password. In both cases, clearing cookies and manually entering the form data seems to help.

In contrast, the band-aid in this PR is about keeping other people from invalidating your password hash simply by adding you to a list using WWSympa. Of course, encountering that situation will increase your chance of entering one of the password reset loop scenarios, which is what we've seen lately.

@ikedas
Copy link
Member

ikedas commented Aug 27, 2018

Hi @mpkut,

Honestly, I have not grasped what causes the bug reported in #167: Password field is overwritten unexpectedly. I hope your first aid will help making it clear. When sympa spews err log, please let us know.

@mpkut
Copy link
Contributor Author

mpkut commented Aug 30, 2018

Hello @ikedas
We installed the updated logging code this evening and pretty much immediately started seeing log messages. Here is a brief sample.

rehash-errors.txt

@mpkut
Copy link
Contributor Author

mpkut commented Aug 30, 2018

And along with do_add and do_subscribe, here are some stemming from do_auth.

do_auth-errors.txt

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.

2 participants