Skip to content

Commit 33b87ae

Browse files
staaldraadstaaldraad
authored
feat: check current password on change (#2364)
## What kind of change does this PR introduce? feature - allows requiring current password when changing the password ## What is the new behavior? Adds config flag `update_password_require_current_password` When set, the currentPassword must be sent in the user update request.
1 parent 8bea8bf commit 33b87ae

4 files changed

Lines changed: 159 additions & 2 deletions

File tree

‎internal/api/apierrors/errorcode.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,8 @@ const (
7171
ErrorCodeReauthenticationNeeded ErrorCode = "reauthentication_needed"
7272
ErrorCodeSamePassword ErrorCode = "same_password"
7373
ErrorCodeReauthenticationNotValid ErrorCode = "reauthentication_not_valid"
74+
ErrorCodeCurrentPasswordMismatch ErrorCode = "current_password_invalid"
75+
ErrorCodeCurrentPasswordRequired ErrorCode = "current_password_required"
7476
ErrorCodeOTPExpired ErrorCode = "otp_expired"
7577
ErrorCodeOTPDisabled ErrorCode = "otp_disabled"
7678
ErrorCodeIdentityNotFound ErrorCode = "identity_not_found"

‎internal/api/user.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
type UserUpdateParams struct {
1919
Email string `json:"email"`
2020
Password *string `json:"password"`
21+
CurrentPassword *string `json:"current_password,omitempty"`
2122
Nonce string `json:"nonce"`
2223
Data map[string]interface{} `json:"data"`
2324
AppData map[string]interface{} `json:"app_metadata,omitempty"`
@@ -165,6 +166,31 @@ func (a *API) UserUpdate(w http.ResponseWriter, r *http.Request) error {
165166
isSamePassword := false
166167

167168
if user.HasPassword() {
169+
// current password required when updating password
170+
if config.Security.UpdatePasswordRequireCurrentPassword {
171+
// user may be in a password reset flow, where they do not have a currentPassword
172+
isRecoverySession := false
173+
for _, claim := range session.AMRClaims {
174+
// password recovery flows can be via otp or a magic link, check if the current session
175+
// was created with one of those
176+
if claim.GetAuthenticationMethod() == "otp" || claim.GetAuthenticationMethod() == "magiclink" {
177+
isRecoverySession = true
178+
break
179+
}
180+
}
181+
if !isRecoverySession {
182+
if params.CurrentPassword == nil || *params.CurrentPassword == "" {
183+
return apierrors.NewBadRequestError(apierrors.ErrorCodeCurrentPasswordRequired, "Current password required when setting new password.")
184+
}
185+
isCurrentPasswordCorrect, _, err := user.Authenticate(ctx, db, *params.CurrentPassword, config.Security.DBEncryption.DecryptionKeys, false, "")
186+
if err != nil {
187+
return err
188+
}
189+
if !isCurrentPasswordCorrect {
190+
return apierrors.NewBadRequestError(apierrors.ErrorCodeCurrentPasswordMismatch, "Current password required when setting new password.")
191+
}
192+
}
193+
}
168194
auth, _, err := user.Authenticate(ctx, db, password, config.Security.DBEncryption.DecryptionKeys, false, "")
169195
if err != nil {
170196
return err

‎internal/api/user_test.go‎

Lines changed: 130 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -297,8 +297,10 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
297297
var cases = []struct {
298298
desc string
299299
newPassword string
300+
currentPassword string
300301
nonce string
301302
requireReauthentication bool
303+
requireCurrentPassword bool
302304
sessionId *uuid.UUID
303305
expected expected
304306
}{
@@ -334,13 +336,48 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
334336
sessionId: r.SessionId,
335337
expected: expected{code: http.StatusOK, isAuthenticated: true},
336338
},
339+
{
340+
desc: "Current password checked when require current password set",
341+
newPassword: "updateToNewpassword123",
342+
currentPassword: "newpassword123", // match to the test case above
343+
nonce: "",
344+
requireReauthentication: false,
345+
requireCurrentPassword: true,
346+
sessionId: r.SessionId,
347+
expected: expected{code: http.StatusOK, isAuthenticated: true},
348+
},
349+
{
350+
desc: "Fails if current password incorrect when require current password set",
351+
newPassword: "newpassword123",
352+
currentPassword: "randompassword",
353+
nonce: "",
354+
requireReauthentication: false,
355+
requireCurrentPassword: true,
356+
sessionId: r.SessionId,
357+
expected: expected{code: http.StatusBadRequest, isAuthenticated: false},
358+
},
359+
{
360+
desc: "Fails if current password not set when required",
361+
newPassword: "newpassword123",
362+
nonce: "",
363+
requireReauthentication: false,
364+
requireCurrentPassword: true,
365+
sessionId: r.SessionId,
366+
expected: expected{code: http.StatusBadRequest, isAuthenticated: false},
367+
},
337368
}
338369

339370
for _, c := range cases {
340371
ts.Run(c.desc, func() {
341372
ts.Config.Security.UpdatePasswordRequireReauthentication = c.requireReauthentication
373+
ts.Config.Security.UpdatePasswordRequireCurrentPassword = c.requireCurrentPassword
374+
375+
userUpdateBody := map[string]string{"password": c.newPassword, "nonce": c.nonce}
376+
if c.requireCurrentPassword {
377+
userUpdateBody["current_password"] = c.currentPassword
378+
}
342379
var buffer bytes.Buffer
343-
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(map[string]string{"password": c.newPassword, "nonce": c.nonce}))
380+
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(userUpdateBody))
344381

345382
req := httptest.NewRequest(http.MethodPut, "http://localhost/user", &buffer)
346383
req.Header.Set("Content-Type", "application/json")
@@ -365,7 +402,96 @@ func (ts *UserTestSuite) TestUserUpdatePassword() {
365402
}
366403
}
367404

405+
func (ts *UserTestSuite) TestUserUpdatePasswordViaRecovery() {
406+
ts.Config.Security.UpdatePasswordRequireCurrentPassword = true
407+
ts.Config.SMTP.MaxFrequency = 60
408+
u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
409+
require.NoError(ts.T(), err)
410+
u.RecoverySentAt = &time.Time{}
411+
require.NoError(ts.T(), ts.API.db.Update(u))
412+
413+
type expected struct {
414+
code int
415+
isAuthenticated bool
416+
}
417+
418+
var cases = []struct {
419+
desc string
420+
newPassword string
421+
currentPassword string
422+
recoveryType models.AuthenticationMethod
423+
expected expected
424+
}{
425+
{
426+
desc: "Current password not required in OTP recovery flow",
427+
newPassword: "newpassword123",
428+
recoveryType: models.OTP,
429+
expected: expected{code: http.StatusOK, isAuthenticated: true},
430+
},
431+
{
432+
desc: "Current password not required in magiclink recovery flow",
433+
newPassword: "newpassword456",
434+
recoveryType: models.MagicLink,
435+
expected: expected{code: http.StatusOK, isAuthenticated: true},
436+
},
437+
{
438+
desc: "Current password required for any other claim",
439+
newPassword: "newpassword456",
440+
recoveryType: models.EmailChange,
441+
expected: expected{code: http.StatusBadRequest, isAuthenticated: true},
442+
},
443+
}
444+
445+
for _, c := range cases {
446+
ts.Run(c.desc, func() {
447+
require.NoError(ts.T(), models.ClearAllOneTimeTokensForUser(ts.API.db, u.ID))
448+
449+
// Create a session
450+
session, err := models.NewSession(u.ID, nil)
451+
require.NoError(ts.T(), err)
452+
require.NoError(ts.T(), ts.API.db.Create(session))
453+
454+
// Add AMR claim to session to simulate recovery flow
455+
require.NoError(ts.T(), models.AddClaimToSession(ts.API.db, session.ID, c.recoveryType))
456+
457+
// Reload session with AMR claims
458+
session, err = models.FindSessionByID(ts.API.db, session.ID, true)
459+
require.NoError(ts.T(), err)
460+
require.NotEmpty(ts.T(), session.AMRClaims, "Session should have AMR claims")
461+
462+
// Generate access token with the recovery authentication method
463+
req := httptest.NewRequest(http.MethodPut, "http://localhost/user", nil)
464+
token, _, err := ts.API.generateAccessToken(req, ts.API.db, u, &session.ID, c.recoveryType)
465+
require.NoError(ts.T(), err)
466+
467+
// Update password without current password
468+
userUpdateBody := map[string]string{"password": c.newPassword}
469+
var buffer bytes.Buffer
470+
require.NoError(ts.T(), json.NewEncoder(&buffer).Encode(userUpdateBody))
471+
472+
req = httptest.NewRequest(http.MethodPut, "http://localhost/user", &buffer)
473+
req.Header.Set("Content-Type", "application/json")
474+
req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token))
475+
476+
// Setup response recorder
477+
w := httptest.NewRecorder()
478+
ts.API.handler.ServeHTTP(w, req)
479+
require.Equal(ts.T(), c.expected.code, w.Code)
480+
481+
// Verify password was updated
482+
u, err = models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
483+
require.NoError(ts.T(), err)
484+
485+
isAuthenticated, _, err := u.Authenticate(context.Background(), ts.API.db, c.newPassword, ts.API.config.Security.DBEncryption.DecryptionKeys, ts.API.config.Security.DBEncryption.Encrypt, ts.API.config.Security.DBEncryption.EncryptionKeyID)
486+
require.NoError(ts.T(), err)
487+
488+
require.Equal(ts.T(), c.expected.isAuthenticated, isAuthenticated)
489+
})
490+
}
491+
}
492+
368493
func (ts *UserTestSuite) TestUserUpdatePasswordNoReauthenticationRequired() {
494+
ts.Config.Security.UpdatePasswordRequireCurrentPassword = false
369495
u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
370496
require.NoError(ts.T(), err)
371497

@@ -429,7 +555,7 @@ func (ts *UserTestSuite) TestUserUpdatePasswordNoReauthenticationRequired() {
429555

430556
func (ts *UserTestSuite) TestUserUpdatePasswordReauthentication() {
431557
ts.Config.Security.UpdatePasswordRequireReauthentication = true
432-
558+
ts.Config.Security.UpdatePasswordRequireCurrentPassword = false
433559
u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
434560
require.NoError(ts.T(), err)
435561

@@ -487,6 +613,7 @@ func (ts *UserTestSuite) TestUserUpdatePasswordReauthentication() {
487613

488614
func (ts *UserTestSuite) TestUserUpdatePasswordLogoutOtherSessions() {
489615
ts.Config.Security.UpdatePasswordRequireReauthentication = false
616+
ts.Config.Security.UpdatePasswordRequireCurrentPassword = false
490617
u, err := models.FindUserByEmailAndAudience(ts.API.db, "test@example.com", ts.Config.JWT.Aud)
491618
require.NoError(ts.T(), err)
492619

@@ -586,6 +713,7 @@ func (ts *UserTestSuite) TestUserUpdatePasswordSendsNotificationEmail() {
586713
for _, c := range cases {
587714
ts.Run(c.desc, func() {
588715
ts.Config.Security.UpdatePasswordRequireReauthentication = false
716+
ts.Config.Security.UpdatePasswordRequireCurrentPassword = false
589717
ts.Config.Mailer.Autoconfirm = false
590718
ts.Config.Mailer.Notifications.PasswordChangedEnabled = c.notificationEnabled
591719

‎internal/conf/configuration.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,6 +731,7 @@ type SecurityConfiguration struct {
731731
RefreshTokenReuseInterval int `json:"refresh_token_reuse_interval" split_words:"true"`
732732
RefreshTokenAllowReuse bool `json:"refresh_token_allow_reuse" split_words:"true"`
733733
UpdatePasswordRequireReauthentication bool `json:"update_password_require_reauthentication" split_words:"true"`
734+
UpdatePasswordRequireCurrentPassword bool `json:"update_password_require_current_password" split_words:"true"`
734735
ManualLinkingEnabled bool `json:"manual_linking_enabled" split_words:"true" default:"false"`
735736
SbForwardedForEnabled bool `json:"sb_forwarded_for_enabled" split_words:"true" default:"false"`
736737

0 commit comments

Comments
 (0)