-
-
Notifications
You must be signed in to change notification settings - Fork 736
Add Apple Sign-In response_mode=form_post support #1385
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
Co-authored-by: mattwoberts <[email protected]>
Co-authored-by: mattwoberts <[email protected]>
Co-authored-by: mattwoberts <[email protected]>
Co-authored-by: mattwoberts <[email protected]>
mattwoberts
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.
@copilot can you look at the review. In particular can you revert your changes to .test.env
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.
Why so many changes to this file?
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 changes were unintended - I accidentally replaced the test environment configuration while setting up my local test environment. This has been reverted in commit 6e0210b.
| if hostname == "appleid.apple.com" && config != nil { | ||
| scope := strings.ToLower(config.Scope) | ||
| if strings.Contains(scope, "name") || strings.Contains(scope, "email") { | ||
| v.Add("response_mode", "form_post") | ||
| } | ||
| } |
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.
Is this something that will be required from other oauth providers in the future, and if so should we make it configurable, or is it unlikey?
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 approach is designed to be extensible. Currently, Apple is the only known provider requiring dynamic parameters based on scope. The implementation handles both:
- Static parameters via the
providerParamsmap (like Twitch's claims parameter) - Dynamic parameters based on config (like Apple's response_mode)
If other providers need similar dynamic behavior in the future, the pattern is already established - just add another hostname check with the appropriate logic. Making it fully configurable (e.g., via database or config file) would add complexity without a clear use case yet.
Co-authored-by: mattwoberts <[email protected]>
response_mode=form_postwhennameoremailscopes are requestedproviderParamsmechanism incustom.goto add Apple-specific parametersproviderParamsinapp/services/oauth/custom.goform_postresponse mode (receive POST instead of GET)Summary
Successfully implemented support for Apple Sign-In's
response_mode=form_postrequirement. The solution:✅ Automatically adds
response_mode=form_postfor Apple providers whennameoremailis in the scope✅ Handles both GET and POST OAuth callbacks
✅ Fully tested with new test cases
✅ Backward compatible with existing OAuth providers
✅ No security vulnerabilities detected
✅ All existing tests pass
✅ Reverted unintended .test.env changes
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.