Repository: supabase/gotrue
Pull request #1161 (2023-06-30 16:01:20)

fix: maintain query params order
## What kind of change does this PR introduce?
* Maintains the order of the query params passed into the email link


Patch:
diff --git a/internal/api/admin.go b/internal/api/admin.go
index a32756fd..024d6ecb 100644
--- a/internal/api/admin.go
+++ b/internal/api/admin.go
@@ -200,7 +200,7 @@ func (a *API) adminUserUpdate(w http.ResponseWriter, r *http.Request) error {
 				return invalidPasswordLengthError(config.PasswordMinLength)
 			}
 
-			if terr := user.UpdatePassword(tx, *params.Password); terr != nil {
+			if terr := user.UpdatePassword(tx, *params.Password, nil); terr != nil {
 				return terr
 			}
 		}
diff --git a/internal/api/logout.go b/internal/api/logout.go
index 5c893140..a2d2c41e 100644
--- a/internal/api/logout.go
+++ b/internal/api/logout.go
@@ -1,19 +1,44 @@
 package api
 
 import (
+	"fmt"
 	"net/http"
 
 	"github.com/supabase/gotrue/internal/models"
 	"github.com/supabase/gotrue/internal/storage"
 )
 
+type LogoutBehavior string
+
+const (
+	LogoutGlobal LogoutBehavior = "global"
+	LogoutLocal  LogoutBehavior = "local"
+	LogoutOthers LogoutBehavior = "others"
+)
+
 // Logout is the endpoint for logging out a user and thereby revoking any refresh tokens
 func (a *API) Logout(w http.ResponseWriter, r *http.Request) error {
 	ctx := r.Context()
 	db := a.db.WithContext(ctx)
 	config := a.config
 
-	a.clearCookieTokens(config, w)
+	scope := LogoutGlobal
+
+	if r.URL.Query() != nil {
+		switch r.URL.Query().Get("scope") {
+		case "", "global":
+			scope = LogoutGlobal
+
+		case "local":
+			scope = LogoutLocal
+
+		case "others":
+			scope = LogoutOthers
+
+		default:
+			return badRequestError(fmt.Sprintf("Unsupported logout scope %q", r.URL.Query().Get("scope")))
+		}
+	}
 
 	s := getSession(ctx)
 	u := getUser(ctx)
@@ -22,15 +47,28 @@ func (a *API) Logout(w http.ResponseWriter, r *http.Request) error {
 		if terr := models.NewAuditLogEntry(r, tx, u, models.LogoutAction, "", nil); terr != nil {
 			return terr
 		}
-		if s != nil {
-			return models.Logout(tx, u.ID)
+
+		if s == nil {
+			return models.LogoutAllRefreshTokens(tx, u.ID)
+		}
+
+		switch scope {
+		case LogoutLocal:
+			return models.LogoutSession(tx, s.ID)
+
+		case LogoutOthers:
+			return models.LogoutAllExceptMe(tx, s.ID, u.ID)
 		}
-		return models.LogoutAllRefreshTokens(tx, u.ID)
+
+		// default mode, log out everywhere
+		return models.Logout(tx, u.ID)
 	})
 	if err != nil {
 		return internalServerError("Error logging out user").WithInternalError(err)
 	}
 
+	a.clearCookieTokens(config, w)
 	w.WriteHeader(http.StatusNoContent)
+
 	return nil
 }
diff --git a/internal/api/user.go b/internal/api/user.go
index d2f8e574..db055147 100644
--- a/internal/api/user.go
+++ b/internal/api/user.go
@@ -6,6 +6,7 @@ import (
 	"net/http"
 
 	"github.com/fatih/structs"
+	"github.com/gofrs/uuid"
 	"github.com/supabase/gotrue/internal/api/provider"
 	"github.com/supabase/gotrue/internal/api/sms_provider"
 	"github.com/supabase/gotrue/internal/conf"
@@ -132,22 +133,18 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
 					return terr
 				}
 			}
-			if terr = user.UpdatePassword(tx, *params.Password); terr != nil {
+
+			var sessionID *uuid.UUID
+			if session != nil {
+				sessionID = &session.ID
+			}
+
+			if terr = user.UpdatePassword(tx, *params.Password, sessionID); terr != nil {
 				return internalServerError("Error during password storage").WithInternalError(terr)
 			}
 			if terr := models.NewAuditLogEntry(r, tx, user, models.UserUpdatePasswordAction, "", nil); terr != nil {
 				return terr
 			}
-			if session != nil {
-				if terr = models.LogoutAllExceptMe(tx, session.ID, user.ID); terr != nil {
-					return terr
-				}
-			} else {
-				// logout all sessions if session id is missing
-				if terr = models.Logout(tx, user.ID); terr != nil {
-					return terr
-				}
-			}
 		}
 
 		if params.Data != nil {
diff --git a/internal/api/verify.go b/internal/api/verify.go
index bc6fc276..cb019573 100644
--- a/internal/api/verify.go
+++ b/internal/api/verify.go
@@ -263,7 +263,7 @@ func (a *API) signupVerify(r *http.Request, ctx context.Context, conn *storage.C
 				if err != nil {
 					internalServerError("error creating user").WithInternalError(err)
 				}
-				if terr = user.UpdatePassword(tx, password); terr != nil {
+				if terr = user.UpdatePassword(tx, password, nil); terr != nil {
 					return internalServerError("Error storing password").WithInternalError(terr)
 				}
 			}
diff --git a/internal/mailer/mailer.go b/internal/mailer/mailer.go
index 9206adc5..b1aac769 100644
--- a/internal/mailer/mailer.go
+++ b/internal/mailer/mailer.go
@@ -25,6 +25,12 @@ type Mailer interface {
 	GetEmailActionLink(user *models.User, actionType, referrerURL string, externalURL *url.URL) (string, error)
 }
 
+type EmailParams struct {
+	Token      string
+	Type       string
+	RedirectTo string
+}
+
 // NewMailer returns a new gotrue mailer
 func NewMailer(globalConfig *conf.GlobalConfiguration) Mailer {
 	mail := gomail.NewMessage()
@@ -64,7 +70,7 @@ func withDefault(value, defaultValue string) string {
 	return value
 }
 
-func getPath(filepath string, params map[string]string) (*url.URL, error) {
+func getPath(filepath string, params *EmailParams) (*url.URL, error) {
 	path := &url.URL{}
 	if filepath != "" {
 		if p, err := url.Parse(filepath); err != nil {
@@ -73,11 +79,8 @@ func getPath(filepath string, params map[string]string) (*url.URL, error) {
 			path = p
 		}
 	}
-	v := url.Values{}
-	for key, val := range params {
-		v.Add(key, val)
+	if params != nil {
+		path.RawQuery = fmt.Sprintf("token=%s&type=%s&redirect_to=%s", url.QueryEscape(params.Token), url.QueryEscape(params.Type), encodeRedirectURL(params.RedirectTo))
 	}
-	// this should never return an error because we're always encoding the values first
-	path.RawQuery, _ = url.QueryUnescape(v.Encode())
 	return path, nil
 }
diff --git a/internal/mailer/template.go b/internal/mailer/template.go
index 0fe1329f..590eab74 100644
--- a/internal/mailer/template.go
+++ b/internal/mailer/template.go
@@ -76,10 +76,10 @@ func (m TemplateMailer) ValidateEmail(email string) error {
 
 // InviteMail sends a invite mail to a new user
 func (m *TemplateMailer) InviteMail(user *models.User, otp, referrerURL string, externalURL *url.URL) error {
-	path, err := getPath(m.Config.Mailer.URLPaths.Invite, map[string]string{
-		"token":       user.ConfirmationToken,
-		"type":        "invite",
-		"redirect_to": encodeRedirectURL(referrerURL),
+	path, err := getPath(m.Config.Mailer.URLPaths.Invite, &EmailParams{
+		Token:      user.ConfirmationToken,
+		Type:       "invite",
+		RedirectTo: referrerURL,
 	})
 
 	if err != nil {
@@ -106,10 +106,10 @@ func (m *TemplateMailer) InviteMail(user *models.User, otp, referrerURL string,
 
 // ConfirmationMail sends a signup confirmation mail to a new user
 func (m *TemplateMailer) ConfirmationMail(user *models.User, otp, referrerURL string, externalURL *url.URL) error {
-	path, err := getPath(m.Config.Mailer.URLPaths.Confirmation, map[string]string{
-		"token":       user.ConfirmationToken,
-		"type":        "signup",
-		"redirect_to": encodeRedirectURL(referrerURL),
+	path, err := getPath(m.Config.Mailer.URLPaths.Confirmation, &EmailParams{
+		Token:      user.ConfirmationToken,
+		Type:       "signup",
+		RedirectTo: referrerURL,
 	})
 	if err != nil {
 		return err
@@ -185,10 +185,10 @@ func (m *TemplateMailer) EmailChangeMail(user *models.User, otpNew, otpCurrent,
 	for _, email := range emails {
 		path, err := getPath(
 			m.Config.Mailer.URLPaths.EmailChange,
-			map[string]string{
-				"token":       email.TokenHash,
-				"type":        "email_change",
-				"redirect_to": encodeRedirectURL(referrerURL),
+			&EmailParams{
+				Token:      email.TokenHash,
+				Type:       "email_change",
+				RedirectTo: referrerURL,
 			},
 		)
 		if err != nil {
@@ -226,10 +226,10 @@ func (m *TemplateMailer) EmailChangeMail(user *models.User, otpNew, otpCurrent,
 
 // RecoveryMail sends a password recovery mail
 func (m *TemplateMailer) RecoveryMail(user *models.User, otp, referrerURL string, externalURL *url.URL) error {
-	path, err := getPath(m.Config.Mailer.URLPaths.Recovery, map[string]string{
-		"token":       user.RecoveryToken,
-		"type":        "recovery",
-		"redirect_to": encodeRedirectURL(referrerURL),
+	path, err := getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{
+		Token:      user.RecoveryToken,
+		Type:       "recovery",
+		RedirectTo: referrerURL,
 	})
 	if err != nil {
 		return err
@@ -254,10 +254,10 @@ func (m *TemplateMailer) RecoveryMail(user *models.User, otp, referrerURL string
 
 // MagicLinkMail sends a login link mail
 func (m *TemplateMailer) MagicLinkMail(user *models.User, otp, referrerURL string, externalURL *url.URL) error {
-	path, err := getPath(m.Config.Mailer.URLPaths.Recovery, map[string]string{
-		"token":       user.RecoveryToken,
-		"type":        "magiclink",
-		"redirect_to": encodeRedirectURL(referrerURL),
+	path, err := getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{
+		Token:      user.RecoveryToken,
+		Type:       "magiclink",
+		RedirectTo: referrerURL,
 	})
 	if err != nil {
 		return err
@@ -297,43 +297,42 @@ func (m TemplateMailer) GetEmailActionLink(user *models.User, actionType, referr
 	var err error
 	var path *url.URL
 
-	referrerURL = encodeRedirectURL(referrerURL)
 	switch actionType {
 	case "magiclink":
-		path, err = getPath(m.Config.Mailer.URLPaths.Recovery, map[string]string{
-			"token":       user.RecoveryToken,
-			"type":        "magiclink",
-			"redirect_to": referrerURL,
+		path, err = getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{
+			Token:      user.RecoveryToken,
+			Type:       "magiclink",
+			RedirectTo: referrerURL,
 		})
 	case "recovery":
-		path, err = getPath(m.Config.Mailer.URLPaths.Recovery, map[string]string{
-			"token":       user.RecoveryToken,
-			"type":        "recovery",
-			"redirect_to": referrerURL,
+		path, err = getPath(m.Config.Mailer.URLPaths.Recovery, &EmailParams{
+			Token:      user.RecoveryToken,
+			Type:       "recovery",
+			RedirectTo: referrerURL,
 		})
 	case "invite":
-		path, err = getPath(m.Config.Mailer.URLPaths.Invite, map[string]string{
-			"token":       user.ConfirmationToken,
-			"type":        "invite",
-			"redirect_to": referrerURL,
+		path, err = getPath(m.Config.Mailer.URLPaths.Invite, &EmailParams{
+			Token:      user.ConfirmationToken,
+			Type:       "invite",
+			RedirectTo: referrerURL,
 		})
 	case "signup":
-		path, err = getPath(m.Config.Mailer.URLPaths.Confirmation, map[string]string{
-			"token":       user.ConfirmationToken,
-			"type":        "signup",
-			"redirect_to": referrerURL,
+		path, err = getPath(m.Config.Mailer.URLPaths.Confirmation, &EmailParams{
+			Token:      user.ConfirmationToken,
+			Type:       "signup",
+			RedirectTo: referrerURL,
 		})
 	case "email_change_current":
-		path, err = getPath(m.Config.Mailer.URLPaths.EmailChange, map[string]string{
-			"token":       user.EmailChangeTokenCurrent,
-			"type":        "email_change",
-			"redirect_to": referrerURL,
+		path, err = getPath(m.Config.Mailer.URLPaths.EmailChange, &EmailParams{
+			Token:      user.EmailChangeTokenCurrent,
+			Type:       "email_change",
+			RedirectTo: referrerURL,
 		})
 	case "email_change_new":
-		path, err = getPath(m.Config.Mailer.URLPaths.EmailChange, map[string]string{
-			"token":       user.EmailChangeTokenNew,
-			"type":        "email_change",
-			"redirect_to": referrerURL,
+		path, err = getPath(m.Config.Mailer.URLPaths.EmailChange, &EmailParams{
+			Token:      user.EmailChangeTokenNew,
+			Type:       "email_change",
+			RedirectTo: referrerURL,
 		})
 	default:
 		return "", fmt.Errorf("invalid email action link type: %s", actionType)
diff --git a/internal/models/user.go b/internal/models/user.go
index d45d4657..446c7f46 100644
--- a/internal/models/user.go
+++ b/internal/models/user.go
@@ -222,13 +222,23 @@ func (u *User) SetPhone(tx *storage.Connection, phone string) error {
 }
 
 // UpdatePassword updates the user's password
-func (u *User) UpdatePassword(tx *storage.Connection, password string) error {
+func (u *User) UpdatePassword(tx *storage.Connection, password string, sessionID *uuid.UUID) error {
 	pw, err := crypto.GenerateFromPassword(context.Background(), password)
 	if err != nil {
 		return err
 	}
 	u.EncryptedPassword = pw
-	return tx.UpdateOnly(u, "encrypted_password")
+	if err := tx.UpdateOnly(u, "encrypted_password"); err != nil {
+		return err
+	}
+
+	if sessionID == nil {
+		// log out user from all sessions to ensure reauthentication after password change
+		return Logout(tx, u.ID)
+	} else {
+		// log out user from all other sessions to ensure reauthentication after password change
+		return LogoutAllExceptMe(tx, *sessionID, u.ID)
+	}
 }
 
 // UpdatePhone updates the user's phone
diff --git a/openapi.yaml b/openapi.yaml
index 8d7acaf0..00294f9e 100644
--- a/openapi.yaml
+++ b/openapi.yaml
@@ -158,6 +158,17 @@ paths:
       security:
         - APIKeyAuth: []
           UserAuth: []
+      parameters:
+        - name: scope
+          in: query
+          description: >
+            (Optional.) Determines how the user should be logged out. When `global` is used, the user is logged out from all active sessions. When `local` is used, the user is logged out from the current session. When `others` is used, the user is logged out from all other sessions except the current one. Clients should remove stored access and refresh tokens except when `others` is used.
+          schema:
+            type: string
+            enum:
+              - global
+              - local
+              - others
       responses:
         204:
           description: No content returned on successful logout.


Test patch:
diff --git a/internal/api/logout_test.go b/internal/api/logout_test.go
index 46d25006..c873532e 100644
--- a/internal/api/logout_test.go
+++ b/internal/api/logout_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -47,18 +48,31 @@ func (ts *LogoutTestSuite) SetupTest() {
 }
 
 func (ts *LogoutTestSuite) TestLogoutSuccess() {
-	req := httptest.NewRequest(http.MethodPost, "http://localhost/logout", nil)
-	req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", ts.token))
-	w := httptest.NewRecorder()
+	for _, scope := range []string{"", "global", "local", "others"} {
+		ts.SetupTest()
 
-	ts.API.handler.ServeHTTP(w, req)
-	require.Equal(ts.T(), http.StatusNoContent, w.Code)
+		reqURL, err := url.ParseRequestURI("http://localhost/logout")
+		require.NoError(ts.T(), err)
 
-	accessTokenKey := fmt.Sprintf("%v-access-token", ts.Config.Cookie.Key)
-	refreshTokenKey := fmt.Sprintf("%v-refresh-token", ts.Config.Cookie.Key)
-	for _, c := range w.Result().Cookies() {
-		if c.Name == accessTokenKey || c.Name == refreshTokenKey {
-			require.Equal(ts.T(), "", c.Value)
+		if scope != "" {
+			query := reqURL.Query()
+			query.Set("scope", scope)
+			reqURL.RawQuery = query.Encode()
+		}
+
+		req := httptest.NewRequest(http.MethodPost, reqURL.String(), nil)
+		req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", ts.token))
+		w := httptest.NewRecorder()
+
+		ts.API.handler.ServeHTTP(w, req)
+		require.Equal(ts.T(), http.StatusNoContent, w.Code)
+
+		accessTokenKey := fmt.Sprintf("%v-access-token", ts.Config.Cookie.Key)
+		refreshTokenKey := fmt.Sprintf("%v-refresh-token", ts.Config.Cookie.Key)
+		for _, c := range w.Result().Cookies() {
+			if c.Name == accessTokenKey || c.Name == refreshTokenKey {
+				require.Equal(ts.T(), "", c.Value)
+			}
 		}
 	}
 }
diff --git a/internal/mailer/mailer_test.go b/internal/mailer/mailer_test.go
index 1992f5a8..290d65dd 100644
--- a/internal/mailer/mailer_test.go
+++ b/internal/mailer/mailer_test.go
@@ -15,10 +15,15 @@ func enforceRelativeURL(url string) string {
 }
 
 func TestGetPath(t *testing.T) {
+	params := EmailParams{
+		Token:      "token",
+		Type:       "signup",
+		RedirectTo: "https://example.com",
+	}
 	cases := []struct {
 		SiteURL  string
 		Path     string
-		Params   map[string]string
+		Params   *EmailParams
 		Expected string
 	}{
 		{
@@ -40,29 +45,16 @@ func TestGetPath(t *testing.T) {
 			Expected: "https://test.example.com/trailingslash/",
 		},
 		{
-			SiteURL: "https://test.example.com",
-			Path:    "f",
-			Params: map[string]string{
-				"key": "val",
-			},
-			Expected: "https://test.example.com/f?key=val",
-		},
-		{
-			SiteURL: "https://test.example.com",
-			Path:    "",
-			Params: map[string]string{
-				"key": "val",
-			},
-			Expected: "https://test.example.com?key=val",
+			SiteURL:  "https://test.example.com",
+			Path:     "f",
+			Params:   &params,
+			Expected: "https://test.example.com/f?token=token&type=signup&redirect_to=https://example.com",
 		},
 		{
-			SiteURL: "https://test.example.com",
-			Path:    "",
-			Params: map[string]string{
-				"key":         "val",
-				"redirect_to": "http://localhost:3000?param=foo",
-			},
-			Expected: "https://test.example.com?key=val&redirect_to=http://localhost:3000?param=foo",
+			SiteURL:  "https://test.example.com",
+			Path:     "",
+			Params:   &params,
+			Expected: "https://test.example.com?token=token&type=signup&redirect_to=https://example.com",
 		},
 	}