Bodhi allows users to give +1 twice, counting it as +2 after a Build is updated or removed

[bowlofeggs]

As of the moment of this filing, this znc-1.6.3-6.el7 update has +2 karma, both of which seem to have come from elyscape. Bodhi should allow users to add further comments with Karma (so that users can change their mind and swap a +1 for a -1 or vice-versa), but it should only count their most recent karma vote in the total.

I am labeling this as critical since it has potential for users to game the system.

[bowlofeggs]

I spent some time yesterday and today trying to figure out how this happened. At first, I believed this happened due to a transaction race condition. The karma attribute of an Update is an integer column in the database that stores a denormalized form of the karma data. Though the two comments were 2.5 minutes apart, it was conceivable that a very heavily loaded server (or database server) would give enough time to start two comment transactions, with both of them giving a +1 comment. In this situation, neither of the transactions would see the comment from the other when they check to see if the user has already given a +1. However, it occurred to me that even though this is possible (and with a sleep statement I was able to get two concurrent transactions to go through the +1 process), they will each increase the karma by only 1 from where they started, and they would have each started at the same value, so the total increase of the two transactions is still only 1.

I have not been able to develop any other theories as to how this might have happened, but I do have a solution that I think is viable: I think it would be an improvement to normalize the karma calculation. I do not know why the karma is denormalized, but my guess is that it was done for performance reasons (to save having to load and process the comments to calculate the karma on an Update). However, most views on an Update (such as the web UI and the CLI) are going to want to render the comments anyway, which means that we aren't saving any database loading time by having the karma be a column on the Update table.

Due to this, I propose that we drop the karma column on the Update table and replace Update.karma with a property that iterates the Comments and counts only each user's most recent karma comment, returning that sum. I believe we will not see a noticeable performance impact by adjusting the karma to be normalized, and it will certainly solve the issue with the particular update mentioned in this issue. I believe it will also simplify a few areas of our code, as the karma rules are a bit complicated, and there is some repeated code in this area as well.

[jeremycline]

I also think normalizing makes good sense although, like you, I am disturbed by the lack of theories. If nothing else, normalizing the karma will cut down on the places this bug (or others) can hide

[bowlofeggs]

I just had a good chat with @puiterwijk where we attempted to explain what happened to this update, and @puiterwijk found the bug. This code attempts to filter out comments that happened before the update's builds were edited:

            # Take all comments since the previous karma reset
            reset_index = 0
            for i, comment in enumerate(self.comments):
                if (comment.user.name == u'bodhi' and ('New build' in
                    comment.text or 'Removed build' in comment.text)):
                    reset_index = i

            mycomments = [c.karma for c in self.comments if c.user.name == author][reset_index:]

@puiterwijk noted that the reset_index is used on the comments after they were filtered by the user's id, rather than being used before the filter. That last line should probably look like this instead:

mycomments = [c.karma for c in self.comments[reset_index:] if c.user.name == author]

@puiterwijk and I talked about whether to make this simple change, or whether to go ahead with the refactor to eliminate the karma column and always calculate it on demand and we agreed that the refactor would still be the preferred way to address this issue. The code as it is is quite complicated and the proposed refactor would greatly simplify the code which should reduce the maintenance burden and increase the product quality as a result.

[bowlofeggs]

It just occurred to me that my transaction theory does reveal another similar problem: if two users manage to start concurrent transactions and they both give a +1, the overall karma will only be increased by 1 and not by 2 because they will both set the karma to the current value + 1 instead of atomically increasing the value upon the COMMIT.

The proposed refactor would also solve this race condition, which makes it even more the right direction to go.