no redirect after sign_in

[TheScarfix]

Environment

• Ruby 2.4.3
• Rails 5.2.0beta2
• Devise 4.4.0

Current behavior

no redirect after sign_in

Expected behavior

redirect to after_sign_in_path (screenshot taken with the same environment except devise 4.3.0 instead of 4.4.0)

[nynhex]

@TheScarfix Can you provide a sample app which reproduces this behavior? I've bootstrapped a simple app with the same versions and haven't been able to reproduce yet.

[TheScarfix]

@nynhex created a new application and it works as intended, any ideas how to find the error in my app?

[woodpigeon]

Having the same issue as @TheScarfix - upgraded to 4.4.0 to use Ruby 2.5.0 and app no longer redirects after a login, stays on sign_in path even though the user is now signed in. Refreshing the page at this point redirects you correctly to the root_path.

It seems that in Devise::SessionsController#create, respond_with resource, location: after_sign_in_path_for(resource) does not honour location - I can put any url in location: and it never redirects there.

If I just apply the Ruby 2.5.0 fix #4668 to Devise 4.3.0 it works OK.

I'm using Devise inside an engine.
I regenerated the devise initialiser and the only differences are

  config.parent_controller = "ApplicationController"
  config.router_name = :<engine name>

Env: Rails 5.1.4 Ruby 2.5.0 Devise 4.4.0

[tegon]

I'm going to take a look at this.

[fmluizao]

I'm having the same problem, Ruby 2.5.0, Rails 5.1.4, Devise 4.4.0.

Using 1009096 works as intended. Tried to review commits after this one to find the problem, but with no luck.

[tegon]

I've done some debugging, and I guess that this commit is causing this.
Before we did not validate the model in the Trackable module, now we do - and if the model is invalid respond_with will not redirect.
Can someone confirm that your model is invalid during the sign in process? It might be requiring a password if the method password_required? is being overridden.

[woodpigeon]

@tegon Well spotted and thank you, yes that seems to be the issue in our case. I should have thought to check that. In Devise::SessionsController#create resource.valid? is false - we have some on: :update conditional validation on User that applies only after the initial User record is created, requiring the presence of things like professional_position that have to be have completed on the the user's profile once they log in.
I'm not sure how common our approach is (or how good!)
We can easily re-work the conditional validation (or add a separate Profile model) if this isn't a bug in Devise.

[tegon]

@woodpigeon Thank you for confirming this. We could do some check inside the Trackable module to only validate it when creating the user, but personally, I think that the user should be valid during the sign in process. We can't leave the code as it was before because it could create invalid users on sign up (see #4673).
First I want to see which are the other use cases. @TheScarfix @fernandoluizao @ryuzee can you confirm if your models are invalid during the sign in process?

[ryuzee]

@tegon I've confirmed that password is empty and resource.errors indicates the model can not be saved.

[TheScarfix]

@tegon can confirm that resource is invalid and resource.errors says the password can't be blank

[tegon]

@ryuzee @TheScarfix did you override the password_required? method?

[TheScarfix]

@tegon no, but I had validation for :password in the User model, deleted it and now it does redirect

[fmluizao]

@tegon I've confirmed that the model is invalid. My bad, not devise 🤦‍♂️

[ryuzee]

@tegon I override the password_required method like this. And changing provider.blank? to super && provider.blank? works fine. Sorry for the bother.

[tegon]

I'm closing this since we'll keep the running the validations in the Trackable module. Since this caused some confusion, I've added a wiki page explaining how to upgrade to v4.4.0:

https://github.com/plataformatec/devise/wiki/How-To:-Upgrade-to-Devise-4.4.0

Feel free to contribute with your solutions. This may be helpful to others in the future.

Thanks!

[joshpencheon]

@tegon

This feels a bit like changing the contract; as far as I'm aware, Devise never previously required the resource to be valid during authentication.

[...] but personally, I think that the user should be valid during the sign in process

If there was previously an implicit assumption that the resource would be valid, that has now been made explicit by the change to Trackable. Would it not be sensible to add this as a proper constraint, rather than leaving invalid users in the broken state of being a) authenticated but b) still being redirected to the sign in page?

In our application, we've now got:

  def active_for_authentication?
    invalid? ? false : super
  end

  def inactive_message
    invalid? ? :record_invalid : super
  end

...I wonder if something like this shouldn't be added to the default behaviour in lib/devise/models/authenticatable.rb ?

[gmcnaughton]

I agree with @joshpencheon that this feels like a substantial change to the contract of sign-in.

In an old product whose validation rules have changed over time, it's reasonable to have legacy users who were valid at signup but are now invalid. Before 4.4.0 these users could still sign in to fix their accounts!

If invalid users can't sign in, any new validation which accidentally impacts a portion of the existing userbase becomes an immediate showstopper, rather than only applying to new signups.

[tegon]

@joshpencheon @gmcnaughton This wasn't an implicit assumption, it worked before without any validations until we found a problem with this approach.
We could handle this inside Trackable with some conditional, that's why I asked what were the errors to have a better understanding of the situation. And it turns out that most of the errors were password-related. I think that even with custom validations this can be handled at the application code, but let me know if that's not the case with your app. We can reconsider it.

[gmcnaughton]

Thanks @tegon, I appreciate you taking the time! 😄

I'm concerned about two issues: legacy accounts and future changes.

1. Legacy accounts. Over time we've added or changed validations on User -- say, requiring a field or changing limits -- in order to get clean data from new users. But we haven't gone back to fix up legacy users. These legacy users could still log in; when they tried to update their account they would be prompted to fix the values at that time. Will we need to manually fix all users to be valid before upgrading to 4.4.x, or risk blocking them from the site?

2. Future changes. Going forward, any time we add or change a validation on User, we could accidentally block a portion of our users from signing in without even realizing it. That's a scary thought!

[tegon]

@gmcnaughton They seem to be the same issue, actually. Do you have any examples of this?
At least in the projects I've worked, we usually had to fix up the base before introducing some new column that is required. If it's a column that we can leave it nil for some users, then a validation isn't needed.
Also, if your users are invalid I think you can have issues with other parts of Devise, such as password recovery and email confirmation.

[starrychloe]

This is still broken!

https://stackoverflow.com/questions/48913707/devise-after-sign-in-path-for-not-working-being-ignored

Devise should not validate upon sign in! If you had some problem before, then you should validate when REGISTERING/CREATING a user, but not when SIGNING IN / UPDATING a user. That renders validate ... on: :update completely useless! As it is, your TRACKABLE is broken now as it still creates a session but doesn't update last_sign_in_at, current_sign_in_ip, etc.

Consider using #update_attribute or #update_columns instead to set the last_sign_in_at fields et al. The key to those methods being "Validations are skipped".

I'm going to have to downgrade to 4.3.

[AlexB52]

Another quick fix could be to override sessions_controller#create and use #sign_in_and_redirect instead of the current code especially if you use this method for omniauth_controller. Otherwise you get 2 different behaviour on sign_in.

While that issue enforces to have a valid record on sign_in, it is a bit weird behaviour because as @starrychloe mentioned it does create a session and does not update the record. Also when you refresh you are redirected to the correct path instead of being stuck sign_in like when the session is created.

Side note: What made my record invalid is that for Rails 5 belongs_to is now a required association. Without

belongs_to :association, optional: true

Your record will be invalid and #after_sign_in_path_for will not redirect to the correct url. This just one of the millions possibilities that could make a resource invalid.

[tegon]

We already removed the validations on #4796, I'll release a version soon.