Make password reset tokens single-use

deployment/community/.env.template

@@ -57,6 +57,8 @@ SECRET_KEY=fixme
 
 #BEARER_TOKEN_EXPIRATION=3600 * 12  # in seconds
 
+#PASSWORD_RESET_TOKEN_EXPIRATION=600  # in seconds
+
 #SECURITY_BEARER_SALT=NODEFAULT
 SECURITY_BEARER_SALT=fixme
 

deployment/enterprise/.env.template

@@ -62,6 +62,8 @@ SECRET_KEY=fixme
 
 #BEARER_TOKEN_EXPIRATION=3600 * 12  # in seconds
 
+#PASSWORD_RESET_TOKEN_EXPIRATION=600  # in seconds
+
 #SECURITY_BEARER_SALT=NODEFAULT
 SECURITY_BEARER_SALT=fixme
 

server/mergin/auth/app.py

@@ -4,6 +4,7 @@
 
 import functools
 import logging
+import secrets
 from typing import Optional
 from blinker import signal
 from flask import current_app, render_template, Flask
@@ -159,15 +160,30 @@ def send_confirmation_email(app, user, url, template, header, **kwargs):
     Send confirmation email from selected template with customizable email subject and confirmation URL.
     Optional kwargs are passed to render_template method if needed for particular template.
     """
-    from ..celery import send_email_async
+    token = generate_confirmation_token(
+        app, user.email, app.config["SECURITY_EMAIL_SALT"]
+    )
+    _send_token_email(app, user, f"{url}/{token}", template, header, **kwargs)
+
 
-    salt = (
-        app.config["SECURITY_EMAIL_SALT"]
-        if url == "confirm-email"
-        else app.config["SECURITY_PASSWORD_SALT"]
+def send_password_reset_email(app: Flask, user: User) -> None:
+    """Issue a new single-use password reset token (revoking any previous one) and email it."""
+    from ..app import db
+
+    token = generate_password_reset_token(app, user)
+    db.session.commit()
+    _send_token_email(
+        app,
+        user,
+        f"change-password/{token}",
+        "email/password_reset.html",
+        "Password reset",
     )
-    token = generate_confirmation_token(app, user.email, salt)
-    confirm_url = f"{url}/{token}"
+
+
+def _send_token_email(app, user, confirm_url, template, header, **kwargs):
+    from ..celery import send_email_async
+
     html = render_template(
         template, subject=header, confirm_url=confirm_url, user=user, **kwargs
     )
@@ -180,6 +196,34 @@ def send_confirmation_email(app, user, url, template, header, **kwargs):
     send_email_async.delay(**email_data)
 
 
+def generate_password_reset_token(app: Flask, user: User) -> str:
+    """Sign a reset token bound to a fresh nonce stored on the user.
+
+    Rotating the nonce revokes previously issued tokens; any password change clears it.
+    """
+    user.password_reset_nonce = secrets.token_urlsafe(32)
+    serializer = URLSafeTimedSerializer(app.config["SECRET_KEY"])
+    payload = {"email": user.email, "nonce": user.password_reset_nonce}
+    return serializer.dumps(payload, salt=app.config["SECURITY_PASSWORD_SALT"])
+
+
+def confirm_password_reset_token(token: str) -> Optional[dict]:
+    """Return a signed, unexpired reset token payload, or None. The nonce still has to be checked against the user."""
+    serializer = URLSafeTimedSerializer(current_app.config["SECRET_KEY"])
+    try:
+        payload = serializer.loads(
+            token,
+            salt=current_app.config["SECURITY_PASSWORD_SALT"],
+            max_age=current_app.config["PASSWORD_RESET_TOKEN_EXPIRATION"],
+        )
+    except BadData:
+        return None
+    # tokens issued before nonces were introduced carried only the email
+    if not isinstance(payload, dict):
+        return None
+    return payload
+
+
 def generate_unlock_token(app: Flask, user: User) -> str:
     """Sign a token binding the current lock episode (email + locked_until) to the user."""
     serializer = URLSafeTimedSerializer(app.config["SECRET_KEY"])

server/mergin/auth/config.py

@@ -13,6 +13,9 @@ class Configuration(object):
     BEARER_TOKEN_EXPIRATION = config(
         "BEARER_TOKEN_EXPIRATION", default=3600 * 12, cast=int
     )  # in seconds
+    PASSWORD_RESET_TOKEN_EXPIRATION = config(
+        "PASSWORD_RESET_TOKEN_EXPIRATION", default=600, cast=int
+    )  # in seconds
     ACCOUNT_EXPIRATION = config("ACCOUNT_EXPIRATION", default=5, cast=int)  # in days
     BCRYPT_LOG_ROUNDS = config("BCRYPT_LOG_ROUNDS", default=12, cast=int)
     # Comma-separated "attempts:seconds" pairs, e.g. "5:300,10:3600"

server/mergin/auth/controller.py

@@ -16,7 +16,9 @@
     auth_required,
     authenticate,
     send_confirmation_email,
+    send_password_reset_email,
     confirm_token,
+    confirm_password_reset_token,
     generate_confirmation_token,
     confirm_unlock_token,
     user_created,
@@ -380,19 +382,13 @@ def password_reset():  # pylint: disable=W0613,W0612
         target_email=form.email.data.strip(),
     )
     if user and user.active and user.can_edit_profile:
-        send_confirmation_email(
-            current_app,
-            user,
-            "change-password",
-            "email/password_reset.html",
-            "Password reset",
-        )
+        send_password_reset_email(current_app, user)
     return "", 200
 
 
 def confirm_new_password(token):  # pylint: disable=W0613,W0612
-    email = confirm_token(token, salt=current_app.config["SECURITY_PASSWORD_SALT"])
-    if not email:
+    payload = confirm_password_reset_token(token)
+    if not payload:
         emit(
             AuthEventType.USER_PASSWORD_RESET_FAILED,
             **request_context(),
@@ -402,6 +398,7 @@ def confirm_new_password(token):  # pylint: disable=W0613,W0612
         )
         abort(400, "Invalid token")
 
+    email = payload["email"]
     user = User.query.filter_by(email=email).first()
     if not user:
         emit(
@@ -412,6 +409,16 @@ def confirm_new_password(token):  # pylint: disable=W0613,W0612
             reason="user_not_found",
         )
         abort(404)
+    # token was already used, superseded by a newer one or revoked by a password change
+    if user.password_reset_nonce != payload["nonce"]:
+        emit(
+            AuthEventType.USER_PASSWORD_RESET_FAILED,
+            **request_context(),
+            target_user_id=user.id,
+            target_email=user.email,
+            reason="token_revoked",
+        )
+        abort(400, "Invalid token")
     if not user.active:
         emit(
             AuthEventType.USER_PASSWORD_RESET_FAILED,
@@ -433,6 +440,7 @@ def confirm_new_password(token):  # pylint: disable=W0613,W0612
 
     form = UserPasswordForm.from_json(request.json)
     if form.validate():
+        # also clears the reset nonce, making the token single-use
         user.assign_password(form.password.data)
         user.reset_lockout()
         db.session.add(user)

server/mergin/auth/listeners.py

@@ -13,7 +13,7 @@
 from .models import User
 
 # Fields excluded from user.updated audit events:
-#   - sensitive values that must never appear in logs (passwd)
+#   - sensitive values that must never appear in logs (passwd, password_reset_nonce)
 #   - high-frequency operational fields (last_signed_in, registration_date)
 #   - lifecycle state fields covered by dedicated events (active, inactive_since)
 #   - is_admin covered by the dedicated user.admin_panel_access.changed event
@@ -22,6 +22,7 @@
 _EXCLUDED_FROM_USER_UPDATED = frozenset(
     {
         "passwd",
+        "password_reset_nonce",
         "last_signed_in",
         "registration_date",
         "active",

server/mergin/auth/models.py

@@ -51,6 +51,8 @@ class User(db.Model):
     )
     last_signed_in = db.Column(db.DateTime(), nullable=True)
     locked_until = db.Column(db.DateTime(), nullable=True)
+    # nonce of the only valid password reset token, cleared on any password change
+    password_reset_nonce = db.Column(db.String(64), nullable=True)
     receive_notifications = db.Column(
         db.Boolean, default=True, nullable=False, index=True
     )
@@ -89,6 +91,7 @@ def assign_password(self, password):
             if password
             else None
         )
+        self.password_reset_nonce = None
 
     def needs_rehash(self):
         """Return True if the stored hash was generated with a different cost factor than configured."""
@@ -267,6 +270,7 @@ def anonymize(self):
             self.username = del_str
             self.email = None
             self.passwd = None
+            self.password_reset_nonce = None
             self.first_name = None
             self.last_name = None
             db.session.commit()

server/mergin/tests/test_audit_events.py

@@ -12,7 +12,10 @@
 from flask import current_app
 
 from ..app import db
-from ..auth.app import generate_confirmation_token, generate_unlock_token
+from ..auth.app import (
+    generate_password_reset_token,
+    generate_unlock_token,
+)
 from ..auth.events import AuthEventType
 from ..auth.models import User
 from ..sync.events import SyncEventType
@@ -82,9 +85,10 @@ def test_user_password_changed(client, audit_capture):
 
 def test_user_password_reset(app, client, audit_capture):
     user = User.query.filter_by(username=DEFAULT_USER[0]).first()
-    token = generate_confirmation_token(
-        app, user.email, app.config["SECURITY_PASSWORD_SALT"]
-    )
+    token = generate_password_reset_token(app, user)
+    db.session.commit()
+    # the nonce is a secret - storing it must not emit user.updated
+    assert len(audit_capture.of_type(AuthEventType.USER_UPDATED)) == 0
 
     client.post(
         f"/app/auth/reset-password/{token}",
@@ -94,6 +98,16 @@ def test_user_password_reset(app, client, audit_capture):
     e = audit_capture.one(AuthEventType.USER_PASSWORD_RESET_COMPLETED)
     assert e.target_user_id == user.id
     assert e.metadata["target_email"] == user.email
+    assert len(audit_capture.of_type(AuthEventType.USER_UPDATED)) == 0
+
+    # reusing the token is rejected
+    client.post(
+        f"/app/auth/reset-password/{token}",
+        json={"password": "NewPass#456", "confirm": "NewPass#456"},
+    )
+    e = audit_capture.one(AuthEventType.USER_PASSWORD_RESET_FAILED)
+    assert e.target_user_id == user.id
+    assert e.metadata["reason"] == "token_revoked"
 
 
 def test_user_created(audit_capture):

server/mergin/tests/test_auth.py

@@ -17,6 +17,7 @@
 from ..auth.app import (
     generate_confirmation_token,
     confirm_token,
+    generate_password_reset_token,
     generate_unlock_token,
 )
 from ..auth.models import User, LoginHistory
@@ -578,9 +579,8 @@ def test_confirm_email(app, client):
 
 def test_confirm_password(app, client):
     user = User.query.filter_by(username="mergin").first()
-    token = generate_confirmation_token(
-        app, user.email, app.config["SECURITY_PASSWORD_SALT"]
-    )
+    token = generate_password_reset_token(app, user)
+    db.session.commit()
 
     form_data = {"password": "ilovemergin#0", "confirm": "ilovemergin#0"}
 
@@ -607,8 +607,8 @@ def test_confirm_password(app, client):
     resp = client.post(
         url_for(
             "/.mergin_auth_controller_confirm_new_password",
-            token=generate_confirmation_token(
-                app, "tests@mergin.com", app.config["SECURITY_PASSWORD_SALT"]
+            token=generate_password_reset_token(
+                app, SimpleNamespace(email="tests@mergin.com")
             ),
         ),
         data=json.dumps(form_data),
@@ -622,14 +622,10 @@ def test_confirm_password(app, client):
     )
     user.active = False
     db.session.add(user)
+    token = generate_password_reset_token(app, user)
     db.session.commit()
     resp = client.post(
-        url_for(
-            "/.mergin_auth_controller_confirm_new_password",
-            token=generate_confirmation_token(
-                app, "tests@mergin.com", app.config["SECURITY_PASSWORD_SALT"]
-            ),
-        )
+        url_for("/.mergin_auth_controller_confirm_new_password", token=token)
     )
     assert resp.status_code == 400
 
@@ -648,9 +644,8 @@ def test_confirm_password_clears_lockout(send_email_mock, app, client):
             )
         assert user.is_locked_out()
 
-        token = generate_confirmation_token(
-            app, user.email, app.config["SECURITY_PASSWORD_SALT"]
-        )
+        token = generate_password_reset_token(app, user)
+        db.session.commit()
         resp = client.post(
             url_for("/.mergin_auth_controller_confirm_new_password", token=token),
             data=json.dumps({"password": "newpass#1", "confirm": "newpass#1"}),
@@ -678,6 +673,74 @@ def test_confirm_password_clears_lockout(send_email_mock, app, client):
         assert not user.is_locked_out()
 
 
+def _confirm_new_password(client, token, password="newpass#1"):
+    return client.post(
+        url_for("/.mergin_auth_controller_confirm_new_password", token=token),
+        data=json.dumps({"password": password, "confirm": password}),
+        headers=json_headers,
+    )
+
+
+@patch("mergin.celery.send_email_async.apply_async")
+def test_reset_password_token_single_use(send_email_mock, app, client):
+    """Reset link is one-off, only the latest link is valid and a regular password change revokes it."""
+    user = add_user("resetuser", "oldpassword")
+
+    # used token cannot be reused
+    token = generate_password_reset_token(app, user)
+    db.session.commit()
+    assert _confirm_new_password(client, token).status_code == 200
+    assert user.password_reset_nonce is None
+    assert _confirm_new_password(client, token, "another#1").status_code == 400
+    assert user.check_password("newpass#1")
+
+    # requesting a new link revokes the older one
+    old_token = generate_password_reset_token(app, user)
+    db.session.commit()
+    resp = client.post(
+        url_for("/.mergin_auth_controller_password_reset"),
+        json={"email": user.email},
+    )
+    assert resp.status_code == 200
+    assert send_email_mock.call_count == 1
+    email_html = send_email_mock.call_args.args[1]["html"]
+    assert old_token not in email_html
+    assert _confirm_new_password(client, old_token).status_code == 400
+    new_token = email_html.split("change-password/")[1].split('"')[0]
+    assert _confirm_new_password(client, new_token, "latest#1").status_code == 200
+
+    # regular password change revokes outstanding link
+    token = generate_password_reset_token(app, user)
+    db.session.commit()
+    login(client, user.username, "latest#1")
+    resp = client.post(
+        url_for("/.mergin_auth_controller_change_password"),
+        json={
+            "old_password": "latest#1",
+            "password": "changed#1",
+            "confirm": "changed#1",
+        },
+    )
+    assert resp.status_code == 200
+    assert _confirm_new_password(client, token).status_code == 400
+    assert user.check_password("changed#1")
+
+
+def test_reset_password_token_expired_or_legacy(app, client):
+    user = add_user("resetuser", "oldpassword")
+    token = generate_password_reset_token(app, user)
+    db.session.commit()
+    with patch.dict(app.config, {"PASSWORD_RESET_TOKEN_EXPIRATION": -1}):
+        assert _confirm_new_password(client, token).status_code == 400
+
+    # token format from before nonces were introduced (email only) is rejected
+    legacy_token = generate_confirmation_token(
+        app, user.email, app.config["SECURITY_PASSWORD_SALT"]
+    )
+    assert _confirm_new_password(client, legacy_token).status_code == 400
+    assert user.check_password("oldpassword")
+
+
 # reset password tests: success, no email, not-existing user (200 - masked)
 test_reset_data = [
     ({"email": "mergin@mergin.com"}, 200),