Changing password in SessionStore when password change forced has no effect

[James Teh]

When AccountManagerPlugin is configured to use SessionStore and a password change is forced due to a password reset, the first password change has no effect; i.e. the password remains unchanged.

Steps to reproduce:

1. Ensure that AccountManagerPlugin is configured to use SessionStore and that there is an account with which to test.
2. Request a password reset for an account using the Forgot Password link.
3. Log in with the temporary password.
4. You will be prompted to change your password, so change it. You will be informed that the change was successful.
5. Log out.
6. Try to log back in with the new password.

• Expected: You should be logged in.
• Actual: You can't log in.
• When this happens, you can still log back in with the temporary password (step 2).
• If you log in and change the password again, this change (not forced) works correctly.

I investigated the code a bit. Here's what I *think* is happening:

1. When saving the password, SessionStore writes the password directly to the database.
2. If a password change is being forced, the code deletes force_change_passwd from the request's session object (req.session) and then saves the session. This happens at source:/accountmanagerplugin/trunk/acct_mgr/web_ui.py@9584#L270:[[br​]]

                   if force_change_password:
                       del(req.session['force_change_passwd'])
                       req.session.save()
   

3. Unfortunately, the session object didn't know about the changed session attribute (the password) because it was written directly to the database (1), so when it is saved (2), it overwrites the password change (1).

Assuming I'm right, either:

1. SessionStore needs to write to the request's session object. However, I don't think there's any way for SessionStore to access that; or
2. The session object somehow needs to be notified that its data has changed. The only way I can see to do this is to call req.session.get_session(req.authname, authenticated=True), but this is pretty ugly and I'm not sure if it has any nasty side effects.

[Steffen Hoffmann]

Thanks for the detailed report and analysis of the situation.

Using the SessionStore was just an episode for me. But since we started to rely on ResetPwStore (inherited from SessionStore) for the 'lost password' procedure I'll definitely go for it within the next release cycle.

[Steffen Hoffmann]

First, I confirm your theory regarding the new password getting overwritten by session.save() with the old value by own tests (log file​ attached). You can observe the

• new password is written to the db at 21:13:27,140 followed by a
• full delete of all session attributes for user 'testss' at 21:13:27,152 and
• all (old) values getting reINSERTed at 21:13:27,153

effect: unintended password reset within 13 milliseconds - quod erat demonstrandum.

Second, the SessionStore is not aware of a request object, nor should we make it so, because

1. it's IPasswordStore design, and is good like this
2. current req object is not reliably related to the user changing the password (i.e. could be an admin as well)

I'm still thinking about the best approach to fix it.

[Steffen Hoffmann]

trac.log snippet from session doing forced password update for user testss

[Steffen Hoffmann]

Replying to jteh:

2. The session object somehow needs to be notified that its data has changed. The only way I can see to do this is to call req.session.get_session(req.authname, authenticated=True), but this is pretty ugly and I'm not sure if it has any nasty side effects.

I don't see a less ugly way to get changed value for key 'password' after some testing. Introducing direct db access into _do_change_password method of AccountModule (as initially cite by yourself) is certainly even worse.

Testing with patched code​ looks ok, no apparent side-effects visible. So your hint is encouraging to me to do the right thing. I'll apply this right-away. Would be good, if you could confirm after update to the corresponding revision.

Thanks in advance for taking care and reporting here.

[Steffen Hoffmann]

trac.log snippet from session doing forced password update for user testss with patched code

[Steffen Hoffmann]

(In [10376]) AccountManagerPlugin: Preserve forced SessionStore password change, refs #8549.

[Steffen Hoffmann]

(In [10393]) AccountManagerPlugin: Releasing version 0.3, pushing development to 0.4.

This new feature release finally propagates a number of solutions into an official release, after some time of testing with trunk, so explicitely closes #442, #816, #2966, #3989, #4160, #6821, #7111, #8534, #8549, #8663, #8813, #8892, #8925, #8936 and #8939.

Should have made this months ago, but felt so many pending issues were too bad for a new release. But it has been a tremendous ticket burndown since last year, so it's really worth considering an upgrade now. See fresh changelog for details.

[Steffen Hoffmann]

(In [10395]) AccountManagerPlugin: Releasing version 0.3, pushing development to 0.4.

This new feature release finally propagates a number of solutions into an official release, after some time of testing with trunk, so explicitely closes #442, #816, #2966, #3989, #4160, #6821, #7111, #8534, #8549, #8663, #8813, #8892, #8925, #8936 and #8939.

Should have made this months ago, but felt so many pending issues were too bad for a new release. But it has been a tremendous ticket burndown since last year, so it's really worth considering an upgrade now. See fresh changelog for details.