Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#8534 closed defect (fixed)

Can't resend password reset email

Reported by: jeffrey@… Owned by: Steffen Hoffmann
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 (2)

20110606_generic_sql.patch (2.3 KB) - added by Steffen Hoffmann 6 years ago.
rewrite SQL 'UPSERT' statements in web_ui.py to be atomic and more portable
20110607_more_generic_sql.patch (9.2 KB) - added by Steffen Hoffmann 6 years ago.
extended patch removing all other occurances of cursor.rowcount too

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Steffen Hoffmann

Keywords: SQL error added
Status: newassigned

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?

comment:2 Changed 6 years ago by jeffrey@…

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

comment:3 in reply to:  2 Changed 6 years ago by Steffen Hoffmann

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.

Changed 6 years ago by Steffen Hoffmann

Attachment: 20110606_generic_sql.patch added

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

comment:4 Changed 6 years ago by Steffen Hoffmann

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.

Changed 6 years ago by Steffen Hoffmann

extended patch removing all other occurances of cursor.rowcount too

comment:5 Changed 6 years ago by Steffen Hoffmann

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.

comment:6 Changed 6 years ago by Steffen Hoffmann

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.

comment:7 Changed 6 years ago by Steffen Hoffmann

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

comment:8 Changed 6 years ago by Steffen Hoffmann

Resolution: fixed
Status: assignedclosed

(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:9 Changed 6 years ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Steffen Hoffmann.
The resolution will be deleted.

Add Comment


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

 
Note: See TracTickets for help on using tickets.