-
-
Notifications
You must be signed in to change notification settings - Fork 751
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 authentication-oauth regression using incorrect callback and redirect_uri #2631
Conversation
…rect_uri were using incorrect protocol and host from configuration Call to defaultsDeep parameters reordered to ensure protocol and host values from both both oauth.defaults and from local variable can be distinguished, with local values as fallback. Previous version was coding local values instead of oauth.default values. This matters if they are not the same, such as when using reverse proxy server. The local host value is from top host value of configuration file defualt.json.
I should let you know that I tried the following with oauth last, instead of second following
This is the suggested solution that works, but it should have an addition.
The following is better to ensure the transport from the configuration is used. I will add another commit to the PR.
|
…not overwritten by previous fix
Thank you for the pull request! When comparing it to v4 I'm thinking the order is just wrong which is what you changed but then we can keep the old host settings. I'll quickly update and it will get out with the new release tomorrow and you can let me know if that fixes it. |
Thanks for the commit. No, changing the order does not fix the problem with my config/default.json. My fix does fix the problem with my config. I thoroughly tested your and my version, to the extent of checking the compiled module js. We need to step back a bit because we may be at cross purposes given the way I configure feathers for authentication. I need to state how I configure and why I configure this way. These reasons, with a sample configuration, are on #2630, so you and others can comment on this if they wish. |
This reverts commit d8f3ddd.
No my bad, you were correct. I thought it would still fill in the correct settings. Will merge your original PR and put it out with the next release shortly. |
@johnheenan @daffl Opened PR #2659 to fix this. |
Fix authentication-oauth regression: authentication callback and redirect_uri were using incorrect protocol and host from configuration
Summary
Please see #2630
Call to defaultsDeep parameters reordered to ensure protocol and host values from both both oauth.defaults and from local variable can be distinguished, with local values as fallback.
Previous version was coding local values instead of oauth.default values. This matters if they are not the same, such as when using reverse proxy server. The local host value is from top host value of configuration file defualt.json.