User doc _rev field not returned on user creation when loginOnRegistration is set to false

[jbgtmartin]

Hi, first all thanks for this updated library!

There is a bug I thing in the createUser() method. At the end, when the method resolves with the created user, the _rev field will only be included if it's returned by the second resolve() (i.e. if we create a user with loginOnRegistration set to false)

It can lead to CouchDB update conflicts, for instance if we try to update the user doc just after creation (as we will try to insert it without the _rev field).

// src/user.ts

// Let's say we have this.config.security.loginOnRegistration = false, and hasError = false.

return new Promise(async (resolve, reject) => {
  newUser = await this.prepareNewUser(newUser);
  if (hasError || !this.config.security.loginOnRegistration) {
    // 1. The promise resolves. It returns newUser instead of finalUser.
    resolve(hasError ? undefined : (newUser as SlUserDoc));
  }
  if (!hasError) {
    // 2. This is executed (even if the promise has already resolved).
    // The doc is inserted here, but resolve() has already been called so finalUser is not returned.
    const finalUser = await this.insertNewUserDocument(newUser, req);
    this.emitter.emit('signup', finalUser, 'local');
    if (this.config.security.loginOnRegistration) {
      resolve(finalUser);
    }
  }
});

Maybe we could change it like this?

• I think that we can also remove the this.config.security.loginOnRegistration check, as we should return the created user no matter if it is set to true or false
• We could also get rid of the Promise constructor

if(hasError)
  return;

else {
  newUser = await this.prepareNewUser(newUser);
  const finalUser = await this.insertNewUserDocument(newUser, req);
  this.emitter.emit('signup', finalUser, 'local');
  return finalUser;
}

PS: I'm using the latest version (0.16.0)

[fynnlyte]

Hmm if I recalled correctly, I made the adjustments in order to prevent name guessing so simply removing it is most likely not the best choice.

But yeah, if this.config.security.loginOnRegistration is false, we can simply resolve with the final user or exit with the error.

[jbgtmartin]

What I meant is that no matter the value of this.config.security.loginOnRegistration, the user doc is returned:

• if it is true, then resolve(finalUser) is called
• if it is false, then resolve(hasError ? undefined : (newUser as SlUserDoc)) is called

So in both cases the user doc is returned, the only difference is that in the second case it's returned before calling insertNewUserDocument(). Is it what prevents name guessing?

[fynnlyte]

Yeah resolving before inserting the doc prevents time name guessing in case loginOnRegistration is switched off. But when it's switched on, name guessing doesn't matter anyways. So the fix would be to simply not resolve twice, i.e. first resolve only if !loginOnRegistration || hasError && loginOnRegistration

[jbgtmartin]

I'm not sure to know exactly what time name guessing is? Is it an attack where you try to register a user, and if the registration takes some time you deduce that the username you tried does not already exist ? So we want the method to return immediately in any case, and we can't wait for the doc to be inserted to get the _rev ?

[fynnlyte]

I meant time based name guessing, as in https://en.wikipedia.org/wiki/Timing_attack or https://cheatsheetseries.owasp.org/cheatsheets/Authentication_Cheat_Sheet.html#authentication-responses

So basically - yes, the function should return early regardless of whether the user is actually created or it fails because the email is already present (which is what an attacker would want to find out)

[jbgtmartin]

Ok now I understand better, thanks for the explanations! So I guess there's nothing to change in the library, and if I need to run some logic after user creation I can listen to the 'signup' event

[fynnlyte]

I'll look into this on the weekend. The 'signup' - event should only fire after the user has been inserted into the database, if the signup was successful.

[jbgtmartin]

Yes, after this event we can get the doc from the database as we know it has been inserted, so I think it's fine.

I'll let you close this issue this weekend, and thanks again for your work with this library!

[fynnlyte]

Yup, thanks for bringing this up, I've explained it in the docstring of createUser. What the code does is expected behaviour, but it is indeed a bit confusing that createUser doesn't return the correct revision.

[jbgtmartin]

Thank you!