Modify

Opened 6 years ago

Closed 5 months ago

#8217 closed defect (fixed)

Race condition when creating new accounts

Reported by: anonymous Owned by: Steffen Hoffmann
Priority: normal Component: AccountManagerPlugin
Severity: major Keywords: race-condition
Cc: Ryan J Ollos Trac Release: 0.12

Description

There is a race condition in AccountManagerPlugin that occurs when a user is creating a new account.

For the htfile backend, no file locking is ever done during the account creation process. For the db backend, the account creation process is spread across multiple transactions.

This means that one request can check to see if an account exists, not find it, start adding a new account, and then discover that another request already did the same thing (which results in it bombing out with an IntegrityError when trying to add the email, name, etc. to the database). Because of the order in which operations occur, this also means that the second request will wipe out the temporary password the first request created, which means the email the new user gets contains an invalid password.

Attachments (0)

Change History (7)

comment:1 Changed 6 years ago by Steffen Hoffmann

Keywords: needinfo race-condition added

Thanks for taking care and reviewing code in the first place.

Second, I think that I already mentioned possible race condition situations in source code comments when making a lot of changes and improvements to password file handling some time ago. And the likelihood of such an event including the one you mention in your report is practically very near to zero as far as I understand the situation.

I decided to go along with it for now, and I did so after discussing this very same issue with another developer.

I'll leave your report open for now in order to not hide any potential problems. But I urge you to present recommendations, how to resolve the issue, as I'm facing a lot of tasks here and can't do it all on my own.

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

Replying to anonymous:

![...] then discover that another request already did the same thing (which results in it bombing out with an IntegrityError when trying to add the email, name, etc. to the database).

At least this will be fixed by the next changes to SQL statements. Then we're always checking for existence, before INSERT, and these transactions are designed to be atomic. A theoretically possible overwrite still remains, AFAIK.

comment:3 Changed 6 years ago by Steffen Hoffmann

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

comment:4 Changed 4 years ago by Steffen Hoffmann

(In [12398]) AccountManagerPlugin: Releasing version 0.4, pushing development to acct_mgr-0.5dev.

Availability of that code as stable release closes #874, #3459, #4677, #5295, #5691, #6616, #7577, #8076, #8685, #8770, #8791, #8990, #9052, #9079, #9090, #9139, #9246, #9252, #9547, #9618, #9676, #9843, #9852, #9940, #10023, #10028, #10123, #10142, #10204, #10276, #10397, #10412, #10594, #10625 and #10644.

Some more issues have been worked-on, yet without confirmed resolution, refs #5464 (for JiraToTracIntegration), #8927 and #10134.

And finally there are some issues and enhancement requests showing progress, but known to require more work to resolve them satisfactorily, refs #843, #1600, #5964, #8217, #8933.

Thanks to all contributors and followers, that enabled and encouraged a good portion of this development work.

comment:5 Changed 4 years ago by Steffen Hoffmann

(In [12495]) AccountManagerPlugin: Optionally prevent overwriting user credentials, refs #8217.

This is an API change to the IPasswordStore interface.

Additionally account manager now adds account details only on success of the previous attempt to create a new password entry in primary password store.

comment:6 Changed 4 years ago by Steffen Hoffmann

Cc: Ryan J Ollos added; anonymous removed
Keywords: needinfo removed
Severity: normalmajor

Both user-editable stores are modified now to account for issues raised here, and I consider this issue as resolved so far.

I confess that it felt more serious when actually working on it lately, especially the potential for overwriting user credentials. The race time-frame dropped from minutes to milliseconds with these changes in place. Please provide further feedback, if you disagree.

comment:7 Changed 5 months ago by Ryan J Ollos

Resolution: fixed
Status: newclosed

Modify Ticket

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

Add Comment


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

 
Note: See TracTickets for help on using tickets.