From b832ee3a87ccbf7263740ed42adcfcb8d182ad0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ryan=20O=E2=80=99Hara?= Date: Sun, 24 Mar 2024 23:42:58 -0700 Subject: [PATCH] refactor: improve error handling in registration - log non-fatal errors - add missing responses and rollbacks on some error paths - use function-based transaction API to prevent missed rollbacks - clean up error type checking --- handlers/auth/register.go | 191 ++++++++++++++++++++------------------ 1 file changed, 103 insertions(+), 88 deletions(-) diff --git a/handlers/auth/register.go b/handlers/auth/register.go index 6e55b769..b5aa09b9 100644 --- a/handlers/auth/register.go +++ b/handlers/auth/register.go @@ -6,13 +6,32 @@ import ( "confesi/lib/response" "confesi/lib/utils" "confesi/lib/validation" + "errors" + "log/slog" "net/http" - "strings" "firebase.google.com/go/auth" "github.com/gin-gonic/gin" + "gorm.io/gorm" ) +type RegistrationError struct { + PublicMessage string + CustomCode int +} + +func (regErr *RegistrationError) Code() int { + if regErr.CustomCode != 0 { + return regErr.CustomCode + } + + return http.StatusBadRequest +} + +func (regErr *RegistrationError) Error() string { + return regErr.PublicMessage +} + // Example creating a Firebase user func (h *handler) handleRegister(c *gin.Context) { @@ -20,6 +39,7 @@ func (h *handler) handleRegister(c *gin.Context) { var req validation.CreateAccountDetails err := utils.New(c).Validate(&req) if err != nil { + response.New(http.StatusBadRequest).Err("invalid").Send(c) return } @@ -31,114 +51,109 @@ func (h *handler) handleRegister(c *gin.Context) { } // start a transaction - tx := h.db.Begin() - // if something goes ary, rollback - defer func() { - if r := recover(); r != nil { - tx.Rollback() - response.New(http.StatusInternalServerError).Err(serverError.Error()).Send(c) - return - } - }() - - // check if user's email is valid - var school db.School - err = tx.Select("id").Where("domain = ?", domain).First(&school).Error - if err != nil { - tx.Rollback() - response.New(http.StatusBadRequest).Err("domain doesn't belong to school").Send(c) - return - } - - // new firebase user - newUser := (&auth.UserToCreate{}). - Email(req.Email). - Password(req.Password). - Disabled(false) - - var firebaseUser *auth.UserRecord - userToSaveToPostgres := db.User{} - var userIdForPostgres string - // ensure the token is valid, aka, there is some valid user - if req.AlreadyExistingAccToken != "" { - - token, err := h.fb.AuthClient.VerifyIDToken(c, req.AlreadyExistingAccToken) - if err != nil { - response.New(http.StatusBadRequest).Err("invalid existing user token").Send(c) - return - } - - // check if this user has already been registered by email - _, err = h.fb.AuthClient.GetUserByEmail(c, req.Email) - if err == nil { - tx.Rollback() - response.New(http.StatusBadRequest).Err("account already upgraded").Send(c) - return - } - - // get firebase account by this UID - _, err = h.fb.AuthClient.GetUser(c, token.UID) + err = h.db.Transaction(func(tx *gorm.DB) error { + // check if user's email is valid + var school db.School + err := tx.Select("id").Where("domain = ?", domain).First(&school).Error if err != nil { - tx.Rollback() - response.New(http.StatusBadRequest).Err("invalid already existing account UID").Send(c) - return - } - // check if found user is anonymous - if token.Claims["provider_id"] != "anonymous" { - tx.Rollback() - response.New(http.StatusBadRequest).Err("already existing account is not anonymous").Send(c) - return + if errors.Is(err, gorm.ErrRecordNotFound) { + return &RegistrationError{PublicMessage: "domain doesn't belong to school"} + } + return err } // new firebase user - userToUpdate := (&auth.UserToUpdate{}). + newUser := (&auth.UserToCreate{}). Email(req.Email). Password(req.Password). Disabled(false) - _, err = h.fb.AuthClient.UpdateUser(c, token.UID, userToUpdate) - userIdForPostgres = token.UID - } else { - firebaseUser, err = h.fb.AuthClient.CreateUser(c, newUser) - if err == nil { - userIdForPostgres = firebaseUser.UID + var userIdForPostgres string + var firebaseError error + // ensure the token is valid, aka, there is some valid user + if req.AlreadyExistingAccToken != "" { + + token, err := h.fb.AuthClient.VerifyIDToken(c, req.AlreadyExistingAccToken) + if err != nil { + return &RegistrationError{PublicMessage: "invalid existing user token"} + } + + // check if this user has already been registered by email + _, err = h.fb.AuthClient.GetUserByEmail(c, req.Email) + if err == nil { + return &RegistrationError{PublicMessage: "account already upgraded"} + } + + // get firebase account by this UID + _, err = h.fb.AuthClient.GetUser(c, token.UID) + if err != nil { + return &RegistrationError{PublicMessage: "invalid already existing account UID"} + } + // check if found user is anonymous + if token.Claims["provider_id"] != "anonymous" { + return &RegistrationError{PublicMessage: "already existing account is not anonymous"} + } + + // new firebase user + userToUpdate := (&auth.UserToUpdate{}). + Email(req.Email). + Password(req.Password). + Disabled(false) + + _, firebaseError = h.fb.AuthClient.UpdateUser(c, token.UID, userToUpdate) + userIdForPostgres = token.UID + } else { + var firebaseUser *auth.UserRecord + firebaseUser, firebaseError = h.fb.AuthClient.CreateUser(c, newUser) + if firebaseError == nil { + userIdForPostgres = firebaseUser.UID + } } - } - if err != nil { - if strings.Contains(err.Error(), "EMAIL_EXISTS") { - tx.Rollback() - response.New(http.StatusConflict).Err("email already exists").Send(c) - } else { - tx.Rollback() - response.New(http.StatusInternalServerError).Err(serverError.Error()).Send(c) + if firebaseError != nil { + if auth.IsEmailAlreadyExists(firebaseError) { + return &RegistrationError{PublicMessage: "email already exists", CustomCode: http.StatusConflict} + } + return firebaseError } - return - } - userToSaveToPostgres.SchoolID = school.ID - userToSaveToPostgres.ID = userIdForPostgres + // save user to postgres + err = h.db.Create(&db.User{ + SchoolID: school.ID, + ID: userIdForPostgres, + }).Error + // we don't catch this error, because it will just show itself in the user's token as "sync: false" or DNE + if err != nil { + slog.Error("Failed to save user to Postgres", "error", err) + } - // save user to postgres - err = h.db.Create(&userToSaveToPostgres).Error - // we don't catch this error, because it will just show itself in the user's token as "sync: false" or DNE + // on success of both user being created in firebase and postgres, change their token to "double verified" via the "sync" field + h.fb.AuthClient.SetCustomUserClaims(c, userIdForPostgres, map[string]interface{}{ + "sync": true, + "roles": []string{}, //! default users have no roles, VERY IMPORTANT + }) + // we don't catch this error, because it will just show itself in the user's token as "sync: false" or DNE - // on success of both user being created in firebase and postgres, change their token to "double verified" via the "sync" field - h.fb.AuthClient.SetCustomUserClaims(c, userIdForPostgres, map[string]interface{}{ - "sync": true, - "roles": []string{}, //! default users have no roles, VERY IMPORTANT + return nil }) - // we don't catch this error, because it will just show itself in the user's token as "sync: false" or DNE - // commit the transaction - err = tx.Commit().Error if err != nil { - tx.Rollback() + var regErr *RegistrationError + if errors.As(err, ®Err) { + response.New(regErr.Code()).Err(regErr.PublicMessage).Send(c) + return + } + response.New(http.StatusInternalServerError).Err(serverError.Error()).Send(c) return } // send response & don't care if email sends - go email.SendVerificationEmail(c, h.fb.AuthClient, req.Email) + go func() { + err := email.SendVerificationEmail(c, h.fb.AuthClient, req.Email) + if err != nil { + slog.Error("Failed to send verification e-mail in registration", "error", err) + } + }() response.New(http.StatusCreated).Send(c) }