Repository: supabase/gotrue
Pull request #830 (2022-11-22 02:16:31)

fix: logout cookies not cleared
## What kind of change does this PR introduce?
* This PR fixes the issue where the cookies are not being cleared if an invalid user's JWT is supplied on logout.


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/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/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)