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

Implement auth0-forwarded-for header passing in Authentication Client #401

Merged
merged 5 commits into from
Aug 6, 2019
Merged

Implement auth0-forwarded-for header passing in Authentication Client #401

merged 5 commits into from
Aug 6, 2019

Conversation

kjarmicki
Copy link
Contributor

Changes

This PR implements auth0-forwarded-for header passing in the Authentication Client.
User-facing methods now accept an additional argument options, right before callback, for example:

client.passwordGrant(userData, {forwardFor: '1.2.3.4'}, callback);

This change is non-breaking. If there is no options object, the callback as a second argument will still work.

References

#191

Testing

  • This change adds unit test coverage

I have also npm linked the module to my existing project and it seemed to do it's job.

Checklist

@kjarmicki kjarmicki requested a review from a team July 26, 2019 12:49
@luisrudge
Copy link
Contributor

Does it make sense to use this as an option per method? Maybe it's better to use as a global config instead?

@luisrudge luisrudge requested review from luisrudge and removed request for a team July 26, 2019 13:39
@kjarmicki
Copy link
Contributor Author

kjarmicki commented Jul 26, 2019

It would be neat to use as a global config, but I don't think it's possible. The IP value has to be taken from individual requests.

@luisrudge
Copy link
Contributor

Why would it change on every request?

@kjarmicki
Copy link
Contributor Author

Because as I understand it, it's about forwarding individual client's IP to Auth0 backend in order to tie brute force attempts with particular IPs.
Reference: https://auth0.com/docs/api-auth/tutorials/using-resource-owner-password-from-server-side#configuring-the-auth0-application-to-receive-and-trust-the-ip-sent-by-your-server

@luisrudge
Copy link
Contributor

@kjarmicki ahh! Of course 😝

@damieng
Copy link
Contributor

damieng commented Jul 26, 2019

@luisrudge Let's make sure we have a cross-platform strategy/design for the other SDKs before we progress too far on this. Auth0.Net already has an issue open about providing similar functionality auth0/auth0.net#274

Whether you need to change IP per request really depends on whether you imagine the lifecycle of the client object being per user request or if you want to pool/hold it around to use it across multiple user requests. I don't think there is anything in it today that forces it to be used one way or another.

@luisrudge
Copy link
Contributor

@damieng I agree, that's a good point. But, considering node-auth0 doesn't hold any state, there's no hard requirement to create one instance per request, right?

@kjarmicki how do you use this in your app? do you create one Management instance per request or one per application?

@kjarmicki
Copy link
Contributor Author

kjarmicki commented Jul 26, 2019

@luisrudge we're initializing the client object once at the start of the application and reusing it throughout the life of the app. I think having a global initialization option would force the module users to re-initialize the client for each request while having it as a method option enables both use cases @damieng mentioned.

Personally, I think a global option way is a bit counterintuitive :) By definition auth0-forwarded-for header is tied to a particular request, so it's kind of like having username and password available as global options too.
I get that it seems like a good idea at first glance (I want to enable it for every request) and originally I wanted to use headers constructor option, but it fell apart when I realized that each time IP will be different.

@luisrudge
Copy link
Contributor

@kjarmicki perfect. Thanks for your feedback. I'll discuss this internally and get back to you.

@kjarmicki
Copy link
Contributor Author

@luisrudge howdy :) any news? If I can help in any way let me know.

@luisrudge
Copy link
Contributor

@kjarmicki we're having some internal discussions on how to tackle this across all of our SDKs. Sorry about the wait. I'll get back to you when I have more information.

@luisrudge
Copy link
Contributor

@kjarmicki did you test this with all the methods? I just contacted our protocols team and only two grant types do this: password and password-realm. Can you modify the PR to apply this header only in those methods? Thanks!

@kjarmicki
Copy link
Contributor Author

@luisrudge I did not, I assumed that all HTTP-based login methods support this header and checked the one we use (passwordGrant). I'll adjust my PR to affect only password and password-realm grant type methods.

@luisrudge
Copy link
Contributor

Thanks for the PR! 🎉

@luisrudge luisrudge merged commit 3cbfba1 into auth0:master Aug 6, 2019
@kjarmicki
Copy link
Contributor Author

Yay! Thank you for support and merge :)
When do you guys plan to release it to npm?

@luisrudge
Copy link
Contributor

Thursday!

@kjarmicki
Copy link
Contributor Author

Perfect 👌

@luisrudge
Copy link
Contributor

released in [email protected]

@cloudworkpro-derek
Copy link

This is not in types :( !

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.

4 participants