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

User profile website only cleaned, when DOB is set #3707

Closed
wants to merge 2 commits into from
Closed

User profile website only cleaned, when DOB is set #3707

wants to merge 2 commits into from

Conversation

yireo
Copy link

@yireo yireo commented Jun 2, 2014

Within the onUserBeforeSave() event, the user profile data are checked to make sure they are valid. The "website" value is converted into Puny-code, while the DOB (Date-Of-Birth) is converted from whatever-format to MySQL-format. However, the "website" value is only checked for, when the DOB value is set. The patch places the "website" check outside of the "dob" check, and into its own if-structure.

To test this, I've used a workaround to see whether the PHP-line itself was executed. I tried to enter some kind of non-Punycode URL that would be converted into proper Punycode but I have failed with this so far. If somebody else has suggestions for this (so: how to enter a non-Punycode URL for testing purpose) that would be awesome.

To test the code with my hack, just add the following line, right after the line mentioning the Puny-code:
$data['profile']['website'] = 'http://example.com';

Test without patch:

  • Enable the profile plugin so you are able to enter both website as DOB in your profile.
  • Hack the code as mentioned above
  • Enter no DOB, but do add a website URL (not "http://example.com").
  • The website is saved properly.
  • Repeat this, but now enter a DOB and a website URL.
  • The hack should change the URL to "http://example.com".

The test above should show you that the hack of setting the URL to "example.com" is only applied when the DOB is set, and it is not applied when the DOB is not set.

Test with patch:

  • Enable the profile plugin so you are able to enter both website as DOB in your profile.
  • Hack the code as mentioned above
  • Enter no DOB, but do add a website URL (not "http://example.com").
  • The hack should change the URL to "http://example.com".
  • Repeat this, but now enter a DOB and a website URL.
  • The hack should change the URL to "http://example.com".

The test with patch should result into the hack being applied all the time - so changing the URL to "example.com", regardless of whether the DOB is set or not.

The test can be run from either the backend (for any user profile) or the frontend (for only your own profile).

Within the onUserBeforeSave() event, the user profile data are checked to make sure they are valid. The "website" value is converted into Puny-code, while the DOB (Date-Of-Birth) is converted from whatever-format to MySQL-format. However, the "website" value is only checked for, when the DOB value is set. The patch places the "website" check outside of the "dob" check, and into its own if-structure.

To test this, I've used a workaround to see whether the PHP-line itself was executed. I tried to enter some kind of non-Punycode URL that would be converted into proper Punycode but I have failed with this so far. If somebody else has suggestions for this (so: how to enter a non-Punycode URL for testing purpose) that would be awesome.

To test the code with my hack, just add the following line, right after the line mentioning the Puny-code:
$data['profile']['website'] = 'http://example.com';

Test without patch:
* Enable the profile plugin so you are able to enter both website as DOB in your profile.
* Hack the code as mentioned above
* Enter no DOB, but do add a website URL (not "http://example.com").
* The website is saved properly.
* Repeat this, but now enter a DOB and a website URL.
* The hack should change the URL to "http://example.com".

The test above should show you that the hack of setting the URL to "example.com" is only applied when the DOB is set, and it is not applied when the DOB is not set.

Test with patch:
* Enable the profile plugin so you are able to enter both website as DOB in your profile.
* Hack the code as mentioned above
* Enter no DOB, but do add a website URL (not "http://example.com").
* The hack should change the URL to "http://example.com".
* Repeat this, but now enter a DOB and a website URL.
* The hack should change the URL to "http://example.com".

The test with patch should result into the hack being applied all the time - so changing the URL to "example.com", regardless of whether the DOB is set or not.

The test can be run from either the backend (for any user profile) or the frontend (for only your own profile).
@waader
Copy link
Contributor

waader commented Jan 14, 2015

Codewise I can follow what you describe here. But I think that no conversion is needed at all as the filter "url" is set for the field "website".

Testing with current staging I enter a IDN URL and no DOB and it is saved punycoded. The same is true when entering a DOB. Reading https://docs.joomla.org/URL_form_field_type suggests to me, that the filter is doing that.

Am I totally wrong?

@yireo
Copy link
Author

yireo commented Jan 15, 2015

Ok, if the filter "url" is set for the field "website", then actually the entire line with JStringPunycode::urlToPunycode should be skipped - it would just be code duplication and because the logic already makes no sense, it is best to clean things up. I hope to get to fix my PR soon.

Remove check for Puny URL entirely, because it is already being filtered through the "url" filter. Removed some unneeded white lines as well.
@yireo
Copy link
Author

yireo commented Jan 15, 2015

I've now updated the PR to remove the website check entirely because the filter "url" is already there.

@waader
Copy link
Contributor

waader commented Jan 15, 2015

@test work! Thanks yireo!

To test:

  • after applying the patch
  • activate the "user profil"-plugin
  • edit a user in the "user manager" and go to tab "user profile"
  • fill in an IDN-URL in field "Web site"; alternativly also add a DOB
  • check in the database table "_user_profiles", where colum "profil_key" = "profile website"
  • the contents of column "profile_value" should be punycoded

@webgras
Copy link
Contributor

webgras commented Jan 30, 2015

@test works for frontend and backend.

@brianteeman
Copy link
Contributor

Thanks for testing - setting RTC


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/3707.

@brianteeman brianteeman added the RTC This Pull Request is Ready To Commit label Jan 30, 2015
@roland-d roland-d modified the milestone: Joomla! 3.4.0 Feb 1, 2015
@roland-d
Copy link
Contributor

roland-d commented Feb 1, 2015

@yireo Can you please update your PR, it no longer applies when I want to commit it. Thanks.

@phproberto phproberto added this to the Joomla! 3.4.1 milestone Mar 15, 2015
@phproberto
Copy link
Contributor

Merged. Thanks!

@yireo
Copy link
Author

yireo commented Mar 16, 2015

Thanks :)

@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants