Skip to content

Commit

Permalink
satellite/console: Don't lose ErrValiation error class
Browse files Browse the repository at this point in the history
There was a defined type (`validationErrors`) for gathering several
validation errors and classify them with the `ErrValdiation errs.Class`.

`errs.Combine` doesn't maintain the classes of the errors to combine,
for example

```
var myClass errs.Class = "My error class"

err1 := myClass.Wrap(erros.New("error 1"))
err2 := myClass.Wrap(erros.New("error 2"))
err3 := errors.New("error 3")

combinedErr := errs.Combine(err1, err2, err3)
myClass.Has(combinedErr) // It returns false

// Even only passing errors with a class and with the same one for all
// of them
combinedErr := errs.Combine(err1, err2)
myClass.Has(combinedErr) // It returns false
```

Hence `validationErrors` didn't return what we expected to return when
calling its `Combine` method.

This commit delete the type and it replaces by `errs.Group` when there
are more than one error, and wrapping the `errs.Group.Err` returned
error with `ErrValiation` error class.

The bug caused the HTTP API server to return a 500 status code as you
can seee in the following log message extracted from the satellite
production logs:

```
code: 500
error: "console service: validation: full name can not be empty; validation: Your password needs at least 6 characters long; validation: mail: no address"
errorVerbose: "console service: validation: full name can not be empty; validation: Your password needs at least 6 characters long; validation: mail: no address
        storj.io/storj/satellite/console.(*Service).CreateUser:593
        storj.io/storj/satellite/console/consoleweb/consoleapi.(*Auth).Register:250
        net/http.HandlerFunc.ServeHTTP:2047
        storj.io/storj/private/web.(*RateLimiter).Limit.func1:90
        net/http.HandlerFunc.ServeHTTP:2047
        github.com/gorilla/mux.(*Router).ServeHTTP:210
        storj.io/storj/satellite/console/consoleweb.(*Server).withRequest.func1:464
        net/http.HandlerFunc.ServeHTTP:2047
        net/http.serverHandler.ServeHTTP:2879
        net/http.(*conn).serve:1930"
message: "There was an error processing your request"
```

The issues was that not being classified with `ErrValidation` class it
was not picked by the correct switch branch of the
`consoleapi.Auth.getStatusCode` method which is in the call chain to
`consoleapi.Auth.Register` method when it calls
`console.Service.CreateUser` and returns an error.

These changes should return the appropriated HTTP status code (Bad
Request) when `console.Service.CreateUser` returns a validation error.

Returning the appropriated HTTP statsus code also makes not to show this
as an error in the server logs because the Bad Request sttatus code gets
logged with debug level.

Change-Id: I869ea85788992ae0865c373860fbf93a40d2d387
  • Loading branch information
ifraixedes committed Feb 28, 2022
1 parent c0297ba commit 8caa4c4
Show file tree
Hide file tree
Showing 4 changed files with 23 additions and 36 deletions.
6 changes: 3 additions & 3 deletions satellite/console/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,8 @@ func (s *Service) CreateUser(ctx context.Context, user CreateUser, tokenSecret R
}

if err := user.IsValid(); err != nil {
return nil, Error.Wrap(err)
// NOTE: error is already wrapped with an appropriated class.
return nil, err
}

registrationToken, err := s.checkRegistrationSecret(ctx, tokenSecret)
Expand Down Expand Up @@ -790,7 +791,7 @@ func (s *Service) ResetPassword(ctx context.Context, resetPasswordToken, passwor
}

if err := ValidatePassword(password); err != nil {
return Error.Wrap(err)
return ErrValidation.Wrap(err)
}

if t.Sub(token.CreatedAt) > s.config.TokenExpirationTime {
Expand Down Expand Up @@ -1328,7 +1329,6 @@ func (s *Service) DeleteProjectMembers(ctx context.Context, projectID uuid.UUID,
// collect user querying errors
for _, email := range emails {
user, err := s.store.Users().GetByEmail(ctx, email)

if err != nil {
userErr.Add(err)
continue
Expand Down
3 changes: 2 additions & 1 deletion satellite/console/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import (

func TestService(t *testing.T) {
testplanet.Run(t, testplanet.Config{
SatelliteCount: 1, StorageNodeCount: 0, UplinkCount: 2},
SatelliteCount: 1, StorageNodeCount: 0, UplinkCount: 2,
},
func(t *testing.T, ctx *testcontext.Context, planet *testplanet.Planet) {
sat := planet.Satellites[0]
service := sat.API.Console.Service
Expand Down
24 changes: 14 additions & 10 deletions satellite/console/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"net/mail"
"time"

"github.com/zeebo/errs"

"storj.io/common/memory"
"storj.io/common/uuid"
)
Expand Down Expand Up @@ -45,15 +47,14 @@ type UserInfo struct {
}

// IsValid checks UserInfo validity and returns error describing whats wrong.
// The returned error has the class ErrValiation.
func (user *UserInfo) IsValid() error {
var errs validationErrors

// validate fullName
if err := ValidateFullName(user.FullName); err != nil {
errs.AddWrap(err)
return ErrValidation.Wrap(err)
}

return errs.Combine()
return nil
}

// CreateUser struct holds info for User creation.
Expand All @@ -76,24 +77,27 @@ type CreateUser struct {
}

// IsValid checks CreateUser validity and returns error describing whats wrong.
// The returned error has the class ErrValiation.
func (user *CreateUser) IsValid() error {
var errs validationErrors
errgrp := errs.Group{}

errs.AddWrap(ValidateFullName(user.FullName))
errs.AddWrap(ValidatePassword(user.Password))
errgrp.Add(
ValidateFullName(user.FullName),
ValidatePassword(user.Password),
)

// validate email
_, err := mail.ParseAddress(user.Email)
errs.AddWrap(err)
errgrp.Add(err)

if user.PartnerID != "" {
_, err := uuid.FromString(user.PartnerID)
if err != nil {
errs.AddWrap(err)
errgrp.Add(err)
}
}

return errs.Combine()
return ErrValidation.Wrap(errgrp.Err())
}

// ProjectLimits holds info for a users bandwidth and storage limits for new projects.
Expand Down
26 changes: 4 additions & 22 deletions satellite/console/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,36 +14,18 @@ const (
// ErrValidation validation related error class.
var ErrValidation = errs.Class("validation")

// validationError is slice of ErrValidation class errors.
type validationErrors []error

// Addf adds a new ErrValidation error to validation.
func (validation *validationErrors) Addf(format string, args ...interface{}) {
*validation = append(*validation, ErrValidation.New(format, args...))
}

// AddWrap adds new ErrValidation wrapped err.
func (validation *validationErrors) AddWrap(err error) {
*validation = append(*validation, ErrValidation.Wrap(err))
}

// Combine returns combined validation errors.
func (validation *validationErrors) Combine() error {
return errs.Combine(*validation...)
}

// ValidatePassword validates password.
// It returns an plain error (not wrapped in a errs.Class) if pass is invalid.
func ValidatePassword(pass string) error {
var errs validationErrors

if len(pass) < passMinLength {
errs.Addf(passwordIncorrectErrMsg, passMinLength)
return errs.New(passwordIncorrectErrMsg, passMinLength)
}

return errs.Combine()
return nil
}

// ValidateFullName validates full name.
// It returns an plain error (not wrapped in a errs.Class) if name is invalid.
func ValidateFullName(name string) error {
if name == "" {
return errs.New("full name can not be empty")
Expand Down

0 comments on commit 8caa4c4

Please sign in to comment.