Repository: markbates/goth
Pull request #361 (2020-09-15 20:44:29)

Update Apple provider to use %20 space encoding instead of +
We have been experiencing intermittent issues with Sign In With Apple when requesting email **and** name scopes. There's no error that is surfaced, but oftentimes the name is not available on first authentication, even if the user allows it.

Honestly we were pretty stumped about this, and fortunately someone from Apple proactively reached out to my company saying they were seeing errors on their end because we were requesting scopes using the wrong encoding: `scope=name+email` instead of `scope=name%20email`. For reasons that are still unclear to us, this only seems to be a problem _sometimes_ (but still hundreds of occurrences a day for us).

This encoding is outside our control, and comes from default behavior in the `oauth2` package and how it escapes query parameters.

I'm open to suggestions for improvement, but I have included a rough approach that works and has caused our "missing name" errors to plummet when testing.

wunderkind2k1: Hey @stapleton92 I think the PR #356 fixes you problem. It is not merged yet and master branch has gotten a little bit ahead if it while it is waiting for the merge.

But to help you out with your problem directly have a look at https://github.com/markbates/goth/pull/356/files#diff-7af721a4e5a3fd8edfcb3f884f51ec3bc62c718a6fd1013f85ecb0c9b256691c this fixes it.

The mentioned PR fixes some more things - you could check out the branch of the PR and use it (as we do in our project) - until its merged into the master someday.
dstapleton92: Hi @wunderkind2k1 thanks for reaching out! That PR is certainly important and we too ran into that issue and had to fix our goth implementation a while back for that very issue. It's nice that you have a PR to fix the core for everyone!

The problem I'm hoping to address in this PR has to do with the generated auth url at apple where we redirect users to begin the process with Apple; your open PR is for properly receiving the `state` and other data in a form-body POST request sent back from Apple to the goth callback.

The key problem is that the authURL we send users to ends up being encoded as `https://appleid.apple.com/auth/authorize?scope=name+email` (omitted other params, important one here is `scope`).

We have noticed, and Apple has reached out to us and confirmed, that the name and email delivery fails significantly frequently if the scope is requested as `name+email` instead of `name%20email`, as Apple is expecting. My PR simply fixes that initial URL we redirect users to in order to begin the auth process.
dstapleton92: Also, any particular guidance on how to get a PR reviewed? The project seems kinda dead. I opened this PR almost two months ago.

Patch:
diff --git a/providers/apple/apple.go b/providers/apple/apple.go
index c5e32a2..1329bb2 100644
--- a/providers/apple/apple.go
+++ b/providers/apple/apple.go
@@ -9,6 +9,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
 	"strings"
 	"time"
 
@@ -101,8 +102,16 @@ func (p Provider) BeginAuth(state string) (goth.Session, error) {
 	if p.formPostResponseMode {
 		opts = append(opts, oauth2.SetAuthURLParam("response_mode", "form_post"))
 	}
+	authURL := p.config.AuthCodeURL(state, opts...)
+	if authURL != "" {
+		if u, err := url.Parse(authURL); err == nil {
+			// Apple requires spaces to be encoded as %20 instead of +
+			u.RawQuery = strings.ReplaceAll(u.RawQuery, "+", "%20")
+			authURL = u.String()
+		}
+	}
 	return &Session{
-		AuthURL: p.config.AuthCodeURL(state, opts...),
+		AuthURL: authURL,
 	}, nil
 }
 


Test patch:
diff --git a/providers/apple/apple_test.go b/providers/apple/apple_test.go
index 2786d6c..dc98c8f 100644
--- a/providers/apple/apple_test.go
+++ b/providers/apple/apple_test.go
@@ -99,3 +99,21 @@ func TestAuthorize(t *testing.T) {
 		a.Fail(errStr)
 	}
 }
+
+func TestBeginAuth(t *testing.T) {
+	a := assert.New(t)
+
+	client := http.DefaultClient
+	p := New(
+		"<clientId>",
+		"<secret>",
+		"https://example-app.com/redirect",
+		client,
+		"name", "email")
+	session, _ := p.BeginAuth("test_state")
+
+	s := session.(*Session)
+
+	// Apple requires spaces to be encoded as %20 instead of +
+	a.Equal(s.AuthURL, "https://appleid.apple.com/auth/authorize?client_id=%3CclientId%3E&redirect_uri=https%3A%2F%2Fexample-app.com%2Fredirect&response_mode=form_post&response_type=code&scope=name%20email&state=test_state")
+}