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

case insensitive management for email address #24117

Merged
merged 29 commits into from Jan 8, 2020
Merged

case insensitive management for email address #24117

merged 29 commits into from Jan 8, 2020

Conversation

alikon
Copy link
Contributor

@alikon alikon commented Mar 7, 2019

Pull Request for Issue #24095 .

Summary of Changes

use lower() to check user email

Testing Instructions

see #24095

Expected result

test@test.fr and Test@test.fr must be detected as the same email address even on db's like Postgresql

Actual result

detected as the different email address even on db's like Postgresql

Note

mysql < 5.7 don't support function based index ... iirc

@mdaoudi123
Copy link

Thanks a lot for your help.

But There is also another file where exists a email comparaison:

  • components/com_users/models/reset.php => function processResetConfirm
  • components/com_users/models/remind.php => function processRemindRequest
  • ...
    and into other files..... (search for "$db->quoteName('email')")

Could you please add the correction to thoses files @alikon

@alikon alikon changed the title [com_user] - case insesitive check for email address case insensitive management for email address Mar 9, 2019
@mdaoudi123
Copy link

mdaoudi123 commented Mar 22, 2019

Can i download the new patch some where?
@alikon
Thanks

@alikon
Copy link
Contributor Author

alikon commented Mar 22, 2019

@mdaoudi123
Copy link

Hello,
Thanks for the patchTester,
I tested the patch and it's OK.
Could you merge this branch and build a new patch?

@alikon alikon marked this pull request as ready for review March 25, 2019 17:57
@alikon
Copy link
Contributor Author

alikon commented Mar 25, 2019

@mdaoudi123 please mark as tested on https://issues.joomla.org/tracker/joomla-cms/24117
this should be carefully tested on mysql side too....and mssql also i'm afraid

@mdaoudi123
Copy link

Hello,
im going to test it into POSTGRES too,
can you tell me how to mark this post as tested?
Thanks

@alikon
Copy link
Contributor Author

alikon commented Mar 26, 2019

Update updatenotification.php
@alikon
Copy link
Contributor Author

alikon commented Jul 21, 2019

thanks @richard67

@richard67
Copy link
Member

Drone failure is not related to this PR, is javascript stuff. PHPCS is ok now.

@richard67
Copy link
Member

@mdaoudi123 Sorry for the inconvenience and the long waiting time. Could you test this PR again with its latest changes?

@richard67
Copy link
Member

@alikon Could you synch this PR with staging by using that button in GitHub at the bottom of your PR?

@brianteeman
Copy link
Contributor

Unless there are conflicts there is no benefit

@richard67
Copy link
Member

Yes but looks less scary for beginner testers.

@richard67
Copy link
Member

richard67 commented Jul 21, 2019

I have tested this item ✅ successfully on a8c025b

Tested on MySQL and PostgreSQL.

On MySQL it does not break anything.

On PostgreSQL it checks now for present email address in case-insensitive way without touching existing user data.

Tested as follows:

Pre-conditions: In Global configuration, set error reporting to maximum or development and watch the PHP error logs during the tests. Check that there are no errors reported.

  1. Without this PR applied, have super admin email address be like e.g. "test@test.local".
  2. Create another user with email address be like e.g. "Test@test.local", i.e. same as 1st one but different case.
    Result: User is created.
  3. Apply this PR.
  4. Go to "Extensions -> Manage -> Database" and fix the database error to apply the schema update sql of this PR.
    Result: Database upt to date.
  5. Create another user with email address be like e.g. "tEst@test.local", i.e. same as 1st and 2nd one but different case.
    Result: User is not created because email address already exists.
  6. Edit the 2nd user but do not change email address and try to save.
    Result: User can't be saved because email address already exists.
  7. Exit user edit without saving.
  8. Work a bit in backend with user-related functions like privacy requests or other stuff related to files changed by this PR.
    Result: You can't add new users or change users so more than one user has same email address, case-independent, but the existing 2 users with same case-independent email addresses do not make problems.

@alikon Could you update your testing instructions to what I wrote here above so other users can test more easily?


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

@alikon alikon closed this Aug 3, 2019
@richard67
Copy link
Member

@alikon why closing this?

@richard67
Copy link
Member

@alikon Did you maybe accidently hit the wrong button, close instead of rebase?

@alikon alikon reopened this Aug 4, 2019
@richard67
Copy link
Member

I have tested this item ✅ successfully on 27d3519


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

@richard67
Copy link
Member

@mdaoudi123 Sorry for the inconvenience.

Could you test this PR again with its latest changes?

And then mark your test result at the issue tracker here https://issues.joomla.org/tracker/joomla-cms/24117, using the "Test this" button at the top left corner? Then just select the appropriate result and submitt.

@jwaisner
Copy link
Member

jwaisner commented Jan 7, 2020

I have tested this item ✅ successfully on 27d3519


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

@SharkyKZ
Copy link
Contributor

SharkyKZ commented Jan 7, 2020

RTC.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jan 7, 2020
@mdaoudi123
Copy link

Thank's a lot.

@mdaoudi123
Copy link

mdaoudi123 commented Jan 7, 2020

@alikon This correction will be available into the new joomla release?.

regards

@alikon
Copy link
Contributor Author

alikon commented Jan 7, 2020

it's up to the maintainers/release lead to decide if and when

@HLeithner
Copy link
Member

Can you please rename the sql file to 3.9.15 then I will merge this.

@alikon
Copy link
Contributor Author

alikon commented Jan 8, 2020

renamed

@HLeithner HLeithner merged commit 3a1b595 into joomla:staging Jan 8, 2020
@HLeithner
Copy link
Member

I'm not the biggest fan this PR but think it solves more problems then it creates.

Thanks

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 8, 2020
@HLeithner HLeithner added this to the Joomla! 3.9.15 milestone Jan 8, 2020
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

10 participants