Skip to content
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

Allow wrapping of lockable Warden after sign-in hook #5310

Closed

Conversation

geoffharcourt
Copy link

@geoffharcourt geoffharcourt commented Nov 6, 2020

For applications using a primary/replica multiple database setup, Rails
6's default connection switching can lead to the Lockable after
sign-in hook firing on GET requests, getting the read-only database,
and then attempting to write while zeroing out the user's failed attempts.

This change adds a customizable wrapper proc that can allow applications
to customize this behavior. For a primary/replica setup using
ActiveRecord's default connection switching, it might look like this:

Devise.setup do |config|
  # default is Proc.new { |hook| hook.call }
  config.warden_hook_save_wrapper = Proc.new do |hook|
    # ensure the writable connection is used to avoid read-only write errors
    ApplicationRecord.connected_to(role: :writing) do
      hook.call
    end
  end
end

This ensures that writes don't happen in a read-only connection if that's the
current connection context.

Fix #5264

@geoffharcourt
Copy link
Author

Note: I'm not sure how to set up proper Warden hook tests here.

@geoffharcourt geoffharcourt marked this pull request as draft November 6, 2020 18:06
@geoffharcourt geoffharcourt changed the title Allow wrapping of lockable Warden after sign-in hook in proc Allow wrapping of lockable Warden after sign-in hook Nov 6, 2020
@geoffharcourt geoffharcourt marked this pull request as ready for review November 6, 2020 18:30
For applications using a primary/replica multiple database setup, Rails
6's default connection switching can lead to the `Lockable` after
sign-in hook firing on `GET` requests, getting the read-only database,
and then attempting to write while zeroing out the user's failed attempts.

This change adds a customizable wrapper proc that can allow applications
to customize this behavior. For a primary/replica setup using
ActiveRecord's default connection switching, it might look like this:

```ruby
Devise.setup do |config|
  config.warden_hook_save_wrapper = Proc.new do |hook|
    ApplicationRecord.connected_to(role: :writing) do
      hook.call
    end
  end
end
```

Fix heartcombo#5264
@mhenrixon
Copy link

Any news on this? Has been a minute now :)

@carlosantoniodasilva
Copy link
Member

Geoff and I have chatted (yesterday actually) about it, and here's the outcome we're going to try: instead of a new configuration to wrap this particular warden hook, we'll expose a model method that will be called from the hook here, similar to how we do for trackable, which will give you control over that in your model code to wrap it and call super.

This way you can at least override all the pieces inside the app to switch to a writing connection in Devise methods. As a second step we can look into making Devise more aware of multi-DB support and have it do the work of switching connections, but that's a bigger change we need to look at more carefully and from a broader perspective.

carlosantoniodasilva added a commit that referenced this pull request Mar 22, 2021
Resetting failed attempts after sign in happened inside a warden hook
specific for the lockable module, but that was hidden inside the hook
implementation and didn't allow any user customization.

One such customization needed for example is to direct these updates to
a write DB when using a multi-DB setup. With the logic hidden in the
warden hook this wasn't possible, now that it's exposed in a model
method much like trackable, we can override the model method to wrap it
in a connection switch block for example, point to a write DB, and
simply call `super`.

Closes #5310
Related to #5264 and #5133
carlosantoniodasilva added a commit that referenced this pull request Mar 22, 2021
Resetting failed attempts after sign in happened inside a warden hook
specific for the lockable module, but that was hidden inside the hook
implementation and didn't allow any user customization.

One such customization needed for example is to direct these updates to
a write DB when using a multi-DB setup. With the logic hidden in the
warden hook this wasn't possible, now that it's exposed in a model
method much like trackable, we can override the model method to wrap it
in a connection switch block for example, point to a write DB, and
simply call `super`.

Closes #5310
Related to #5264 and #5133
@carlosantoniodasilva
Copy link
Member

@geoffharcourt @mhenrixon I'm giving this one a shot in #5369, let me know about any feedback/concerns there please. Thanks!

carlosantoniodasilva added a commit that referenced this pull request Mar 22, 2021
Resetting failed attempts after sign in happened inside a warden hook
specific for the lockable module, but that was hidden inside the hook
implementation and didn't allow any user customization.

One such customization needed for example is to direct these updates to
a write DB when using a multi-DB setup. With the logic hidden in the
warden hook this wasn't possible, now that it's exposed in a model
method much like trackable, we can override the model method to wrap it
in a connection switch block for example, point to a write DB, and
simply call `super`.

Closes #5310
Related to #5264 and #5133
carlosantoniodasilva added a commit that referenced this pull request Mar 22, 2021
Resetting failed attempts after sign in happened inside a warden hook
specific for the lockable module, but that was hidden inside the hook
implementation and didn't allow any user customization.

One such customization needed for example is to direct these updates to
a write DB when using a multi-DB setup. With the logic hidden in the
warden hook this wasn't possible, now that it's exposed in a model
method much like trackable, we can override the model method to wrap it
in a connection switch block for example, point to a write DB, and
simply call `super`.

Closes #5310
Related to #5264 and #5133
@geoffharcourt
Copy link
Author

Closing in favor of #5369! Thank you @carlosantoniodasilva!

carlosantoniodasilva added a commit that referenced this pull request Apr 2, 2021
Resetting failed attempts after sign in happened inside a warden hook
specific for the lockable module, but that was hidden inside the hook
implementation and didn't allow any user customization.

One such customization needed for example is to direct these updates to
a write DB when using a multi-DB setup. With the logic hidden in the
warden hook this wasn't possible, now that it's exposed in a model
method much like trackable, we can override the model method to wrap it
in a connection switch block for example, point to a write DB, and
simply call `super`.

Closes #5310
Related to #5264 and #5133
carlosantoniodasilva added a commit that referenced this pull request Apr 2, 2021
Resetting failed attempts after sign in happened inside a warden hook
specific for the lockable module, but that was hidden inside the hook
implementation and didn't allow any user customization.

One such customization needed for example is to direct these updates to
a write DB when using a multi-DB setup. With the logic hidden in the
warden hook this wasn't possible, now that it's exposed in a model
method much like trackable, we can override the model method to wrap it
in a connection switch block for example, point to a write DB, and
simply call `super`.

Closes #5310
Related to #5264 and #5133
carlosantoniodasilva added a commit that referenced this pull request Apr 2, 2021
Resetting failed attempts after sign in happened inside a warden hook
specific for the lockable module, but that was hidden inside the hook
implementation and didn't allow any user customization.

One such customization needed for example is to direct these updates to
a write DB when using a multi-DB setup. With the logic hidden in the
warden hook this wasn't possible, now that it's exposed in a model
method much like trackable, we can override the model method to wrap it
in a connection switch block for example, point to a write DB, and
simply call `super`.

Closes #5310
Related to #5264 and #5133
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: a way to handle multi-DB ActiveRecord setups
3 participants