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

[ESD-5519] Fix issue where authz header was overridden in code exchange #86

Merged
merged 3 commits into from
Apr 16, 2020

Conversation

adamjmcgrath
Copy link
Contributor

@adamjmcgrath adamjmcgrath commented Apr 15, 2020

Description

Revert 82f1a0d which was overriding the Authz header and fix tests

Testing

I'll raise a separate story to come up with a way to regression test issues like these. Added a simple unit test

Checklist

  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@adamjmcgrath adamjmcgrath requested review from a team and joshcanhelp April 15, 2020 14:03
Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

So the difference is that 1.0.0 sets all the headers but this fix/0.8.1 just sets the initial headers which are added on to later?

Do you think there is any value in setting the auth method as POST by default? It is a bit confusing to expect those values in the body and see them in the header. Especially since the Application is set to POST token endpoint auth by default. The fact that the server will accept it in a header or POST, regardless of the setting, might be useful in this case because setting it to POST in the library would not be breaking.

@adamjmcgrath
Copy link
Contributor Author

adamjmcgrath commented Apr 16, 2020

So the difference is that 1.0.0 sets all the headers but this fix/0.8.1 just sets the initial headers which are added on to later?

1.0.0 overrides any header set by openid-client because I mistakingly replaced a deep merge with a shallow one.

This fix reverts that change, so headers from the SDK are merged with openid-client and both sets are used (including the Authorization header set by openid-client, whose omission was causing this bug)

It is a bit confusing to expect those values in the body and see them in the header.

Is it confusing because our API is documented to expect the credentials as POST params? https://auth0.com/docs/api/authentication#authorization-code-flow

Because the spec seems to prefer the header over the request body https://tools.ietf.org/html/rfc6749#section-2.3.1

Especially since the Application is set to POST token endpoint auth by default.

I thought the only method to use against the token endpoint is a POST?

test/client.tests.js Outdated Show resolved Hide resolved
test/client.tests.js Outdated Show resolved Hide resolved
@stevehobbsdev stevehobbsdev self-requested a review April 16, 2020 15:07
@joshcanhelp
Copy link
Contributor

Is it confusing because our API is documented to expect the credentials as POST params? https://auth0.com/docs/api/authentication#authorization-code-flow

Partially, yes. But the default Application Token Endpoint Authentication Method is "Post" which is described as "application uses HTTP POST parameters." It's confusing because the Application is (likely/default) set to accept the secret as a parameter but it's being sent in a header by the application and still being accepted. My worry is that this 1) complicates debugging and 2) may eventually fail if the server stops accepting it either way.

I don't think this mis-match issue should stop this PR from being merged, though. It might be good to clarify with the team that handles this endpoint whether we should be doing it one way or the other in SDKs.

Copy link
Contributor

@joshcanhelp joshcanhelp left a comment

Choose a reason for hiding this comment

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

Tested locally and works!

@adamjmcgrath adamjmcgrath merged commit e70153f into master Apr 16, 2020
@lbalmaceda lbalmaceda deleted the fix-headers-overwrite branch April 16, 2020 19:41
@panva
Copy link
Contributor

panva commented Apr 17, 2020

I think it was better when set directly on the client, just properly, setHttpOptionsDefaults is a singleton applied to all instances of openid-client in the code developers may have present elsewhere.

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.

4 participants