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

Encode auth0Client in login redirect URL #448

Closed
wants to merge 1 commit into from
Closed

Conversation

pnu
Copy link
Contributor

@pnu pnu commented Apr 24, 2018

Apply same encoding as in logout(). Not encoding will result this parameter to have value "Array".

Not encoding also outputs a PHP warning: Notice: Array to string conversion in …/WP_Auth0_LoginManager.php on line 151.

Outputting this notice to the output buffer causes another warning: Warning: Cannot modify header information - headers already sent ….

This means that the login does not work, unless the warning are suppressed (as usually is the case for production.) However, this is an issue for development environments when warnings are enabled.

@joshcanhelp
Copy link
Contributor

@pnu - Thanks so much for the contribution here!

I was actually just in the process of refactoring this a bit and found this issue as well. It's actually one more step to get this to work right as that static method returns the correct information but in the wrong format. You want the value for Auth0-Client array that's returned, which is already encoded.

You can see that in the branch I'm working on here (code is not reviewed or thoroughly tested quite yet):

https://github.com/auth0/wp-auth0/blob/change-auto-login-refactor/lib/WP_Auth0_LoginManager.php#L616

String concatenation of the Array creates a PHP warning
and results this parameter to have value "Array".
@pnu
Copy link
Contributor Author

pnu commented Apr 25, 2018

Great, thanks! I was also wondering if double-encoding is really what's wanted, but that was already done that way in logout() (same file), so I ended up copying it from there. I'm assuming both (login/logout) should use the same format for this parameter?

I'm not sure how soon your branch is landing, so in case you want to merge this PR before that, here are the corrected changes (now including the same change also for logout()).

This would be otherwise mostly harmless issue (except auth0 not getting correct stats), but in a development environment the emitted PHP warning (Notice: Array to string conversion …) stops the login, because one can't modify headers (ie. do the redirect) after outputting the body (ie. the warning notice).

@joshcanhelp joshcanhelp mentioned this pull request May 1, 2018
@joshcanhelp
Copy link
Contributor

joshcanhelp commented May 3, 2018

Fixed with #453 and #449 ... thanks @pnu!

@joshcanhelp joshcanhelp closed this May 3, 2018
@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.

2 participants