Repository: supabase/gotrue
Pull request #387 (2022-02-17 17:45:05)

fix: return error as json on POST verify
## What kind of change does this PR introduce?
* Fixes an issue where the error isn't being returned as a json response on `POST /verify` 


J0: Looks fine but we need to figure out why the tests are failing. Can do that later in the afternoon I suppose

Patch:
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 9b12f021..29253b21 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -1,9 +1,10 @@
 name: Build and Release on Push to Master
 
 on:
-  push:
-    branches:
-      - master
+  workflow_run:
+    workflows: [Test]
+    types: [completed]
+    branches: [master]
 
 jobs:
   release:
@@ -15,6 +16,7 @@ jobs:
       status: ${{ steps.pre-release.outputs.release != steps.post-release.outputs.release }}
 
     runs-on: ubuntu-18.04
+    if: ${{ github.event.workflow_run.conclusion == 'success' }}
     steps:
       - uses: actions/checkout@v2
 
@@ -52,10 +54,12 @@ jobs:
     if: success() && needs.release.outputs.status == 'true'
     steps:
       - uses: actions/checkout@v2
+        with:
+          fetch-depth: 0
 
       - uses: actions/setup-go@v2
         with:
-          go-version: "^1.14.4" # The Go version to download (if necessary) and use.
+          go-version: "^1.17.0" # The Go version to download (if necessary) and use.
 
       - run: make deps
 
diff --git a/api/provider/azure.go b/api/provider/azure.go
index fe78cf92..760660df 100644
--- a/api/provider/azure.go
+++ b/api/provider/azure.go
@@ -10,7 +10,7 @@ import (
 )
 
 const (
-	defaultAzureAuthBase = "login.microsoftonline.com"
+	defaultAzureAuthBase = "login.microsoftonline.com/common"
 	defaultAzureAPIBase  = "graph.microsoft.com"
 )
 
@@ -38,7 +38,7 @@ func NewAzureProvider(ext conf.OAuthProviderConfiguration, scopes string) (OAuth
 	}
 
 	authHost := chooseHost(ext.URL, defaultAzureAuthBase)
-	apiPath := chooseHost(ext.URL, defaultAzureAPIBase)
+	apiPath := chooseHost(ext.ApiURL, defaultAzureAPIBase)
 
 	oauthScopes := []string{"openid"}
 
@@ -51,8 +51,8 @@ func NewAzureProvider(ext conf.OAuthProviderConfiguration, scopes string) (OAuth
 			ClientID:     ext.ClientID,
 			ClientSecret: ext.Secret,
 			Endpoint: oauth2.Endpoint{
-				AuthURL:  authHost + "/common/oauth2/v2.0/authorize",
-				TokenURL: authHost + "/common/oauth2/v2.0/token",
+				AuthURL:  authHost + "/oauth2/v2.0/authorize",
+				TokenURL: authHost + "/oauth2/v2.0/token",
 			},
 			RedirectURL: ext.RedirectURI,
 			Scopes:      oauthScopes,
diff --git a/api/token.go b/api/token.go
index b99e6b66..a315d5f4 100644
--- a/api/token.go
+++ b/api/token.go
@@ -77,7 +77,11 @@ func (p *IdTokenGrantParams) getVerifier(ctx context.Context) (*oidc.IDTokenVeri
 	case "azure":
 		oAuthProvider = config.External.Azure
 		oAuthProviderClientId = oAuthProvider.ClientID
-		provider, err = oidc.NewProvider(ctx, "https://login.microsoftonline.com/common/v2.0")
+		url := oAuthProvider.URL
+		if url == "" {
+			url = "https://login.microsoftonline.com/common"
+		}
+		provider, err = oidc.NewProvider(ctx, url + "/v2.0")
 	case "facebook":
 		oAuthProvider = config.External.Facebook
 		oAuthProviderClientId = oAuthProvider.ClientID
diff --git a/api/verify.go b/api/verify.go
index 3c5f8b69..a48aca9f 100644
--- a/api/verify.go
+++ b/api/verify.go
@@ -82,14 +82,6 @@ func (a *API) Verify(w http.ResponseWriter, r *http.Request) error {
 		aud := a.requestAud(ctx, r)
 		user, terr = a.verifyUserAndToken(ctx, tx, params, aud)
 		if terr != nil {
-			var herr *HTTPError
-			if errors.As(terr, &herr) {
-				if errors.Is(herr.InternalError, redirectWithQueryError) {
-					rurl := a.prepErrorRedirectURL(herr, r, params.RedirectTo)
-					http.Redirect(w, r, rurl, http.StatusSeeOther)
-					return nil
-				}
-			}
 			return terr
 		}
 
@@ -126,8 +118,20 @@ func (a *API) Verify(w http.ResponseWriter, r *http.Request) error {
 		}
 		return nil
 	})
+
 	if err != nil {
-		return err
+		if r.Method == http.MethodPost {
+			// do not redirect POST requests
+			return err
+		}
+		var herr *HTTPError
+		if errors.As(err, &herr) {
+			if errors.Is(herr.InternalError, redirectWithQueryError) {
+				rurl := a.prepErrorRedirectURL(herr, r, params.RedirectTo)
+				http.Redirect(w, r, rurl, http.StatusSeeOther)
+				return nil
+			}
+		}
 	}
 
 	// GET requests should return to the app site after confirmation
diff --git a/conf/configuration.go b/conf/configuration.go
index b4796747..ee9bc0a5 100644
--- a/conf/configuration.go
+++ b/conf/configuration.go
@@ -17,6 +17,7 @@ type OAuthProviderConfiguration struct {
 	Secret      string `json:"secret"`
 	RedirectURI string `json:"redirect_uri" split_words:"true"`
 	URL         string `json:"url"`
+	ApiURL      string `json:"api_url"`
 	Enabled     bool   `json:"enabled"`
 }
 


Test patch:
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
index dd047717..bcdc7830 100644
--- a/.github/workflows/test.yml
+++ b/.github/workflows/test.yml
@@ -10,7 +10,7 @@ jobs:
   test:
     strategy:
       matrix:
-        go-version: [1.16.x]
+        go-version: [1.17.x]
     runs-on: ubuntu-18.04
     services:
       postgres:
diff --git a/api/external_azure_test.go b/api/external_azure_test.go
index 0626e9a7..b0599c45 100644
--- a/api/external_azure_test.go
+++ b/api/external_azure_test.go
@@ -41,7 +41,7 @@ func (ts *ExternalTestSuite) TestSignupExternalAzure() {
 func AzureTestSignupSetup(ts *ExternalTestSuite, tokenCount *int, userCount *int, code string, user string) *httptest.Server {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch r.URL.Path {
-		case "/common/oauth2/v2.0/token":
+		case "/oauth2/v2.0/token":
 			*tokenCount++
 			ts.Equal(code, r.FormValue("code"))
 			ts.Equal("authorization_code", r.FormValue("grant_type"))
@@ -60,6 +60,7 @@ func AzureTestSignupSetup(ts *ExternalTestSuite, tokenCount *int, userCount *int
 	}))
 
 	ts.Config.External.Azure.URL = server.URL
+	ts.Config.External.Azure.ApiURL = server.URL
 
 	return server
 }
diff --git a/api/verify_test.go b/api/verify_test.go
index 979b53b4..65c4dc7f 100644
--- a/api/verify_test.go
+++ b/api/verify_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -173,16 +174,9 @@ func (ts *VerifyTestSuite) TestExpiredConfirmationToken() {
 	u.ConfirmationSentAt = &sentTime
 	require.NoError(ts.T(), ts.API.db.Update(u))
 
-	// Request body
-	var buffer bytes.Buffer
-	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
-		"type":  signupVerification,
-		"token": u.ConfirmationToken,
-	}))
-
 	// Setup request
-	req := httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer)
-	req.Header.Set("Content-Type", "application/json")
+	reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", signupVerification, u.ConfirmationToken)
+	req := httptest.NewRequest(http.MethodGet, reqURL, nil)
 
 	// Setup response recorder
 	w := httptest.NewRecorder()
@@ -203,21 +197,21 @@ func (ts *VerifyTestSuite) TestInvalidOtp() {
 	u.ConfirmationSentAt = &sentTime
 	require.NoError(ts.T(), ts.API.db.Update(u))
 
-	type expected struct {
-		code      int
-		fragments string
+	type ResponseBody struct {
+		Code int    `json:"code"`
+		Msg  string `json:"msg"`
 	}
 
-	expectedResponse := expected{
-		code:      http.StatusSeeOther,
-		fragments: "error_code=410&error_description=Token+has+expired+or+is+invalid",
+	expectedResponse := ResponseBody{
+		Code: http.StatusGone,
+		Msg:  "Token has expired or is invalid",
 	}
 
 	cases := []struct {
 		desc     string
 		sentTime time.Time
 		body     map[string]interface{}
-		expected
+		expected ResponseBody
 	}{
 		{
 			desc:     "Expired Sms OTP",
@@ -268,11 +262,15 @@ func (ts *VerifyTestSuite) TestInvalidOtp() {
 			// Setup response recorder
 			w := httptest.NewRecorder()
 			ts.API.handler.ServeHTTP(w, req)
-			assert.Equal(ts.T(), c.expected.code, w.Code)
 
-			url, err := w.Result().Location()
+			b, err := ioutil.ReadAll(w.Body)
 			require.NoError(ts.T(), err)
-			assert.Equal(ts.T(), c.expected.fragments, url.Fragment)
+			var resp ResponseBody
+			err = json.Unmarshal(b, &resp)
+			require.NoError(ts.T(), err)
+			assert.Equal(ts.T(), c.expected.Code, resp.Code)
+			assert.Equal(ts.T(), c.expected.Msg, resp.Msg)
+
 		})
 	}
 }
@@ -285,16 +283,9 @@ func (ts *VerifyTestSuite) TestExpiredRecoveryToken() {
 	u.RecoverySentAt = &sentTime
 	require.NoError(ts.T(), ts.API.db.Update(u))
 
-	// Request body
-	var buffer bytes.Buffer
-	require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]interface{}{
-		"type":  recoveryVerification,
-		"token": u.RecoveryToken,
-	}))
-
 	// Setup request
-	req := httptest.NewRequest(http.MethodPost, "http://localhost/verify", &buffer)
-	req.Header.Set("Content-Type", "application/json")
+	reqURL := fmt.Sprintf("http://localhost/verify?type=%s&token=%s", "signup", u.RecoveryToken)
+	req := httptest.NewRequest(http.MethodGet, reqURL, nil)
 
 	// Setup response recorder
 	w := httptest.NewRecorder()
@@ -528,11 +519,11 @@ func (ts *VerifyTestSuite) TestVerifyBannedUser() {
 
 			w := httptest.NewRecorder()
 			ts.API.handler.ServeHTTP(w, req)
-			assert.Equal(ts.T(), http.StatusSeeOther, w.Code)
+			assert.Equal(ts.T(), http.StatusUnauthorized, w.Code)
 
-			url, err := w.Result().Location()
+			b, err := ioutil.ReadAll(w.Body)
 			require.NoError(ts.T(), err)
-			assert.Equal(ts.T(), "error=unauthorized_client&error_code=401&error_description=Error+confirming+user", url.Fragment)
+			assert.Equal(ts.T(), "{\"code\":401,\"msg\":\"Error confirming user\"}", string(b))
 		})
 	}
 }