fix(lifecycle): OAuth pw 500, invite lockout, reminder re-arm, partial-unique
Drift-audit Group 8 (lifecycle gaps): - change_password no longer 500s for OAuth-only users: short-circuit when password_hash is None (verify_password would crash on None) so the route returns a clean 4xx instead of a 500. - register_with_invitation no longer locks the invitee out on a username collision: create the user FIRST, then mark the token used, so a failed creation (409) leaves the single-use invite valid for retry. - update_event re-arms reminder_sent_at when start_dt/reminder_minutes change, so a rescheduled event fires again instead of being permanently suppressed. - Migration 0061: uq_topic_per_rulebook / uq_rule_per_topic become PARTIAL unique indexes (WHERE deleted_at IS NULL). Trashing 'X' then recreating it no longer 500s on the dead row's title. Model __table_args__ updated to match. Deferred: per-occurrence reminders for recurring events (event_scheduler) — needs a per-occurrence reminder-state design, not a one-line gate tweak. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,49 @@
|
|||||||
|
"""partial-unique topic/rule titles (ignore soft-deleted rows)
|
||||||
|
|
||||||
|
Revision ID: 0061
|
||||||
|
Revises: 0060
|
||||||
|
Create Date: 2026-06-02
|
||||||
|
|
||||||
|
Topics and rules are soft-deleted (SoftDeleteMixin), but uq_topic_per_rulebook
|
||||||
|
and uq_rule_per_topic were plain UNIQUE constraints. Trashing a topic/rule
|
||||||
|
named "X" then creating a new "X" — or restoring into a reused title slot —
|
||||||
|
collided with the dead row and raised an unhandled 500. Replace the full
|
||||||
|
UNIQUE constraints with partial unique indexes that only consider live
|
||||||
|
(deleted_at IS NULL) rows.
|
||||||
|
"""
|
||||||
|
from alembic import op
|
||||||
|
import sqlalchemy as sa
|
||||||
|
|
||||||
|
|
||||||
|
revision = "0061"
|
||||||
|
down_revision = "0060"
|
||||||
|
branch_labels = None
|
||||||
|
depends_on = None
|
||||||
|
|
||||||
|
|
||||||
|
def upgrade() -> None:
|
||||||
|
op.drop_constraint("uq_topic_per_rulebook", "rulebook_topics", type_="unique")
|
||||||
|
op.create_index(
|
||||||
|
"uq_topic_per_rulebook",
|
||||||
|
"rulebook_topics",
|
||||||
|
["rulebook_id", "title"],
|
||||||
|
unique=True,
|
||||||
|
postgresql_where=sa.text("deleted_at IS NULL"),
|
||||||
|
)
|
||||||
|
op.drop_constraint("uq_rule_per_topic", "rules", type_="unique")
|
||||||
|
op.create_index(
|
||||||
|
"uq_rule_per_topic",
|
||||||
|
"rules",
|
||||||
|
["topic_id", "title"],
|
||||||
|
unique=True,
|
||||||
|
postgresql_where=sa.text("deleted_at IS NULL"),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def downgrade() -> None:
|
||||||
|
op.drop_index("uq_rule_per_topic", table_name="rules")
|
||||||
|
op.create_unique_constraint("uq_rule_per_topic", "rules", ["topic_id", "title"])
|
||||||
|
op.drop_index("uq_topic_per_rulebook", table_name="rulebook_topics")
|
||||||
|
op.create_unique_constraint(
|
||||||
|
"uq_topic_per_rulebook", "rulebook_topics", ["rulebook_id", "title"]
|
||||||
|
)
|
||||||
@@ -1,6 +1,6 @@
|
|||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
|
|
||||||
from sqlalchemy import BigInteger, Boolean, Column, DateTime, ForeignKey, Integer, Table, Text, UniqueConstraint
|
from sqlalchemy import BigInteger, Boolean, Column, DateTime, ForeignKey, Index, Integer, Table, Text, text
|
||||||
from sqlalchemy.orm import Mapped, mapped_column
|
from sqlalchemy.orm import Mapped, mapped_column
|
||||||
|
|
||||||
from fabledassistant.models import Base
|
from fabledassistant.models import Base
|
||||||
@@ -42,8 +42,13 @@ class Rulebook(Base, SoftDeleteMixin):
|
|||||||
|
|
||||||
class RulebookTopic(Base, SoftDeleteMixin):
|
class RulebookTopic(Base, SoftDeleteMixin):
|
||||||
__tablename__ = "rulebook_topics"
|
__tablename__ = "rulebook_topics"
|
||||||
|
# Partial unique: a title is unique among LIVE topics in a rulebook, so a
|
||||||
|
# trashed topic doesn't block recreating/restoring the same title.
|
||||||
__table_args__ = (
|
__table_args__ = (
|
||||||
UniqueConstraint("rulebook_id", "title", name="uq_topic_per_rulebook"),
|
Index(
|
||||||
|
"uq_topic_per_rulebook", "rulebook_id", "title",
|
||||||
|
unique=True, postgresql_where=text("deleted_at IS NULL"),
|
||||||
|
),
|
||||||
)
|
)
|
||||||
|
|
||||||
id: Mapped[int] = mapped_column(BigInteger, primary_key=True)
|
id: Mapped[int] = mapped_column(BigInteger, primary_key=True)
|
||||||
@@ -76,8 +81,13 @@ class RulebookTopic(Base, SoftDeleteMixin):
|
|||||||
|
|
||||||
class Rule(Base, SoftDeleteMixin):
|
class Rule(Base, SoftDeleteMixin):
|
||||||
__tablename__ = "rules"
|
__tablename__ = "rules"
|
||||||
|
# Partial unique: title unique among LIVE rules in a topic (soft-deleted
|
||||||
|
# rules don't block recreating/restoring the same title).
|
||||||
__table_args__ = (
|
__table_args__ = (
|
||||||
UniqueConstraint("topic_id", "title", name="uq_rule_per_topic"),
|
Index(
|
||||||
|
"uq_rule_per_topic", "topic_id", "title",
|
||||||
|
unique=True, postgresql_where=text("deleted_at IS NULL"),
|
||||||
|
),
|
||||||
)
|
)
|
||||||
|
|
||||||
id: Mapped[int] = mapped_column(BigInteger, primary_key=True)
|
id: Mapped[int] = mapped_column(BigInteger, primary_key=True)
|
||||||
|
|||||||
@@ -106,6 +106,11 @@ async def change_password(user_id: int, current_password: str, new_password: str
|
|||||||
user = await session.get(User, user_id)
|
user = await session.get(User, user_id)
|
||||||
if not user:
|
if not user:
|
||||||
return None
|
return None
|
||||||
|
# OAuth-only accounts have no local password hash — verify_password
|
||||||
|
# would crash on None. Treat as "no local credential to change" (the
|
||||||
|
# route turns None into a clean 4xx, not a 500).
|
||||||
|
if user.password_hash is None:
|
||||||
|
return None
|
||||||
if not verify_password(current_password, user.password_hash):
|
if not verify_password(current_password, user.password_hash):
|
||||||
return None
|
return None
|
||||||
user.password_hash = hash_password(new_password)
|
user.password_hash = hash_password(new_password)
|
||||||
@@ -325,12 +330,23 @@ async def register_with_invitation(raw_token: str, username: str, password: str)
|
|||||||
if not invitation or invitation.used or invitation.expires_at < datetime.now(timezone.utc):
|
if not invitation or invitation.used or invitation.expires_at < datetime.now(timezone.utc):
|
||||||
return None
|
return None
|
||||||
|
|
||||||
invitation.used = True
|
invitation_id = invitation.id
|
||||||
await session.commit()
|
invite_email = invitation.email
|
||||||
|
|
||||||
# Create user outside the invitation session
|
# Create the user FIRST, then consume the token. create_user can fail (e.g.
|
||||||
user = await create_user(username, password, invitation.email)
|
# a username collision raises and the route returns 409); marking the token
|
||||||
logger.info("User '%s' registered via invitation for %s", username, invitation.email)
|
# used before that would burn this single-use invite and lock the invitee
|
||||||
|
# out of retrying. Ordering it after means a failed creation leaves the
|
||||||
|
# token valid.
|
||||||
|
user = await create_user(username, password, invite_email)
|
||||||
|
|
||||||
|
async with async_session() as session:
|
||||||
|
invitation = await session.get(InvitationToken, invitation_id)
|
||||||
|
if invitation is not None:
|
||||||
|
invitation.used = True
|
||||||
|
await session.commit()
|
||||||
|
|
||||||
|
logger.info("User '%s' registered via invitation for %s", username, invite_email)
|
||||||
return user
|
return user
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -331,6 +331,11 @@ async def update_event(user_id: int, event_id: int, **fields) -> Event | None:
|
|||||||
for key, value in fields.items():
|
for key, value in fields.items():
|
||||||
if key in allowed and (value is not None or key in nullable):
|
if key in allowed and (value is not None or key in nullable):
|
||||||
setattr(event, key, value)
|
setattr(event, key, value)
|
||||||
|
# Re-arm the reminder when the timing changes, so an event moved to a
|
||||||
|
# new (future) time — or given a new lead time — fires again instead of
|
||||||
|
# being permanently suppressed by a stale reminder_sent_at.
|
||||||
|
if "start_dt" in fields or "reminder_minutes" in fields:
|
||||||
|
event.reminder_sent_at = None
|
||||||
await session.commit()
|
await session.commit()
|
||||||
await session.refresh(event)
|
await session.refresh(event)
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user