From 06315c55b8a2838b6d13ef9fc278d947574d356d Mon Sep 17 00:00:00 2001 From: Peter Grlica Date: Fri, 1 Jul 2022 16:33:02 +0200 Subject: [PATCH] Forbid password reuse --- auth/handlers/handle_change-password.go | 2 +- auth/handlers/handle_change-password_test.go | 2 +- system/service/auth.go | 42 ++++++++++++++++---- system/service/auth_actions.gen.go | 42 ++++++++++++++++++-- system/service/auth_actions.yaml | 5 ++- system/service/auth_test.go | 2 +- system/service/user.go | 26 +++++++++--- system/types/credential.go | 7 ++-- 8 files changed, 103 insertions(+), 25 deletions(-) diff --git a/auth/handlers/handle_change-password.go b/auth/handlers/handle_change-password.go index f64e74e92..9ec298e84 100644 --- a/auth/handlers/handle_change-password.go +++ b/auth/handlers/handle_change-password.go @@ -38,7 +38,7 @@ func (h *AuthHandlers) changePasswordProc(req *request.AuthReq) (err error) { case service.AuthErrInternalLoginDisabledByConfig().Is(err), service.AuthErrPasswordNotSecure().Is(err), service.AuthErrPasswordChangeFailedForUnknownUser().Is(err), - service.AuthErrPasswodResetFailedOldPasswordCheckFailed().Is(err): + service.AuthErrPasswordResetFailedOldPasswordCheckFailed().Is(err): req.SetKV(map[string]string{ "error": err.Error(), }) diff --git a/auth/handlers/handle_change-password_test.go b/auth/handlers/handle_change-password_test.go index 6cd097736..b03b86458 100644 --- a/auth/handlers/handle_change-password_test.go +++ b/auth/handlers/handle_change-password_test.go @@ -102,7 +102,7 @@ func Test_changePasswordProc(t *testing.T) { fn: func(_ *settings.Settings) { authService = &authServiceMocked{ changePassword: func(ctx context.Context, userID uint64, oldPassword, newPassword string) (err error) { - return service.AuthErrPasswodResetFailedOldPasswordCheckFailed() + return service.AuthErrPasswordResetFailedOldPasswordCheckFailed() }, } }, diff --git a/system/service/auth.go b/system/service/auth.go index 2ec3a77cd..33aaa0a5c 100644 --- a/system/service/auth.go +++ b/system/service/auth.go @@ -352,7 +352,7 @@ func (svc auth) InternalSignUp(ctx context.Context, input *types.User, password return err } - if c = cc.CompareHashAndPassword(password); c == nil { + if c = cc.CompareHashAndPassword(password, true); c == nil { return AuthErrInvalidCredentials(aam) } @@ -491,7 +491,7 @@ func (svc auth) InternalLogin(ctx context.Context, email string, password string return err } - c := cc.CompareHashAndPassword(password) + c := cc.CompareHashAndPassword(password, true) if c == nil { return AuthErrInvalidCredentials(aam) } @@ -506,8 +506,8 @@ func (svc auth) InternalLogin(ctx context.Context, email string, password string } // checkPassword returns true if given (encrypted) password matches any of the credentials -func (svc auth) checkPassword(password string, cc types.CredentialSet) bool { - return cc.CompareHashAndPassword(password) != nil +func (svc auth) CheckPassword(password string, validOnly bool, cc types.CredentialSet) bool { + return cc.CompareHashAndPassword(password, validOnly) != nil } // SetPassword sets new password for a user @@ -515,7 +515,8 @@ func (svc auth) checkPassword(password string, cc types.CredentialSet) bool { // This function also records an action func (svc auth) SetPassword(ctx context.Context, userID uint64, password string) (err error) { var ( - u *types.User + u *types.User + cc types.CredentialSet aam = &authActionProps{ user: u, @@ -540,6 +541,23 @@ func (svc auth) SetPassword(ctx context.Context, userID uint64, password string) aam.setUser(u) ctx = internalAuth.SetIdentityToContext(ctx, u) + cc, _, err = store.SearchCredentials(ctx, svc.store, types.CredentialFilter{ + Kind: credentialsTypePassword, + OwnerID: userID, + Deleted: filter.StateInclusive}) + + if err != nil { + return err + } + + if svc.CheckPassword(password, true, cc) { + return AuthErrPasswordResetFailedOldPasswordCheckFailed(aam) + } + + if svc.CheckPassword(password, false, cc) { + return AuthErrPasswordSetFailedReusedPasswordCheckFailed(aam) + } + if err != svc.SetPasswordCredentials(ctx, userID, password) { return err } @@ -606,13 +624,21 @@ func (svc auth) ChangePassword(ctx context.Context, userID uint64, oldPassword, aam.setUser(u) ctx = internalAuth.SetIdentityToContext(ctx, u) - cc, _, err = store.SearchCredentials(ctx, svc.store, types.CredentialFilter{Kind: credentialsTypePassword, OwnerID: userID}) + cc, _, err = store.SearchCredentials(ctx, svc.store, types.CredentialFilter{ + Kind: credentialsTypePassword, + OwnerID: userID, + Deleted: filter.StateInclusive}) + if err != nil { return err } - if !svc.checkPassword(oldPassword, cc) { - return AuthErrPasswodResetFailedOldPasswordCheckFailed(aam) + if !svc.CheckPassword(oldPassword, true, cc) { + return AuthErrPasswordResetFailedOldPasswordCheckFailed(aam) + } + + if svc.CheckPassword(newPassword, false, cc) { + return AuthErrPasswordSetFailedReusedPasswordCheckFailed(aam) } if err != svc.SetPasswordCredentials(ctx, userID, newPassword) { diff --git a/system/service/auth_actions.gen.go b/system/service/auth_actions.gen.go index e63db6b40..bd3318c0d 100644 --- a/system/service/auth_actions.gen.go +++ b/system/service/auth_actions.gen.go @@ -1098,12 +1098,12 @@ func AuthErrPasswordChangeFailedForUnknownUser(mm ...*authActionProps) *errors.E return e } -// AuthErrPasswodResetFailedOldPasswordCheckFailed returns "system:auth.passwodResetFailedOldPasswordCheckFailed" as *errors.Error +// AuthErrPasswordResetFailedOldPasswordCheckFailed returns "system:auth.passwordResetFailedOldPasswordCheckFailed" as *errors.Error // // // This function is auto-generated. // -func AuthErrPasswodResetFailedOldPasswordCheckFailed(mm ...*authActionProps) *errors.Error { +func AuthErrPasswordResetFailedOldPasswordCheckFailed(mm ...*authActionProps) *errors.Error { var p = &authActionProps{} if len(mm) > 0 { p = mm[0] @@ -1114,14 +1114,48 @@ func AuthErrPasswodResetFailedOldPasswordCheckFailed(mm ...*authActionProps) *er p.Format("failed to change password, old password does not match", nil), - errors.Meta("type", "passwodResetFailedOldPasswordCheckFailed"), + errors.Meta("type", "passwordResetFailedOldPasswordCheckFailed"), errors.Meta("resource", "system:auth"), errors.Meta(authPropsMetaKey{}, p), // translation namespace & key errors.Meta(locale.ErrorMetaNamespace{}, "system"), - errors.Meta(locale.ErrorMetaKey{}, "auth.errors.passwodResetFailedOldPasswordCheckFailed"), + errors.Meta(locale.ErrorMetaKey{}, "auth.errors.passwordResetFailedOldPasswordCheckFailed"), + + errors.StackSkip(1), + ) + + if len(mm) > 0 { + } + + return e +} + +// AuthErrPasswordSetFailedReusedPasswordCheckFailed returns "system:auth.passwordSetFailedReusedPasswordCheckFailed" as *errors.Error +// +// +// This function is auto-generated. +// +func AuthErrPasswordSetFailedReusedPasswordCheckFailed(mm ...*authActionProps) *errors.Error { + var p = &authActionProps{} + if len(mm) > 0 { + p = mm[0] + } + + var e = errors.New( + errors.KindInternal, + + p.Format("failed to set password, already used", nil), + + errors.Meta("type", "passwordSetFailedReusedPasswordCheckFailed"), + errors.Meta("resource", "system:auth"), + + errors.Meta(authPropsMetaKey{}, p), + + // translation namespace & key + errors.Meta(locale.ErrorMetaNamespace{}, "system"), + errors.Meta(locale.ErrorMetaKey{}, "auth.errors.passwordSetFailedReusedPasswordCheckFailed"), errors.StackSkip(1), ) diff --git a/system/service/auth_actions.yaml b/system/service/auth_actions.yaml index cd9dc8891..8d2aba862 100644 --- a/system/service/auth_actions.yaml +++ b/system/service/auth_actions.yaml @@ -130,9 +130,12 @@ errors: - error: passwordChangeFailedForUnknownUser message: "failed to change password for the unknown user" - - error: passwodResetFailedOldPasswordCheckFailed + - error: passwordResetFailedOldPasswordCheckFailed message: "failed to change password, old password does not match" + - error: passwordSetFailedReusedPasswordCheckFailed + message: "failed to set password, already used" + - error: passwordCreateFailedForUnknownUser message: "failed to create password for the unknown user" diff --git a/system/service/auth_test.go b/system/service/auth_test.go index f498fac36..fcf8fef3e 100644 --- a/system/service/auth_test.go +++ b/system/service/auth_test.go @@ -446,7 +446,7 @@ func Test_auth_checkPassword(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if tt.rval != svc.checkPassword(tt.args.password, tt.args.cc) { + if tt.rval != svc.CheckPassword(tt.args.password, true, tt.args.cc) { t.Errorf("auth.checkPassword() expecting rval to be %v", tt.rval) } }) diff --git a/system/service/user.go b/system/service/user.go index 726aee85d..9168b5212 100644 --- a/system/service/user.go +++ b/system/service/user.go @@ -52,6 +52,7 @@ type ( userAuth interface { CheckPasswordStrength(string) bool + CheckPassword(string, bool, types.CredentialSet) bool SetPasswordCredentials(context.Context, uint64, string) error RemovePasswordCredentials(context.Context, uint64) error RemoveAccessTokens(context.Context, *types.User) error @@ -686,7 +687,9 @@ func (svc user) Unsuspend(ctx context.Context, userID uint64) (err error) { // Expecting setter to have permissions to update users func (svc user) SetPassword(ctx context.Context, userID uint64, newPassword string) (err error) { var ( - u *types.User + u *types.User + cc types.CredentialSet + uaProps = &userActionProps{user: &types.User{ID: userID}} a = UserActionSetPassword ) @@ -698,14 +701,14 @@ func (svc user) SetPassword(ctx context.Context, userID uint64, newPassword stri uaProps.setUser(u) - if u.Kind == types.SystemUser { - return UserErrNotAllowedToUpdateSystem() - } - if !svc.ac.CanUpdateUser(ctx, u) { return UserErrNotAllowedToUpdate() } + if u.Kind == types.SystemUser { + return UserErrNotAllowedToUpdateSystem() + } + if err = svc.auth.RemoveAccessTokens(ctx, u); err != nil { return } @@ -715,6 +718,19 @@ func (svc user) SetPassword(ctx context.Context, userID uint64, newPassword stri return svc.auth.RemovePasswordCredentials(ctx, userID) } + cc, _, err = store.SearchCredentials(ctx, svc.store, types.CredentialFilter{ + Kind: credentialsTypePassword, + OwnerID: userID, + Deleted: filter.StateInclusive}) + + if err != nil { + return err + } + + if svc.auth.CheckPassword(newPassword, false, cc) { + return AuthErrPasswordSetFailedReusedPasswordCheckFailed() + } + if !svc.auth.CheckPasswordStrength(newPassword) { return UserErrPasswordNotSecure() } diff --git a/system/types/credential.go b/system/types/credential.go index be7779bba..f573ea990 100644 --- a/system/types/credential.go +++ b/system/types/credential.go @@ -37,10 +37,9 @@ func (u *Credential) Valid() bool { } // CompareHashAndPassword returns first valid credentials with matching hash -func (set CredentialSet) CompareHashAndPassword(password string) *Credential { - // We need only valid credentials (skip deleted, expired) - for _, c := range set { - if !c.Valid() { +func (cc CredentialSet) CompareHashAndPassword(password string, validOnly bool) *Credential { + for _, c := range cc { + if validOnly && !c.Valid() { continue }