-
-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Paranoid mode still autosaves its associated authenticatable model #217
Comments
Thank you for a thorough explanation – however, I am not totally sure what's going on. Paranoid is for not telling the user whether a user record with their supplied email address exists. Isn't what you're describing here something else? |
The upshot is that for an account that doesn't exist, Passwordless:
|
Ohh, I get it. That's not good.
Breaking it how? |
So I haven't adequately tested it yet, but... I believe that The point of If that is how it works, that means that our workaround fixes the autosave problem but breaks the "paranoid" feature. |
So, we just tested it and fortunately, it looks like this is not an issue! The |
And after digging just a little bit deeper, I see that the code surrounding this has changed between Rails 7.0 and 7.1: See the docs for config.active_record.belongs_to_required_validates_foreign_key. The app I've been testing is running rails 7.1, but is still configured to use the rails 7.0 defaults. But, if I'm understanding this correctly, this config setting doesn't actually make any difference to this issue. The relevant source code for it here: https://github.com/rails/rails/blob/9fd8b33ebba3c281c6cc5bbf8f48cde38c6fb0da/activerecord/lib/active_record/associations/builder/belongs_to.rb#L126-L141. And, athough the rails 7.1 default does add an In summary, I think it should be safe to simply set |
And I see now that |
This prevents the authenticable model from being auto-created when `config.paranoid = true`. Fixes mikker#217.
This prevents the authenticable model from being auto-created when `config.paranoid = true`. Fixes #217.
* Disable autosave of session.authenticatable This prevents the authenticable model from being auto-created when `config.paranoid = true`. Fixes #217. * Add assertion --------- Co-authored-by: nick evans <[email protected]>
Some real nice detective work there. Thank you so much! I'll get a new release out soon |
When
config.paranoid = true
, the authenticatable model is automatically created and saved to the database. This is unexpected and unwanted.The
authenticatable
relationship is defined here:passwordless/app/models/passwordless/session.rb
Lines 9 to 13 in 971093e
From https://api.rubyonrails.org/v7.1.3/classes/ActiveRecord/AutosaveAssociation.html
So, when
handle_resource_not_found
does this:passwordless/app/controllers/passwordless/sessions_controller.rb
Lines 203 to 206 in 971093e
And then does this:
passwordless/app/controllers/passwordless/sessions_controller.rb
Lines 23 to 26 in 971093e
...assuming there are no failing validations on
@resource
, then both the Passwordless::Session and the authenticatable association are saved.We worked around this by adding the following to our
config/initializers/passwordless.rb
:As a simple fix, I propose adding
autosave: false
to the association. Although, that may result in breaking the "paranoid" aspect ofconfig.paranoid = true
. I haven't looked into the best way to handle this in the controller (yet), and automatically creating the authenticatable record was the more immediately pressing issue for us.The text was updated successfully, but these errors were encountered: