Rework single sign out support to work in Rails 3.1/3.2 #48

Merged
merged 2 commits into from Mar 4, 2013

Projects

None yet

7 participants

@jeremyhaile
Collaborator

This pull request is to address problems with the single sign out approach to work in Rails 3.1/3.2. For more information see this issue:
#47 (comment)

This pull request switches the approach from trying to monkey patch the session store and instead requires a filter to be added to ApplicationController like so:
include DeviseCasAuthenticatable::SingleSignOut::StoreSessionIdFilter

I'm mostly sending this pull request to start a discussion on the right approach. A couple of questions:
1) should we automatically include this filter in ApplicationController if it's defined?
2) should we be using Rack middleware instead?

@nicolai86

Thanks for taking time to review my problems with the Single Sign Out support.

However, I don't think that adding a before filter to fetch the session_id is a good way to handle it. I'd vote for using the supported stores the way they are integrated in Rails. E.g. like seen here for the active_record_store.

This allows us to separate session handling in different objects, which are loaded at startup once, according to the session store used.

@nbudin
Owner

Jeremy - I think your question #2 is an intriguing idea (using Rack middleware). That seems like it might potentially be a cleaner approach that would loosen our coupling to Rails session internals here. Could you elaborate on how that might work?

@nicolai86

+1 for the rack middleware approach. Should work with Rails 2.3 as well, even tho the setup would be manual.

@jeremyhaile
Collaborator

@leahpar - I'm not sure I understand the approach you're voting for. We have to hook into the request process or session store in some way to store the mapping from the CAS ticket to the session ID. How are you suggesting we do that?

@nbudin - instead of using a before_filter to capture the session ID, we could insert Rack middleware that does the same thing. Rails depends on Rack for the session ID anyways, so I think we could eliminate or lessen the Rails dependency by using middleware instead of a filter to capture the session ID/CAS ticket mapping.

@jeremyhaile
Collaborator

OK - I'll take a stab at replacing the filter I'm currently using with Rack middleware. I'll push to my branch and update the pull request once I get it working.

@jeremyhaile
Collaborator

Check out the most recent commit - I've switched it to be rack middleware instead of a filter. And it automatically injects itself into the rack middleware stack using a railtie.

@nbudin
Owner

Thanks Jeremy - this looks a lot cleaner.

I'd like to get this into devise_cas_authenticatable's automated test suite somehow. If you'd like to write some tests, that would be fantastic, or if you'd rather I do so, I'm also happy to.

@jeremyhaile
Collaborator

Hey Nat - I'm happy to write some tests. Just wanted to make sure you guys were ok with the approach first. Also would appreciate some integration testing if someone has a project/cas server they can test the single sign out with. I'll update this PR with some tests within the next day or two.

@lidaobing lidaobing commented on the diff Jul 5, 2012
lib/devise_cas_authenticatable/single_sign_out.rb
else
- set_session_without_storage(env, sid, session_data)
- end
+ log.error "Cannot process logout request because this Rails application's session store is "+
@lidaobing
lidaobing Jul 5, 2012

should be logger.error

@lidaobing lidaobing commented on the diff Jul 5, 2012
lib/devise_cas_authenticatable/single_sign_out.rb
+ begin
+ if ::DeviseCasAuthenticatable::SingleSignOut.rails3?
+ # => Rails 3
+ ::Rails.application.config.session_store
+ else
+ # => Rails 2
+ ActionController::Base.session_store
+ end
+ rescue NameError => e
+ # for older versions of Rails (prior to 2.3)
+ ActionController::Base.session_options[:database_manager]
+ end
+ end
+
+ def current_session_store
+ app = Rails.application.app
@lidaobing
lidaobing Jul 5, 2012

should be app = Rails.application

@endel

Hey @jeremyhaile, I've just pushed some commits decopling ActiveRecord, fixed a typo when logging error and iterating through middlewares properly to find the session instance.

https://github.com/endel/devise_cas_authenticatable/commits/master

@sotrachhun

Hi nbudin, I got the same problem(single logout) with your latest gem.
I hope you can merge with jeremyhaile soon.

Thanks for your hard work.

@nbudin
Owner

Whenever @jeremyhaile is comfortable merging, I'm comfortable merging. He's a co-maintainer, so technically he doesn't need my permission. :)

@stephenlam

What is the status of this merge? Having issues with single sign out as well. Thanks.

@nbudin
Owner

@Klarerwind - does this pull request fix the issue for you? Or, perhaps, does @endel's branch work? I have no way of testing single sign out myself, which is why I'm reluctant to just merge. I'm happy to take others' advice on which branch works, but I just don't want to break things accidentally by blindly merging code.

@endel

Hey guys, sorry for the delay to give a feedback.

There was a problem using ActiveRecord::SessionStore and ActionDispatch::Session::CacheStore, which I've solved today.

@Klarerwind could you please give a try on my master branch and check if it works for you?

Here is my devise cas configuration:

Devise.setup do |config|
  config.cas_base_url = "http://your.cas-server.com"
  config.cas_logout_url_param = 'destination'
  config.cas_enable_single_sign_out = true
  ...
end
@nbudin
Owner

Just to note, as per usual I have no single sign out setup myself, so I can't test it. Would appreciate @Klarerwind, @jeremyhaile, or anyone else who can test it and let me know if it works for you. Thanks!

@jeremyhaile
Collaborator

@nbudin - I'm no longer working at the company where I was using this, but I've asked @betawaffle if he can test it.

@nbudin
Owner

Ah, ok. Thank you for all your work on this feature!

@nbudin nbudin merged commit b1233f8 into nbudin:master Mar 4, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment