feature: switch back about switch_user HOT 16 CLOSED

original_user should also be accessible in controller and view guards

from switch_user.

Hi there, you're not the first person to ask about implementing something like this (see issue #9) and I agree that it would be a really nice feature to have. My concern is that it pokes a really big hole in the security of a person's site, so I've been thinking about how to implement it in a way that doesn't keep me awake at night worrying.

Here's my thought on how it might work. Once you're logged in as a user, you can send a message to the SwitchUserController that you want to make the current user sticky. Showing the switch_user drop-down menu on the page would be determined by the current_user and the sticky_user. You can send a message to the SwitchUserController when your done to unstick the user.

The sticky user would be stored in a session variable, which cannot be modified by the client, so we can trust it. You also don't get to say which user you want to make sticky in the controller action, it would always use the current_user.

Thoughts ?

from switch_user.

Hey, I agree with you for the most part. I'm just not very sure about the term "sticky". IMHO, it should be "original_user".. but in any case:

Perhaps "sticky" feature should be controlled by sticky_guard that let application author decide whether or not he wants it to be enabled and for whom:

config.sticky_guard = lambda { |current_user, request| current_user.admin? }

If guard evaluates to true, when user switches to another account, controller should store the original user into session variable, and make it accessible to the app and to the other guards.

from switch_user.

Thanks for your input! I do like the idea of a guard because it's used elsewhere in the code and it will be conceptually more familiar.

I think we'd need to pass the original_user to the guard as well, otherwise when we switch users the current user could become a non-admin and the guard would then evaluate to false.

config.sticky_guard = lambda { |current_user, original_user, request|
  current_user.admin? || original_user.admin?
}

I'm also thinking that by default the guard should be off.

config.sticky_guard = lambda { |current_user, original_user, request| false }

Let me know what you think.

from switch_user.

I also realized that the other guards will also need access to original_user, because to them current_user might be a non admin and so the guard might say, don't allow access. I think this is going to need to be accessed through the controller. Something like this:

config.controller_guard = lambda {|current_user, request, controller|  (controller.original_user && controller.original_user.admin?) || current_user.admin?  }

from switch_user.

You can get also the current_user and the request from the controller. So, controller_guard could just accept the "controller" method. However, i see that you want this because you want to maintain backward compatibility.

One more question. Do you think that controller and view guards are any different? Can you think of a case where controller guard would evaluate to something different than the view guard?

How about just crating the "guard" config method and passing the controller to it:

config.guard = lambda { |c| (c.original_user && c.original_user.admin?) || c.current_user.admin? }

from switch_user.

Agreed, I can't see any situation where the view and controller guards would be different. I think I'll deprecate the view guard and then ignore it in future releases.

I like the idea of crating up the parameters. Any thoughts on how to deprecate that gracefully for the other guards?

I've started writing the code for sticky users and I'm seeing some of the code creaking under the pressure of a second controller action with a separate guard. This is a good thing - letting the requirements improve the design.

Thanks for your input, it's been helpful!

from switch_user.

Perhaps you can deprecate both controller and view guards, in favor of just guard. That way you can keep the old ones with same set of parameters, but issue deprecation warnings whenever they are used. In controller/view you should check to see whether just guard is present (and use it if it is), otherwise use the old controller/view guard respectively. What do you think?

from switch_user.

I like the idea of falling back to the old guards if the new ones aren't present and I agree that we give the new guards higher precedence. I still think we need a second guard for the sticky user switching as I'd want that feature off by default because of the power it grants. Let's keep batting around ideas.

Does it seem strange to you that the complex boolean logic for our guard lives in a lambda that is almost certainly not being tested by the user? We could still support the lambda functionality, but what if moving forward we encourage people to define controller_guard and sticky_user_guard in a class ? We defined a base class and the user can subclass to define their own rules.

class SwitchUserGuard
  attr_reader :current_user, :controller, :request

  def initialize(current_user, controller, request)
    # assign these to instance variables
  end

  def controller_guard
    current_user && current_user.admin?
  end

  def stick_user_guard
    false
  end
end

Now we have something that's easily testable and, as the kinds of things we want to make available change, we don't need to worry about breaking people's config in the lambdas.

from switch_user.

I like that idea. Having guard class is a good thing! Users can test it easily and it is more safe. Here is my idea on how it should look like:

Config:

config.guard = :switch_user_guard # the default option if the class is present

Base Class (provided by gem):

class SwitchUser::Guard
  attr_reader :controller

  def initialize(controller)
    @controller = controller
  end

  def controller_guard
    Rails.env.development?
  end

  def stick_user_guard
    false
  end

private
  def current_user
    @controller.current_user
  end

  def original_user
    @controller.original_user
  end

  def request
    @controller.request
  end
end

Application-specific guard:

class SwitchUserGuard << ::SwitchUser::Guard

  def stick_user_guard
    true # sticky enabled for everyone
  end

  def controller_guard
    (original_user || current_user).admin?
  end

end

from switch_user.

I'm not really sure that we should define the original_user on the controller, since it might collide with user's setup. Perhaps something like this would be better:

#...
  def original_user
    @original_user ||= load_original_user
  end

  def load_original_user
    scope_identifier = @controller.session["switch_user_original_user_scope_identifier"]
    SwitchUser::UserLoader.prepare(scope_identifier: scope_identifier).user
  end
#...

from switch_user.

Hi guys, any updates on this? I see this issue is still open. If this was not implemented yet, then how would you solve this problem outside this gem? Just store it in the session?

from switch_user.

Exactly, sessions are the way to go. I haven't had any time to look at implementing this. I did start something a few weeks ago, but quickly realized some internal refactoring would be required in order to provide a maintainable solution.

This is an outline of what you'd need to do:

1. on switching a user, store the original_user_id in a session
2. when checking if a user still has access to switch user consider the user_id as well as the original_user_id
3. when logging out clear the original_user_id

You can read the above comments to see what we're deliberating. One of these nights, I'll sit down and get this finished...

from switch_user.

Progress!

HI @assembler, I've pushed up some of the code required to accomplish this. The providers now have a function to remember current_user even after the current_user has been switched. The remembered user will also be passed to the 3rd argument of your guard lambda.

I still need to add a method for forgetting the current_user and add also wire up the view to a new controller action. I was thinking that a check_box could be use to signal to the controller action that the currently selected user should be remembered.

from switch_user.

I've pushed up this feature to master. You'll need to adjust your config in order to take advantage of this:

config.controller_guard = lambda { |current_user, request, original_user|
  current_user.try(:admin?) || original_user.try(:admin?)
}
config.switch_back = true

from switch_user.

👍

from switch_user.