Skip to content
Snippets Groups Projects
Commit 6aff4e91 authored by Martin Mareš's avatar Martin Mareš
Browse files

Bezpečné transakce při zakládání uživatelů/účastníků/účastí

Closes #314:

Pokud organizátor odešle POST na přidání nového účastníka soutěže
dvakrát rychle po sobě (třeba kvůli nějakému automatickému retry
po rozpadu spojení), dvě různé DB transakce se snaží založit uživatele
se stejným loginem. Jedna z nich selže na unikátnosti sloupce email.

V defaultní úrovni izolace transakcí (READ COMMITTED) to nemá žádné
hezké řešení. Nepomůže SELECT ... FOR UPDATE, jelikož ten zamyká pouze
nalezené řádky, nikoliv neexistenci dalších řádků vyhovujících podmínce.

Co by se dalo dělat:

(1) Zvýšit úroveň izolace aspoň na READ REPEATABLE. To vyřeší problém,
    ale současně může začít víceméně jakákoliv zapisující transakce
    failovat. Vyžadovalo by dopsat retry do prakticky všech míst v OSMO,
    kde je nějaký commit.

(2) Retryovat specificky transakce na zakládání užívatelů (a účastí apod.).
    Tohle nejde snadno, jelikoz jsou i součástí dlouho běžících transakcí
    v importech (zatím jsme se snažíli, aby byl celý import atomický
    a v případě selhání se celý rollbackoval). To by možná mohly vyřešit
    subtransakce.

(3) Zamykat celou tabulku s uživateli, než na ní provedeme první SELECT.
    To by asi vyřešilo problém, ale byl by potřeba zápisový zámek, takže
    by paralelně nemohla běžet žádná čtení. A také by se to potenciálně
    mohlo deadlockovat (potřebujeme v jedné transakci postupně lock na
    uživatele, účastníky a účasti a locky platí až do konce transakce).

(4) Používat INSERT ... ON CONFLICT <něco>. To vypadá bezpečně, jen to není
    moc pohodlné, zejména proto, že s tím nepočítá ORM, takže je potřeba
    dělat všechno ručně.

Zatím jsem zvolil (4), protože mi přijde, že to změny udržuje lokální
a funguje i s dlouhými transakcemi při importu. Výhledově bych se ale
chtěl zamyslet nad tím, jak takové věci řešit co nejuniverzálněji
a nejpohodlněji.
parent 37185c2f
No related branches found
No related tags found
1 merge request!129Bezpečné transakce při zakládání uživatelů/účastníků/účastí
...@@ -7,6 +7,7 @@ import email.errors ...@@ -7,6 +7,7 @@ import email.errors
import email.headerregistry import email.headerregistry
import re import re
import secrets import secrets
from sqlalchemy.dialects.postgresql import insert as pgsql_insert
from typing import Optional, Tuple from typing import Optional, Tuple
import mo import mo
...@@ -96,22 +97,37 @@ def change_user_to_org(user, reason: str): ...@@ -96,22 +97,37 @@ def change_user_to_org(user, reason: str):
def find_or_create_user(email: str, krestni: Optional[str], prijmeni: Optional[str], is_org: bool, reason: str, allow_change_user_to_org=False) -> Tuple[db.User, bool, bool]: def find_or_create_user(email: str, krestni: Optional[str], prijmeni: Optional[str], is_org: bool, reason: str, allow_change_user_to_org=False) -> Tuple[db.User, bool, bool]:
sess = db.get_session() sess = db.get_session()
user = sess.query(db.User).with_for_update().filter_by(email=email).one_or_none() user = sess.query(db.User).filter_by(email=email).one_or_none()
is_new = user is None is_new = False
is_change_user_to_org = False is_change_user_to_org = False
if user is None: # HACK: Podmínku je nutné zapsat znovu místo užití is_new, jinak si s tím mypy neporadí if user is None: # HACK: Podmínku je nutné zapsat znovu místo užití is_new, jinak si s tím mypy neporadí
if not krestni or not prijmeni: if not krestni or not prijmeni:
raise mo.CheckError('Osoba s daným e-mailem zatím neexistuje, je nutné uvést její jméno.') raise mo.CheckError('Osoba s daným e-mailem zatím neexistuje, je nutné uvést její jméno.')
user = db.User(email=email, first_name=krestni, last_name=prijmeni, is_org=is_org)
sess.add(user) res = sess.connection().execute(
sess.flush() # Aby uživatel dostal user_id pgsql_insert(db.User.__table__)
.values(
email=email,
first_name=krestni,
last_name=prijmeni,
is_org=is_org,
)
.on_conflict_do_nothing()
.returning(db.User.user_id)
)
user = sess.query(db.User).filter_by(email=email).one()
if res.fetchall():
is_new = True
logger.info(f'{reason.title()}: Založen uživatel user=#{user.user_id} email=<{user.email}>') logger.info(f'{reason.title()}: Založen uživatel user=#{user.user_id} email=<{user.email}>')
mo.util.log( mo.util.log(
type=db.LogType.user, type=db.LogType.user,
what=user.user_id, what=user.user_id,
details={'action': 'create-user', 'reason': reason, 'new': db.row2dict(user)}, details={'action': 'create-user', 'reason': reason, 'new': db.row2dict(user)},
) )
else:
if (krestni and user.first_name != krestni) or (prijmeni and user.last_name != prijmeni): if (krestni and user.first_name != krestni) or (prijmeni and user.last_name != prijmeni):
raise mo.CheckError(f'Osoba již registrována s odlišným jménem {user.full_name()}') raise mo.CheckError(f'Osoba již registrována s odlišným jménem {user.full_name()}')
if (user.is_admin or user.is_org) != is_org: if (user.is_admin or user.is_org) != is_org:
...@@ -123,13 +139,15 @@ def find_or_create_user(email: str, krestni: Optional[str], prijmeni: Optional[s ...@@ -123,13 +139,15 @@ def find_or_create_user(email: str, krestni: Optional[str], prijmeni: Optional[s
raise CheckErrorOrgIsUser('Nelze předefinovat účastníka na organizátora.') raise CheckErrorOrgIsUser('Nelze předefinovat účastníka na organizátora.')
else: else:
raise mo.CheckError('Nelze předefinovat organizátora na účastníka.') raise mo.CheckError('Nelze předefinovat organizátora na účastníka.')
return user, is_new, is_change_user_to_org return user, is_new, is_change_user_to_org
def find_or_create_participant(user: db.User, year: int, school_id: Optional[int], birth_year: Optional[int], grade: Optional[str], reason: str) -> Tuple[db.Participant, bool]: def find_or_create_participant(user: db.User, year: int, school_id: Optional[int], birth_year: Optional[int], grade: Optional[str], reason: str) -> Tuple[db.Participant, bool]:
sess = db.get_session() sess = db.get_session()
part = sess.query(db.Participant).get((user.user_id, year)) part = sess.query(db.Participant).get((user.user_id, year))
is_new = part is None is_new = False
if part is None: if part is None:
prev_part = sess.query(db.Participant).filter_by(user_id=user.user_id).order_by(db.Participant.year.desc()).limit(1).one_or_none() prev_part = sess.query(db.Participant).filter_by(user_id=user.user_id).order_by(db.Participant.year.desc()).limit(1).one_or_none()
if not school_id: if not school_id:
...@@ -144,19 +162,37 @@ def find_or_create_participant(user: db.User, year: int, school_id: Optional[int ...@@ -144,19 +162,37 @@ def find_or_create_participant(user: db.User, year: int, school_id: Optional[int
raise mo.CheckError('Osoba s daným e-mailem zatím není zaregistrovaná do ročníku, je nutné uvést rok narození.') raise mo.CheckError('Osoba s daným e-mailem zatím není zaregistrovaná do ročníku, je nutné uvést rok narození.')
if not grade: if not grade:
raise mo.CheckError('Osoba s daným e-mailem zatím není zaregistrovaná do ročníku, je nutné uvést ročník.') raise mo.CheckError('Osoba s daným e-mailem zatím není zaregistrovaná do ročníku, je nutné uvést ročník.')
part = db.Participant(user=user, year=year, school=school_id, birth_year=birth_year, grade=grade)
sess.add(part) res = sess.connection().execute(
pgsql_insert(db.Participant.__table__)
.values(
user_id=user.user_id,
year=year,
school=school_id,
birth_year=birth_year,
grade=grade,
)
.on_conflict_do_nothing()
.returning(db.Participant.user_id)
)
part = sess.query(db.Participant).get((user.user_id, year))
assert part is not None
if res.fetchall():
is_new = True
logger.info(f'{reason.title()}: Založen účastník #{user.user_id}') logger.info(f'{reason.title()}: Založen účastník #{user.user_id}')
mo.util.log( mo.util.log(
type=db.LogType.participant, type=db.LogType.participant,
what=user.user_id, what=user.user_id,
details={'action': 'create-participant', 'reason': reason, 'new': db.row2dict(part)}, details={'action': 'create-participant', 'reason': reason, 'new': db.row2dict(part)},
) )
else:
if ((school_id and part.school != school_id) if ((school_id and part.school != school_id)
or (grade and part.grade != grade) or (grade and part.grade != grade)
or (birth_year and part.birth_year != birth_year)): or (birth_year and part.birth_year != birth_year)):
raise mo.CheckError('Účastník již zaregistrován s odlišnou školou/ročníkem/rokem narození') raise mo.CheckError('Účastník již zaregistrován s odlišnou školou/ročníkem/rokem narození')
return part, is_new return part, is_new
...@@ -165,27 +201,47 @@ def find_or_create_participation(user: db.User, contest: db.Contest, place: Opti ...@@ -165,27 +201,47 @@ def find_or_create_participation(user: db.User, contest: db.Contest, place: Opti
place = contest.place place = contest.place
sess = db.get_session() sess = db.get_session()
pion = None
is_new = False
retry = False
while pion is None:
pions = (sess.query(db.Participation) pions = (sess.query(db.Participation)
.filter_by(user=user) .filter_by(user=user)
.filter(db.Participation.contest.has(db.Contest.round == contest.round)) .filter(db.Participation.contest.has(db.Contest.round == contest.round))
.all()) .all())
is_new = pions == [] if len(pions) == 0:
assert not retry
retry = True
res = sess.connection().execute(
pgsql_insert(db.Participation.__table__)
.values(
user_id=user.user_id,
contest_id=contest.contest_id,
place_id=place.place_id,
state=db.PartState.active,
)
.on_conflict_do_nothing()
.returning(db.Participation.user_id)
)
if res.fetchall():
is_new = True
elif len(pions) == 1:
pion = pions[0]
else:
raise mo.CheckError('Již se tohoto kola účastní ve více oblastech, což by nemělo být možné')
if pion.place != place:
raise mo.CheckError(f'Již se tohoto kola účastní v {contest.round.get_level().name_locative("jiném", "jiné", "jiném")} ({pion.place.get_code()})')
if is_new: if is_new:
pion = db.Participation(user=user, contest=contest, place_id=place.place_id, state=db.PartState.active)
sess.add(pion)
logger.info(f'{reason.title()}: Založena účast user=#{user.user_id} contest=#{contest.contest_id} place=#{place.place_id}') logger.info(f'{reason.title()}: Založena účast user=#{user.user_id} contest=#{contest.contest_id} place=#{place.place_id}')
mo.util.log( mo.util.log(
type=db.LogType.participant, type=db.LogType.participant,
what=user.user_id, what=user.user_id,
details={'action': 'add-to-contest', 'reason': reason, 'new': db.row2dict(pion)}, details={'action': 'add-to-contest', 'reason': reason, 'new': db.row2dict(pion)},
) )
elif len(pions) == 1:
pion = pions[0]
if pion.place != place:
raise mo.CheckError(f'Již se tohoto kola účastní v {contest.round.get_level().name_locative("jiném", "jiné", "jiném")} ({pion.place.get_code()})')
else:
raise mo.CheckError('Již se tohoto kola účastní ve více oblastech, což by nemělo být možné')
return pion, is_new return pion, is_new
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment