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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update username with value from OAuth provider #754

Merged
merged 2 commits into from Mar 9, 2020
Merged

Update username with value from OAuth provider #754

merged 2 commits into from Mar 9, 2020

Conversation

Binnette
Copy link
Collaborator

Here is a fix for the issue #302 (changing username in OSM do not sync in uMap).

I tracked this problem deep down the following issues:

This issue is related to the fact that in "Python Social Auth - Core", the field "username" is protected and can not be updated. So if you change your username in OSM, GitHub, BitBucket or Twitter, it will never be synced in uMap. 馃槥

The best solution is a fix in "Python Social Auth - Core", we should allow developers to ignore those "default protected fields", i will probably make a PR to social-core.

So i made a workaround in uMap to fix this issue and it is working nicely. To do so, iI defined my own pipeline and used it instead of the default pipeline.

This PR will fix following issues: #491, #302

@Binnette Binnette self-assigned this Feb 29, 2020
Copy link
Member

@yohanboniface yohanboniface left a comment

Choose a reason for hiding this comment

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

I'm ok to go this way, so we finally fix the username issue (that's seem to be annoying to OSM backend maintainers), but OAuth is very tricky and in overriding a social_auth stage we make each future upgrade breakable. This can ends with big trouble.
So I'd say we should make a PR to social_auth before trying to workaround on our side, see how social_auth maintainers react, if it's fast good, if there are discussions we merge on our side but try to follow upstream fix until it's merged.
What do you think?

umap/utils.py Outdated Show resolved Hide resolved
umap/utils.py Show resolved Hide resolved
@Binnette
Copy link
Collaborator Author

Binnette commented Mar 1, 2020

Hi @yohanboniface, I totally agree you. Before merging this PR, I will PR social_auth and see what happens.

It seems that social_auth releases are less frequents lately. So my workaround can be valuable. 馃惔

@Binnette
Copy link
Collaborator Author

Binnette commented Mar 1, 2020

So here is my PR python-social-auth/social-core#444 to social core. If i want to remove my workaround, I will need python-social-auth/social-core#348 to be merged too.

@yohanboniface yohanboniface merged commit 4000da1 into umap-project:master Mar 9, 2020
@Binnette Binnette deleted the sync-username-on-connect branch March 9, 2020 20:06
@yohanboniface
Copy link
Member

@Binnette seems that PRs have been merged upstream. Want to update on our side before the release ?

@Binnette
Copy link
Collaborator Author

I don't have time to do it now. I'm busy on an other project 馃槄 https://github.com/opencovid19-fr/data/

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

2 participants