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

Add Devise.available_router_name to support routing correctly with engines #183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jules2689
Copy link

@jules2689 jules2689 commented Jan 5, 2020

Problem

  • When an app has an engine, the rails routes for that engine are prefixed with some engine scope (let's say my_engine for this issue)
  • By default, rails will look in the my_engine when going to a URL within that engine
  • The controller in the engine is not a devise_controller, so 2FA gets applied.
  • However, just using send(change_path) is not enough as we will look for scope_two_factor_authentication_path in the current app/engine. So this ends up meaning my_engine.scope_two_factor_authentication_path

So steps...

  1. Mount an engine inside of your Rails app with 2FA (I'm using https://github.com/code-mancers/rapidfire)
  2. Attempt to go to a url inside of that engine while you are not signed in
  3. Be redirected to sign in
  4. Be possible redirected to 2FA... but don't be fooled! Some silent errors have gone on and an exception happened. Your redirect_url for when you've signed in is now lost forever

Solution

  • By sending it to Devise.available_router_name instead, we get main_app.scope_two_factor_authentication_path (or whatever the router name is) and everything works from within the engine

I couldn't get tests to run locally, otherwise, I'll try to get a test written. I've confirmed this works for my app, however, and I'm using my fork in production.

@jules2689
Copy link
Author

@Houdini anything I can do to get this merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant