Check if the record is persisted in `update_tracked_fields!` #4796

tegon · 2018-03-05T21:54:24Z

In some cases, invalid records could be created during the signup process because we were calling save(validate: false) inside the update_tracked_fields! method. See #4673 for more information.
This was fixed on #4674 by calling save directly, but it caused some trouble and confusion since it changed Devise's behavior significantly.
We talked about on #4790 and it doesn't even make sense to call save on an object that isn't persisted yet, so I've added a guard clause to the update_tracked_fields! method.

In some cases, invalid records could be created during the signup process because we were calling `save(validate: false)` inside the `update_tracked_fields!` method. See #4673 for more information. This was fixed on #4674 by calling `save` directly, but it caused some trouble and confusion since it changed Devise's behavior significantly. We talked about on #4790 and it doesn't even make sense to call `save` on an object that isn't persisted yet, so I've added a guard clause to the `update_tracked_fields!` method. Fixes #4790

rafaelfranca · 2018-03-05T22:08:10Z

lib/devise/models/trackable.rb

+        # `save` here because invalid users can be saved if we don't.
+        # See https://github.com/plataformatec/devise/issues/4673 for more details.
+        return if new_record?
+
        update_tracked_fields(request)


Should not the update_tracked_fields still be called when it is a new record?

When the signup succeeds the record would already be persisted here. This is being called from a Warden's after_set_user callback. The callback is triggered when we call set_user from the sign_in helper, which is called after the record is created in registrations controller.

Makes sense. Is there any way we can test this to make sure it doesn't happen anymore?

The test introduced in https://github.com/plataformatec/devise/pull/4674/files#diff-8aa58f219abef0280c8857f9161c087bR42 is still passing, so I think we're good to go.

But how about the tests to make sure that every request don't run validations?

I see, I'm not sure of how we should test this 🤔
Do you think we should check here if the validations were run or in the sessions controller?

One way to test this is write an integration test, add a validation to the model that sets a global value in the class if the validation is run. This way we can test if a request to any action sets this global value.

I think it's worth to write a unit test to the Trackable module to check if the record is saved not running the validations 🤔 .

rafaelfranca · 2018-03-12T19:06:33Z

test/integration/authenticatable_test.rb

+  test 'sign in should not run model validations' do
+    sign_in_as_user
+
+    refute $validations_performed


When I said a global I was thinking more in a class variable inside the model

I get it now 😅
I've changed to use a class variable

feliperenan

💪

charlesoft · 2018-03-13T13:15:33Z

test/integration/trackable_test.rb

@@ -3,6 +3,12 @@
 require 'test_helper'

 class TrackableHooksTest < Devise::IntegrationTest
+  test "sign in with HTTP should not run model validations" do
+    create_user


Is create_user already called internally when we use sign_in_as_user method? 🤔

charlesoft

For some reason, this is necessary now. It seems to be ultimately caused by this change heartcombo/devise#4796 which stopped validating the user on save after updating the sign-in tracking fields. With that change in place, after calling `lookup_user.update!` earlier in this method the updated values - including the `provider` and `uid` - stick around on the user object that's in memory. Manually reloading the user object after a failed update resolves this.

carlosantoniodasilva · 2021-04-02T18:40:03Z

Hey folks, taking a look at this, I'm not sure I see how/why a record would get to this call via a warden hook while not being persisted. This must be something custom someone is doing along the way, since as you pointed out @tegon, the registrations controller calls sign up which will in turn delegate to warden's set_user eventually, but it calls save before any of that:

devise/app/controllers/devise/registrations_controller.rb

Lines 19 to 24 in e8e0c27

    
           resource.save 
        
           yield resource if block_given? 
        
           if resource.persisted? 
        
             if resource.active_for_authentication? 
        
               set_flash_message! :notice, :signed_up 
        
               sign_up(resource_name, resource)

.

So, unless we're talking about some custom code in the registration that's not calling save and/or checking for the errors before calling set_user (or sign_up), I'm not sure I can see a path for us to save a record not being persisted here, thus I'm not sure we need the new_record? check in trackable.

If I remove that check, only the model tests fail, the integration ones don't, which is more proof that the workflow as a whole is working I think.

tegon · 2021-04-05T14:11:05Z

@carlosantoniodasilva It has been a long time, but I think you're right - it can't happen with Devise's default setup. If I recall correctly, we ended up adding this patch because it was affecting multiple applications out there. I believe this could happen if the apps had other validations on their models apart from the Devise ones. Since the trackable module runs apart from registrations, that's the way we prevented it from happening in there.

carlosantoniodasilva · 2021-04-05T14:42:15Z

Makes sense. I still think the contract should be that you're not running the hooks for an unsaved model, I'll see about that in a future major version or something. Thanks!

tegon added the Needs review label Mar 5, 2018

tegon self-assigned this Mar 5, 2018

tegon requested review from rafaelfranca and feliperenan March 5, 2018 21:54

rafaelfranca reviewed Mar 5, 2018

View reviewed changes

rafaelfranca reviewed Mar 12, 2018

View reviewed changes

tegon force-pushed the let-signup-validations branch 3 times, most recently from 3dd45e9 to fb61222 Compare March 12, 2018 20:47

feliperenan approved these changes Mar 12, 2018

View reviewed changes

charlesoft reviewed Mar 13, 2018

View reviewed changes

Ensure Devise isn't performing model validations

c24e754

tegon force-pushed the let-signup-validations branch from fb61222 to c24e754 Compare March 13, 2018 13:23

charlesoft approved these changes Mar 13, 2018

View reviewed changes

tegon mentioned this pull request Mar 13, 2018

4.3.0 to 4.4.0 breaks secondary user persisting in sessions #4789

Closed

tegon merged commit 5664b19 into master Mar 14, 2018

tegon deleted the let-signup-validations branch March 14, 2018 18:21

tegon mentioned this pull request Mar 15, 2018

no redirect after sign_in #4742

Closed

carlosantoniodasilva mentioned this pull request Apr 2, 2021

Devise ignores (some) validations on signup #4673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check if the record is persisted in `update_tracked_fields!` #4796

Check if the record is persisted in `update_tracked_fields!` #4796

tegon commented Mar 5, 2018

rafaelfranca Mar 5, 2018

tegon Mar 5, 2018

rafaelfranca Mar 5, 2018

tegon Mar 6, 2018

rafaelfranca Mar 6, 2018

tegon Mar 8, 2018

rafaelfranca Mar 8, 2018

charlesoft Mar 11, 2018

rafaelfranca Mar 12, 2018

tegon Mar 12, 2018

feliperenan left a comment

charlesoft Mar 13, 2018 •

edited

Loading

charlesoft left a comment

carlosantoniodasilva commented Apr 2, 2021

tegon commented Apr 5, 2021 •

edited

Loading

carlosantoniodasilva commented Apr 5, 2021

Check if the record is persisted in update_tracked_fields! #4796

Check if the record is persisted in update_tracked_fields! #4796

Conversation

tegon commented Mar 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feliperenan left a comment

Choose a reason for hiding this comment

charlesoft Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

charlesoft left a comment

Choose a reason for hiding this comment

carlosantoniodasilva commented Apr 2, 2021

tegon commented Apr 5, 2021 • edited Loading

carlosantoniodasilva commented Apr 5, 2021

Check if the record is persisted in `update_tracked_fields!` #4796

Check if the record is persisted in `update_tracked_fields!` #4796

charlesoft Mar 13, 2018 •

edited

Loading

tegon commented Apr 5, 2021 •

edited

Loading