Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/controllers/login.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ def clear_main_auth():
"""Clear main site auth session keys only."""
session.pop('name', None)
session.pop('email', None)
session.pop('session_version', None)
session.pop('login_attempt', None)


Expand Down Expand Up @@ -87,6 +88,7 @@ def login_post():
clear_main_auth()
session['email'] = user['email']
session['name'] = user['name']
session['session_version'] = user.get('session_version', 0)
flash('Login successful!', FLASH_SUCCESS)

# Retrieve and clear the redirect from session
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/password.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ def change_password_post():
return 'Could not hash new password', 500

try:
update_user_password(username, hashed_new_password)
# keep *this* browser logged in; other sessions die with the bump
session['session_version'] = update_user_password(username, hashed_new_password)
except Exception as e:
print(f"Error updating user password: {e}")
return 'Could not update password', 500
Expand Down
23 changes: 19 additions & 4 deletions app/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ def user_create(name, email, password):
'name': name,
'email': email,
'password': password,
'session_version': 0,
'visible': True,
'deleted': False
}
Expand All @@ -161,12 +162,19 @@ def user_create(name, email, password):


def update_user_password(username, hashed_password):
"""Update user password.
"""Update user password and bump session_version.

Bumping session_version invalidates every existing signed cookie session
for this user. Callers that want to keep the current browser logged in
must refresh session['session_version'] after this returns.

Args:
username: The username
hashed_password: The new hashed password

Returns:
The new session_version value

Raises:
ValueError: If username or password is empty
ErrNoResult: If no user found
Expand All @@ -178,14 +186,21 @@ def update_user_password(username, hashed_password):

collection = get_collection('user')

result = collection.update_one(
# yes, $inc on a missing field starts at 0 then becomes 1. old accounts work.
result = collection.find_one_and_update(
{'name': username},
{'$set': {'password': hashed_password}}
{
'$set': {'password': hashed_password},
'$inc': {'session_version': 1},
},
return_document=True,
)

if result.matched_count == 0:
if result is None:
raise ErrNoResult("No user found with the provided username")

return result.get('session_version', 1)


def user_get_unread_notifications(username):
"""Get the count of unread notifications for a user.
Expand Down
28 changes: 27 additions & 1 deletion app/services/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,36 @@
from flask import session as flask_session


SESSION_VERSION_KEY = 'session_version'


def init_session(app, config):
"""Initialize session configuration."""
app.config['SESSION_COOKIE_NAME'] = config.get('CookieName', 'crackmesone')
app.config['SECRET_KEY'] = config.get('SecretKey', 'change-me-in-production')
# Session options can be extended here

@app.before_request
def validate_session_version():
"""Drop auth cookies that predate a password change/reset."""
username = flask_session.get('name')
if not username:
return

from app.models.errors import ErrNoResult, ErrUnavailable
from app.models.user import user_by_name

try:
user = user_by_name(username)
except ErrNoResult:
clear_session()
return
except ErrUnavailable:
# don't log people out because Mongo hiccuped
return

expected = user.get('session_version', 0)
if flask_session.get(SESSION_VERSION_KEY, 0) != expected:
clear_session()


def get_session():
Expand All @@ -21,6 +46,7 @@ def clear_session():
"""Clear main site auth session keys only."""
flask_session.pop('name', None)
flask_session.pop('email', None)
flask_session.pop(SESSION_VERSION_KEY, None)
flask_session.pop('login_attempt', None)


Expand Down
5 changes: 4 additions & 1 deletion review/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -1079,7 +1079,10 @@ def reset_user_password(user_email):

result = g_crackmesone_db.user.update_one(
{"email": {"$regex": f"^{escaped}$", "$options": "i"}},
{"$set": {"password": hashed.decode('utf-8')}}
{
"$set": {"password": hashed.decode('utf-8')},
"$inc": {"session_version": 1},
},
)

if result.modified_count != 1:
Expand Down
1 change: 1 addition & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ def _authenticate(client, user):
with client.session_transaction() as session:
session['name'] = user['name']
session['email'] = user['email']
session['session_version'] = user.get('session_version', 0)
return client


Expand Down
4 changes: 3 additions & 1 deletion tests/test_models_extended.py
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,9 @@ def test_user_visibility_counts_password_and_notification_helpers(app, db, alice
user_decrement_unread_notifications('alice', 20)
assert user_get_unread_notifications('alice') == 0
update_user_password('alice', 'new-hash')
assert db.user.find_one({'name': 'alice'})['password'] == 'new-hash'
alice_after = db.user.find_one({'name': 'alice'})
assert alice_after['password'] == 'new-hash'
assert alice_after['session_version'] == 1


def test_expired_password_reset_cleanup(app, db):
Expand Down
28 changes: 28 additions & 0 deletions tests/test_password_reset.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,34 @@ def test_valid_reset_token_changes_password_and_is_single_use(client, db, alice)
assert client.get(f'/reset-password/{token}').status_code == 302


def test_password_reset_invalidates_existing_sessions(app, db, alice):
"""Stolen cookies should stop working after a successful reset."""
victim = app.test_client()
with victim.session_transaction() as session:
session['name'] = alice['name']
session['email'] = alice['email']
session['session_version'] = alice.get('session_version', 0)

assert victim.get('/change-password').status_code == 200

token = create_reset_token('alice@example.test')
attacker = app.test_client()
with patch('app.controllers.password_reset.notify_password_reset_complete'):
reset = attacker.post(f'/reset-password/{token}', data={
'new_password': 'replacement-password',
'new_password_verify': 'replacement-password',
})
assert reset.status_code == 302
assert db.user.find_one({'name': 'alice'})['session_version'] == 1

denied = victim.get('/change-password')
assert denied.status_code == 302
assert denied.location == '/'
with victim.session_transaction() as session:
assert 'name' not in session
assert 'session_version' not in session


def test_expired_reset_token_is_rejected(client, db, alice):
db.password_reset_tokens.insert_one({
'email': 'alice@example.test',
Expand Down
1 change: 1 addition & 0 deletions tests/test_routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def test_login_with_valid_synthetic_user(client, alice):
with client.session_transaction() as session:
assert session['name'] == 'alice'
assert session['email'] == 'alice@example.test'
assert session['session_version'] == 0


def test_login_with_wrong_password_does_not_authenticate(client, alice):
Expand Down