Skip to content

Replace ban notice text with structured banned message #1084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Louyijun wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Replace ban notice text with structured banned message #1084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions server/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
Common exception definitions
"""

from datetime import timezone

import humanize

from server.timing import datetime_now
Expand Down Expand Up @@ -39,6 +41,16 @@ def message(self):
"moderation@faforever.com</i>"
)

def to_dict(self):
expires_at = self.ban_expiry
if expires_at.tzinfo is None:
expires_at = expires_at.replace(tzinfo=timezone.utc)
return {
"command": "banned",
"expires_at": expires_at.astimezone(timezone.utc).isoformat(),
"reason": self.ban_reason,
}

def _ban_duration_text(self):
ban_duration = self.ban_expiry - datetime_now()
if ban_duration.days > 365 * 100:
Expand Down
6 changes: 1 addition & 5 deletions server/lobbyconnection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,7 @@ async def on_message_received(self, message):
"text": e.message
})
except BanError as e:
await self.send({
"command": "notice",
"style": "error",
"text": e.message()
})
await self.send(e.to_dict())
await self.abort(e.message())
except ClientError as e:
self._logger.warning(
Expand Down
24 changes: 6 additions & 18 deletions tests/integration_tests/test_login.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,9 @@ async def test_server_ban(lobby_server, user):
proto = await connect_client(lobby_server)
await perform_login(proto, user)
msg = await proto.read_message()
assert msg == {
"command": "notice",
"style": "error",
"text": (
"You are banned from FAF forever. <br>Reason: <br>Test permanent ban"
"<br><br><i>If you would like to appeal this ban, please send an "
"email to: moderation@faforever.com</i>"
)
}
assert msg["command"] == "banned"
assert msg["reason"] == "Test permanent ban"
assert "expires_at" in msg
Comment on lines +41 to +43
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Assert expires_at is UTC ISO-8601, not just present

Current checks won’t fail if the server returns a non-ISO or non-UTC timestamp. Parse and validate timezone to lock the contract required by Issue #640.

Proposed test hardening 
+from datetime import datetime, timezone
 from time import time
@@
     msg = await proto.read_message()
     assert msg["command"] == "banned"
     assert msg["reason"] == "Test permanent ban"
-    assert "expires_at" in msg
+    expires_at = datetime.fromisoformat(msg["expires_at"])
+    assert expires_at.tzinfo is not None
+    assert expires_at.utcoffset() == timezone.utc.utcoffset(expires_at)
@@
     msg = await proto.read_message()
     assert msg["command"] == "banned"
     assert msg["reason"] == "Test permanent ban"
-    assert "expires_at" in msg
+    expires_at = datetime.fromisoformat(msg["expires_at"])
+    assert expires_at.tzinfo is not None
+    assert expires_at.utcoffset() == timezone.utc.utcoffset(expires_at)

Also applies to: 70-72

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/integration_tests/test_login.py` around lines 41 - 43, The test
currently only asserts that "expires_at" exists in msg; instead parse
msg["expires_at"] (used in the assertions around the msg dict) as an ISO-8601
timestamp and validate it represents UTC (either tzinfo == timezone.utc or an
offset of +00:00 / a trailing "Z") and that parsing does not raise; replace the
simple presence check with this parse+UTC-check for both occurrences (the block
asserting msg["command"]=="banned" / msg["reason"] and the similar asserts at
lines 70-72) so the test fails if the server returns a non-ISO or non-UTC
timestamp.



@pytest.mark.parametrize("user", [
Expand All @@ -73,15 +67,9 @@ async def test_server_ban_token(lobby_server, user, jwk_priv_key, jwk_kid):
"unique_id": "some_id"
})
msg = await proto.read_message()
assert msg == {
"command": "notice",
"style": "error",
"text": (
"You are banned from FAF forever. <br>Reason: <br>Test permanent ban"
"<br><br><i>If you would like to appeal this ban, please send an "
"email to: moderation@faforever.com</i>"
)
}
assert msg["command"] == "banned"
assert msg["reason"] == "Test permanent ban"
assert "expires_at" in msg


@pytest.mark.parametrize("user", ["ban_revoked", "ban_expired"])
Expand Down
12 changes: 3 additions & 9 deletions tests/integration_tests/test_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -845,15 +845,9 @@ async def test_server_ban_prevents_hosting(lobby_server, database, command):
await proto.send_message({"command": command})

msg = await proto.read_message()
assert msg == {
"command": "notice",
"style": "error",
"text": (
"You are banned from FAF forever. <br>Reason: <br>Test live ban<br>"
"<br><i>If you would like to appeal this ban, please send an email "
"to: moderation@faforever.com</i>"
)
}
assert msg["command"] == "banned"
assert msg["reason"] == "Test live ban"
assert "expires_at" in msg


@fast_forward(5)
Expand Down
17 changes: 6 additions & 11 deletions tests/unit_tests/test_lobbyconnection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import re
from hashlib import sha256
from unittest import mock

Expand Down Expand Up @@ -1303,18 +1302,14 @@ async def test_abort_connection_if_banned(
lobbyconnection.player.id = 203
with pytest.raises(BanError) as banned_error:
await lobbyconnection.abort_connection_if_banned()
assert banned_error.value.message() == (
"You are banned from FAF forever. <br>Reason: <br>Test permanent ban"
"<br><br><i>If you would like to appeal this ban, please send an email "
"to: moderation@faforever.com</i>"
)
assert banned_error.value.to_dict()["command"] == "banned"
assert banned_error.value.to_dict()["reason"] == "Test permanent ban"
assert "expires_at" in banned_error.value.to_dict()

# test user who is banned for another 46 hours
lobbyconnection.player.id = 204
with pytest.raises(BanError) as banned_error:
await lobbyconnection.abort_connection_if_banned()
assert re.match(
r"You are banned from FAF for 1 day and 2[12]\.[0-9]+ hours. <br>"
"Reason: <br>Test ongoing ban with 46 hours left",
banned_error.value.message()
)
assert banned_error.value.to_dict()["command"] == "banned"
assert banned_error.value.to_dict()["reason"] == "Test ongoing ban with 46 hours left"
assert "expires_at" in banned_error.value.to_dict()
Loading