-
-
Notifications
You must be signed in to change notification settings - Fork 949
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
feat: add twitter SSO #3778
feat: add twitter SSO #3778
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3778 +/- ##
==========================================
- Coverage 78.37% 78.13% -0.24%
==========================================
Files 348 349 +1
Lines 24011 24113 +102
==========================================
+ Hits 18818 18841 +23
- Misses 3772 3852 +80
+ Partials 1421 1420 -1 ☔ View full report in Codecov by Sentry. |
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.
Looks good, just some minor questions.
@@ -327,6 +327,7 @@ require ( | |||
|
|||
require ( | |||
github.com/coreos/go-oidc/v3 v3.9.0 | |||
github.com/dghubble/oauth1 v0.7.2 |
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.
To implement "Login with X", use the gologin packages which provide login handlers for OAuth1 and OAuth2 providers.
To call the Twitter, Digits, or Tumblr OAuth1 APIs, use the higher level Go API clients.
Did you check that note in the repo's readme?
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.
The usage later looks valid IMO.
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 this should be fine!
// We need to cheat so that callback validates on return | ||
c.CallbackURL = c.CallbackURL + fmt.Sprintf("?state=%s&code=unused", state) |
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.
What exactly is the cheat referred to here? Do we need to set a code, but it gets ignored by twitter? Will it still be in the URL when being redirected back?
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.
Good question - I took over some of the code from PR #517 where this originally was at. I believe that OAuth1 doesn't return a code parameter like OAuth2, and that would break the provider logic. So the added this workaround? I'm not sure to be honest.
Closes #517 #2117
ory/docs#1658