Skip to content

Commit 5ac2e4f

Browse files
authored
Rate limit account confirmation and password reset emails (#545)
* Rate limit account confirmation and password reset emails * Address PR feedback * Add help email to confirmation email * Update database diagram * Fix create_db_diagram * Update database diagram
1 parent c3cccf3 commit 5ac2e4f

File tree

11 files changed

+287
-10
lines changed

11 files changed

+287
-10
lines changed

Makefile

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,28 @@ create_db: start
2727
done
2828
@echo "Postgres is up"
2929
## Creating database
30-
docker-compose run --rm web python ../create_db.py
30+
docker compose run --rm web python ./create_db.py
31+
32+
.PHONY: create_db_diagram
33+
create_db_diagram:
34+
# Create new dot file showing current version of schema
35+
eralchemy -i postgresql://openoversight:terriblepassword@localhost/openoversight-dev -o database/schema.new.dot
36+
# Sort new version of schema file
37+
sort database/schema.new.dot -o schema.new.dot.sorted
38+
# Create old schema file if it does not exist and then sort it
39+
touch database/schema.dot
40+
sort database/schema.dot -o schema.dot.sorted
41+
# Create a new diagram if there are changes, otherwise clean up files
42+
@if diff schema.dot.sorted schema.new.dot.sorted > /dev/null 2>&1; then \
43+
echo 'No schema changes detected!'; \
44+
rm database/schema.new.dot; \
45+
else \
46+
echo 'Detected schema changes, making new DB relationship diagram!'; \
47+
mv database/schema.new.dot database/schema.dot; \
48+
dot -Tpng -o database/database_relationships.png -Grankdir=TB -Kdot database/schema.dot; \
49+
fi
50+
# Remove all sorted files
51+
rm schema.dot.sorted schema.new.dot.sorted
3152

3253
.PHONY: dev
3354
dev: create_empty_secret build start create_db populate

OpenOversight/app/auth/views.py

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,10 @@
3434
ResetPasswordEmail,
3535
)
3636
from OpenOversight.app.utils.auth import admin_required
37-
from OpenOversight.app.utils.constants import KEY_APPROVE_REGISTRATIONS
37+
from OpenOversight.app.utils.constants import (
38+
KEY_APPROVE_REGISTRATIONS,
39+
KEY_AUTH_EMAIL_COOLDOWN_HOURS,
40+
)
3841
from OpenOversight.app.utils.flask import sitemap
3942
from OpenOversight.app.utils.forms import set_dynamic_default
4043
from OpenOversight.app.utils.general import validate_redirect_url
@@ -175,6 +178,20 @@ def confirm(token):
175178
@auth.route("/confirm")
176179
@login_required
177180
def resend_confirmation():
181+
now = datetime.now(timezone.utc)
182+
if (
183+
current_user.last_confirmation_sent_at
184+
and current_user.last_confirmation_sent_at
185+
> now - current_app.config[KEY_AUTH_EMAIL_COOLDOWN_HOURS]
186+
):
187+
flash(
188+
"We already sent a confirmation email to you recently. Please try again later."
189+
)
190+
return redirect(url_for("main.index"))
191+
192+
current_user.last_confirmation_sent_at = now
193+
db.session.commit()
194+
178195
token = current_user.generate_confirmation_token()
179196
EmailClient.send_email(
180197
ConfirmAccountEmail(current_user.email, user=current_user, token=token)
@@ -211,11 +228,21 @@ def password_reset_request():
211228
form = PasswordResetRequestForm()
212229
if form.validate_on_submit():
213230
user = User.by_email(form.email.data).first()
214-
if user:
231+
now = datetime.now(timezone.utc)
232+
if user and (
233+
not user.last_reset_sent_at
234+
or user.last_reset_sent_at
235+
< now - current_app.config[KEY_AUTH_EMAIL_COOLDOWN_HOURS]
236+
):
237+
user.last_reset_sent_at = now
238+
db.session.commit()
239+
215240
token = user.generate_reset_token()
216241
EmailClient.send_email(
217242
ResetPasswordEmail(user.email, user=user, token=token)
218243
)
244+
# Show message regardless of whether an email was sent to avoid revealing
245+
# whether an account is associated with the email
219246
flash("An email with instructions to reset your password has been sent to you.")
220247
return redirect(url_for("auth.login"))
221248
else:

OpenOversight/app/models/config.py

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,15 @@
11
import json
22
import os
3+
from datetime import timedelta
34

45
from OpenOversight.app.utils.constants import (
56
KEY_APPROVE_REGISTRATIONS,
7+
KEY_AUTH_EMAIL_COOLDOWN_HOURS,
8+
KEY_DATABASE_URI,
9+
KEY_ENV,
10+
KEY_ENV_DEV,
11+
KEY_ENV_PROD,
12+
KEY_ENV_TESTING,
613
KEY_MAIL_PASSWORD,
714
KEY_MAIL_PORT,
815
KEY_MAIL_SERVER,
@@ -37,7 +44,7 @@ def __init__(self):
3744

3845
# DB Settings
3946
self.SQLALCHEMY_TRACK_MODIFICATIONS = False
40-
self.SQLALCHEMY_DATABASE_URI = os.environ.get("SQLALCHEMY_DATABASE_URI")
47+
self.SQLALCHEMY_DATABASE_URI = os.environ.get(KEY_DATABASE_URI)
4148

4249
# Protocol Settings
4350
self.SITEMAP_URL_SCHEME = "http"
@@ -79,6 +86,10 @@ def __init__(self):
7986

8087
# User settings
8188
self.APPROVE_REGISTRATIONS = os.environ.get(KEY_APPROVE_REGISTRATIONS, False)
89+
# Time a user must wait between consecutive confirm account or reset password requests
90+
self.AUTH_EMAIL_COOLDOWN_HOURS = timedelta(
91+
hours=int(os.environ.get(KEY_AUTH_EMAIL_COOLDOWN_HOURS, 1))
92+
)
8293

8394
# Map data
8495
with open("OpenOversight/map.json") as f:
@@ -107,6 +118,8 @@ def __init__(self):
107118
self.SQLALCHEMY_ENGINE_OPTIONS = {
108119
"connect_args": {"cached_statements": 0},
109120
}
121+
# Prevent user env settings from interfering with tests
122+
self.AUTH_EMAIL_COOLDOWN_HOURS = timedelta(hours=1)
110123

111124

112125
class ProductionConfig(BaseConfig):
@@ -117,8 +130,8 @@ def __init__(self):
117130

118131

119132
config: dict[str, BaseConfig] = {
120-
"development": DevelopmentConfig(),
121-
"testing": TestingConfig(),
122-
"production": ProductionConfig(),
133+
KEY_ENV_DEV: DevelopmentConfig(),
134+
KEY_ENV_TESTING: TestingConfig(),
135+
KEY_ENV_PROD: ProductionConfig(),
123136
}
124-
config["default"] = config.get(os.environ.get("ENV", ""), DevelopmentConfig())
137+
config["default"] = config.get(os.environ.get(KEY_ENV, ""), DevelopmentConfig())

OpenOversight/app/models/database.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,28 @@
5858
Base: DeclarativeMeta = db.Model
5959

6060

61+
class TZDateTime(TypeDecorator):
62+
"""
63+
Store tz-aware datetimes as tz-naive UTC datetimes in sqlite.
64+
https://docs.sqlalchemy.org/en/20/core/custom_types.html#store-timezone-aware-timestamps-as-timezone-naive-utc
65+
"""
66+
67+
cache_ok = True
68+
impl = db.DateTime
69+
70+
def process_bind_param(self, value, dialect):
71+
if dialect.name == "sqlite" and value is not None:
72+
if not value.tzinfo or value.tzinfo.utcoffset(value) is None:
73+
raise TypeError("tzinfo is required")
74+
value = value.astimezone(timezone.utc).replace(tzinfo=None)
75+
return value
76+
77+
def process_result_value(self, value, dialect):
78+
if dialect.name == "sqlite" and value is not None:
79+
value = value.replace(tzinfo=timezone.utc)
80+
return value
81+
82+
6183
class BaseModel(Base):
6284
__abstract__ = True
6385

@@ -944,6 +966,8 @@ class User(UserMixin, BaseModel):
944966
db.ForeignKey("users.id", ondelete="SET NULL", name="users_disabled_by_fkey"),
945967
unique=False,
946968
)
969+
last_confirmation_sent_at = db.Column(TZDateTime(timezone=True))
970+
last_reset_sent_at = db.Column(TZDateTime(timezone=True))
947971

948972
dept_pref = db.Column(
949973
db.Integer, db.ForeignKey("departments.id", name="users_dept_pref_fkey")

OpenOversight/app/models/emails.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,18 @@ def __init__(self, receiver: str, user, token: str):
8686
subject = (
8787
f"{current_app.config[KEY_OO_MAIL_SUBJECT_PREFIX]} Confirm Your Account"
8888
)
89-
body = render_template(f"{self.EMAIL_PATH}confirm.txt", user=user, token=token)
90-
html = render_template(f"{self.EMAIL_PATH}confirm.html", user=user, token=token)
89+
body = render_template(
90+
f"{self.EMAIL_PATH}confirm.txt",
91+
user=user,
92+
token=token,
93+
help_email=current_app.config[KEY_OO_HELP_EMAIL],
94+
)
95+
html = render_template(
96+
f"{self.EMAIL_PATH}confirm.html",
97+
user=user,
98+
token=token,
99+
help_email=current_app.config[KEY_OO_HELP_EMAIL],
100+
)
91101
super().__init__(body, html, subject, receiver)
92102

93103

OpenOversight/app/templates/auth/email/confirm.html

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
</p>
88
<p>Alternatively, you can paste the following link in your browser's address bar:</p>
99
<p>{{ url_for('auth.confirm', token=token, _external=True) }}</p>
10+
<p>If you did not create this account, reach out to {{ help_email }} to disable your account.</p>
1011
<p>Sincerely,</p>
1112
<p>The OpenOversight Team</p>
1213
<p>

OpenOversight/app/templates/auth/email/confirm.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ To confirm your account please click on the following link:
66

77
{{ url_for('auth.confirm', token=token, _external=True) }}
88

9+
If you did not create this account, reach out to {{ help_email }} to disable your account.
10+
911
Sincerely,
1012

1113
The OpenOversight Team

OpenOversight/app/utils/constants.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
# Config Key Constants
1313
KEY_ALLOWED_EXTENSIONS = "ALLOWED_EXTENSIONS"
1414
KEY_APPROVE_REGISTRATIONS = "APPROVE_REGISTRATIONS"
15+
KEY_AUTH_EMAIL_COOLDOWN_HOURS = "AUTH_EMAIL_COOLDOWN_HOURS"
1516
KEY_DATABASE_URI = "SQLALCHEMY_DATABASE_URI"
1617
KEY_ENV = "ENV"
1718
KEY_ENV_DEV = "development"
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
"""Add email last sent columns
2+
3+
Revision ID: edab445d1714
4+
Revises: 99c50fc8d294
5+
Create Date: 2025-04-04 08:42:14.823515
6+
7+
"""
8+
9+
import sqlalchemy as sa
10+
from alembic import op
11+
12+
13+
revision = "edab445d1714"
14+
down_revision = "99c50fc8d294"
15+
16+
17+
def upgrade():
18+
with op.batch_alter_table("users", schema=None) as batch_op:
19+
batch_op.add_column(
20+
sa.Column(
21+
"last_confirmation_sent_at", sa.DateTime(timezone=True), nullable=True
22+
)
23+
)
24+
batch_op.add_column(
25+
sa.Column("last_reset_sent_at", sa.DateTime(timezone=True), nullable=True)
26+
)
27+
28+
29+
def downgrade():
30+
with op.batch_alter_table("users", schema=None) as batch_op:
31+
batch_op.drop_column("last_reset_sent_at")
32+
batch_op.drop_column("last_confirmation_sent_at")

OpenOversight/tests/routes/test_auth.py

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
from datetime import datetime, timedelta, timezone
12
from http import HTTPStatus
23
from unittest import TestCase
34
from urllib.parse import urlparse
@@ -203,6 +204,49 @@ def test_user_can_get_a_confirmation_token_resent(client, session):
203204
)
204205

205206

207+
@pytest.mark.parametrize(
208+
"last_confirmation_sent_at,is_rate_limited",
209+
[
210+
(None, False),
211+
(datetime.now(timezone.utc) - timedelta(hours=2), False),
212+
(datetime.now(timezone.utc) - timedelta(minutes=10), True),
213+
],
214+
)
215+
def test_user_rate_limited_if_resending_confirmation_too_soon(
216+
client, session, last_confirmation_sent_at, is_rate_limited
217+
):
218+
# Should only send email if not rate limited
219+
log_capture = TestCase.assertNoLogs if is_rate_limited else TestCase.assertLogs
220+
221+
with (
222+
current_app.test_request_context(),
223+
log_capture(current_app.logger) as log,
224+
):
225+
_, user = login_user(client)
226+
user.last_confirmation_sent_at = last_confirmation_sent_at
227+
session.commit()
228+
229+
rv = client.get(url_for("auth.resend_confirmation"), follow_redirects=True)
230+
231+
if not is_rate_limited:
232+
assert b"A new confirmation email has been sent to you." in rv.data
233+
assert (
234+
f"{current_app.config[KEY_OO_MAIL_SUBJECT_PREFIX]} Confirm Your Account"
235+
in str(log.output)
236+
)
237+
238+
# check that confirmation time was updated
239+
assert user.last_confirmation_sent_at > datetime.now(
240+
timezone.utc
241+
) - timedelta(seconds=1)
242+
else:
243+
assert (
244+
b"We already sent a confirmation email to you recently. Please try again later."
245+
in rv.data
246+
)
247+
assert user.last_confirmation_sent_at == last_confirmation_sent_at
248+
249+
206250
def test_user_can_get_password_reset_token_sent(client, session):
207251
with (
208252
current_app.test_request_context(),
@@ -247,6 +291,49 @@ def test_user_can_get_password_reset_token_sent_with_differently_cased_email(
247291
)
248292

249293

294+
@pytest.mark.parametrize(
295+
"last_reset_sent_at,is_rate_limited",
296+
[
297+
(None, False),
298+
(datetime.now(timezone.utc) - timedelta(hours=2), False),
299+
(datetime.now(timezone.utc) - timedelta(minutes=10), True),
300+
],
301+
)
302+
def test_user_rate_limited_if_resending_reset_too_soon(
303+
client, session, last_reset_sent_at, is_rate_limited
304+
):
305+
with (
306+
current_app.test_request_context(),
307+
TestCase.assertLogs(current_app.logger) as log,
308+
):
309+
user = User.query.filter_by(is_administrator=True).first()
310+
user.last_reset_sent_at = last_reset_sent_at
311+
session.commit()
312+
313+
form = PasswordResetRequestForm(email=user.email)
314+
rv = client.post(
315+
url_for("auth.password_reset_request"),
316+
data=form.data,
317+
follow_redirects=True,
318+
)
319+
320+
assert b"An email with instructions to reset your password" in rv.data
321+
if not is_rate_limited:
322+
assert (
323+
f"{current_app.config[KEY_OO_MAIL_SUBJECT_PREFIX]} Reset Your Password"
324+
in str(log.output)
325+
)
326+
assert user.last_reset_sent_at > datetime.now(timezone.utc) - timedelta(
327+
seconds=1
328+
)
329+
else:
330+
assert (
331+
f"{current_app.config[KEY_OO_MAIL_SUBJECT_PREFIX]} Reset Your Password"
332+
not in str(log.output)
333+
)
334+
assert user.last_reset_sent_at == last_reset_sent_at
335+
336+
250337
def test_user_can_get_reset_password_with_valid_token(client, session):
251338
with current_app.test_request_context():
252339
user = User.query.filter_by(is_administrator=True).first()

0 commit comments

Comments
 (0)