From 1ede54ddf6dfd2d4ba039eb7e23b74bcac65b643 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Wed, 8 Jun 2022 20:38:03 +0200 Subject: [feature] More consistent API error handling (#637) * update templates * start reworking api error handling * update template * return AP status at web endpoint if negotiated * start making api error handling much more consistent * update account endpoints to new error handling * use new api error handling in admin endpoints * go fmt ./... * use api error logic in app * use generic error handling in auth * don't export generic error handler * don't defer clearing session * user nicer error handling on oidc callback handler * tidy up the sign in handler * tidy up the token handler * use nicer error handling in blocksget * auth emojis endpoint * fix up remaining api endpoints * fix whoopsie during login flow * regenerate swagger docs * change http error logging to debug --- internal/api/client/auth/callback.go | 95 +++++++++++++++++++++++------------- 1 file changed, 60 insertions(+), 35 deletions(-) (limited to 'internal/api/client/auth/callback.go') diff --git a/internal/api/client/auth/callback.go b/internal/api/client/auth/callback.go index a5c58647c..34a4995c8 100644 --- a/internal/api/client/auth/callback.go +++ b/internal/api/client/auth/callback.go @@ -30,7 +30,9 @@ import ( "github.com/gin-contrib/sessions" "github.com/gin-gonic/gin" "github.com/google/uuid" + "github.com/superseriousbusiness/gotosocial/internal/api" "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/oidc" "github.com/superseriousbusiness/gotosocial/internal/validate" @@ -40,11 +42,14 @@ import ( func (m *Module) CallbackGETHandler(c *gin.Context) { s := sessions.Default(c) - // first make sure the state set in the cookie is the same as the state returned from the external provider + // check the query vs session state parameter to mitigate csrf + // https://auth0.com/docs/secure/attack-protection/state-parameters + state := c.Query(callbackStateParam) if state == "" { m.clearSession(s) - c.JSON(http.StatusForbidden, gin.H{"error": "state query not found on callback"}) + err := fmt.Errorf("%s parameter not found on callback query", callbackStateParam) + api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGet) return } @@ -52,84 +57,104 @@ func (m *Module) CallbackGETHandler(c *gin.Context) { savedState, ok := savedStateI.(string) if !ok { m.clearSession(s) - c.JSON(http.StatusForbidden, gin.H{"error": "state not found in session"}) + err := fmt.Errorf("key %s was not found in session", sessionState) + api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGet) return } if state != savedState { m.clearSession(s) - c.JSON(http.StatusForbidden, gin.H{"error": "state mismatch"}) + err := errors.New("mismatch between query state and session state") + api.ErrorHandler(c, gtserror.NewErrorUnauthorized(err, err.Error()), m.processor.InstanceGet) return } + // retrieve stored claims using code code := c.Query(callbackCodeParam) + if code == "" { + m.clearSession(s) + err := fmt.Errorf("%s parameter not found on callback query", callbackCodeParam) + api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, err.Error()), m.processor.InstanceGet) + return + } - claims, err := m.idp.HandleCallback(c.Request.Context(), code) - if err != nil { + claims, errWithCode := m.idp.HandleCallback(c.Request.Context(), code) + if errWithCode != nil { m.clearSession(s) - c.JSON(http.StatusForbidden, gin.H{"error": err.Error()}) + api.ErrorHandler(c, errWithCode, m.processor.InstanceGet) return } - // We can use the client_id on the session to retrieve info about the app associated with the client_id + // We can use the client_id on the session to retrieve + // info about the app associated with the client_id clientID, ok := s.Get(sessionClientID).(string) if !ok || clientID == "" { m.clearSession(s) - c.JSON(http.StatusInternalServerError, gin.H{"error": "no client_id found in session during callback"}) + err := fmt.Errorf("key %s was not found in session", sessionClientID) + api.ErrorHandler(c, gtserror.NewErrorBadRequest(err, helpfulAdvice), m.processor.InstanceGet) return } + app := >smodel.Application{} if err := m.db.GetWhere(c.Request.Context(), []db.Where{{Key: sessionClientID, Value: clientID}}, app); err != nil { m.clearSession(s) - c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("no application found for client id %s", clientID)}) + safe := fmt.Sprintf("application for %s %s could not be retrieved", sessionClientID, clientID) + var errWithCode gtserror.WithCode + if err == db.ErrNoEntries { + errWithCode = gtserror.NewErrorBadRequest(err, safe, helpfulAdvice) + } else { + errWithCode = gtserror.NewErrorInternalError(err, safe, helpfulAdvice) + } + api.ErrorHandler(c, errWithCode, m.processor.InstanceGet) return } - user, err := m.parseUserFromClaims(c.Request.Context(), claims, net.IP(c.ClientIP()), app.ID) - if err != nil { + user, errWithCode := m.parseUserFromClaims(c.Request.Context(), claims, net.IP(c.ClientIP()), app.ID) + if errWithCode != nil { m.clearSession(s) - c.JSON(http.StatusForbidden, gin.H{"error": err.Error()}) + api.ErrorHandler(c, errWithCode, m.processor.InstanceGet) return } s.Set(sessionUserID, user.ID) if err := s.Save(); err != nil { m.clearSession(s) - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + api.ErrorHandler(c, gtserror.NewErrorInternalError(err), m.processor.InstanceGet) return } c.Redirect(http.StatusFound, OauthAuthorizePath) } -func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, ip net.IP, appID string) (*gtsmodel.User, error) { +func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, ip net.IP, appID string) (*gtsmodel.User, gtserror.WithCode) { if claims.Email == "" { - return nil, errors.New("no email returned in claims") + err := errors.New("no email returned in claims") + return nil, gtserror.NewErrorBadRequest(err, err.Error()) } // see if we already have a user for this email address + // if so, we don't need to continue + create one user := >smodel.User{} err := m.db.GetWhere(ctx, []db.Where{{Key: "email", Value: claims.Email}}, user) if err == nil { - // we do! so we can just return it return user, nil } if err != db.ErrNoEntries { - // we have an actual error in the database - return nil, fmt.Errorf("error checking database for email %s: %s", claims.Email, err) + err := fmt.Errorf("error checking database for email %s: %s", claims.Email, err) + return nil, gtserror.NewErrorInternalError(err) } // maybe we have an unconfirmed user err = m.db.GetWhere(ctx, []db.Where{{Key: "unconfirmed_email", Value: claims.Email}}, user) if err == nil { - // user is unconfirmed so return an error - return nil, fmt.Errorf("user with email address %s is unconfirmed", claims.Email) + err := fmt.Errorf("user with email address %s is unconfirmed", claims.Email) + return nil, gtserror.NewErrorForbidden(err, err.Error()) } if err != db.ErrNoEntries { - // we have an actual error in the database - return nil, fmt.Errorf("error checking database for email %s: %s", claims.Email, err) + err := fmt.Errorf("error checking database for email %s: %s", claims.Email, err) + return nil, gtserror.NewErrorInternalError(err) } // we don't have a confirmed or unconfirmed user with the claimed email address @@ -138,10 +163,10 @@ func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, i // check if the email address is available for use; if it's not there's nothing we can so emailAvailable, err := m.db.IsEmailAvailable(ctx, claims.Email) if err != nil { - return nil, fmt.Errorf("email %s not available: %s", claims.Email, err) + return nil, gtserror.NewErrorBadRequest(err) } if !emailAvailable { - return nil, fmt.Errorf("email %s in use", claims.Email) + return nil, gtserror.NewErrorConflict(fmt.Errorf("email address %s is not available", claims.Email)) } // now we need a username @@ -149,12 +174,12 @@ func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, i // make sure claims.Name is defined since we'll be using that for the username if claims.Name == "" { - return nil, errors.New("no name returned in claims") + err := errors.New("no name returned in claims") + return nil, gtserror.NewErrorBadRequest(err, err.Error()) } // check if we can just use claims.Name as-is - err = validate.Username(claims.Name) - if err == nil { + if err = validate.Username(claims.Name); err == nil { // the name we have on the claims is already a valid username username = claims.Name } else { @@ -166,12 +191,12 @@ func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, i // lowercase the whole thing lower := strings.ToLower(underscored) // see if this is valid.... - if err := validate.Username(lower); err == nil { - // we managed to get a valid username - username = lower - } else { - return nil, fmt.Errorf("couldn't parse a valid username from claims.Name value of %s", claims.Name) + if err := validate.Username(lower); err != nil { + err := fmt.Errorf("couldn't parse a valid username from claims.Name value of %s: %s", claims.Name, err) + return nil, gtserror.NewErrorBadRequest(err, err.Error()) } + // we managed to get a valid username + username = lower } var iString string @@ -185,7 +210,7 @@ func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, i for i := 1; !found; i++ { usernameAvailable, err := m.db.IsUsernameAvailable(ctx, username+iString) if err != nil { - return nil, err + return nil, gtserror.NewErrorInternalError(err) } if usernameAvailable { // no error so we've found a username that works @@ -223,7 +248,7 @@ func (m *Module) parseUserFromClaims(ctx context.Context, claims *oidc.Claims, i // create the user! this will also create an account and store it in the database so we don't need to do that here user, err = m.db.NewSignup(ctx, username, "", requireApproval, claims.Email, password, ip, "", appID, emailVerified, admin) if err != nil { - return nil, fmt.Errorf("error creating user: %s", err) + return nil, gtserror.NewErrorInternalError(err) } return user, nil -- cgit v1.2.3