Paranoid mode still autosaves its associated authenticatable model

[nevans]

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

	belongs_to(
	:authenticatable,
	polymorphic: true,
	inverse_of: :passwordless_sessions
	)

From https://api.rubyonrails.org/v7.1.3/classes/ActiveRecord/AutosaveAssociation.html

Note that autosave: false is not same as not declaring :autosave. When the :autosave option is not present then new association records are saved but the updated association records are not saved.

So, when handle_resource_not_found does this:

passwordless/app/controllers/passwordless/sessions_controller.rb

Lines 203 to 206 in 971093e

	def handle_resource_not_found
	if Passwordless.config.paranoid
	@resource = authenticatable_class.new(email: normalized_email_param)
	@skip_after_session_save_callback = true

And then does this:

passwordless/app/controllers/passwordless/sessions_controller.rb

Lines 23 to 26 in 971093e

	handle_resource_not_found unless @resource = find_authenticatable
	@session = build_passwordless_session(@resource)
	if @session.save

...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:

reflection = Passwordless::Session.reflect_on_association :authenticatable
reflection.options[:autosave] = false

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. 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.

[mikker]

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?

[avdi]

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:

1. Behaves as if it exists (correctly) by generating a session and associated ephemeral account...
2. ...then accidentally creates the account by saving the should-be-ephemeral authenticatable as a side-effect of saving the session.

[mikker]

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?

[nevans]

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.

[nevans]

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.

[nevans]

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. :)

[nevans]

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.

[mikker]

Some real nice detective work there. Thank you so much! I'll get a new release out soon