Modify

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#8534 closed defect (fixed)

Can't resend password reset email

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

20110606_generic_sql.patch (2.3 KB) - added by hasienda 3 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 hasienda 3 years ago.
extended patch removing all other occurances of cursor.rowcount too

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by hasienda

  • Keywords SQL error added
  • Status changed from new to assigned

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 follow-up: Changed 3 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 3 years ago 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.

Changed 3 years ago by hasienda

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

comment:4 Changed 3 years ago 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.

Changed 3 years ago by hasienda

extended patch removing all other occurances of cursor.rowcount too

comment:5 Changed 3 years ago 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.

comment:6 Changed 3 years ago 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.

comment:7 Changed 3 years ago by hasienda

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

comment:8 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:9 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.