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

Allow dynamic configs #55

Merged
merged 1 commit into from
Aug 23, 2022
Merged

Conversation

ryanzidago
Copy link
Contributor

Hey @swelham,

I propose the following PR to allow UeberauthMicrosoft to be used with multi-tenant applications (where a single Microsoft config won't be sufficient; i.e. applications that let users to login via their own Microsoft accounts).

It somehow relates to this comment here and this one here.

Basically what I'm doing is passing a bunch of options from the conn all the way down to the Oauth.client/1 function. This way, I can do the following in my own app controller:

  def request(conn, %{"provider" => "microsoft", "key" => key}) do
    config = get_config_for_key(key)
    config = Application.fetch_env!(:ueberauth, Ueberauth.Strategy.Microsoft.OAuth)[key]

    {
      Ueberauth.Strategy.Microsoft,
      [
        tenant_id: config[:tenant_id],
        client_id: config[:client_id],
        client_secret: config[:client_secret],
        callback_path: "/auth/microsoft/callback/#{key}",
        site: "https://graph.microsoft.com",
        authorize_url:
          "https://login.microsoftonline.com/#{config[:tenant_id]}/oauth2/v2.0/authorize",
        token_url: "https://login.microsoftonline.com/#{config[:tenant_id]}/oauth2/v2.0/token",
        request_opts: [ssl_options: [versions: [:"tlsv1.2"]]]
      ]
    }
  end

You can see here that I select the proper Microsoft credentials based on some path params.
Let me know what you think. Happy to discuss a different approach.

@swelham
Copy link
Owner

swelham commented Aug 22, 2022

Hey @ryanzidago, apologies for the slow response.

From a quick look, this is looking great. I will take a proper look later today and get back to you.

@swelham
Copy link
Owner

swelham commented Aug 23, 2022

This looks good to me 👍

Thanks for the PR.

@swelham swelham merged commit 954cd51 into swelham:master Aug 23, 2022
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.

2 participants