Open JibbityJobbity opened 9 months ago
Clodged something together, see PR
Thanks for the PR! Left a comment on it. Since this is an upstream non-compliance issue, I feel this is not a bug on us, but something we now unfortunately have to find a workaround for.
It doesn't matter where this bug originates; previous versions of next-auth were offering this feature, and the whole point of this library is so that developers don't have to deal with exactly these kinds of issues. So if the maintainers see this as out of scope, it seems like all of us who have a requirement to use AzureAD provider with common tenant will have to find an alternative solution.
Is there a way to help ensure that this PR makes it in soon?
Is there a way to help ensure that this PR makes it in soon?
Review, feedback or consolidating solutions around the other issues/PRs is appreciated. I feel somewhat out of my comfort zone here :)
That being said, maybe the issue needs to stay open until Microsoft gets their stuff sorted? Then we can get rid of this hack. I've seen LinkedIn having a similar issue and they seem to be in contact with LinkedIn, but AFAIK nobody has bothered Microsoft about this.
That being said, maybe the issue needs to stay open until Microsoft gets their stuff sorted?
They will most certainly not ;)
JFYI: I guess I have the same problem, but with the azure-ad-b2c provider. :(
@balazsorban44 Why isn't the fix for this being merged? Is this library still being maintained, the fix PR has been up for over 2 months. This is a pretty big whole in v5 and stopping me from upgrading. I know there are workarounds, but if fixes like this don't get attention, I am loosing confidence in this library.
Feel free to lose confidence or find an alternative, no hard feelings. ππ The project is definitely being maintained, but this is my (our) side project and you or your company isn't paying me/us to prioritize this issue. OSS !==
free work.
FWIW, Azure AD is being replaced by Microsoft Entra ID, which we recently added support for. Will be out in the new release.
Also, this is technically an issue at Microsoft, not a bug in Auth.js. I understand the frustration though, just saying that the critique is knocking on the wrong door in the first place.
I do appreciate that this is open source and there is no expectation to get free work. It's just confusing when PRs with fixes for a big regression in the new version don't get any attention, since that's the only way to engage with open source projects. Yes the issue has to do with Microsoft's none standard approach, but who cares, the point of this library is to abstract exactly those details away and that is what it did in the previous version, so this issue blocks many from upgrading.
The PR https://github.com/nextauthjs/next-auth/pull/9718 tried to address the concern you mentioned in your review 2 months ago and it looks pretty minimally invasive, so it's unclear why it can't be merged. Maybe there's another issue with it and some of us could help address it, but we need a maintainer to give another review, otherwise there's no way to contribute to this open source project.
It would be very much appreciated if someone could take a look and see if the PR is good to go or needs further changes.
Just checked with the new Entra Id provider. The issue seems to be the same there.
@balazsorban44 what would you suggest as a solution to this problem? I see a couple of possibilities:
I really love the library and the comfort it gives and this is the only hurdle that I`ve encountered (Yes I know this is not an error on your end but rather on Microsofts end).
@balazsorban44 Entra ID is not a replacement for AD, it's just a rename. Don't take my word for it though, this is in your own documentation (https://authjs.dev/getting-started/providers/microsoft-entra-id) :)
The project is definitely being maintained, but this is my (our) side project and you or your company isn't paying me/us to prioritize this issue. OSS !== free work.
But you/your company is also not paying the community to debug/fix issues, right? So are you saying nobody should be contributing to authjs, because OSS !== free work?
@jon-snowman
But you/your company is also not paying the community to debug/fix issues, right?
I honestly hope that after re-reading your own statement, you will see how far-fetched it sounds. Are you suggesting that I am supposed to pay the Auth.js community to help debug/fix their own issues?
I have neither a company, nor personal gain from fixing this or any other issue. The people complaining here on the other hand, do. They also get paid to work on this, by their employer. I don't. This is my hobby project. It is provided as-is, as stated in our LICENSE.
If our free customer support is not adequate, there are alternatives, like patch-package
, or even other auth libraries.
Anyway... I found motivation/free time to work on this, and will have another look now.
Provider type
Azure Active Directory
Environment
System: OS: macOS 14.2.1 CPU: (12) arm64 Apple M2 Max Memory: 65.83 MB / 32.00 GB Shell: 5.9 - /bin/zsh Binaries: Node: 21.2.0 - ~/.nvm/versions/node/v21.2.0/bin/node npm: 10.2.3 - ~/.nvm/versions/node/v21.2.0/bin/npm pnpm: 8.12.0 - ~/Library/pnpm/pnpm bun: 1.0.14 - ~/.bun/bin/bun Browsers: Brave Browser: 120.1.61.109 Safari: 17.2.1 npmPackages: @auth/core: ^0.18.6 => 0.18.6 @auth/sveltekit: ^0.4.0 => 0.4.0
Reproduction URL
https://github.com/JibbityJobbity/next-auth-example/
Describe the issue
Azure's implementation of OAuth requires manipulating/replacing parts of the URL based on your tenant ID which you are using. Next-auth's codebase frequently has the returned authentication/other endpoints checked and raises an error otherwise. This happens on any app with the common tenant ID, meaning just a generic Microsoft login like you would expect with just about any other OAuth provider. But you can have a look at how I'm using it in my project.
There are two checks where I've noticed that it's an issue.
The location the first check fails is in
getAuthorizationUrl
in@auth/core/lib/actions/signin/authorization-url.js
, where we have this checko.processDiscoveryResponse
expect the response url to behttps://login.microsoftonline.com/common/v2.0
when what we get back in actuality ishttps://login.microsoftonline.com/{tenantid}/v2.0
. Microsoft expects users to replace{tenantid}
withcommon
, or whatever other tenant ID that's supplied when the AzureAD provider is set up. For this, @panva has kindly provided a code snippet:The second one is in
handleOauth
in whereo.processAuthorizationCodeOpenIDResponse
gets called. The way it eventually makes its way down will get the user's specific "issuer" tenant in the returned url. We tell the check to expect 'https://login.microsoftonline.com/common/v2.0' when the returned URL in the oauth4web library will get the same URL from Microsoft, but havecommon
replaced with some tenant ID, I assume it's the user's.I'd make a PR but I have no clue how to fit a solution in that's compatible with the project's architecture.
How to reproduce
Use the AzureAD provider with the "common" tenant ID and log in.
Expected behavior
Login to work with Microsoft's special endpoints with the tenant IDs that it expects. The checks drill themselves all the way down to oauth4web, but we kinda tell it to check for the wrong thing.