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.

Sign up for GitHub

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

Jump to bottom

fix(providers): Handle Azure AD tenants correctly #9718

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(providers): Handle Azure AD tenants correctly #9718

wants to merge 1 commit into from
+65 −15

Conversation

JibbityJobbity
Copy link

@JibbityJobbity JibbityJobbity commented Jan 23, 2024 •
edited

☕️ Reasoning

Endpoints returned by Azure AD want us to edit the path so that each request gets routed to their proper tenant IDs.
The old implementation didn't handle this properly when using the "common" tenant ID, which basically lets us use it normally like any other provider. We would receive the template endpoint paths and use those in error checking and stuff rather than the ones we actually use.

I've just thrown something up and I've done some quick testing. It seems to work normally. I'll test custom tenants at some point and I suspect it won't work until I test and fix it.

🧢 Checklist

  • Documentation
  • Tests
  • Ready to be merged

🎫 Affected issues

Fixes: #9635

📌 Resources

@vercel Vercel
Copy link

vercel bot commented Jan 23, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
auth-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 2, 2024 11:47pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-auth-docs ⬜️ Ignored (Inspect) Visit Preview Mar 2, 2024 11:47pm

@github-actions github-actions bot added the core Refers to `@auth/core` label Jan 23, 2024
@JibbityJobbity JibbityJobbity changed the title fix: Handle Azure AD tenants correctly fix(providers): Handle Azure AD tenants correctly Jan 23, 2024
@JibbityJobbity
Copy link
Author

Testing needed

  • Common tenant group
  • organizations and consumers tenant groups
  • Any other specific tenant ID
  • Other providers still work

balazsorban44
balazsorban44 requested changes Jan 25, 2024
View reviewed changes
Copy link
Member

@balazsorban44 balazsorban44 left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any non-compliance needs to be handled at the provider level. we don't want branching based on specific providers, it's not maintainable in the long term

This will need a different solution. Something similar to the conform method on token.

some useful info at the end of the thread here #8831

@balazsorban44 balazsorban44 mentioned this pull request Jan 25, 2024
Closed
@panva
Copy link
Contributor

panva commented Jan 25, 2024 •
edited

I think there should be two distinct providers

One where you know the tenant you're working with and you're NOT using any of the "common" issuers (the existing one with the exception that tenant id is required).

And then another one which can be used with one of the "common" issuers

  'https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration'
  'https://login.microsoftonline.com/organizations/v2.0/.well-known/openid-configuration'
  'https://login.microsoftonline.com/consumers/v2.0/.well-known/openid-configuration'

This one has the workarounds in and requires explicit configuration to

  • allow login from all tenants
  • allow login from specific tenants (by providing a list of tenant ids), the ID Token's tid would be checked to match the expected tenants.

@panva panva mentioned this pull request Jan 25, 2024
Closed
3 tasks
@JibbityJobbity
Copy link
Author

While we all know the real solution to this, I think by the time I have a crack at this, we will know what ends up happening. But that's definitely considerable if conform() doesn't end up happening.
However in the meantime, I think the conform() method is the first way forward with this to try. It provides a way for us to sort out LinkedIn too, but I'd be careful about getting too comfortable with hacking around providers as I can imagine conform() would make it easier to do exactly that.

@JibbityJobbity JibbityJobbity force-pushed the azure-ad-common-tenant branch 2 times, most recently from e4ffecd to cfaf8da Compare January 27, 2024 11:21
@JibbityJobbity
Copy link
Author

I've done what I can for now. Completely open to suggestions and feedback.

I know this doesn't quite match the sample, but I wanted the check to work the other way around. Rather than having it try to check against the "fake" URL with {tenantid}, I thought it would be cleaner to perform a real check.

@vercel Vercel
Copy link

vercel bot commented Feb 3, 2024

@JibbityJobbity is attempting to deploy a commit to the authjs Team on Vercel.

A member of the Team first needs to authorize it.

@JibbityJobbity JibbityJobbity force-pushed the azure-ad-common-tenant branch 2 times, most recently from b8e41d1 to db8b784 Compare February 3, 2024 01:34
JibbityJobbity
JibbityJobbity commented Feb 3, 2024
View reviewed changes
packages/core/src/providers/azure-ad.ts Outdated

return {
...authServer,
issuer: authServer.issuer.replace("common", tenantId),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't right. Need to detect the tenant that's being used. Probably just replace the provider's tenant ID with whatever we expect from the JWT.

@panva
Copy link
Contributor

panva commented Feb 3, 2024

I'm not finding all the individual bits from my original recommendation nor the recommended split from #9718 (comment) so I'm assuming you're going with your own workarounds.

@JibbityJobbity
Copy link
Author

JibbityJobbity commented Feb 3, 2024 •
edited

I'm not finding all the individual bits from my original recommendation nor the recommended split from #9718 (comment) so I'm assuming you're going with your own workarounds.

It's a mix between what you suggested in the other issue and the conform methods. It still achieves the same thing, which is satisfying those underlying checks in your library while bringing the conformity to the provider level, as suggested by @balazsorban44. If this isn't good enough then I'll change the PR to satisfy the solution you gave earlier. The special serverConform is a bit icky so I see where you're coming from.

Anyway, I just did some testing with the other grouped tenants. The issuer URL comes back with some weird tenant ID instead of getting the user to replace "{tenantid}" or some other value we can expect. Replacing "{tenantid}" only seems to work with "common" and doesn't work with "organizations" or "consumers". This keeps rearing its ugly head, you almost need something to the effect of this to override the check to bypass it:

    const discoveryResponse: Response | undefined = await o.discoveryRequest(issuer)
    let json = await discoveryResponse.json();
    const as = await o.processDiscoveryResponse(new URL(json.issuer), discoveryResponse!)

I'll need another go at this. I'll shoot for tomorrow.

@JibbityJobbity
Copy link
Author

This seems to work nicely :) I'm largely happy with the current solution now.

@JibbityJobbity
fix(providers): Handle Azure AD tenants correctly
ab0a2a5 
The way Microsoft integrates their tenant system will change the underlying
endpoints underneath the oauth/openid ones it provides. This works around
the non-conformity and makes it functional.
@JibbityJobbity
Copy link
Author

Bump, rebased.

@amplicity
Copy link

hey! great work here, we could really use this. would love to see this get merged soon.

@MrLoh
Copy link

MrLoh commented Mar 12, 2024

It would be great to see this get merged. I'm sure it blocks a lot of people from adopting v5.

jo-sm added a commit to clearyai/next-auth that referenced this pull request Mar 13, 2024
@jo-sm
Fixes login for Azure users
13944a6 
Original PR on next-auth: nextauthjs#9718

Patched waiting for this to be released upstream
@jaysin586
Copy link

Is there an update on this?

@ladparth
Copy link

ladparth commented Apr 8, 2024 •
edited

Hello,
Thank you for working on this PR. I am also stuck on this stage of my development. Is there any update on this fix or is there any temporary fix i can do while this PR is in process?

@JibbityJobbity
Copy link
Author

Hello, Thank you for working on this PR. I am also stuck on this stage of my development. Is there any update on this fix or is there any temporary fix i can do while this PR is in process?

I'll rebase, but I haven't heard anything from any of the maintainers since I got it working.
Do keep in mind they may be busy with their lives, but there are some other hacks to make it work by configuring the provider itself. This PR would just mean that nobody would have to be concerned about this again.

@ladparth
Copy link

ladparth commented Apr 8, 2024

Thank you @JibbityJobbity

@MrLoh MrLoh mentioned this pull request Apr 19, 2024
Open
@murilobd
Copy link

murilobd commented May 9, 2024

Thanks @JibbityJobbity for that PR! Hopefully maintainers of this repo will merge it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@panva panva panva left review comments

@ThangHuuVu ThangHuuVu Awaiting requested review from ThangHuuVu ThangHuuVu is a code owner

@balazsorban44 balazsorban44 Awaiting requested review from balazsorban44

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
core Refers to `@auth/core` providers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AzureAD common tenant URLs aren't being checked properly
8 participants
@JibbityJobbity @panva @amplicity @MrLoh @jaysin586 @ladparth @murilobd @balazsorban44