From 5eeff97ffe4af48ccad83c3bcffb370879af2880 Mon Sep 17 00:00:00 2001 From: kkrime <66622793+kkrime@users.noreply.github.com> Date: Wed, 29 Jan 2025 10:29:00 +0000 Subject: [PATCH] feat(session/v2): user password lockout error response (#9233) # Which Problems Are Solved Adds `failed attempts` field to the grpc response when a user enters wrong password when logging in FYI: this only covers the senario above; other senarios where this is not applied are: SetPasswordWithVerifyCode setPassword ChangPassword setPasswordWithPermission # How the Problems Are Solved Created new grpc message `CredentialsCheckError` - `proto/zitadel/message.proto` to include `failed_attempts` field. Had to create a new package - `github.com/zitadel/zitadel/internal/command/errors` to resolve cycle dependency between `github.com/zitadel/zitadel/internal/command` and `github.com/zitadel/zitadel/internal/command`. # Additional Changes - none # Additional Context - Closes https://github.com/zitadel/zitadel/issues/9198 --------- Co-authored-by: Iraq Jaber --- internal/api/grpc/gerrors/zitadel_errors.go | 19 ++++++- .../api/grpc/gerrors/zitadel_errors_test.go | 53 +++++++++++++++++++ internal/command/errors/errors.go | 9 ++++ internal/command/user_human_password.go | 28 ++++++++-- proto/zitadel/message.proto | 16 ++++-- .../zitadel/session/v2/session_service.proto | 4 +- 6 files changed, 118 insertions(+), 11 deletions(-) create mode 100644 internal/command/errors/errors.go diff --git a/internal/api/grpc/gerrors/zitadel_errors.go b/internal/api/grpc/gerrors/zitadel_errors.go index 8e131c2d3..d679054da 100644 --- a/internal/api/grpc/gerrors/zitadel_errors.go +++ b/internal/api/grpc/gerrors/zitadel_errors.go @@ -7,7 +7,9 @@ import ( "github.com/zitadel/logging" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + "google.golang.org/protobuf/protoadapt" + commandErrors "github.com/zitadel/zitadel/internal/command/errors" "github.com/zitadel/zitadel/internal/zerrors" "github.com/zitadel/zitadel/pkg/grpc/message" ) @@ -23,7 +25,9 @@ func ZITADELToGRPCError(err error) error { msg := key msg += " (" + id + ")" - s, err := status.New(code, msg).WithDetails(&message.ErrorDetail{Id: id, Message: key}) + errorInfo := getErrorInfo(id, key, err) + + s, err := status.New(code, msg).WithDetails(errorInfo) if err != nil { logging.WithError(err).WithField("logID", "GRPC-gIeRw").Debug("unable to add detail") return status.New(code, msg).Err() @@ -71,3 +75,16 @@ func ExtractZITADELError(err error) (c codes.Code, msg, id string, ok bool) { return codes.Unknown, err.Error(), "", false } } + +func getErrorInfo(id, key string, err error) protoadapt.MessageV1 { + var errorInfo protoadapt.MessageV1 + + var wpe *commandErrors.WrongPasswordError + if err != nil && errors.As(err, &wpe) { + errorInfo = &message.CredentialsCheckError{Id: id, Message: key, FailedAttempts: wpe.FailedAttempts} + } else { + errorInfo = &message.ErrorDetail{Id: id, Message: key} + } + + return errorInfo +} diff --git a/internal/api/grpc/gerrors/zitadel_errors_test.go b/internal/api/grpc/gerrors/zitadel_errors_test.go index 4e5604790..1d41825fd 100644 --- a/internal/api/grpc/gerrors/zitadel_errors_test.go +++ b/internal/api/grpc/gerrors/zitadel_errors_test.go @@ -2,11 +2,16 @@ package gerrors import ( "errors" + "fmt" "testing" + "github.com/stretchr/testify/assert" "google.golang.org/grpc/codes" + "google.golang.org/protobuf/protoadapt" + commandErrors "github.com/zitadel/zitadel/internal/command/errors" "github.com/zitadel/zitadel/internal/zerrors" + "github.com/zitadel/zitadel/pkg/grpc/message" ) func TestCaosToGRPCError(t *testing.T) { @@ -43,6 +48,54 @@ func TestCaosToGRPCError(t *testing.T) { } } +func Test_getErrorInfo(t *testing.T) { + tests := []struct { + name string + id string + key string + err error + result protoadapt.MessageV1 + }{ + { + name: "parent error nil, return message.ErrorDetail{}", + id: "id", + key: "key", + result: &message.ErrorDetail{Id: "id", Message: "key"}, + }, + { + name: "parent error nil not commandErrors.WrongPasswordError{}, return message.ErrorDetail{}", + id: "id", + key: "key", + err: fmt.Errorf("normal error not commandErrors.WrongPasswordError{}"), + result: &message.ErrorDetail{Id: "id", Message: "key"}, + }, + { + name: "parent error not nil type commandErrors.WrongPasswordError{}, return message.CredentialsCheckError{}", + id: "id", + key: "key", + err: &commandErrors.WrongPasswordError{FailedAttempts: 22}, + result: &message.CredentialsCheckError{Id: "id", Message: "key", FailedAttempts: 22}, + }, + { + name: "parent error not nil wrapped commandErrors.WrongPasswordError{}, return message.CredentialsCheckError{}", + id: "id", + key: "key", + err: func() error { + err := fmt.Errorf("normal error") + wpa := &commandErrors.WrongPasswordError{FailedAttempts: 26} + return fmt.Errorf("%w: %w", err, wpa) + }(), + result: &message.CredentialsCheckError{Id: "id", Message: "key", FailedAttempts: 26}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errorInfo := getErrorInfo(tt.id, tt.key, tt.err) + assert.Equal(t, tt.result, errorInfo) + }) + } +} + func Test_Extract(t *testing.T) { type args struct { err error diff --git a/internal/command/errors/errors.go b/internal/command/errors/errors.go new file mode 100644 index 000000000..45f2f5a4d --- /dev/null +++ b/internal/command/errors/errors.go @@ -0,0 +1,9 @@ +package errors + +type WrongPasswordError struct { + FailedAttempts int32 +} + +func (wpe *WrongPasswordError) Error() string { + return "" +} diff --git a/internal/command/user_human_password.go b/internal/command/user_human_password.go index a67a4b91d..1d91dc05e 100644 --- a/internal/command/user_human_password.go +++ b/internal/command/user_human_password.go @@ -3,11 +3,13 @@ package command import ( "context" "errors" + "fmt" "time" "github.com/zitadel/logging" "github.com/zitadel/passwap" + commandErrors "github.com/zitadel/zitadel/internal/command/errors" "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" @@ -364,7 +366,10 @@ func checkPassword(ctx context.Context, userID, password string, es *eventstore. return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-3n77z", "Errors.User.NotFound") } if wm.UserState == domain.UserStateLocked { - return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-JLK35", "Errors.User.Locked") + wrongPasswordError := &commandErrors.WrongPasswordError{ + FailedAttempts: int32(wm.PasswordCheckFailedCount), + } + return nil, zerrors.ThrowPreconditionFailed(wrongPasswordError, "COMMAND-JLK35", "Errors.User.Locked") } if wm.EncodedHash == "" { return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-3nJ4t", "Errors.User.Password.NotSet") @@ -374,7 +379,7 @@ func checkPassword(ctx context.Context, userID, password string, es *eventstore. ctx, spanPasswordComparison := tracing.NewNamedSpan(ctx, "passwap.Verify") updated, err := hasher.Verify(wm.EncodedHash, password) spanPasswordComparison.EndWithError(err) - err = convertPasswapErr(err) + err = convertLoginPasswapErr(wm.PasswordCheckFailedCount+1, err) commands := make([]eventstore.Command, 0, 2) // recheck for additional events (failed password checks or locks) @@ -383,7 +388,10 @@ func checkPassword(ctx context.Context, userID, password string, es *eventstore. return nil, recheckErr } if wm.UserState == domain.UserStateLocked { - return nil, zerrors.ThrowPreconditionFailed(nil, "COMMAND-SFA3t", "Errors.User.Locked") + wrongPasswordError := &commandErrors.WrongPasswordError{ + FailedAttempts: int32(wm.PasswordCheckFailedCount), + } + return nil, zerrors.ThrowPreconditionFailed(wrongPasswordError, "COMMAND-SFA3t", "Errors.User.Locked") } if err == nil { @@ -416,6 +424,20 @@ func (c *Commands) passwordWriteModel(ctx context.Context, userID, resourceOwner return writeModel, nil } +func convertLoginPasswapErr(passwordCheckFailedCount uint64, err error) error { + if err == nil { + return nil + } + if errors.Is(err, passwap.ErrPasswordMismatch) { + wrongPasswordError := &commandErrors.WrongPasswordError{ + FailedAttempts: int32(passwordCheckFailedCount), + } + err = fmt.Errorf("%w: %w", err, wrongPasswordError) + return ErrPasswordInvalid(err) + } + return zerrors.ThrowInternal(err, "COMMAND-CahN2", "Errors.Internal") +} + func convertPasswapErr(err error) error { if err == nil { return nil diff --git a/proto/zitadel/message.proto b/proto/zitadel/message.proto index ede9ab529..97345b613 100644 --- a/proto/zitadel/message.proto +++ b/proto/zitadel/message.proto @@ -5,11 +5,17 @@ package zitadel.v1; option go_package = "github.com/zitadel/zitadel/pkg/grpc/message"; message ErrorDetail { - string id = 1; - string message = 2; + string id = 1; + string message = 2; +} + +message CredentialsCheckError { + string id = 1; + string message = 2; + int32 failed_attempts = 3; } message LocalizedMessage { - string key = 1; - string localized_message = 2; -} \ No newline at end of file + string key = 1; + string localized_message = 2; +} diff --git a/proto/zitadel/session/v2/session_service.proto b/proto/zitadel/session/v2/session_service.proto index f531c0a59..74e20263f 100644 --- a/proto/zitadel/session/v2/session_service.proto +++ b/proto/zitadel/session/v2/session_service.proto @@ -379,7 +379,7 @@ message Checks { ]; optional CheckPassword password = 2 [ (grpc.gateway.protoc_gen_openapiv2.options.openapiv2_field) = { - description: "\"Checks the password and updates the session on success. Requires that the user is already checked, either in the previous or the same request.\""; + description: "\"Checks the password and updates the session on success. Requires that the user is already checked, either in the previous or the same request. On failed password check id: \"COMMAND-3M0fs\" wll be returned. On user locked out id: \"COMMAND-JLK35\"/\"COMMAND-SFA3t\" will be returned\""; } ]; optional CheckWebAuthN web_auth_n = 3 [ @@ -493,4 +493,4 @@ message CheckOTP { example: "\"3237642\""; } ]; -} \ No newline at end of file +}