-
Notifications
You must be signed in to change notification settings - Fork 389
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
Fix statuscode of backchannel logout response #1692
Conversation
Thank you for the PR and trying to improve the SDK. I think this is a breaking change, and 204 is actually not entirely against the spec (even though 200 might be more correct). From the spec:
Personally, I'd argue 204 makes more sense (as 204 exists exactly for the no content purpose), but 200 might be more in-line with the exact specification. Based on that I would vote against merging this now, but consider if this is something we would like to change or not in a future major version. |
I see your point. And yes, 204 would be the correct status code in terms of http. The OIDC specification on the other hand uses the key word "MUST" and not "SHOULD". Therefore status code 200 should be the one we return. To make the change non-breaking we could introduce Do you consider the removal of the |
Given
I do not disagree, but this is an Auth0-specific SDK. Not an OIDC compliant SDK (but yes, we do try to follow the spec as much as possible).
In this case, I see no direct issue with using 204, as the spec clearly says that everyone should expect the status code to be 200 or 204. Therefore I think we should be fine to stick to 204 until we get to a new major and there is no need to add additional complexity to make the status code configurable. Unless there is some actual problem with our current version that needs to be addresses, that would change the conversation. Is there? |
We actually use the SDK with a different provider, which (contrary to the specification) expects the status code 200. By the way, we have already fixed the problem on our side with a workaround: // src/app/api/auth/logout/backchannel/route.ts
import { AppRouteHandlerFnContext } from '@auth0/nextjs-auth0';
import { NextRequest } from 'next/server';
import { getAuth0Instance } from '../../../../utils/auth0/node';
export async function POST(
request: NextRequest,
context: AppRouteHandlerFnContext,
): Promise<Response> {
const response = await getAuth0Instance().handleBackchannelLogout(request, context);
// Workaround to return 200 as status code
if (response.status === 204) {
return new Response(response.body, {
status: 200,
statusText: response.statusText,
headers: response.headers,
});
}
return response;
} |
This is not a use-case we support. This SDK is specifically for Auth0 and is not intended to be used with other providers nor is it intended to be OIDC compliant to work with other IDPs. It may or may not work, there is no guarentee for that. But we do have internals specifically for Auth0.
Yeah they should allow 204.
That is appreciated a lot, thank you for that 🙏 . But as said, this SDK isn't intended to be used with other providers in its current state, and we do not want to add additional complexity to support another provider (who arguably is less in line with the spec than this SDK is, as it should allow 204). Closing this, as I believe this isn't a change we can merge anyway due to the breaking change, nor do we think this is something we want to enable in an opt-in manner for the time being. But we may look into improving this in a next major version, as we do try and follow the specification where possible. |
📋 Changes
The handler for backchannel logout requests previously responded with status code
204
although the specification requires status code200
.The internal function
res.send204()
was used. Since the handler for backchannel logout requests was the only place in the code that used this function, it was removed and replaced bysendEmpty200
.📎 References
OpenID Connect Back-Channel Logout 1.0
🎯 Testing
Test library is changed accordingly.