mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
fix(service): report password change as partial success when revocation fails (W2-2)
UpdateUserPassword commits first; when DeleteOtherSessions then errored the handler returned 500 and skipped the audit row — telling the user the change failed while the new password was already live, walking them into retrying with a dead password and tripping the confirm lockout. The committed change now always audits and reports success; revocation gets one bounded compensating retry, and a persistent failure surfaces as a 200 + warning (sessions_revoked count) the client can show. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
}
|
||||
|
||||
+29
-10
@@ -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.
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user