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

Add custom headers to social token request [SDK-2080] #351

Merged
merged 6 commits into from
Dec 15, 2020

Conversation

TLFilip
Copy link
Contributor

@TLFilip TLFilip commented Sep 22, 2020

Changes

For security reasons we need an option to add custom headers for token request. Such a possibility exists for email/password authentication and refresh token. This PR enables it for social login by adding WebAuthProvider.Builder method withHeaders.

Testing

To test it inspect the network traffic and notice custom headers in token request when executing OAuth2 flow.

  • This change adds unit test coverage

  • This change adds integration test coverage

  • This change has been tested on the latest version of the platform/language or why not

Checklist

@TLFilip TLFilip requested a review from a team September 22, 2020 12:41
@TLFilip TLFilip changed the title Add pkce cusom headers Add custom headers Sep 22, 2020
@TLFilip TLFilip changed the title Add custom headers Add custom headers to social token request Sep 22, 2020
@TLFilip
Copy link
Contributor Author

TLFilip commented Oct 1, 2020

Hey @lbalmaceda let me know if there’s a chance to merge that PR and what is required to do so. Thanks!

@danielphillips
Copy link

@cocojoe Hi Martin, as discussed a while ago, could you look into this PR please?

@lbalmaceda lbalmaceda changed the title Add custom headers to social token request Add custom headers to social token request [SDK-2080] Oct 19, 2020
@lbalmaceda
Copy link
Contributor

Apologies for missing this one. I might be able to take a look next week. I'm tracking this internally under SDK-2080.

@lbalmaceda lbalmaceda added the needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue label Oct 19, 2020
Copy link
Contributor

@lbalmaceda lbalmaceda left a comment

Choose a reason for hiding this comment

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

Hey @TLFilip, sorry for the time it took us to review this. We're currently working on the next major and making some improvements (this included). We hope to deliver that soon.

That said, I think makes sense to have this in the current major before that new one is shipped. The PR looks good. I've left a change request to refactor a bit of the methods, especially given that most of the classes there are package private.

@@ -320,6 +327,14 @@ private void addPKCEParameters(Map<String, String> parameters, String redirectUr
}
}

private void addPKCEHeaders(@NonNull Map<String, String> httpHeaders) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd merge this method into addPKCEParameters. Please, make the createPKCE() method take the headers in addition to the redirectUri. That way they are passed in the construction of the PKCE instance. That would as well remove the setter (pkce.setHeaders)

@lbalmaceda lbalmaceda added waiting for customer This issue is waiting for a response from the issue or PR author and removed needs investigation An issue that has more questions to answer or otherwise needs work to fully understand the issue labels Dec 15, 2020
@TLFilip TLFilip requested a review from a team as a code owner December 15, 2020 11:48
@lbalmaceda lbalmaceda merged commit 00360e3 into auth0:master Dec 15, 2020
@lbalmaceda
Copy link
Contributor

Thanks for the changes @TLFilip

@lbalmaceda lbalmaceda added this to the v1-Next milestone Dec 15, 2020
@lbalmaceda lbalmaceda added CH: Added and removed waiting for customer This issue is waiting for a response from the issue or PR author labels Dec 15, 2020
@lbalmaceda lbalmaceda modified the milestones: v1-Next, 1.30.0 Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants