sorcery icon indicating copy to clipboard operation
sorcery copied to clipboard

Support overwriting config per controller and multi-class logins

Open sinsoku opened this issue 5 years ago • 12 comments

This pull request is intended to support logins for multiple roles, such as User and Admin.

First, change the configuration to use an instance instead of Module to allow overwriting it per controllers. In a controller, you can access it with sorcery_config.

Next, add login_session_key to the configuration so that we can change the session key from :user_id. It also supports Proc to be able to change the session key automatically according to the user_class.

These changes allow you to maintain multiple sessions.

class Admins::BaseController < ApplicationController
  before_action :set_admin_to_sorcery_config

  private

  def set_admin_to_sorcery_config
    sorcery_config.user_class = Admin
    sorcery_config.login_session_key = :admin_id
  end
end

NOTE

I'd like to get opinions from Sorcery maintainers, as being able to override config per controllers is the notable design change. Overriding the config is a big change, so we might to separate pull requests.

sinsoku avatar Jun 28 '20 11:06 sinsoku

@sinsoku Honestly, I'm looking at starting a rewrite of Sorcery from scratch, so maybe it would make more sense for us to discuss how to implement these changes with the new plugin and config system instead. @Fustrate made a gist with one potential way we could rework the config to be clearer/less bloated, so I would take a look at that and see how it would fit in with the changes you're wanting to make.

joshbuker avatar Jul 03 '20 22:07 joshbuker

@athix an entire rewrite from scratch? I saw the repo and followed it. That sounds incredibly lofty, and like it would be dangerous to transition to. I have all of my projects using Sorcery and I'd rather not have to do some kind of big rework to keep using it if 1.x comes out with breaking changes.

What's the reasoning behind a full rewrite vs just refactoring in stages?

crimson-knight avatar Jul 08 '20 15:07 crimson-knight

@sinsoku I was thinking about solving this problem using a simple permissions-based system. I don't have a rock-solid solution yet but I'm constantly bumping into it with my current projects to the point that I will need to take a few days and make something solid that I can use now and preferably re-use in the future.

crimson-knight avatar Jul 08 '20 15:07 crimson-knight

@crimson-knight I would say that it sounds a lot scarier on paper than what my intentions with it actually are. Essentially the rewrite is more to clear out 10 years of work-arounds and start with a fresh base with good testing from square 0, than anything else. The migration plan for existing apps would be:

  • Change gem requirements
    • sorcery -> sorcery-core
    • Add sorcery-oauth if you're using the External submodule
  • Update your config (the wiki will include a guided how-to with examples)
  • Find and replace method names (this may not be needed, depending on if method names make sense as-is)
  • Done

Getting a Sorcery v1 out that reworks some of the major pain points we've been seeing has been on the docket for years now. Just finally making the commitment to do it now.

joshbuker avatar Jul 08 '20 16:07 joshbuker

@sinsoku @crimson-knight for permissions, have you looked into using the pundit gem? It works well with Sorcery, and may provide some of the functionality you're looking for.

joshbuker avatar Jul 08 '20 16:07 joshbuker

Isn't the intention of this PR to add multi-model authentications to allow two independent log-in flows eventually? I've been looking into that as well.

Currently, Sorcery allows defining only one model (e.g. User). But what if your app needs two (or more) completely isolated login systems?

For example, a shopping cart where shop owners can log in to their admin panel, and on the other side, a frontend where their customers can log in to manage their account with that particular shop. Throwing everything in one table isn't really ideal in such a scenario.

Maybe something to consider for the rewrite? :)

wout avatar Jul 08 '20 17:07 wout

@wout Ahhh, okay that makes a lot more sense than what I was assuming, interesting. Yeah that sounds worth considering for sure, thanks for clarifying that!

So this is less the authorization aspect, and more supporting multiple separate login tables?

joshbuker avatar Jul 08 '20 17:07 joshbuker

@wout that's easy to do though, and doesn't necessarily require an entirely new or different login system. I've already done this without a ton of extra setup/work and it's been very flexible without any issues. I guess I can add that to my documentation list 😅 🤷‍♂️

crimson-knight avatar Jul 08 '20 17:07 crimson-knight

So this is less the authorization aspect, and more supporting multiple separate login tables?

@athix Not sure, but from what's in the description I suppose that's the case with this PR.

I guess I can add that to my documentation list.

@crimson-knight That would be great! 😀️ I'll need something like that eventually for two apps I'm working on, so I'd love to see how it's done.

wout avatar Jul 08 '20 17:07 wout

@sinsoku I'm now at the configuration part of the refactor, are you available to discuss this sometime?

I'm considering making the configuration a centralized class rather than splitting it between controller and model, and updating it so that you can do custom configuration as a part of the authenticates_with_sorcery! call. Running into some issues with how the model config differs from the controller, and am considering if that's because of a bad approach, or just because previously only the model config was configurable per class.

Essentially how I'm thinking it would work:

  • Defaults loaded in from the sorcery code.
  • Initializer overwrites defaults with user's preferred settings. In most apps, this will be the only config needed.
  • authenticates_with_sorcery! do |config| will yield with the config instance for the calling class, either the model or controller depending on where it's called from. This is how you would change settings on a per class basis. Using this block is optional, and will replace any settings from the initializer if they conflict.
  • Once the yield is over, authenticates_with_sorcery! will continue with the rest of the usual include/extend process, using the config instance specific to that class.

I'll try out some ideas based off of your PR here. Please let me know if you would be interested in helping.

joshbuker avatar Aug 02 '20 02:08 joshbuker

Theoretically this should now be implemented on the v1 rework.

joshbuker avatar Aug 05 '20 05:08 joshbuker

I'm sorry, the reply was delayed.

Isn't the intention of this PR to add multi-model authentications to allow two independent log-in flows eventually? I've been looking into that as well.

That's right.

My use case required two log-in flows for users and administrators. I've tried overriding the config per /users/login and /admins/login paths, but I couldn't because Sorcery::Controller::Config is not thread safe.

This pull request is intended to make config thread-safe so that we can override the config per request. I want to change not only the model but also other settings such as the session timeout and the redirect path when login fails depending on the login flow.

sinsoku avatar Aug 09 '20 16:08 sinsoku