diff --git a/Server/api/profile_handler.go b/Server/api/profile_handler.go index 00aaa177..4a26eec2 100644 --- a/Server/api/profile_handler.go +++ b/Server/api/profile_handler.go @@ -228,10 +228,22 @@ func handleChangePassword(svc *service.Services, limiter *auth.RateLimiter) http keepSessionID = sess.ID } - if _, err := svc.Users.ChangePassword(user.ID, hash, keepSessionID); err != nil { + res, err := svc.Users.ChangePassword(user.ID, hash, keepSessionID) + if err != nil { + // Only reachable when the password itself failed to commit. writeServiceError(w, err) return } + if res.RevokeFailed { + // Partial success: the password IS changed; only revoking the + // other sessions failed. A 5xx here would tell the user to retry + // with a password that no longer works. + writeJSON(w, http.StatusOK, map[string]any{ + "warning": "password changed, but other sessions could not be revoked; revoke them from the sessions list", + "sessions_revoked": res.SessionsRevoked, + }) + return + } w.WriteHeader(http.StatusNoContent) } diff --git a/Server/service/user.go b/Server/service/user.go index a2cac293..4332556d 100644 --- a/Server/service/user.go +++ b/Server/service/user.go @@ -51,24 +51,43 @@ func (s *UserService) UpdateProfile(ctx context.Context, userID int64, username return user, nil } +// ChangePasswordResult reports a completed password change. RevokeFailed is +// set when the password committed but other sessions could not be revoked — +// a partial success the caller must surface as a warning, never as a 5xx: +// the old password is already unusable, so telling the user the change +// "failed" walks them into retrying with a dead password and tripping the +// password-confirm lockout. +type ChangePasswordResult struct { + SessionsRevoked int64 + RevokeFailed bool +} + // ChangePassword updates the user's password and revokes other sessions. -// Returns the number of other sessions revoked. -func (s *UserService) ChangePassword(userID int64, newPasswordHash string, keepSessionID int64) (int64, error) { +func (s *UserService) ChangePassword(userID int64, newPasswordHash string, keepSessionID int64) (ChangePasswordResult, error) { if err := s.st.UpdateUserPassword(userID, newPasswordHash); err != nil { - return 0, fmt.Errorf("%w: failed to update password", ErrInternal) + return ChangePasswordResult{}, fmt.Errorf("%w: failed to update password", ErrInternal) } + + // The password is committed from here on: every path below reports + // success and writes the audit row. + var res ChangePasswordResult revoked, err := s.st.DeleteOtherSessions(userID, keepSessionID) + res.SessionsRevoked = revoked if err != nil { slog.Error("UserService.ChangePassword DeleteOtherSessions", "err", err, "user_id", userID) - // The password was updated, but other sessions could not be revoked, so - // devices authenticated under the old password remain valid. Surface this - // as a failure instead of silently reporting success — a password change - // is a security action and the caller must be able to warn/retry. - return revoked, fmt.Errorf("%w: password changed but failed to revoke other sessions", ErrInternal) + // One bounded compensating retry: revocation is the security tail of + // the change and a single immediate retry covers transient write-lock + // contention. ponytail: one retry, add backoff only if logs show it. + if revokedRetry, retryErr := s.st.DeleteOtherSessions(userID, keepSessionID); retryErr == nil { + res.SessionsRevoked += revokedRetry + } else { + res.RevokeFailed = true + } } _ = s.st.LogAudit(userID, "password_change", "user", userID, "password changed") - slog.Info("password changed", "user_id", userID, "sessions_revoked", revoked) - return revoked, nil + slog.Info("password changed", "user_id", userID, + "sessions_revoked", res.SessionsRevoked, "revoke_failed", res.RevokeFailed) + return res, nil } // ListSessions returns all active sessions for a user. diff --git a/Server/store/memstore.go b/Server/store/memstore.go index 8837b2e9..da73d39a 100644 --- a/Server/store/memstore.go +++ b/Server/store/memstore.go @@ -426,8 +426,13 @@ func (m *MemStore) UpdateUserProfile(_ int64, _ string, _ *string) error { panic("memstore: not implemented: UpdateUserProfile") } -func (m *MemStore) UpdateUserPassword(_ int64, _ string) error { - panic("memstore: not implemented: UpdateUserPassword") +func (m *MemStore) UpdateUserPassword(userID int64, hash string) error { + m.mu.Lock() + defer m.mu.Unlock() + if u, ok := m.users[userID]; ok { + u.PasswordHash = hash + } + return nil } func (m *MemStore) UpdateUserStatus(id int64, status string) error {