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

Allow configuration of outgoing request protocol #254

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

garry-jeromson
Copy link
Contributor

@garry-jeromson garry-jeromson commented Jan 28, 2021

Remove hardcoded HTTPS in URLs for outgoing requests to make the library more friendly to integration testing, where SSL is a pain.

Closes #253.

@garry-jeromson garry-jeromson requested a review from a team as a code owner January 28, 2021 22:37
@lbalmaceda
Copy link
Contributor

Hi @garry-jeromson, the changes look fine. However, I think this is one of those cases where the request module (or even our AuthenticationBase subclasses) could be patched to prevent this request from being executed at all returning a mock response, as shown here or even intercepting the URL value and if it starts with.. say "https://your-test-domain.auth0.com/" you could change it to "http://your-test-domain.auth0.com/" before calling the actual method. WDYT?

@lbalmaceda lbalmaceda added the waiting for customer This issue is waiting for a response from the issue or PR author label Feb 23, 2021
@garry-jeromson
Copy link
Contributor Author

garry-jeromson commented Feb 24, 2021

Howdy @lbalmaceda, thanks for looking them over!

Patching is always an option (indeed, we already do this on a unit-test level), but being able to set the protocol via an argument is much easier for the user. I do see your point, as real calls to Auth0 will always use HTTPS; I guess it depends on if the testing use case is common enough to make the SDK friendly towards it.

My two cents: there are many situations where an over-the-wire test double is preferable to mocking or patching, as it allows black-box testing of an app without fiddling in its insides. Indeed, fiddling in the insides of an app for such integration or CDC tests kind of defeats the very purpose of the tests, which is to test the app as-is (and often, without patching access to any Python code - test inputs are network requests from another process). Going down the patching road, we'd have to configure some kind of different "app mode" which enables or disables the mocks/patches. Parameterising the protocol used in the Auth0 calls (and then feeding it with an environment variable, which is easily changed) would allow for such testing without having to add extra test-only logic to our production code.

@garry-jeromson
Copy link
Contributor Author

Did you have a chance to consider this yet, @lbalmaceda? Would be really great to have the changes accepted, as it would prevent us (and potential future microservice-CDC-test-inclined types) from having to go down the fork road.

@lbalmaceda
Copy link
Contributor

@garry-jeromson yes, we discussed and while we would prefer you to patch this on your side, we also agree that there should be more testing flexibility and that more than often setting up SSL is a headache.

Can you please rebase the branch?

Remove hardcoded HTTPS in URLs for outgoing requests to make the library more friendly to integration testing, where SSL is a pain.

See auth0#253.
@lbalmaceda lbalmaceda merged commit 07e4b67 into auth0:master Mar 31, 2021
@lbalmaceda lbalmaceda removed the waiting for customer This issue is waiting for a response from the issue or PR author label Mar 31, 2021
@lbalmaceda lbalmaceda modified the milestones: v3-Next, 3.16.0 Apr 9, 2021
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.

Allow configuration of outgoing request protocol
2 participants