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.

Sign up for GitHub

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

Jump to bottom

Get Apple user first & last name from self.data #483

Merged
merged 1 commit into from
Jan 13, 2021
Merged

Get Apple user first & last name from self.data #483

merged 1 commit into from
Jan 13, 2021

Conversation

yuekui
Copy link
Contributor

@yuekui yuekui commented Jun 22, 2020 •
edited
Loading

Proposed changes

Decoded id_token doesn't contain name info, so we have to get such info from self.data

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (PEP8, lint, formatting, renaming, etc)
  • Refactoring (no functional changes, no api changes)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build related changes (build process, tests runner, etc)
  • Other (please describe):

Checklist

  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

Other information

Solves #482 #501

@mbaechtold
Copy link

Thanks for tackling the issue #482, @yuekui 👍

@stale
Copy link

stale bot commented Aug 22, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added stale Stale issues (closing soon) and removed stale Stale issues (closing soon) labels Aug 22, 2020
@alexislg2
Copy link

Thank you. Is is planned to merge this PR?

@yuekui
Copy link
Contributor Author

yuekui commented Oct 8, 2020

Thank you. Is is planned to merge this PR?

Yeah, I hope @omab could help 🤝

mateuszmandera added a commit to mateuszmandera/zulip that referenced this pull request Oct 22, 2020
@mateuszmandera
auth: Expect name in request params in Apple auth.
247c014 
The name used to be included in the id_token, but this seems to have
been changed by Apple and now it's sent in the `user` request param.

python-social-auth/social-core#483 is the
upstream PR for this - but upstream is currently unmaintained, so we
have to monkey patch.

We also alter the tests to reflect this situation. Tests no longer put
the name in the id_token, but rather in the `user` request param in the
browser flow, just like it happens in reality.

An adaptation has to be made in the native flow - since the name won't
be included by Apple in the id_token anymore, the app, when POSTing
to the /complete/apple/ endpoint,
can (and should for better user experience)
add the `user` param formatted as json of {'firstName': 'John',
'lastName': 'Doe'} dict. This is also reflected by the change in the
native flow tests.
mateuszmandera added a commit to mateuszmandera/zulip that referenced this pull request Oct 22, 2020
@mateuszmandera
auth: Expect name in request params in Apple auth.
68a65cf 
The name used to be included in the id_token, but this seems to have
been changed by Apple and now it's sent in the `user` request param.

python-social-auth/social-core#483 is the
upstream PR for this - but upstream is currently unmaintained, so we
have to monkey patch.

We also alter the tests to reflect this situation. Tests no longer put
the name in the id_token, but rather in the `user` request param in the
browser flow, just like it happens in reality.

An adaptation has to be made in the native flow - since the name won't
be included by Apple in the id_token anymore, the app, when POSTing
to the /complete/apple/ endpoint,
can (and should for better user experience)
add the `user` param formatted as json of {'firstName': 'John',
'lastName': 'Doe'} dict. This is also reflected by the change in the
native flow tests.
@mateuszmandera mateuszmandera mentioned this pull request Oct 22, 2020
Merged
mateuszmandera added a commit to mateuszmandera/zulip that referenced this pull request Oct 22, 2020
@mateuszmandera
auth: Expect name in request params in Apple auth.
921b36e 
The name used to be included in the id_token, but this seems to have
been changed by Apple and now it's sent in the `user` request param.

python-social-auth/social-core#483 is the
upstream PR for this - but upstream is currently unmaintained, so we
have to monkey patch.

We also alter the tests to reflect this situation. Tests no longer put
the name in the id_token, but rather in the `user` request param in the
browser flow, just like it happens in reality.

An adaptation has to be made in the native flow - since the name won't
be included by Apple in the id_token anymore, the app, when POSTing
to the /complete/apple/ endpoint,
can (and should for better user experience)
add the `user` param formatted as json of
{"email": "[email protected]", "name": {"firstName": "Full", "lastName": "Name"}}
dict. This is also reflected by the change in the
native flow tests.
@timabbott
Copy link

@omab any chance you can review and merge this PR? Apple will reject any app submitted connecting to a server without this change (because they require importing the change), so Apple authentication is currently broken without this PR.

timabbott pushed a commit to zulip/zulip that referenced this pull request Oct 22, 2020
@mateuszmandera @timabbott
auth: Expect name in request params in Apple auth.
48f80fc 
The name used to be included in the id_token, but this seems to have
been changed by Apple and now it's sent in the `user` request param.

python-social-auth/social-core#483 is the
upstream PR for this - but upstream is currently unmaintained, so we
have to monkey patch.

We also alter the tests to reflect this situation. Tests no longer put
the name in the id_token, but rather in the `user` request param in the
browser flow, just like it happens in reality.

An adaptation has to be made in the native flow - since the name won't
be included by Apple in the id_token anymore, the app, when POSTing
to the /complete/apple/ endpoint,
can (and should for better user experience)
add the `user` param formatted as json of
{"email": "[email protected]", "name": {"firstName": "Full", "lastName": "Name"}}
dict. This is also reflected by the change in the
native flow tests.
AstonBraham pushed a commit to AstonBraham/zulip that referenced this pull request Nov 9, 2020
@mateuszmandera @AstonBraham
auth: Expect name in request params in Apple auth.
82bdb5b 
The name used to be included in the id_token, but this seems to have
been changed by Apple and now it's sent in the `user` request param.

python-social-auth/social-core#483 is the
upstream PR for this - but upstream is currently unmaintained, so we
have to monkey patch.

We also alter the tests to reflect this situation. Tests no longer put
the name in the id_token, but rather in the `user` request param in the
browser flow, just like it happens in reality.

An adaptation has to be made in the native flow - since the name won't
be included by Apple in the id_token anymore, the app, when POSTing
to the /complete/apple/ endpoint,
can (and should for better user experience)
add the `user` param formatted as json of
{"email": "[email protected]", "name": {"firstName": "Full", "lastName": "Name"}}
dict. This is also reflected by the change in the
native flow tests.
@stale
Copy link

stale bot commented Dec 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale issues (closing soon) label Dec 21, 2020
@mbaechtold
Copy link

mbaechtold commented Dec 22, 2020 •
edited
Loading

@yuekui, what could we do about the failing checks reported by the Travis CI integration?

@stale stale bot removed the stale Stale issues (closing soon) label Dec 22, 2020
@yuekui
Copy link
Contributor Author

yuekui commented Dec 22, 2020 •
edited
Loading

@yuekui, what could we do about the failing checks reported by the Travis CI integration?

#515 would fix the failed cases, we could simply wait for the PR being merged or just sync with that branch

@mbaechtold
Copy link

#515 has been merged. Could you please rebase onto the latest main branch, @yuekui ?

@yuekui
Get user first & last name from self.data
1a68beb 
Decoded `id_token` doesn't contain name info
@omab omab merged commit 521ecac into python-social-auth:master Jan 13, 2021
@mbaechtold
Copy link

Awesome! Thanks to everyone! 😃

@mateuszmandera
Copy link
Contributor

Thanks! When is the next release expected?

@yuekui
Copy link
Contributor Author

yuekui commented Feb 24, 2021

Hi @nijel any chance we could have a new release with this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yuekui @mbaechtold @alexislg2 @timabbott @mateuszmandera @omab