Looking for feedback on deprecating using Rails session to pass values to the app

Background

The canonical way to handle errors in an OmniAuth strategy is to call fail!(:some_label, some_exception) - this method invokes the OmniAuth on_failure hook which, by default, is the FailureEndpoint rack middleware. FailureEndpoint redirects you to /auth/failure which you must implement in your app. A common change to this is to replace FailureEndpoint with a different rack middleware which calls one of the controller actions in your app e.g.

# config/initializers/omniauth.rb
OmniAuth.config.on_failure = Proc.new { |env| AuthenticationsController.action(:failure).call(env) }

Current behaviour

Currently this gem returns values via the Rails session. The Realme FLT is in session[:uid] if everything went well and the error is in session[:realme_error] if not.

The issues

This works fine but isn't really "conventional" for OmniAuth. There is also a potential edge case where the Rails app is using the session for other stuff and the error message we add tips it over the size limit of what can be stored in a cookie (4KB).

My proposal

I would like to deprecate using the Rails session to return values to the Rails app and instead use the conventional request.env["omniauth.auth"]. Below is a rough example of a controller from an app that receives values from this gem that way:

class AuthenticationsController < ApplicationController
  skip_before_action :verify_authenticity_token
  skip_before_action :authenticate_user!

  def realme
    realme_uid = request.env["omniauth.auth"]["uid"]
    realme_cms_login_access_token = request.env["omniauth.auth"]["info"]["realme_cms_lat"]

    # sign in the user etc.
  end

  def failure
    exception = request.env["omniauth.error"] # a reference to the exception instance class
    error_type = request.env["omniauth.error.type"] # the first symbol passed to fail!()
    erroring_strategy = request.env["omniauth.error.strategy"] # a reference to the strategy instance that threw the error

    redirect_to(new_user_session_path, alert: exception.message)
  end
end

Backwards compatibility

I don't want to break existing users so I propose keeping the old session functionality around in a use_session_to_store_results option.

You could then make a choice about whether that option defaults to true or not, perhaps in sync with a version bump.

Feedback I'd like

I'm happy to make a PR for this but wanted to run this past you first to make sure it's something you would be ok considering. Obviously the final decision is made based on the look & feel of the PR.

DigitalNZ / omniauth-realme