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

Use the session method in OmniAuth::Strategy #1076

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

achouippe
Copy link

@achouippe achouippe commented Jun 14, 2022

Context

In our Rails application we use omniauth_openid_connect (0.4.0) to perform an OpenID Connect authentication flow. We want to override the session mechanism to store omniauth related attributes in a dedicated cookie (other session attributes remain in the regular session state).

For that purpose, in our application we create a subclass of OmniAuth::Strategies::OpenIDConnect which overrides the OmniAuth::Strategy.session method. Our implementation of the session method provides a wrapper that stores omniauth* attributes in a dedicated secured cookie.

Problem

By overriding OmniAuth::Strategy.session, we expected to be able to implement a custom session mechanism but it is not possible:

Problem we have, is that OmniAuth::Strategy both uses OmniAuth::Strategy.session and env['rack.session'] (here and here).

Because OmniAuth::Strategy directly accesses env['rack.session'] without using its session method, we cannot inject the expected custom mechanism.

Suggested solution

In this PR we replace direct usages of env['rack.session'] by the session method in OmniAuth::Strategy.

It still leaves some questions:

  • The same change was introduced by Use session method instead of @env['rack.session'] #818, but it only touched the mock_call! method. Method request_call has not been touched, is there a rational behind this ?

  • We cannot replace direct usage of env['rack.session'] here by a call to session because @env is not set yet. Maybe we should also set @env before checking that the session exists ?

@env = env

unless session
   error = OmniAuth::NoSessionError.new('You must provide a session to use OmniAuth.')
   raise(error)
end
  • Would replacing env['rack.session'] by session break some kind of 'contracts' or encapsulation mechanisms with other omniauth strategies that subclass OmniAuth::Strategy ?

Thank you.

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.

None yet

1 participant