From 373735c2ede05c6a1149de3464a111392a634c64 Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Mon, 28 Oct 2024 14:55:32 +0800 Subject: [PATCH 1/3] chore(refactor): admin delete should use retrieveRequestParams --- internal/api/admin.go | 20 ++++++++------------ internal/api/helpers.go | 8 ++------ 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/internal/api/admin.go b/internal/api/admin.go index d6ff069ff9..d2c45aa8ff 100644 --- a/internal/api/admin.go +++ b/internal/api/admin.go @@ -2,7 +2,6 @@ package api import ( "context" - "encoding/json" "net/http" "time" @@ -512,21 +511,18 @@ func (a *API) adminUserDelete(w http.ResponseWriter, r *http.Request) error { user := getUser(ctx) adminUser := getAdminUser(ctx) - var err error + // ShouldSoftDelete defaults to false params := &adminUserDeleteParams{} - body, err := getBodyBytes(r) - if err != nil { - return internalServerError("Could not read body").WithInternalError(err) - } - if len(body) > 0 { - if err := json.Unmarshal(body, params); err != nil { - return badRequestError(ErrorCodeBadJSON, "Could not read params: %v", err) + if err := retrieveRequestParams(r, params); err != nil { + if err.(*HTTPError).ErrorCode == ErrorCodeBadJSON { + // request body is empty so the default behavior should be to hard delete the user + params.ShouldSoftDelete = false + } else { + return err } - } else { - params.ShouldSoftDelete = false } - err = a.db.Transaction(func(tx *storage.Connection) error { + err := a.db.Transaction(func(tx *storage.Connection) error { if terr := models.NewAuditLogEntry(r, tx, adminUser, models.UserDeletedAction, "", map[string]interface{}{ "user_id": user.ID, "user_email": user.Email, diff --git a/internal/api/helpers.go b/internal/api/helpers.go index 04458103b2..bcf4aff500 100644 --- a/internal/api/helpers.go +++ b/internal/api/helpers.go @@ -57,11 +57,6 @@ func isStringInSlice(checkValue string, list []string) bool { return false } -// getBodyBytes returns a byte array of the request's Body. -func getBodyBytes(req *http.Request) ([]byte, error) { - return utilities.GetBodyBytes(req) -} - type RequestParams interface { AdminUserParams | CreateSSOProviderParams | @@ -82,6 +77,7 @@ type RequestParams interface { VerifyFactorParams | VerifyParams | adminUserUpdateFactorParams | + adminUserDeleteParams | ChallengeFactorParams | struct { Email string `json:"email"` @@ -94,7 +90,7 @@ type RequestParams interface { // retrieveRequestParams is a generic method that unmarshals the request body into the params struct provided func retrieveRequestParams[A RequestParams](r *http.Request, params *A) error { - body, err := getBodyBytes(r) + body, err := utilities.GetBodyBytes(r) if err != nil { return internalServerError("Could not read body into byte slice").WithInternalError(err) } From 834d03c46dfc2f2c76370431aa2e52bbb4d57f4e Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Mon, 28 Oct 2024 15:22:10 +0800 Subject: [PATCH 2/3] chore(refactor): use retrieveRequestParams in captcha middleware --- internal/api/helpers.go | 2 ++ internal/api/middleware.go | 12 +++++++----- internal/security/captcha.go | 15 ++------------- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/internal/api/helpers.go b/internal/api/helpers.go index bcf4aff500..8a9f3267ed 100644 --- a/internal/api/helpers.go +++ b/internal/api/helpers.go @@ -9,6 +9,7 @@ import ( "github.com/pkg/errors" "github.com/supabase/auth/internal/conf" "github.com/supabase/auth/internal/models" + "github.com/supabase/auth/internal/security" "github.com/supabase/auth/internal/utilities" ) @@ -78,6 +79,7 @@ type RequestParams interface { VerifyParams | adminUserUpdateFactorParams | adminUserDeleteParams | + security.GotrueRequest | ChallengeFactorParams | struct { Email string `json:"email"` diff --git a/internal/api/middleware.go b/internal/api/middleware.go index 853b8d528d..ff821e14ac 100644 --- a/internal/api/middleware.go +++ b/internal/api/middleware.go @@ -15,6 +15,7 @@ import ( "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/observability" "github.com/supabase/auth/internal/security" + "github.com/supabase/auth/internal/utilities" "github.com/didip/tollbooth/v5" "github.com/didip/tollbooth/v5/limiter" @@ -116,12 +117,13 @@ func (a *API) verifyCaptcha(w http.ResponseWriter, req *http.Request) (context.C return ctx, nil } - verificationResult, err := security.VerifyRequest(req, strings.TrimSpace(config.Security.Captcha.Secret), config.Security.Captcha.Provider) - if err != nil { - if strings.Contains(err.Error(), "request body was not JSON") { - return nil, badRequestError(ErrorCodeValidationFailed, "Request body for CAPTCHA verification was not a valid JSON object") - } + body := &security.GotrueRequest{} + if err := retrieveRequestParams(req, body); err != nil { + return nil, err + } + verificationResult, err := security.VerifyRequest(body, utilities.GetIPAddress(req), strings.TrimSpace(config.Security.Captcha.Secret), config.Security.Captcha.Provider) + if err != nil { return nil, internalServerError("captcha verification process failed").WithInternalError(err) } diff --git a/internal/security/captcha.go b/internal/security/captcha.go index 051a33fa72..aeacb6338b 100644 --- a/internal/security/captcha.go +++ b/internal/security/captcha.go @@ -11,6 +11,7 @@ import ( "time" "fmt" + "github.com/pkg/errors" "github.com/supabase/auth/internal/utilities" ) @@ -45,25 +46,13 @@ func init() { Client = &http.Client{Timeout: defaultTimeout} } -func VerifyRequest(r *http.Request, secretKey, captchaProvider string) (VerificationResponse, error) { - bodyBytes, err := utilities.GetBodyBytes(r) - if err != nil { - return VerificationResponse{}, err - } - - var requestBody GotrueRequest - - if err := json.Unmarshal(bodyBytes, &requestBody); err != nil { - return VerificationResponse{}, errors.Wrap(err, "request body was not JSON") - } - +func VerifyRequest(requestBody *GotrueRequest, clientIP, secretKey, captchaProvider string) (VerificationResponse, error) { captchaResponse := strings.TrimSpace(requestBody.Security.Token) if captchaResponse == "" { return VerificationResponse{}, errors.New("no captcha response (captcha_token) found in request") } - clientIP := utilities.GetIPAddress(r) captchaURL, err := GetCaptchaURL(captchaProvider) if err != nil { return VerificationResponse{}, err From 05c4e2e0b487411d1ee46e89733635fa72f6376a Mon Sep 17 00:00:00 2001 From: Kang Ming Date: Mon, 28 Oct 2024 17:11:16 +0800 Subject: [PATCH 3/3] chore: only parse body if it's non-empty --- internal/api/admin.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/api/admin.go b/internal/api/admin.go index d2c45aa8ff..63cde06a6e 100644 --- a/internal/api/admin.go +++ b/internal/api/admin.go @@ -14,6 +14,7 @@ import ( "github.com/supabase/auth/internal/models" "github.com/supabase/auth/internal/observability" "github.com/supabase/auth/internal/storage" + "github.com/supabase/auth/internal/utilities" "golang.org/x/crypto/bcrypt" ) @@ -513,11 +514,10 @@ func (a *API) adminUserDelete(w http.ResponseWriter, r *http.Request) error { // ShouldSoftDelete defaults to false params := &adminUserDeleteParams{} - if err := retrieveRequestParams(r, params); err != nil { - if err.(*HTTPError).ErrorCode == ErrorCodeBadJSON { - // request body is empty so the default behavior should be to hard delete the user - params.ShouldSoftDelete = false - } else { + if body, _ := utilities.GetBodyBytes(r); len(body) != 0 { + // we only want to parse the body if it's not empty + // retrieveRequestParams will handle any errors with stream + if err := retrieveRequestParams(r, params); err != nil { return err } }