Closed nevans closed 2 months ago
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?
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:
authenticatable
as a side-effect of saving the session.Ohh, I get it. That's not good.
As a simple fix, I propose adding autosave: false to the association. Although, that may result in breaking the "paranoid" aspect of config.paranoid = true.
Breaking it how?
Breaking it how?
So I haven't adequately tested it yet, but...
I believe that belongs_to
creates a validation that will fail when the authenticatable object isn't persisted. So then the if @session.save
statement will evaluate the else
clause.
The point of config.paranoid = true
is that whoever is inputting an email address into the form shouldn't be given any information about whether or not that email exists in our system (unless they can read email sent to that address). And evaluating the else
clause breaks that.
If that is how it works, that means that our workaround fixes the autosave problem but breaks the "paranoid" feature.
So I haven't adequately tested it yet, but...
So, we just tested it and fortunately, it looks like this is not an issue! The Passwordless::Session
object is saved successfully, the authenticatable_type
field has been set, but the authenticatable_id
field is nil
. I think that the default rails belongs_to
presence validation works very simply and literally: it checks that session.authenticatable.present?
is truthy, but it does not check that it has been persisted, nor does it check that session.authenticatable_id.present?
is truthy.
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 :if
condition to the validation, it does not change the actual validation itself. It's still just model.requires_presence_of reflection.name
, i.e: requires_presence_of :authenticatable
, which is simply going to check that session.authenticatable.present?
. And Session.new(authenticatable: authenticatable_class.new(email:)).authenticatable.present?
will always be truthy.
In summary, I think it should be safe to simply set autosave: false
. :)
And I see now that passwordless
also adds an explicit presence validation for :authenticatable
, but this will behave identically to the implicit belongs_to
validation so it doesn't change the analysis.
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: https://github.com/mikker/passwordless/blob/971093ee814df13ec0b485840cb8fbd55d2179db/app/models/passwordless/session.rb#L9-L13From https://api.rubyonrails.org/v7.1.3/classes/ActiveRecord/AutosaveAssociation.html
So, when
handle_resource_not_found
does this: https://github.com/mikker/passwordless/blob/971093ee814df13ec0b485840cb8fbd55d2179db/app/controllers/passwordless/sessions_controller.rb#L203-L206 And then does this: https://github.com/mikker/passwordless/blob/971093ee814df13ec0b485840cb8fbd55d2179db/app/controllers/passwordless/sessions_controller.rb#L23-L26...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.