-
Notifications
You must be signed in to change notification settings - Fork 204
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 support for multiple managers in middleware #67
Conversation
My 2¢ on this is that perhaps another way to do it, rather than deep merging and the potential conflicts there in, would be to look for Basically build an internal stack of warden's, similar to how rack handles things. |
This would definitely break existing behavior. I would not be comfortable with merging the configurations for different warden managers. I think this would lead to unpredictable behavior. My initial thought is to have a warden manager that can take over the environment, storing off the existing manager if there is one, and then when passing the request back upstream it could replace the original. I'm not sure how this would really go when trying to access the session. It would require some work on where in the session a particular manager stored it's data otherwise one manager would clobber another. I wonder if another alternative would be to add your strategy to one that is already there, and just use a different scope (setup with a custom failure app etc). This would make more sense to me. If we're going to go this way, perhaps we could make the Warden::Manager a singleton since we're not expecting there to be any downstream ones present. Thoughts? |
That was my concern too... It seems far more sensible to either create a chain/stack of warden's or create a singleton that can be added to (with additional strategies) etc. |
Well, that would mean that the application would only see the manager that is closer to the app. So this would actually be what we have now, except that the downstream manager would be the active one instead of the upstream one. Another possibility would be to have multiple warden managers stored in the env with different names. Instead of I was thinking something along these lines: use Warden::Manager do |config|
config.warden_name = 'github-rails'
# ...
end
request.env['warden.github-rails'].authenticate!(:admin) This would also prevent scope clashes between multiple managers. A further nice addition would be that the rack specs would be respected. The specs says:
What do you think? |
This allows multiple warden managers to live side by side without interfering.
Any thoughts on this? |
I think this is a sensible approach to allowing multiple 'installs' of warden to operate simultaneously, but essentially neither warden would be aware of the other. Any other middleware taking care of operating with warden would need to be aware of the specific 'install(s)' that they need to talk to (given that one of warden's strengths is the whole rack orientated throw/catch cycle). E.g. This would complicate interoperability but does allow packaged versions of warden to play nice as an ecosystem. |
What are the specific cases where one warden would need to be aware of the other? The only problem with this would be intercepting 401. If the one downstream intercepts it, the other can't handle it. But then again, you probably want only one to do that, so you could place that warden downstream of the other. |
When one warden fails to authenticate but one elsewhere can (differing sets of strategies), the first warden could falsely return 401 when another warden can handle the call. Your setup works because you have your specialised install on top of the generic device "install", there are likely scenarios the other way around too, what if your app denied someone access because of github that devise would otherwise grant? Mostly I was thinking of scenarios of communication between middleware expecting env['warden'] and how they would work, or rather wouldn't work with namespaced wardens... |
I'm working on a gem for rails (warden-github-rails) that inserts a warden middleware. Unfortunately, when the rails app already has a warden middleware (e.g. when using devise), the middleware downstream is ignored.
I'd like to propose and discuss a possibility to make multiple warden managers work.
Inside
Warden::Manager#call
there is the check for an upstream manager:I think an easy approach would be some kind of configuration merging such as:
Would you guys think this would make sense at all?
Obviously the example above is a naïve approach. It might make sense to only copy over certain configs. One downside of simply merging is that it is not possible to have conditional assignments à la:
Another thought I had was to allow all configs to be scoped, similarly to specifying
:strategies
per scope. This would allow a warden "plugin" to have a smaller footprint.Any thoughts?
PS: See the initial discussion with @josevalim at the devise email list.