Opened 6 years ago

Closed 6 weeks ago

#8217 closed defect (fixed)

Race condition when creating new accounts

Reported by: anonymous Owned by: hasienda
Priority: normal Component: AccountManagerPlugin
Severity: major Keywords: race-condition
Cc: rjollos Trac Release: 0.12


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 hasienda

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

comment:3 Changed 6 years ago by hasienda

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

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

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

comment:6 Changed 4 years ago by hasienda

  • Cc rjollos added; anonymous removed
  • Keywords needinfo removed
  • 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.

comment:7 Changed 6 weeks ago by rjollos

  • Resolution set to fixed
  • Status changed from new to closed

Add Comment

Modify Ticket

as closed The owner will remain hasienda.
The resolution will be deleted. Next status will be 'reopened'.

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

Note: See TracTickets for help on using tickets.