-
-
Notifications
You must be signed in to change notification settings - Fork 468
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
Perform authentication in Rack middleware #177
Conversation
Looks good to me. Couple of style questions. |
Looks good to me, useful change |
@@ -5,9 +5,9 @@ GIT | |||
shoulda-matchers (1.0.0.beta3) | |||
|
|||
PATH | |||
remote: /Users/croaky/dev/clearance | |||
remote: /Users/jferris/Source/clearance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this, and is it obviously a bug in appraisal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen this before. I think it must be from the gem bundling itself, and should probably be considered a bug in appraisal. I'll see what I can come up with next time I'm working on appraisal.
|
||
def current_user | ||
@current_user ||= with_remember_token do |token| | ||
::User.find_by_remember_token(token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An existing but seldom-raised complaint is that the Clearance user collection must be the User
class. Maybe pass this in from the middleware, allowing people to change it as desired.
cough Elixir cough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that change, but I think it should be done separately.
Perform authentication in Rack middleware
This moves most of the stuff from
Clearance::Authentication
into aClearance::Session
object, and delegates from the controllers to that. A piece of Rack middleware injects the session object into the Rack environment hash.This lets you keep using the same methods in Rails controllers, but provides
env[:clearance]
for Rack middleware/apps you might be using. This is summed up pretty well in the README changes I made: f845887#diff-1The major reason I want to do this is to move more things out of Rails action filters and into middleware. Things like Saucy's account filters, Kissmetrics and identity handling, and so on can all move into middleware this way. I find middleware easier to deal with in general, particularly in tests.
This also starts us down the road of making it easier to introduce alternative authentication methods, which has historically been a pain in Clearance, since we can make a new piece of middleware that injects its own session. A little refactoring of
Clearance::Session
would make that much easier.The only downside I can find to this is that Rails functional tests don't go through the middleware stack by default. That means that, out of the box, this branch won't work with functional tests as-is. You'll need to require
clearance/testing
to make them work. We could try to automatically include the helpers if we think that's a problem.