Repository: supabase/gotrue
Pull request #825 (2022-11-20 14:48:33)

fix: don't update user metadata on subsequent signups
Subsequent unconfirmed user signups allow modifying the originally passed user metadata without confirming that the password is correct. Given that this endpoint is meant to be used only once, no subsequent editing of the user metadata should be allowed.

This can be achieved by calling the user update endpoint while authenticated.

hf: > LGTM - should we also remove the other `WebhookConfig` related code as well?

Yeah maybe in subsequent PRs, we don't use this anyway.

Patch:
diff --git a/api/auth.go b/api/auth.go
index 3b13fdfe..56796924 100644
--- a/api/auth.go
+++ b/api/auth.go
@@ -21,13 +21,15 @@ func (a *API) requireAuthentication(w http.ResponseWriter, r *http.Request) (con
 		return nil, err
 	}
 
-	ctx, err := a.parseJWTClaims(token, r, w)
+	ctx, err := a.parseJWTClaims(token, r)
 	if err != nil {
+		a.clearCookieTokens(config, w)
 		return ctx, err
 	}
 
 	ctx, err = a.maybeLoadUserOrSession(ctx)
 	if err != nil {
+		a.clearCookieTokens(config, w)
 		return ctx, err
 	}
 	return ctx, err
@@ -62,7 +64,7 @@ func (a *API) extractBearerToken(r *http.Request) (string, error) {
 	return matches[1], nil
 }
 
-func (a *API) parseJWTClaims(bearer string, r *http.Request, w http.ResponseWriter) (context.Context, error) {
+func (a *API) parseJWTClaims(bearer string, r *http.Request) (context.Context, error) {
 	ctx := r.Context()
 	config := a.config
 
@@ -71,7 +73,6 @@ func (a *API) parseJWTClaims(bearer string, r *http.Request, w http.ResponseWrit
 		return []byte(config.JWT.Secret), nil
 	})
 	if err != nil {
-		a.clearCookieTokens(config, w)
 		return nil, unauthorizedError("invalid JWT: unable to parse or verify signature, %v", err)
 	}
 
diff --git a/api/middleware.go b/api/middleware.go
index 51b46218..b132c0f3 100644
--- a/api/middleware.go
+++ b/api/middleware.go
@@ -116,8 +116,9 @@ func (a *API) requireAdminCredentials(w http.ResponseWriter, req *http.Request)
 		return nil, err
 	}
 
-	ctx, err := a.parseJWTClaims(t, req, w)
+	ctx, err := a.parseJWTClaims(t, req)
 	if err != nil {
+		a.clearCookieTokens(a.config, w)
 		return nil, err
 	}
 
diff --git a/api/signup.go b/api/signup.go
index f44e1d16..681e4db0 100644
--- a/api/signup.go
+++ b/api/signup.go
@@ -104,9 +104,7 @@ func (a *API) Signup(w http.ResponseWriter, r *http.Request) error {
 				return UserExistsError
 			}
 
-			if err := user.UpdateUserMetaData(tx, params.Data); err != nil {
-				return internalServerError("Database error updating user").WithInternalError(err)
-			}
+			// do not update the user because we can't be sure of their claimed identity
 		} else {
 			user, terr = a.signupNewUser(ctx, tx, params)
 			if terr != nil {
diff --git a/api/ssoadmin.go b/api/ssoadmin.go
index d8d877b2..7bacf136 100644
--- a/api/ssoadmin.go
+++ b/api/ssoadmin.go
@@ -354,8 +354,8 @@ func (a *API) adminSSOProvidersUpdate(w http.ResponseWriter, r *http.Request) er
 		modified = true
 	}
 
-	var createDomains []string
-	keepDomains := make(map[string]bool)
+	var createDomains []models.SSODomain
+	keepProviders := make(map[string]bool)
 
 	for _, domain := range params.Domains {
 		existingProvider, err := models.FindSSOProviderByDomain(db, domain)
@@ -364,43 +364,57 @@ func (a *API) adminSSOProvidersUpdate(w http.ResponseWriter, r *http.Request) er
 		}
 		if existingProvider != nil {
 			if existingProvider.ID == provider.ID {
-				keepDomains[domain] = true
+				keepProviders[domain] = true
 			} else {
 				return badRequestError("SSO domain '%s' already assigned to another provider (%s)", domain, existingProvider.ID.String())
 			}
 		} else {
-			createDomains = append(createDomains, domain)
+			modified = true
+			createDomains = append(createDomains, models.SSODomain{
+				Domain:        domain,
+				SSOProviderID: provider.ID,
+			})
 		}
 	}
 
-	var updatedDomains []models.SSODomain
+	var deleteDomains []models.SSODomain
 
 	for _, domain := range provider.SSODomains {
-		if keepDomains[domain.Domain] {
-			updatedDomains = append(updatedDomains, domain)
+		if !keepProviders[domain.Domain] {
+			modified = true
+			deleteDomains = append(deleteDomains, domain)
 		}
 	}
 
-	for _, domain := range createDomains {
-		updatedDomains = append(updatedDomains, models.SSODomain{
-			Domain: domain,
-		})
-	}
-
-	modified = modified || len(createDomains) > 0
-
-	provider.SSODomains = updatedDomains
-
-	if !provider.SAMLProvider.AttributeMapping.Equal(&params.AttributeMapping) {
+	updateAttributeMapping := !provider.SAMLProvider.AttributeMapping.Equal(&params.AttributeMapping)
+	if updateAttributeMapping {
 		modified = true
 		provider.SAMLProvider.AttributeMapping = params.AttributeMapping
 	}
 
 	if modified {
 		if err := db.Transaction(func(tx *storage.Connection) error {
-			return tx.Eager().Update(provider)
+			if terr := tx.Eager().Update(provider); terr != nil {
+				return terr
+			}
+
+			if terr := tx.Destroy(deleteDomains); terr != nil {
+				return terr
+			}
+
+			if terr := tx.Eager().Create(createDomains); terr != nil {
+				return terr
+			}
+
+			if updateAttributeMapping {
+				if terr := tx.Eager().Update(&provider.SAMLProvider); terr != nil {
+					return terr
+				}
+			}
+
+			return tx.Eager().Load(provider)
 		}); err != nil {
-			return err
+			return unprocessableEntityError("Updating SSO provider failed, likely due to a conflict. Try again?").WithInternalError(err)
 		}
 	}
 


Test patch:
diff --git a/api/auth_test.go b/api/auth_test.go
index dbfeae15..ecb24db8 100644
--- a/api/auth_test.go
+++ b/api/auth_test.go
@@ -63,8 +63,7 @@ func (ts *AuthTestSuite) TestParseJWTClaims() {
 
 	req := httptest.NewRequest(http.MethodGet, "http://localhost", nil)
 	req.Header.Set("Authorization", "Bearer "+userJwt)
-	w := httptest.NewRecorder()
-	ctx, err := ts.API.parseJWTClaims(userJwt, req, w)
+	ctx, err := ts.API.parseJWTClaims(userJwt, req)
 	require.NoError(ts.T(), err)
 
 	// check if token is stored in context
@@ -172,8 +171,7 @@ func (ts *AuthTestSuite) TestMaybeLoadUserOrSession() {
 			req := httptest.NewRequest(http.MethodGet, "http://localhost", nil)
 			req.Header.Set("Authorization", "Bearer "+userJwt)
 
-			w := httptest.NewRecorder()
-			ctx, err := ts.API.parseJWTClaims(userJwt, req, w)
+			ctx, err := ts.API.parseJWTClaims(userJwt, req)
 			require.NoError(ts.T(), err)
 			ctx, err = ts.API.maybeLoadUserOrSession(ctx)
 			if c.ExpectedError != nil {
diff --git a/api/hook_test.go b/api/hook_test.go
index 8b2d448c..8311d6e2 100644
--- a/api/hook_test.go
+++ b/api/hook_test.go
@@ -179,20 +179,4 @@ func TestHookTimeout(t *testing.T) {
 	assert.Equal(t, 3, callCount)
 }
 
-func TestHookNoServer(t *testing.T) {
-	config := &conf.WebhookConfig{
-		URL:        "http://somewhere.something.com",
-		Retries:    1,
-		TimeoutSec: 1,
-	}
-	w := Webhook{
-		WebhookConfig: config,
-	}
-	_, err := w.trigger()
-	require.Error(t, err)
-	herr, ok := err.(*HTTPError)
-	require.True(t, ok)
-	assert.Equal(t, http.StatusBadGateway, herr.Code)
-}
-
 func squash(f func() error) { _ = f }
diff --git a/api/mfa_test.go b/api/mfa_test.go
index 40e7e1e6..fef3e672 100644
--- a/api/mfa_test.go
+++ b/api/mfa_test.go
@@ -392,7 +392,7 @@ func (ts *MFATestSuite) TestSessionsMaintainAALOnRefresh() {
 	data := &AccessTokenResponse{}
 	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
 
-	ctx, err := ts.API.parseJWTClaims(data.Token, req, w)
+	ctx, err := ts.API.parseJWTClaims(data.Token, req)
 	require.NoError(ts.T(), err)
 	ctx, err = ts.API.maybeLoadUserOrSession(ctx)
 	require.NoError(ts.T(), err)
@@ -418,7 +418,7 @@ func (ts *MFATestSuite) TestMFAFollowedByPasswordSignIn() {
 
 	data := &AccessTokenResponse{}
 	require.NoError(ts.T(), json.NewDecoder(w.Body).Decode(&data))
-	ctx, err := ts.API.parseJWTClaims(data.Token, req, w)
+	ctx, err := ts.API.parseJWTClaims(data.Token, req)
 	require.NoError(ts.T(), err)
 	ctx, err = ts.API.maybeLoadUserOrSession(ctx)
 	require.NoError(ts.T(), err)