TracAccountManager may corrupt ticket_change entries where field = 'comment' when changing a user ID in ticket_change entries where field = 'cc'

[anonymous]

TracAccountManager 0.5dev-r13381 with all *IdChanger Module enabled may corrupt ticket_change entries when changing a user ID, leading to Trac detected an internal error: ValueError: invalid literal for int() with base 10: 'newuser'. This does not apply to all ticket_changes, but to isolated cc field changes only.

Sample entries before change:

sqlite> select * from ticket_change where ticket = 1364 limit 4;
1364|1389367881566265|someuser|cc|newuser|olduser
1364|1389367881566265|someuser|comment|1|
1364|1389367939770929|someuser|cc|olduser|newuser
1364|1389367939770929|someuser|comment|2|

As can be seen, someuser was unsure as to what username should be in cc (which by the way is the reason i updated the account manager from stable to trunk to get rid of olduser entirely using this cool new feature ).

Unfortunately, the corrupt entries after the change lead to exceptions when rendering the ticket:

sqlite> select * from ticket_change where ticket = 1364 limit 4;
1364|1389367881566265|someuser|cc|newuser|newuser
1364|1389367881566265|someuser|comment|1|newuser
1364|1389367939770929|someuser|cc|newuser|newuser
1364|1389367939770929|someuser|comment|newuser|

its the last line 1364|1389367939770929|someuser|comment|newuser| that trac cannot render, it should have stayed 1364|1389367939770929|someuser|comment|2|

The other lines also seem to be wrong, but they do not lead to an exception, just read strange when rendered.

I think the best option is to leave ticket_change where field in ('cc','comment') unchanged, my guess is that it's ok to show the olduser in the cc field change history, while the important thing is to change the actual cc field in the ticket.

[Steffen Hoffmann]

This is a detailed, thus valuable issue analyze. Because of the reported side-effects I'll dig into the flawed code ASAP and hope to fix it.

Thanks a lot for elaborating on the issue.

Btw, any chance for you to leave an email contact here (by adding a comment while logged-in or at least with email in session preferences. Note that for privacy this Trac instance will hide you full email to non-admin users.

[Steffen Hoffmann]

Replying to anonymous:

I think the best option is to leave ticket_change where field in ('cc','comment') unchanged, my guess is that it's ok to show the olduser in the cc field change history, while the important thing is to change the actual cc field in the ticket.

Don't think so. How could a mismatch of change history and current ticket values be good? Better find and correct the flaw in db logic, which is what I'm currently struggling with. Probably time for some more unit tests.

[Thomas Moschny]

Was also hit by this, and this was observed because of the invalid literal for int() with base 10 exceptions.

However, a closer look shows that the problem is more severe, actually causes data loss: In case someone changed the observers field (CC) and put a comment for a ticket in one go, that comment will be overwritten and is lost.

The reason is, that in the ticket_change table, both changes, the one for the cc field, and the one for the comment field will have the exact same time stamp. The current code will update both rows, because it only checks for ticket id and time stamp, thus wrongly overwrite the oldvalue and/or newvalue in the field='comment' row, in addition to the desired changes in the field='cc' row.

I think this simple patch should fix the issue:

acct_mgr/model.py

@@ -233 +233 @@
                             db("""
                                 UPDATE %s SET %s=%%s
                                 WHERE ticket=%%s AND time=%%s
+                                AND field='cc'
                                 """ % (table, column),
                                (', '.join(cc), int(row[0]), int(row[1])))
                             result += 1

[Ryan J Ollos]

In 17868:

TracAccountManager 0.6dev: Fix overwrite in ticket_change

Patch by Thomas Moschny.

Fixes #11510.