ActiveSession is incompatible with Geo OAuth
Noted in the 10-8-stable-ee
branch, but coming soon to a ce-to-ee merge:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/17867 was merged to CE today (2nd May), introducing app/models/active_session.rb
and a set of Warden callbacks in config/initializers/warden.rb
:
Warden::Manager.after_authentication do |user, auth, opts|
ActiveSession.cleanup(user)
end
Warden::Manager.after_set_user only: :fetch do |user, auth, opts|
ActiveSession.set(user, auth.request)
end
Warden::Manager.before_logout do |user, auth, opts|
ActiveSession.destroy(user || auth.user, auth.request.session.id)
end
The EE-only spec for Geo, https://gitlab.com/gitlab-org/gitlab-ee/blob/master/ee/spec/controllers/oauth/geo_auth_controller_spec.rb#L99, fails with the addition of this code:
Failures:
1) Oauth::GeoAuthController GET logout access_token error logs out when correct access_token is informed
Failure/Error: redis.srem(lookup_key_name(user.id), session_id)
NoMethodError:
undefined method `id' for nil:NilClass
# ./app/models/active_session.rb:63:in `block in destroy'
# ./lib/gitlab/redis/wrapper.rb:17:in `block in with'
# /home/lupine/.gem/ruby/2.3.7/gems/connection_pool-2.2.1/lib/connection_pool.rb:64:in `block (2 levels) in with'
# /home/lupine/.gem/ruby/2.3.7/gems/connection_pool-2.2.1/lib/connection_pool.rb:63:in `handle_interrupt'
# /home/lupine/.gem/ruby/2.3.7/gems/connection_pool-2.2.1/lib/connection_pool.rb:63:in `block in with'
# /home/lupine/.gem/ruby/2.3.7/gems/connection_pool-2.2.1/lib/connection_pool.rb:60:in `handle_interrupt'
# /home/lupine/.gem/ruby/2.3.7/gems/connection_pool-2.2.1/lib/connection_pool.rb:60:in `with'
# ./lib/gitlab/redis/wrapper.rb:17:in `with'
# ./app/models/active_session.rb:61:in `destroy'
# ./config/initializers/warden.rb:21:in `block (2 levels) in <top (required)>'
# /home/lupine/.gem/ruby/2.3.7/gems/warden-1.2.6/lib/warden/hooks.rb:14:in `block in _run_callbacks'
# /home/lupine/.gem/ruby/2.3.7/gems/warden-1.2.6/lib/warden/hooks.rb:9:in `each'
# /home/lupine/.gem/ruby/2.3.7/gems/warden-1.2.6/lib/warden/hooks.rb:9:in `_run_callbacks'
# /home/lupine/.gem/ruby/2.3.7/gems/warden-1.2.6/lib/warden/manager.rb:51:in `_run_callbacks'
# /home/lupine/.gem/ruby/2.3.7/gems/warden-1.2.6/lib/warden/proxy.rb:262:in `block in logout'
# /home/lupine/.gem/ruby/2.3.7/gems/warden-1.2.6/lib/warden/proxy.rb:260:in `each'
# /home/lupine/.gem/ruby/2.3.7/gems/warden-1.2.6/lib/warden/proxy.rb:260:in `logout'
# /home/lupine/.gem/ruby/2.3.7/gems/devise-4.2.0/lib/devise/controllers/sign_in_out.rb:77:in `sign_out'
# ./ee/app/controllers/oauth/geo_auth_controller.rb:41:in `logout'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/metal/implicit_render.rb:4:in `send_action'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/abstract_controller/base.rb:198:in `process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/metal/rendering.rb:10:in `process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/abstract_controller/callbacks.rb:20:in `block in process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:117:in `call'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:555:in `block (2 levels) in compile'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:505:in `call'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:498:in `block (2 levels) in around'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:298:in `block in halting_and_conditional'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:497:in `block in around'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:505:in `call'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:498:in `block (2 levels) in around'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:313:in `block (2 levels) in halting'
# /home/lupine/.gem/ruby/2.3.7/gems/sentry-raven-2.7.2/lib/raven/integrations/rails/controller_transaction.rb:7:in `block in included'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:441:in `instance_exec'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:441:in `block in make_lambda'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:312:in `block in halting'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:497:in `block in around'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:505:in `call'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:92:in `__run_callbacks__'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:778:in `_run_process_action_callbacks'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/callbacks.rb:81:in `run_callbacks'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/abstract_controller/callbacks.rb:19:in `process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/metal/rescue.rb:29:in `process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/metal/instrumentation.rb:32:in `block in process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/notifications.rb:164:in `block in instrument'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
# /home/lupine/.gem/ruby/2.3.7/gems/activesupport-4.2.10/lib/active_support/notifications.rb:164:in `instrument'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/metal/instrumentation.rb:30:in `process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/metal/params_wrapper.rb:250:in `process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/activerecord-4.2.10/lib/active_record/railties/controller_runtime.rb:18:in `process_action'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/abstract_controller/base.rb:137:in `process'
# /home/lupine/.gem/ruby/2.3.7/gems/actionview-4.2.10/lib/action_view/rendering.rb:30:in `process'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/test_case.rb:639:in `process'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/test_case.rb:67:in `process'
# /home/lupine/.gem/ruby/2.3.7/gems/devise-4.2.0/lib/devise/test/controller_helpers.rb:33:in `block in process'
# /home/lupine/.gem/ruby/2.3.7/gems/devise-4.2.0/lib/devise/test/controller_helpers.rb:100:in `catch'
# /home/lupine/.gem/ruby/2.3.7/gems/devise-4.2.0/lib/devise/test/controller_helpers.rb:100:in `_catch_warden'
# /home/lupine/.gem/ruby/2.3.7/gems/devise-4.2.0/lib/devise/test/controller_helpers.rb:33:in `process'
# /home/lupine/.gem/ruby/2.3.7/gems/gon-6.1.0/lib/gon/spec_helpers.rb:15:in `process'
# /home/lupine/.gem/ruby/2.3.7/gems/actionpack-4.2.10/lib/action_controller/test_case.rb:514:in `get'
# ./ee/spec/controllers/oauth/geo_auth_controller_spec.rb:100:in `block (4 levels) in <top (required)>'
# /home/lupine/.gem/ruby/2.3.7/gems/rspec-retry-0.4.5/lib/rspec/retry.rb:98:in `block in run'
# /home/lupine/.gem/ruby/2.3.7/gems/rspec-retry-0.4.5/lib/rspec/retry.rb:88:in `loop'
# /home/lupine/.gem/ruby/2.3.7/gems/rspec-retry-0.4.5/lib/rspec/retry.rb:88:in `run'
# /home/lupine/.gem/ruby/2.3.7/gems/rspec-retry-0.4.5/lib/rspec_ext/rspec_ext.rb:12:in `run_with_retry'
# /home/lupine/.gem/ruby/2.3.7/gems/rspec-retry-0.4.5/lib/rspec/retry.rb:22:in `block (2 levels) in setup'
Finished in 4.68 seconds (files took 13.43 seconds to load)
8 examples, 1 failure
Failed examples:
rspec ./ee/spec/controllers/oauth/geo_auth_controller_spec.rb:99 # Oauth::GeoAuthController GET logout access_token error logs out when correct access_token is informed
In particular, both user
and auth.user
in the before_logout
warden callback shown about are nil
, despite current_user
being set in this EE-only controller.
We need to decide what the correct fix is here. I think this is a genuine bug, rather than a spec-only breakage, and it's holding up 10.8rc1.
/cc @DouweM @koffeinfrei