RIPAGlobal / omniauth-entra-id

OAuth 2 authentication with the Azure ActiveDirectory V2 API.
MIT License
64 stars 35 forks source link

login_hint into oauth request params #8

Closed grdmnt closed 1 year ago

grdmnt commented 2 years ago

Is your feature request related to a problem? Please describe. I'm trying to add in login_hint into the omniauth request url, but it doesn't pass in the param.

Describe the solution you'd like By doing azure_sso_url(login_hint: "email@domain.com") it should redirect to the azure login with the login_hint param inside query params. I already forked this and added this in into our project, but thought of sharing and I can open a PR if people need it.

Describe alternatives you've considered N/A

Additional context N/A

pond commented 2 years ago

If you have a working fork, I'd certainly be interesting to see the diff & we'd definitely consider pulling that into the main feature set.

A little more information on the intended use case / end-user workflow and outcome would be helpful for context.

grdmnt commented 2 years ago

I'm currently using it to prefill emails since we ask our users to put their email first for some checks on our end before redirecting them to Azure.

I just added this line for this to work:

options.authorize_params.login_hint = request.params['login_hint'] if defined? request && request.params['login_hint']
pond commented 1 year ago

Apologies for the ongoing delay. We've not forgotten, just heavily overloaded with other work at present.

pond commented 1 year ago

I finally got around to looking at it and actually, I don't think we need any code changes - but I really ought to improve our README.md because this looks like a case of inadequate/bad documentation.

It transpires that the gem accepts a custom "provider" class, which can return its own authorize_params starting-point Hash, and can get data needed for that Hash from an object which is given to its constructor by our gem.

To explain this - you probably have an initializer that looks like this if not using Devise:

use OmniAuth::Builder do
  provider(
    :azure_activedirectory_v2,
    {
      client_id:     ENV['AZURE_CLIENT_ID'],
      client_secret: ENV['AZURE_CLIENT_SECRET'],
    }
  )
end

...or if using Devise, then in config/initializers/devise.rb

config.omniauth(
  :azure_activedirectory_v2,
  {
    client_id:     ENV['AZURE_CLIENT_ID'],
    client_secret: ENV['AZURE_CLIENT_SECRET'],
  }
)

There's an alternative form for these. You can provide instead of that Hash of key-value option pairs, a class which, when instantiated, describes those same options in the form of attribute read accessors. This is mentioned (by the documentation of the abandoned gem, from which ours is derived) at https://github.com/marknadig/omniauth-azure-oauth2#usage as a way to assign tenants dynamically, but it's usable for just about anything.

Let's look at the class first, then the way to configure its use after that. Our gem initialises that class with an instance of "self" (a subclass of OmniAuth::Strategies::OAuth2) and this object in turn gives access to the request object. I think you have modified method OmniAuth::StrategiesAzureActivedirectoryV2#client in your fork given the line of code you quote, so you'll probably have seen in there that it starts with:

if options.tenant_provider
  provider = options.tenant_provider.new(self)
else
  provider = options  # if pass has to config, get mapped right on to options
end

...and that tenant_provider is actually going to be our custom class. Whether it uses the tenant provider, or the "normal" OmniAuth::Strategy::Options instance in options for OmniAuth, it then goes to query that object for, well, almost everything; client ID and secret, tenant ID and base Azure URL for Azure specifically and then, importantly, it lets you return a custom starting point for authorize_params, which by default just starts as an empty Hash. It then goes on to overwrite a few fields there:

options.authorize_params = provider.authorize_params if provider.respond_to?(:authorize_params)
options.authorize_params.domain_hint = provider.domain_hint if provider.respond_to?(:domain_hint) && provider.domain_hint
options.authorize_params.prompt = request.params['prompt'] if defined? request && request.params['prompt']
options.authorize_params.scope = (provider.scope if provider.respond_to?(:scope) && provider.scope) || DEFAULT_SCOPE

That's pretty much it now - we know we can provide a custom class to configure AzureActivedirectoryV2, that this class is initialised with an object that lets us get at request and that the class can define a method called authorize_params which will return the starting point Hash used for the OmniAuth flow thereafter.

Here's an example class.

class CustomOmniAuthAzureProvider
  def initialize(strategy)
    @strategy = strategy
  end

  def client_id
    ENV['AZURE_CLIENT_ID']
  end

  def client_secret
    ENV['AZURE_CLIENT_SECRET']
  end

  def authorize_params
    ap = {}

    if @strategy.request && @strategy.request.params['login_hint']
      ap['login_hint'] = @strategy.request.params['login_hint']
    end

    return ap
  end
end

You'd replace any options passed in the old options Hash configuration (notably e.g. prompt) with accessor methods similar to #client_id or #client_secret above. Then configure via:

use OmniAuth::Builder do
  provider(
    :azure_activedirectory_v2,
    CustomOmniAuthAzureProvider
  )
end

...or if using Devise, then in config/initializers/devise.rb

config.omniauth(
  :azure_activedirectory_v2,
  CustomOmniAuthAzureProvider
)

...and I think that should do the trick.

Let me know how you get on. If this doesn't solve the issue then we could put in the one-liner from your fork, but hopefully that is in fact not necessary. In the mean time I'll be updating README.md to describe the above mechanism.

pond commented 1 year ago

No matter what the outcome here, https://github.com/RIPAGlobal/omniauth-azure-activedirectory-v2/pull/10 adds documentation as described above.

grdmnt commented 1 year ago

I completely missed this update! My bad on that. I will test this out on our setup but after reading your latest comment explaining the setup, it makes perfect sense. Thanks for a thorough explanation, this would definitely be a better approach than what I did prior.