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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Twitter OAuth2 support #39664

Closed
1 task done
Tratcher opened this issue Jan 20, 2022 · 12 comments · Fixed by dotnet/AspNetCore.Docs#24838
Closed
1 task done

Twitter OAuth2 support #39664

Tratcher opened this issue Jan 20, 2022 · 12 comments · Fixed by dotnet/AspNetCore.Docs#24838
Assignees
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer Docs This issue tracks updating documentation

Comments

@Tratcher
Copy link
Member

Tratcher commented Jan 20, 2022

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

The current Twitter authentication implementation uses OAuth1a. Twitter now supports OAuth2 which is much simpler to work with and maintain. It almost works with the default OAuth base class, except that the clientid and secret need to be sent to the token endpoint in the authorization header rather than the body.

Describe the solution you'd like

Consider any or all of the following:
A) Deprecate the OAuth1a implementation. Updating the implementation in place to OAuth2 would be breaking anyways.
B) Implement a new OAuth2 Twitter auth handler. This could be done here in ASP.NET Core 7 or in aspnet-contrib, they'd ship faster and give downlevel support.

Additional context

Here's some sample code based on our SocialSample that gets Twitter OAuth2 working in a minimal way. This doesn't include fetching claims.

        var backchannel = new HttpClient();
        var byteArray = Encoding.ASCII.GetBytes(Configuration["twitter2:clientid"] + ":" + Configuration["twitter2:clientsecret"]);
        backchannel.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Basic", Convert.ToBase64String(byteArray));

            // https://developer.twitter.com/en/docs/authentication/oauth-2-0/authorization-code
            .AddOAuth("Twitter2-AccessToken", "Twitter2 AccessToken only", o =>
            {
                o.ClientId = Configuration["twitter2:clientid"];
                o.ClientSecret = Configuration["twitter2:clientsecret"];
                o.CallbackPath = new PathString("/signin-twitter2-token");
                o.AuthorizationEndpoint = "https://twitter.com/i/oauth2/authorize";
                o.TokenEndpoint = "https://api.twitter.com/2/oauth2/token";
                o.SaveTokens = true;
                o.UsePkce = true;
                o.Scope.Add("users.read");
                o.Backchannel = backchannel;
            })

A more complete implementation would look like this:
https://github.com/aspnet-contrib/AspNet.Security.OAuth.Providers/blob/51f4c0065774d10ce18aec6c73c9a040d150e107/src/AspNet.Security.OAuth.Notion/NotionAuthenticationHandler.cs#L28

@Tratcher Tratcher added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jan 20, 2022
@robmen
Copy link

robmen commented Jan 20, 2022

FYI, there are still parts of the Twitter API that are only available via the OAuth 1a authentication mechanism.

@martincostello
Copy link
Member

martincostello commented Jan 20, 2022

If it's useful, I had a play at implementing this with aspnet-contrib the other week: https://github.com/martincostello/AspNet.Security.OAuth.Providers/tree/Twitter-OAuth2/src/AspNet.Security.OAuth.Twitter

I wasn't super happy with it as-is so I've parked it for now.

The main issue seemed to be that Twitter rejects the callback URL if it's too long with a non-descript 400 error, so I fudged the state into a cookie.

@Tratcher
Copy link
Member Author

Thanks @martincostello!

How long was too long? Was there anything extra in your state? The minimal example above didn't run into that.

@martincostello
Copy link
Member

I'll re-run it tomorrow and inspect.

It's possible that there might have been extra stuff being picked up from cruft up in the local clone of the sample I have on my laptop being around for a long time testing lots of different providers.

I don't remember it seeming too extremely long, and I couldn't find any docs on Twitter's end that explicitly put a cap on it.

I vaguely remember way back I had a similar problem with Amazon, but that was an issue that turned out to be hitting the IIS path length limit on my side of things.

@martincostello
Copy link
Member

I checked the state parameter, and it had a length of 304 characters and unprotected it contained items for .redirect, .xsrf and code_verifier.

However, if I comment out my BuildChallengeUrl() and HandleRemoteAuthenticateAsync() overrides now, it works as expected and I don't get an HTTP 400.

I wonder if in the last two weeks Twitter have fixed something in their OAuth 2.0 service that means it now works with the state parameter as-is.

I did also clear my localhost cookies before I started, so maybe that was interfering somehow.

@martincostello
Copy link
Member

@Tratcher I've cleaned up my branch and removed the redundant code if you want to take a look: aspnet-contrib/AspNet.Security.OAuth.Providers#644

I've no strong feeling either way over us having a Twitter provider in aspnet-contrib, or you updating the Twitter provider here to use OAuth 2.0 as a breaking-change for .NET 7 instead.

@Tratcher
Copy link
Member Author

@martincostello I talked to @blowdart. We'd encourage you to ship the new package from aspnet-contrib. That provides OAuth2 support on 6.0 and I assume you can ship it soon? There's no need to make people wait for 7.0.

FYI, there are still parts of the Twitter API that are only available via the OAuth 1a authentication mechanism.

@robmen do you have a reference for that?

I think we'll leave the OAuth1a implementation alone for now. We can obsolete it if Twitter ever discontinues it.

@martincostello
Copy link
Member

Thanks @Tratcher - assuming Kévin over at aspnet-contrib is happy with it, then we could ship the OAuth 2.0 provider from our side this month, and then it'll also get .NET 7.0 support later this year.

There's also a Twitter document here I was looking at earlier that compares the capabilities of what you can do with the different Twitter API versions which have the different OAuth token requirements (if I understood it correctly).

@robmen
Copy link

robmen commented Jan 21, 2022

Yeah, this is the Twitter documentation showing differences between Twitter 2 API (using OAuth 2) and old API (using OAuth 1.0a): https://developer.twitter.com/en/docs/twitter-api/migrate/twitter-api-endpoint-map

@adityamandaleeka
Copy link
Member

Triage: we should doc the existence of this.

@adityamandaleeka adityamandaleeka added the Docs This issue tracks updating documentation label Jan 21, 2022
@adityamandaleeka adityamandaleeka added this to the .NET 7 Planning milestone Jan 21, 2022
@ghost
Copy link

ghost commented Jan 21, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@martincostello
Copy link
Member

The aspnet-contrib Twitter OAuth 2.0 provider is now available from NuGet.org: https://www.nuget.org/packages/AspNet.Security.OAuth.Twitter/6.0.3

@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer Docs This issue tracks updating documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants