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

Fix callback URL used with wordpress as headless CMS #342

Closed
wants to merge 2 commits into from
Closed

Fix callback URL used with wordpress as headless CMS #342

wants to merge 2 commits into from

Conversation

jeroneemou
Copy link

@jeroneemou jeroneemou changed the title - fix callback URL used with wordpress as headless CMS Fix callback URL used with wordpress as headless CMS Aug 23, 2017
@cocojoe
Copy link
Member

cocojoe commented Oct 25, 2017

Thanks for the PR. Out of interest what is the output of your home_url vs site_url ?
Didn't you have to change URLs in your Auth0 dashboard?

@cocojoe cocojoe changed the base branch from master to dev October 25, 2017 13:42
@glena
Copy link
Contributor

glena commented Oct 25, 2017

we actually had a similar issue a while ago, we did moved from site_url to home_url

here is some context #110

@jeroneemou
Copy link
Author

jeroneemou commented Oct 25, 2017

@cocojoe
test:
headless wordpress (site_url): https://cms-test.example.com
custom renderer (home_url): https://test.example.com
stage:
headless wordpress (site_url): https://cms-stage.example.com
custom renderer (home_url): https://stage.example.com
prod:
headless wordpress (site_url): https://cms.example.com
custom renderer (home_url): https://example.com

In short, if wordpress is not used to render content, auth won't work. Because callback /index.php?auth0=1&code=... goes to our custom renderer - which does not have a clue how to handle this - not on wordpress

@glena
Actually I think that what you did is wrong in general. It maybe fixed your issue with multiple wordpress instances but you cannot rely on home_url being wordpress instance - that is what this options has been built for.

@cocojoe
Copy link
Member

cocojoe commented Oct 25, 2017

@jeroneemou So I think to move forward here, this would need to be wrapped up as an optional feature flag that can be managed in the dashboard.
I also feel this options should provide information to ensure the user is aware that they will need to update their callback URL etc in the Auth0 dashboard to support the site_url.

Your PR may work for your use case. However, usage in the wild is unpredictable, hence wrapping it up as opt-in behaviour.

Thx
CC @glena

@jeroneemou
Copy link
Author

jeroneemou commented Oct 25, 2017

@cocojoe
In general site_url() references the field labeled "WordPress Address (URL)" and it should be used whenever you want to interact with wordpress - i.e. callback from auth0.
These functions are just named badly 😞 - more here

But yeah, like you said, wrapping it would be nice - also maybe include some event so it can be easily overwritten by another plugin for some other edge cases 👍

Cheers

@cocojoe
Copy link
Member

cocojoe commented Oct 25, 2017

Interestingly there is PR #339 that suggests using wp_login_url() which is a wrapper around site_url. It's confusing as then you have the wordpress docs

home_url() is the preferred method, as it avoids the above redirect. ¯_(ツ)_/¯

Hence I feel wrapper is the safest option to avoid potential legacy issues.

@jeroneemou
Copy link
Author

#339 is irrelevant as it deals with login URL not callback URL on /index.php or home_url( '/index.php?auth0=1' ) but replacing home_url( '/wp-login.php' ) with wp_login_url(..) is anyhow absolutely correct. And if you go in detail wp_login_url(...) uses exactly site_url('wp-login.php', 'login'); because it needs to land on wordpress instance!

ad home_url() it is preferred for use in templates (for resources) - as it avoids redirect
we are talking about recommendations for wordpress.COM specific usage - not .ORG as selfhosted solution

@jeroneemou
Copy link
Author

Heyo @cocojoe, what will be next? Are you working on something or should I dive into this - to have it short I would just focus on callback issue for OAuth with code payload and do this with some wrapper and option enable/disable in plugin itself

@cocojoe
Copy link
Member

cocojoe commented Oct 26, 2017

Hi @jeroneemou please feel free to dive in with what you mention. One thing ti possibly bear in mind here, there is a bigger PR dropping in soon. Now that I have read this, there is one area that maybe possibly be in conflict for you, although only in an SSO enable scenario.

https://github.com/auth0/wp-auth0/pull/350/files#diff-d375f757ba3b9628375037db61d19ae4

@jeroneemou
Copy link
Author

Hi @cocojoe, I'm sorry to ask so straight forward, but I will have not time for it currently. Do you think you could escalate this internally (Auth0) and get someone on board? I find this as crucial bug.
Thanks

@joshcanhelp
Copy link
Contributor

@jeroneemou - Again, apologies for the delay. We were caught up in updating to Lock 11 (high priority fix/improvement) and other issues were on pause.

So, in short, good catch here, you're right on with that change. I'm going to review the codebase now to figure out if this needs to be corrected anywhere else and we'll get this in the soonest release we can.

@cocojoe
Copy link
Member

cocojoe commented Jan 15, 2018

Closing as superseded in #360

@cocojoe cocojoe closed this Jan 15, 2018
@jeroneemou jeroneemou deleted the change-callback-redirect-uri branch January 15, 2018 21:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants