Skip to content

Compare lowercase email when updating from ldap - #33813

Merged
artonge merged 1 commit into
masterfrom
fix/infinite_email_change_notification
Sep 6, 2022
Merged

Compare lowercase email when updating from ldap#33813
artonge merged 1 commit into
masterfrom
fix/infinite_email_change_notification

Conversation

@artonge

@artongeartonge commented Sep 1, 2022•
edited
Loading

Copy link
Copy Markdown
Collaborator

I dug into it again, and the issue is much simpler than I previously though.

The fix is to compare the old email with the new lower case email before sending the event.

Scenario:
- Nextcloud has an email with capital letters for a user. No idea why, as it seems we are storing it in lowercases.
- LDAP has the same email address.
- The user log in, calling processAttributes, which call updateEmail, which compare emails as they are stored ($currentEmail !== $email)
- But when we are about to update the email, we once again check old and new values, but this time we lower case the new value

This means that Nextcloud will try to update the email again and again because we compare an address email with its lower case equivalent. Remember that Nextcloud has an email with capital letter in its database.
So the user will receive a continuous stream of email signaling an email change as we later compare email as they are before triggering the event.

@artongeartonge added bug 3. to review Waiting for reviews feature: ldap feature: emails php Pull requests that update Php code labels Sep 1, 2022
@artongeartonge added this to the Nextcloud 25 milestone Sep 1, 2022
@artongeartonge self-assigned this Sep 1, 2022
@artonge
artongeforce-pushed the fix/infinite_email_change_notification branch from 1115490 to b96c8feCompareSeptember 1, 2022 17:00
@blizzzblizzz mentioned this pull request Sep 1, 2022
Merged
@blizzz

blizzz commented Sep 2, 2022

Copy link
Copy Markdown
Member

If the email already is being received lowercase it should also stay lower case. I would not know why or where this change would happen. The fix will surely work, however I am not sure this is should be necessary, when the issue might be elsewhere. Could you reproduce this behaviour?

come-nc
come-nc reviewed Sep 5, 2022
View reviewed changes

@come-nccome-nc left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead fix setEMailAddress to correctly handle this usecase?
Either we support storing uppercase email and we should be able to update it. Or we do not and it should be ensured by the system.

Comment threadapps/user_ldap/lib/User/User.php
@blizzzblizzz mentioned this pull request Sep 6, 2022
Merged
@artonge
artongeforce-pushed the fix/infinite_email_change_notification branch from b96c8fe to e184d57CompareSeptember 6, 2022 08:58
@artonge

artonge commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

My first analysis was wrong, I update the first comment, please review again, and thanks for the feedbacks :)

@artonge

artonge commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable24

@artonge

artonge commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable23

@artonge

artonge commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

/backport to stable22

@artonge

artonge commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

CI failure look unrelated

@artonge

artonge commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

/rebase

@artonge

artonge commented Sep 6, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Just to be sure ^

@artonge
I dug into it again, and the issue is much simpler than I previously …
6c11944
…though.
- LDAP has an email address with capital letters
- NC store this address in lower case
- When the user logs in, we compare the [stored email with the new lower case email](https://github.com/nextcloud/server/blob/master/lib/private/AllConfig.php#L259-L261) before storing it. Here, both email will be the same, so we won't store the new email address with upper case letters. Which is what we want.
- We then [compare emails as they are before triggering an event](https://github.com/nextcloud/server/blob/master/lib/private/User/User.php#L202-L204), they won't match, so the user will receive an email signaling an email change every time he logs in.
The fix is to compare the old email with the new lower case email before sending the event.
Signed-off-by: Louis Chemineau <louis@chmn.me>
@nextcloud-command
nextcloud-commandforce-pushed the fix/infinite_email_change_notification branch from e184d57 to 6c11944CompareSeptember 6, 2022 13:18
@artonge
artonge merged commit 1f96717 into masterSep 6, 2022
@artonge
artonge deleted the fix/infinite_email_change_notification branch September 6, 2022 15:10
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Sep 6, 2022
Merged
This was referenced Sep 6, 2022
Merged
Merged
@ChristophWurstChristophWurst mentioned this pull request Sep 28, 2022
Closed
CarlSchwan added a commit that referenced this pull request Oct 17, 2022
@CarlSchwan
Fix case sensitivity of email when saving settings
ef97425
Otherwise we detect a email change all the time and since email are
immutable in ldap this prevent updating other fields.
Related: #33813
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwanCarlSchwan mentioned this pull request Oct 17, 2022
Merged
PVince81 pushed a commit that referenced this pull request Dec 16, 2022
@CarlSchwan@PVince81
Fix case sensitivity of email when saving settings
7f44a66
Otherwise we detect a email change all the time and since email are
immutable in ldap this prevent updating other fields.
Related: #33813
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloudBot pushed a commit that referenced this pull request Dec 16, 2022
@CarlSchwan@backportbot-nextcloud
Fix case sensitivity of email when saving settings
b813d05
Otherwise we detect a email change all the time and since email are
immutable in ldap this prevent updating other fields.
Related: #33813
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloudBot pushed a commit that referenced this pull request Dec 16, 2022
@CarlSchwan@backportbot-nextcloud
Fix case sensitivity of email when saving settings
ff6279e
Otherwise we detect a email change all the time and since email are
immutable in ldap this prevent updating other fields.
Related: #33813
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
backportbot-nextcloudBot pushed a commit that referenced this pull request Dec 16, 2022
@CarlSchwan@backportbot-nextcloud
Fix case sensitivity of email when saving settings
9738419
Otherwise we detect a email change all the time and since email are
immutable in ldap this prevent updating other fields.
Related: #33813
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
akhil1508 pushed a commit to e-foundation/server that referenced this pull request Dec 19, 2022
@CarlSchwan@akhil1508
Fix case sensitivity of email when saving settings
b399b8f
Otherwise we detect a email change all the time and since email are
immutable in ldap this prevent updating other fields.
Related: nextcloud#33813
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@timkrueger

timkrueger commented Dec 28, 2022

Copy link
Copy Markdown

The backports for 23 and 24 are still missing because they were reverted in the past:

@come-nc

come-nc commented Jan 2, 2023

Copy link
Copy Markdown
Contributor

/backport to stable23

@come-nc

come-nc commented Jan 2, 2023

Copy link
Copy Markdown
Contributor

/backport to stable24

@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Jan 2, 2023
Merged
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request Jan 2, 2023
Merged
s8321414 pushed a commit to MODAODF/odfweb that referenced this pull request May 4, 2023
@CarlSchwantony
Fix case sensitivity of email when saving settings
b777bc3
Otherwise we detect a email change all the time and since email are
immutable in ldap this prevent updating other fields.
Related: nextcloud/server#33813
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CarlSchwanCarlSchwanCarlSchwan approved these changes
@come-nccome-nccome-nc approved these changes
@blizzzblizzzAwaiting requested review from blizzz

Assignees

@artongeartonge

Labels

3. to reviewWaiting for reviewsbugfeature: emailsfeature: ldapphpPull requests that update Php code

Projects

None yet

Milestone

Nextcloud 25

Development

Successfully merging this pull request may close these issues.

5 participants

@artonge@blizzz@timkrueger@come-nc@CarlSchwan