Ticket #8534 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

Can't resend password reset email

Reported by: jeffrey@endrift.com Assigned to: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: normal Keywords: SQL error
Cc: Trac Release: 0.12

Description

One of my users tried to reset his password, but never got his email (my Trac never sent the email, and I can't figure out why, but this seems to be a separate problem), so he tried again, and got an exception from the database saying there was duplicate primary key. I'm using a db password store with a MySQL database. I am using TracAccountManager?-0.3dev_r9785

The exception is generated by this block of code in _do_reset_password in web_ui.py: 

            cursor.execute("UPDATE session_attribute SET value=%s "
                           "WHERE name=%s AND sid=%s AND authenticated=1",
                           (1, "force_change_passwd", username))
            if not cursor.rowcount:
                cursor.execute("INSERT INTO session_attribute "
                               "(sid,authenticated,name,value) "
                               "VALUES (%s,1,%s,%s)",
                               (username, "force_change_passwd", 1))

Notably, the UPDATE says 0 affected rows because there is already a row in there that has that value. As such, it sees no rows were updated, and it tries to insert the row (which is already there), and that throws an exception.

Attachments

20110606_generic_sql.patch (2.3 kB) - added by hasienda on 06/06/11 21:55:34.
rewrite SQL 'UPSERT' statements in web_ui.py to be atomic and more portable
20110607_more_generic_sql.patch (9.2 kB) - added by hasienda on 06/07/11 21:47:52.
extended patch removing all other occurances of cursor.rowcount too

Change History

06/06/11 00:48:08 changed by hasienda

  • status changed from new to assigned.
  • keywords set to SQL error.

You're most probably right about the rowcount attribute not being 100% cross-db-compatible.

I've checked with SQLite backend - zero problems, but that there's no place in Trac core, where this cursor attribute is used today. So replacing the test with a SELECT SQL statement is the safest bet to on compatibility.

Would you be so kind as to test and report back, if I make a possible solution available here, please?

(follow-up: ↓ 3 ) 06/06/11 02:32:26 changed by jeffrey@endrift.com

I can test possible fixes, TOCTTOU/non-atomic problems notwithstanding.

For what it's worth, the aforementioned emails not being sent was due to the correct plugins not being activated in the trac.ini; it was an entirely separate issue (and a misconfiguration, not a bug).

(in reply to: ↑ 2 ) 06/06/11 20:29:26 changed by hasienda

Replying to jeffrey@endrift.com:

I can test possible fixes, TOCTTOU/non-atomic problems notwithstanding.

Don't worry, I've got advice from current Trac devs, including good references to current source code, and have done quite some db hacking recently. So this will be atomic for sure.

For what it's worth, the aforementioned emails not being sent was due to the correct plugins not being activated in the trac.ini; it was an entirely separate issue (and a misconfiguration, not a bug).

This is of course valuable information. I hate open ends. Better announce a fix/solution, so other can find and us it later. Thanks for telling us. Now I'll go and try to give you something to test ASAP. Let's have a patch against current trunk, and actually commit that changes, if it's working for you. Would be great, if you could even test pre-/after-patch.

06/06/11 21:55:34 changed by hasienda

  • attachment 20110606_generic_sql.patch added.

rewrite SQL 'UPSERT' statements in web_ui.py to be atomic and more portable

06/06/11 22:03:48 changed by hasienda

There are some changes in the patch, that are not strikly related but more general code cleanup. At least it's more conform to Trac coding style. I feel, this is good for code quality, i.e. could be seen as an invitation for review by other devs.

Patch is against current trunk code, that has already some changes compared to your last version. Please tell me too, if there are other issues you do encounter while testing. Meanwhile I'll go over other parts of this plugin to do similar changes as needed.

06/07/11 21:47:52 changed by hasienda

  • attachment 20110607_more_generic_sql.patch added.

extended patch removing all other occurances of cursor.rowcount too

06/07/11 21:55:13 changed by hasienda

The second patch should finally take down all remaining non-portable SQL code.

This patch is not cumulative, but applies on top of current trunk code. So if you feel like testing even more fixed code, please take a fresh checkout or remove the first patch before applying the bigger one.

I'm looking forward to your test results.

06/12/11 20:43:17 changed by hasienda

I've done some testing on my own and by now I'm reasonable confident, that it'll work.

Still tests with db backends other than SQLite are missing and any feedback regarding SQL portability is highly appreciated.

06/12/11 21:35:45 changed by hasienda

(In [10282]) AccountManagerPlugin: Replace SQL statements with more portable code, refs #8217 and #8534.

07/07/11 22:11:23 changed by hasienda

  • status changed from assigned to closed.
  • resolution set to fixed.

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

07/07/11 23:10:25 changed 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/Change #8534 (Can't resend password reset email)




Change Properties
Action