Ticket #8217 (new defect)

Opened 2 years ago

Last modified 5 months ago

Race condition when creating new accounts

Reported by: anonymous Assigned to: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: major Keywords: race-condition
Cc: rjollos 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

Change History

11/29/10 01:20:14 changed by hasienda

  • keywords set to needinfo race-condition.

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.

(in reply to: ↑ description ) 06/07/11 22:23:25 changed by hasienda

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.

06/12/11 21:35:45 changed by hasienda

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

12/01/12 16:55:52 changed by hasienda

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

12/30/12 22:51:01 changed by hasienda

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

12/30/12 22:57:48 changed by hasienda

  • keywords changed from needinfo race-condition to race-condition.
  • cc set to rjollos.
  • severity changed from normal to major.

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.

Add/Change #8217 (Race condition when creating new accounts)




Change Properties
Action