Skip to content
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

Merged
merged 3 commits into from
Dec 18, 2011
Merged

Perform authentication in Rack middleware #177

merged 3 commits into from
Dec 18, 2011

Conversation

jferris
Copy link
Contributor

@jferris jferris commented Dec 1, 2011

This moves most of the stuff from Clearance::Authentication into a Clearance::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-1

The 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.

@croaky
Copy link
Contributor

croaky commented Dec 1, 2011

Looks good to me. Couple of style questions.

@hgmnz
Copy link

hgmnz commented Dec 1, 2011

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
Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor

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

Copy link
Contributor Author

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.

croaky pushed a commit that referenced this pull request Dec 18, 2011
Perform authentication in Rack middleware
@croaky croaky merged commit 498bff3 into master Dec 18, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants