-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix google OpenID Connect #747
Conversation
And use jwkest to decode signature, because it supports more than just HS256.
And delay caching of discovery document until all parameters are set to avoid using cache when document interpretation failed.
Conflicts: social/tests/backends/open_id.py
'fullname': response.get('name'), | ||
'first_name': response.get('given_name'), | ||
'last_name': response.get('family_name'), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be closer to
values = {'username': '', 'email': '', 'fullname': '',
'first_name': '', 'last_name': ''}
fullname = values.get('name') or ''
first_name = values.get('given_name') or ''
last_name = values.get('family_name') or ''
email = values.get('email') or ''
if not fullname and first_name and last_name:
fullname = first_name + ' ' + last_name
elif fullname:
try:
first_name, last_name = fullname.rsplit(' ', 1)
except ValueError:
last_name = fullname
username_key = self.setting('USERNAME_KEY') or self.USERNAME_KEY
values.update({'fullname': fullname, 'first_name': first_name,
'last_name': last_name,
'username': values.get(username_key) or
(first_name.title() + last_name.title()),
'email': email})
return values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate your suggestion, but these fields have been standardized for OIDC in http://openid.net/specs/openid-connect-core-1_0.html#rfc.section.5.1. I therefore would prefer to use these as a sensible default. I guess most users would override the OpenIdConnectAuth to customize this for the actual values the backend provides (if the backend doesn't adhere to the RFC).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly talking about username_key = self.setting('USERNAME_KEY') or self.USERNAME_KEY
Which lets users override the username in their own settings file and exists in other backends.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the USERNAME_KEY setting.
Left a few comments that I could see on first glance. This is what I need to start to flesh out part 2 of #750 so hopefully it gets merged in. |
This makes the backend more resilient to temporarily unavailable resources.
Thanks for your suggestions, @nickcatal! I've added some fixes. I hope it gets merged in too! |
Any reason this hasn't been merged yet? Have been working on something similar lately and could lend a hand getting this merged if needed. |
1078c07
to
de9f179
Compare
I've ported this PR into social-core. Thanks! |
This PR fixes logins with Google OpenID Connect. It merges #687 and #693.