Can't resend password reset email

[jeffrey@…]

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.

[Steffen Hoffmann]

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?

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

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

[Steffen Hoffmann]

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

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

[Steffen Hoffmann]

extended patch removing all other occurances of cursor.rowcount too

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

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

[Steffen Hoffmann]

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

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