-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
sources/oauth/twitter: update fields, scopes and profile URL to match current X API #18670
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for authentik-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for authentik-integrations ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for authentik-docs canceled.
|
sayah-y
left a comment
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.
Thanks
| authorization_url = "https://twitter.com/i/oauth2/authorize" | ||
| access_token_url = "https://api.twitter.com/2/oauth2/token" # nosec | ||
| profile_url = "https://api.twitter.com/2/users/me" | ||
| profile_url = "https://api.twitter.com/2/users/me?user.fields=verified,username,name,profile_image_url,confirmed_email" |
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.
According to this doc, fields id, name and username are marked as default (always included).
I think it's better to specify them not to depends on the X Api changes. But in this case, id is missing.
It will give:
profile_url = "https://api.twitter.com/2/users/me?user.fields=id,name,username,confirmed_email,profile_image_url,verified"
| data = info.get("data", {}) | ||
| email = ( | ||
| data.get("confirmed_email") | ||
| or data.get("email") |
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 think we can delete this line because this doc tells thats this field is named confirmed_email, no need to check for an alternative name.
| """Twitter OAuth2 Redirect""" | ||
|
|
||
| def get_additional_parameters(self, source): # pragma: no cover | ||
| scopes = ["users.read", "tweet.read"] |
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.
According to this doc, to get the email, we must add the following scope: users.email
…d, clean email parsing
|
#18670 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #18670 +/- ##
==========================================
- Coverage 93.21% 93.09% -0.12%
==========================================
Files 933 929 -4
Lines 51255 51176 -79
==========================================
- Hits 47775 47643 -132
- Misses 3480 3533 +53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
sayah-y
left a comment
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.
Thanks for taking into account my feedbacks 👍
This updates the Twitter OAuth2 source so it aligns with the current X API requirements. The previous implementation did not request the required user.fields and was missing newer scopes, which caused the Twitter login flow to fail.
This PR updates the profile URL to include the correct fields, adds the required scopes, and expands the user property mapping to match the response returned by X.
closes #18466