-
Notifications
You must be signed in to change notification settings - Fork 321
feat(auth): Adds Twitch identity provider support #4642
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
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
7b651e1 to
1709781
Compare
|
Still in progress, just wanted to save and push my work already :) |
| } | ||
|
|
||
| /// Signature for parsing the data from the provider's token response. | ||
| typedef ParseTokenResponse<T> = T Function(Map<String, dynamic> responseBody); |
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.
@vfiruz97 What do you think about that? I needed to change something on that, since Twitch for example gives back more than an access_token String. Instead it has refresh_token and more in the response which is needed.
Im not very used to generics, but I found it a good way to make it possible to define the success response body of a token exchange individually for each idp.
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.
@vfiruz97 What do you think about that? I needed to change something on that, since Twitch for example gives back more than an access_token String. Instead it has refresh_token and more in the response which is needed.
Hi @89pleasure,
Thanks for pointing this out, you're right. Providers often return more than just access_token, like refresh_token and expires_in, so it makes sense to leave room for that.
I looked into the generics approach you suggested. While it's very flexible, it adds some cascading complexity. We'd need to pass through several classes (OAuth2PkceServerConfig, OAuth2PkceUtil, etc.), which makes the API harder to use and reason about.
-class OAuth2PkceServerConfig {
+class OAuth2PkceServerConfig<T> {
- final ParseAccessToken parseAccessToken;
+ final ParseAccessToken<T> parseAccessToken;
-class OAuth2PkceUtil {
+class OAuth2PkceUtil<T> {
- final OAuth2PkceServerConfig config;
+ final OAuth2PkceServerConfig<T> config;
- Future<String> exchangeCodeForToken({
+ Future<T> exchangeCodeForToken({
- late final OAuth2PkceServerConfig oauth2Config;
+ late final OAuth2PkceServerConfig<String> oauth2Config;
- _oauth2Util = OAuth2PkceUtil(config: config.oauth2Config);
+ _oauth2Util = OAuth2PkceUtil<String>(config: config.oauth2Config);A simpler alternative
Instead of generics, we could use a dedicated response object. This supports the Twitch use case while keeping the developer experience clean and straightforward:
class OAuth2PkceTokenResponse {
final String accessToken;
final String? refreshToken;
final int? expiresIn;
final String? tokenType;
/// Raw parsed response for provider-specific fields
final Map<String, dynamic> raw;
const OAuth2PkceTokenResponse({
required this.accessToken,
this.refreshToken,
this.expiresIn,
this.tokenType,
required this.raw,
});
}
....
typedef ParseTokenResponse = OAuth2PkceTokenResponse Function(Map<String, dynamic> responseBody);No generic pollution, extensible, better DX. What do you think about moving in this direction?
I'm happy to hear @marcelomendoncasoares's thoughts on this as well.
|
Thanks for this valuable contribution, @89pleasure! Since you had to base your work from #4546, the diff is carrying a lot of changes and will make the revision harder. We are still waiting for the docs, but I'll merge #4546 already so you can rebase from main! |
Thanks @marcelomendoncasoares ! Im on it. But I recognized that the merge into main has changes the PR does not have. Seems to be force pushed too. That makes a rebase on main pretty hard to distinguish the changes now. |
…laces in pubspec.yaml
…factor GitHub sign-in service
…rade `flutter_web_auth_2`
Co-authored-by: Marcelo Mendonça Soares <marcelo.me.soares@gmail.com>
This reverts commit 06dab83.
1bc35d2 to
6d4a586
Compare
You should do a rebase using the |
Thanks @marcelomendoncasoares, I am happy, finally to get my PR merged! The docs on its way. Needs your attention :) serverpod/serverpod_docs#402 |
This PR adds Twitch OAuth authentication support to the serverpod_auth module. It implements a complete Twitch identity provider (IDP) with PKCE (Proof Key for Code Exchange) flow for enhanced security across all platforms. This is possible to the great work of @vfiruz97
Key features:
Cross-platform support (iOS, Android, Web, macOS, Windows, Linux)
Customizable sign-in button widget with Twitch branding
Complete server-side token exchange, user authentication, and account management
Integration with existing Serverpod auth system
Comprehensive test coverage including integration tests
No new dependencies added
Pre-launch Checklist
///), and made sure that the documentation follows the same style as other Serverpod documentation. I checked spelling and grammar.If you need help, consider asking for advice on the discussion board.
Breaking changes
This is a new feature. I changed the
exchangeCodeForTokento a generic method to support a different response schemas. Twitch is using a response token approach similar to Apple which won't work the way it is implemented right now. Since im new to dart development this needs further testing.