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

Changes in URL branching #31

Merged
merged 1 commit into from
Oct 16, 2024
Merged

Changes in URL branching #31

merged 1 commit into from
Oct 16, 2024

Conversation

Jureamer
Copy link
Contributor

@Jureamer Jureamer commented Aug 2, 2024

  • when both a custom policy and tenant name are present, handle the branching for assigning the token URL and authorize URL.

- when both a custom policy and tenant name are present, handle the branching for assigning the token URL and authorize URL.
@Jureamer
Copy link
Contributor Author

Jureamer commented Aug 2, 2024

I have updated the code to support B2C login.
You can refer to the related documentation here: Request a Token.

While the existing README includes examples and references related to the custom_policy environment variable,
the implementation in the code was not fully accurate.

To support the URL change, the tenant_name environment variable is now also required.

It seems that the README may need to be updated accordingly. If you would like, I can make additional modifications to the documentation.

@pond
Copy link
Member

pond commented Sep 5, 2024

@Jureamer

Sorry for the delay here. Just as I was getting around to this (a month late, sigh) some very valid points came up in #33 and I was thinking that this might all get folded into a V3.

OTOH, you might not want to take that leap, and might prefer this to go into V2 ASAP.

Any preference?

On the PR itself - code and test coverage all looks good and I can add a bit into README.md myself, or if you wanna throw it into this PR, be my guest. Either way, once the above is answered, I'd say we're good to merge.

@pond pond merged commit 5b480b2 into RIPAGlobal:master Oct 16, 2024
@pond
Copy link
Member

pond commented Oct 16, 2024

This will go into V3 under the rename omniauth-entra-id - to be released soon, I hope

@pond
Copy link
Member

pond commented Oct 16, 2024

Thanks, merged. I think I see an error where if custom provider is given but tenant name isn't, it used to build an appropriate URL but your updated logic does not. Changed to:

EDIT - reverted - originally I said this:

base_url = if options.custom_policy && options.tenant_name
  "https://#{options.tenant_name}.b2clogin.com/#{options.tenant_name}.onmicrosoft.com/#{options.custom_policy}"
elsif options.custom_policy
  "#{options.base_azure_url}/#{options.tenant_id}/#{options.custom_policy}/#{oauth2}/token"
else
  "#{options.base_url}/#{options.tenant_id}"
end

EDIT: however I've read and re-read docs and searched extensively; it seems custom policies only are documented for the still-not-renamed Azure Active Directory B2C stuff, where a tenant name is also required. Entra doesn't seem to have that concept. Presumably, custom policy names wouldn't have worked if given in that use case, but wouldn't have been given anyway because they wouldn't have worked.

@Jureamer
Copy link
Contributor Author

Jureamer commented Nov 6, 2024

@pond
Sorry for the delayed response. First, thank you for reviewing and merging my work. If I have any further questions while using it, I'll raise them in the issue tracker. Thank you again.

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