expire_session_data_after_sign_in! is not completely correct

[scaryzet]

The code is:

def expire_session_data_after_sign_in!
  session.keys.grep(/^devise\./).each { |k| session.delete(k) }
end

The problem is that session.keys can be empty if the session is not yet loaded. You can see that methods keys and values in ActionDispatch::Request::Session are not triggering session loading (frankly speaking I can't even think up a reason why it's done so, also Rack::Session::Abstract::ID does it the same way, please explain it to me if anyone knows why).

The effect is that expire_session_data_after_sign_in! does nothing in some curcumstances. I hit it when created a two-line action just invoking this helper and doing redirect.

def cancel
  expire_session_data_after_sign_in!
  redirect_to new_registration_path
end

I fixed my code by adding a call to session.empty? at the beginning to force session initialization. But I believe this is not how it should be.

[josevalim]

You are right, this is a bug in Rack. :)

I fixed my code by adding a call to session.empty? at the beginning to force session initialization. But I believe this is not how it should be.

Unfortunately, that's very likely to be the proper fix, since there is no API to force load. At some points we also do session.inspect exactly to force it to be loaded.

Can you please send a pull request with comments on the code about the need for such call?

[scaryzet]

Can you please send a pull request with comments on the code about the need for such call?

Well, I'm forced to make my own RegistrationsController not inherited from Devise::RegistrationsController. I've taken the cancel action from there. I need such an action to be able to cancel a registration via omniauth. Maybe I just need to manually remove session keys in question, that would be more explicit and wouldn't touch the guts of devise. Thanks.

[josevalim]

I mean, we should force the empty? call inside expire_session_data_after_sign_in! itself. :)

[scaryzet]

It doesn't deserve a pull request but here it is.