heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
24k stars 5.55k forks source link

Sharing Devise on Multiple Engines #2827

Closed Bharat311 closed 10 years ago

Bharat311 commented 10 years ago

Hi, I have an application which has multiple rails engines mounted on it. Some of these engines use devise for authentication.

Now, it is easy to setup devise to work with one engine - by setting the 'router_name' and 'parent_controller' in config/initializer/devise.rb file. But since, in this case, each DeviseController must inherit from each engine's own Application Controller and defining multiple devise initializer files simply overrides the values.

I think an optimum solution for this is to allow devise the ability to be able to mount to multiple engines. One way i propose for this is to have the devise provides Mixins for each controller and we should have a generator class that generates the controllers classes using those mixins and for each engine, which can be easily configured in the initializer file.

Another solution for this problem is to separate out all authentication to a single engine (and it can be configured to use devise easily), but that would require moving/copying of the layouts and js, css, etc. assets as well to that authentication engine, thus violating the DRY principle.

josevalim commented 10 years ago

Thanks @Bharat311!

Question: Why does Devise need to be mounted multiple times? Do you really want a different authentication end-point for each engine?

This proposed change is definitely a big change on how Devise works and given the use case, I am more inclined to say it is an use case we don't support in Devise.

Bharat311 commented 10 years ago

@josevalim, thanks for the quick response. In my application, we have several small apps that are using the same backend. The functionality of the sub-apps are mutually exclusive so we have separated them out to engines. And yes, we really want a different authentication end-point for each of them.

Actually we have 3 different roles in our app - Customer, Merchant and Admin and each of these roles has been extracted out to a separate engine as they perform features which are unique to them, so it not ideal to have them in the same app. Using separate engines for each of them, we are able to segregate the functionality, and it makes a lot of sense as we have dedicated engine for each role.

I did some changes to Devise in my own fork. I would be obliged if you take a look and let me know your views on them. Its been sometime since we have been using it successfully on production.

https://github.com/Bharat311/devise/tree/f-mixin

josevalim commented 10 years ago

I have added some comments. It seems a nice solution that still keeps the best of both worlds but I am still unsure due to the meta-programming involved. :(

Bharat311 commented 10 years ago

@josevalim, oh sorry, forgot to add the file. Just added it:

https://github.com/Bharat311/devise/commit/afc03526418df77f7c6a0ebf40cda69d1e7a6cac

Bharat311 commented 10 years ago

Hi @josevalim, i think we can remove the metaprogramming altogether by removing the generator class.

We could provide a config variable _generatecontrollers (defaults: true) which will define whether Devise provides the controllers. ( Though, I am not sure as to what could be the better way to do this. )

Now instead of having definitions in the controllers we can have them in the mixins and have devise controllers file like:

  class Devise::SessionsController < DeviseController
    include Devise::Mixins::Session
  end

For cases such as mine, we simply need to set the _generatecontrollers to false and define our own controllers per engine using the mixins.

  class TestEngine::SessionsController < TestEngine::DeviseController
    include Devise::Mixins::Session
  end

This way making minimum changes to the existing structure of Devise, we are also solving our issue. What are your views regarding this?

The general idea could be taken from the code here: https://github.com/Bharat311/devise/tree/f-mixin-v2

josevalim commented 10 years ago

@Bharat311 I am sorry but we decided to not merge those changes. It is a very specific corner case but it adds a bunch of complexity to Devise source. Thank you for the discussion!

abezzub commented 10 years ago

I am running into the same problem :(

johnnyshields commented 10 years ago

@abezzub you can check out our Devise fork here which supports this https://github.com/Bharat311/devise/tree/f-mixin (note f-mixin branch).

In order to use it, the routes for each user model will need to live in their own Rails Engine. Then you can specify the engines in devise.rb as follows:

  # The engines which should be Devise-aware:
  config.controller_scopes = [:my_engine, :engine_b, :engine_c]

In each engine's routes.rb you'll need to add a devise_for call for the user class which is relevant to that engine. For example, assume MyEngine is used with CustomerUser class:

MyEngine::Engine.routes.draw do
  devise_for :customer_users, class_name: 'MyNamespace::CustomerUser', path: '/users', controllers: {
          registrations: 'engine_a/registrations',
          sessions:      'engine_a/sessions',
          confirmations: 'engine_a/confirmations',
          passwords:     'engine_a/passwords'
    }, path_names: { registration: 'me' }

  authenticated :customer_user do
    # your user routes
  end
end

@Bharat311 please correct if wrong

chibicode commented 9 years ago

@josevalim, thanks for the quick response. In my application, we have several small apps that are using the same backend. The functionality of the sub-apps are mutually exclusive so we have separated them out to engines. And yes, we really want a different authentication end-point for each of them.

We have a similar problem :/ Didn't realize this was unsupported b/c it was not written on the wiki. So I added an entry here: https://github.com/plataformatec/devise/wiki/How-To:-Use-devise-inside-a-mountable-engine#warning

sudrao commented 9 years ago

We have exactly the same requirement as @Bharat311! There are regular and admin users for the website and we have a separate engine for admins mounted in the main app. The mixin approach will work well for us.

mltsy commented 6 years ago

I also find myself wishing this were possible. It has been very useful/promising to use Engines to organize mostly separate apps that all reference the same data, but I'm unable to do so, because Devise breaks in this use case. I could look into alternate authentication for the Engines, but it sure would be much nicer to just use Devise for everything.

tibomogul commented 6 years ago

You can look at how Spree Commerce does it. It uses multiple engines, including one for devise auth. It is easy to extend using more engines as extensions.

mediafinger commented 2 years ago

I am running into the same issue of having one modular Rails monolith with multiple engines - and multiple models like Admin and User that have different requirements for the authentication setup.

Is there by now a solution by Devise - which would allow to use the gem and not a fork or monkey-patches?

Or is this still unsolved and needs to be worked around?

rickgorman commented 1 year ago

Could this be addressed by allowing the parent_controller to be specified within the devise_for block in the routing layer, kind of like this:

devise_for :regular_user, parent_controller: "RegularUser::ApplicationController"
devise_for :admin_user, parent_controller: "AdminUser::ApplicationController"

Posting this because I ran into a similar issue when trying to set up two categories of users, albeit on the same monolith. The use case is a multi-sided marketplace where each category of user has a similar set of paths (i.e. "/", "/profile", "/foobar", etc) that each route to different controllers based on the type of user. This is important as an additional layer of security to prevent producers from seeing consumer's data and vice-versa.

I'm using authenticate blocks in routes.rb to differentiate routing between these two types of users like so:

authenticate :consumer_user do
  root "consumer_user/home#index"
end

authenticate :producer_user do
  root "producer_user/home#index"
end

This works great, except that devise wants to inherit from its config's parent_controller, which is defined via an initializer:

https://github.com/heartcombo/devise/blob/ec0674523e7909579a5a008f16fb9fe0c3a71712/app/controllers/devise_controller.rb#L4

As a result, all devise controller inherit from ApplicationController and I had to add a bunch of methods in ApplicationController that deal with disambiguating between the different user categories, like this one for analytics:

class ApplicationController < ActionController::Base
  def analytics(authenticatable: current_consumer_user || current_producer_user)
    @analytics ||=
      case authenticatable
      when ->(a) { a.is_a?(ConsumerUser) }
        ConsumerUser::Analytics.new(
          consumer_user: authenticatable,
          browser: browser.name
        )
      when ->(a) { a.is_a?(ProducerUser) }
        ProducerUser::Analytics.new(
          producer_user: authenticatable,
          browser: browser.name
        )
      end
  end
end

It's not the end of the world, but it does require these polymorphic disambiguators that can make things a bit hard to reason about. If there's interest from the team I can work to put together a PR to allow for a dynamic parent_controller option. Thanks.