Sign in user after confimation success

[GAKINDUSTRIES]

• Issue: #1278
• Topic: Bug: Registration broken if multiple users signed in

Digging into the code while trying to tackle this issue, I found a few more:

1. Wrong use of signed_in? helper: This helper checks if a user object is signed in. We should use user_signed_in? instead.
2. We don't even need to check for signed_in user here: Since this request comes after registration_controller method and the only params that are coming from the generated url are confirmation_token and redirect_url, there's no way that a user is logged at this point (considering that we are not using cookies to keep the session)
3. confirmation_token generated but not saved: In this line we are generating the token for the user but it's never saved in the database.
   I attach the code of create_token method:

def create_token(client_id: nil, token: nil, expiry: nil, **token_extras)
    client_id ||= SecureRandom.urlsafe_base64(nil, false)
    token     ||= SecureRandom.urlsafe_base64(nil, false)
    expiry    ||= (Time.zone.now + token_lifespan).to_i

    tokens[client_id] = {
      token: BCrypt::Password.create(token),
      expiry: expiry
    }.merge!(token_extras)

    clean_old_tokens

    [client_id, token, expiry]
  end

Thus, we create the user with a token that can't be used later. We need to save the generated token (@resource.save!) to reflect the new token at the database level.

In this PR I removed the logged in conditional and I log in the user (using store: false to avoid saving in session) and save! the record to update it in the database. I also reorder the methods inside the controller (to keep CRUD order) and fix a small indentation problem.

Note: I removed the test within unauthenticated context since it does not make sense anymore.

[iv-mexx]

In this PR I removed the logged in conditional and I log in the user (using store: false to avoid saving in session) and save! the record to update it in the database.

Signing in the user automatically after confirming was removed from Devise for a good reason!
http://blog.plataformatec.com.br/2013/08/devise-3-1-now-with-more-secure-defaults/

We don't even need to check for signed_in user here

Not sure if thats the reason here, but in Devise, you can Configure a "Trial" Periode in which the user can be logged in (for a certain amount of time) before he has to confirm the User.

In this case, a user would be already logged in at this point in time, so the check does make sense

[GAKINDUSTRIES]

@iv-mexx thanks for your comment. I get the reasons why we shouldn't do it. I was trying to sign in the user there for callbacks to be executed but the thing is that we actually already have a signed_in user. Let me explain it:

While digging into the code, I found that after the confirm_token statement, the current_user is set up. Here's a gif that shows it:

This actually happens because user_signed_in? invokes current_user, which calls [set_user_by_token(https://github.com/lynndylanhurley/devise_token_auth/blob/master/app/controllers/devise_token_auth/concerns/set_user_by_token.rb#L27) method. This method returns @resource (even if enable_standard_devise_support = true). Thus, we already have a logged in user.

What I think that we can do here is use local variable instead of instance variable (resource will prevent for having a signed_in user after confirm_token), remove sign_in statement and generate the url for non-logged users. I still think that we should remove the logged_in option since we are not managing cookies here, so Trial period may not apply here.

What do you think?

[MaicolBen]

Remove this as http://blog.plataformatec.com.br/2013/08/devise-3-1-now-with-more-secure-defaults/, we weren't doing anything with the tokens previously, so it doesn't make sense to create new tokens here.

[MaicolBen]

@GAKINDUSTRIES Good work! but I don't understand this "I still think that we should remove the logged_in option"

[exseniorastronaut]

@MaicolBen why hasn't this merged?