Race condition when creating initial user could allow multiple users to be created.

[baus]

In /core/server/models/user.js in the add() function the database is asynchronously queried for the number of users in the database, and if one exists, then the user cannot create a new user.

It is possible multiple for connections to query the database for the number of users before one is created. In that case, multiple users will be created.

[javorszky]

Right, sooo... working on this thing.

Fix will include:

• a progress bar of sorts
• unattaching the handler from the submit button
• making sure in the backend Ghost behaves correctly

[ErisDS]

We'll fix the UI issues with #1841.

For the backend I was thinking the best way to do this may be to use transactions?

[baus]

I opened a second PR on this issue. My first one still had a race condition. This is more difficult to solve than it seems because it requires atomic operations at the database level -- specifically conditional inserts.

[baus]

I just read @ErisDS comment on my previous PR and realized my new PR still doesn't meet the guidelines. I'll give this one more try. Third time's a charm.

[ErisDS]

You shouldn't need to open a new PR - with a little git foo the old one can be cleaned up 👍

[baus]

I've been doing some work on this issue. I know this is an unlikely scenario, and will be resolved with multiple user support, but I do think there is value in developing procedures to deal with race conditions issues in the future.

I wrote a test which can reproduce the issue in my branch.

I haven't submitted a PR on this because I don't have a solution ready, but I have an idea on how I want to address it. Here are some of my notes

I looked into using Knex transactions, but they depend on limiting connections the database. As soon as there is more than one process accessing the database (which will be more common when load balancing is available in Node.js 0.12), then Knex transactions won't address the issue.

I would prefer to add a constraint into the database using CHECK. But this has a couple issues.

1. schema.js doesn't support CHECK

2. MySQL doesn't support CHECK

As the application gets more complex, I think it is going to be difficult to support multiple databases. Maybe there is another solution, but since Ghost requires a SQL database, I personally think the most straight forward solution is to push the problem down to the database.

[sebgie]

I have spent some time today to look into this issue. You are absolutely right, that this issue will be irrelevant when multiple users are supported.

Since supporting multiple databases is important for Ghost and special SQL queries are hard to maintain, I think that the solution to this problem is adding a semaphore/mutex/lock and ensure it is treated as a critical section. Do you know any other functions that contain a similar problem?

[baus]

Unfortunately, since it is event driven and single threaded, there really isn't a concept of a semaphore, mutex, or lock in Node. The best you can do is perform I/O synchronously in one thread, which will pause all connections until that I/O completes. I think the solution is to make writes to the user table atomic at the database level, but that is hard to do for multiple databases.

I believe race conditions occur at other places in the code, but I haven't look at them as carefully as this issue. I'm pretty sure a similar problem exists when creating numbered slugs for posts.

[baus]

I hope this latest PR does the trick. I would had preferred to use the database to enforce the constraint, but that is difficult to do given the limitations on the database selection and schema creation. This strategy of using closures to maintain state does not work in load-balanced or multi-process environments, because it depends on the state within one process.

[ErisDS]

@sebgie I think the new approach to the initial owner should resolve this?

[sebgie]

Yes, this race condition is not relevant due to support for multi user anymore.