Skip to content

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 16, 2022

Fix #18603 . Should we keep the old email address or delete it when sync user with new email from ldap?

@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. backport/v1.16 labels Feb 16, 2022
@lunny lunny added this to the 1.17.0 milestone Feb 16, 2022
@lafriks
Copy link
Member

lafriks commented Feb 16, 2022

If old email address source is LDAP, it should be deleted

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 16, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 16, 2022
@zeripath
Copy link
Contributor

CI failure is related

@lunny
Copy link
Member Author

lunny commented Feb 17, 2022

Details

We don't know that. LDAP user could add a new email address and change the primary email. So let it be and user could delete the old email manually.

@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@cb41f5c). Click here to learn what that means.
The diff coverage is 34.61%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #18786   +/-   ##
=======================================
  Coverage        ?   46.61%           
=======================================
  Files           ?      853           
  Lines           ?   122512           
  Branches        ?        0           
=======================================
  Hits            ?    57111           
  Misses          ?    58507           
  Partials        ?     6894           
Impacted Files Coverage Δ
services/auth/source/ldap/source_sync.go 37.17% <0.00%> (ø)
models/user/user.go 57.97% <37.50%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb41f5c...642fc31. Read the comment docs.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 24, 2022
@lunny lunny merged commit f1b1472 into go-gitea:main Feb 24, 2022
@lunny lunny deleted the lunny/fix_ldap_emails branch February 24, 2022 06:14
@lunny lunny added the backport/done All backports for this PR have been created label Feb 24, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 24, 2022
* giteaofficial/main:
  Fix ldap user sync missed email in email_address table (go-gitea#18786)
  Update assignees check to include any writing team and change org sidebar (go-gitea#18680)
  Set max text height to prevent overflow (go-gitea#18862)
  Lock gofumpt to v0.3.0 and run it (go-gitea#18866)
IsActivated: true,
IsPrimary: true,
})
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be if err != nil {

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

@lunny lunny mentioned this pull request Feb 24, 2022
lunny added a commit to lunny/gitea that referenced this pull request Feb 24, 2022
* Fix ldap user sync missed email in email_address table

* Fix test
6543 pushed a commit that referenced this pull request Feb 24, 2022
* Fix ldap user sync missed email in email_address table (#18786)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
* Fix ldap user sync missed email in email_address table

* Fix test
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent e-mail address sync from LDAP
6 participants