Modify

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#8549 closed defect (fixed)

Changing password in SessionStore when password change forced has no effect

Reported by: jteh Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: major Keywords: password db overwrite
Cc: Trac Release: 0.12

Description

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.

Attachments (2)

20110702_trac-log_SessionStore-user_forced-passwd-chg.txt (2.1 KB) - added by hasienda 3 years ago.
trac.log snippet from session doing forced password update for user testss
20110702_trac-log_SessionStore-user_forced-passwd-chg_with-get-session.txt (2.8 KB) - added by hasienda 3 years ago.
trac.log snippet from session doing forced password update for user testss with patched code

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by hasienda

  • Keywords password db overwrite added
  • Severity changed from normal to major

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.

comment:2 Changed 3 years ago by hasienda

  • Status changed from new to assigned

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.

Changed 3 years ago by hasienda

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

comment:3 in reply to: ↑ description Changed 3 years ago by hasienda

Replying to jteh:

  1. 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.

Changed 3 years ago by hasienda

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

comment:4 Changed 3 years ago by hasienda

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

comment:5 Changed 3 years ago by hasienda

  • Resolution set to fixed
  • Status changed from assigned to closed

(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.

comment:6 Changed 3 years ago by hasienda

(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.

Add Comment

Modify Ticket

Action
as closed .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from hasienda. Next status will be 'closed'.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.