RIPAGlobal / omniauth-azure-activedirectory-v2

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

Shouldn't custom_policy be included in the main Azure OAuth URL - not just in the token URL? #22

Open jrodrigosm opened 9 months ago

jrodrigosm commented 9 months ago

Describe the bug

I have been testing the gem to login against an Azure AD B2C instance, with a custom policy. Even though I configure the custom policy in my initializer, the authorization URL does not include it. The generated authorization URL is "#{base_azure_url}/#{tenant_id}/oauth2/v2.0/authorize". But, according to Microsoft's docs, it seems that the URL should actually be "#{base_azure_url}/#{tenant_id}/#{custom_policy}/oauth2/v2.0/authorize".

To Reproduce

Just check the file lib/omniauth/strategies/azure_activedirectory_v2.rb, where the method OmniAuth::Strategies::AzureActivedirectoryV2#client is defined as:

def client
  # ... (other not relevant code here) ...
  options.custom_policy =
    provider.respond_to?(:custom_policy) ? provider.custom_policy : nil

  options.client_options.authorize_url = "#{options.base_azure_url}/#{options.tenant_id}/oauth2/v2.0/authorize"
  options.client_options.token_url =
    if options.custom_policy
      "#{options.base_azure_url}/#{options.tenant_id}/#{options.custom_policy}/oauth2/v2.0/token"
    else
      "#{options.base_azure_url}/#{options.tenant_id}/oauth2/v2.0/token"
    end

  super
end

As you can see in the previous code snippet, the custom_policy is being included in the token URL, but not included in the authorize URL.

Expected behavior

If I did not miss anything in Microsoft's docs, I think the authorize URL should also include the custom policy: "#{base_azure_url}/#{tenant_id}/#{custom_policy}/oauth2/v2.0/authorize".

What do you think? Might I be right? If so, if you need me, I can create a PR. I haven't reviewed all the gem's code, but it looks like the patch would be easy:

def client
  # ...

  options.client_options.authorize_url =
    if options.custom_policy
      "#{options.base_azure_url}/#{options.tenant_id}/#{options.custom_policy}/oauth2/v2.0/authorize"
    else
      "#{options.base_azure_url}/#{options.tenant_id}/oauth2/v2.0/authorize"
    end

  # ...
end
pond commented 8 months ago

@jrodrigosm That documentation is for the B2C Azure, and while I don't understand the three hundred super confusing similar versions of Azure AD (now Entra), I can at least see that we're definitely not doing this:

https://{tenant}.b2clogin.com/{tenant}.onmicrosoft.com/...

...as per your reference at https://learn.microsoft.com/en-us/azure/active-directory-b2c/authorization-code-flow - we are as you point out using options.base_azure_url, which means by default we're doing this:

https://login.microsoftonline.com/...

...from what README.md links as https://learn.microsoft.com/en-us/azure/active-directory/develop/v2-oauth2-auth-code-flow - a totally different doc, though it looks very similar! Here, you will see at https://learn.microsoft.com/en-us/entra/identity-platform/v2-oauth2-auth-code-flow#request-an-authorization-code that there is no requirement for a policy nor is such a property documented.

What's interesting, though, is that no policy parameter is documented at the above reference at all for any of the documented endpoints. So we have:

I'm really not sure what to make of that!

Are you able to confirm that if you make the suggested amendment, OAuth to Azure AD (Entra) does work as expected and does use the custom policy specified? Or is your suggestion entirely hypothetical at the moment?

pond commented 5 months ago

@jrodrigosm Any thoughts about the above?

Jureamer commented 1 month ago

@pond I had the same experience as @jrodrigosm . When changing the base_url to B2C and specifying a custom_policy, it did not work as Jordan mentioned. If possible, could we adjust the authorize_url to accommodate the custom_policy when specifying a b2c custom policy and changing the base_url?

If needed, I can test this and submit a PR.

pond commented 1 month ago

@Jureamer For sure, a PR with tests would be really great, thanks - but it would be good if we could also actually get a URL for a Microsoft document explaining all this, so that we understand the basis of what we're doing, and that README.md can be updated with appropriately comprehensive instructions for users of the gem.