Project

General

Profile

Development #65411

auth_fc: locker le sub pendant les création de compte/liaison

Added by Benjamin Dauvergne 9 months ago. Updated 3 months ago.

Status:
En cours
Priority:
Normal
Category:
-
Target version:
-
Start date:
19 May 2022
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

On le fait déjà pour l'email, mais pour éviter toute action concurrente on devrait aussi locker le sub, ça simplifiera le code normalement.


Files

History

#1

Updated by Paul Marillonnet 5 months ago

  • Status changed from Nouveau to En cours
  • Assignee set to Paul Marillonnet
#2

Updated by Paul Marillonnet 5 months ago

À discuter mais je ne pense pas qu’on puisse pour autant retirer les try: … except sur les IntegrityError sur la liaison et la création, j’ai l’impression qu’ils proviennent de l’hypothèse de non-unicité des subs servis par FC auquel cas le verrou introduit ici ne peut pas s’y substituer.

#3

Updated by Paul Marillonnet 5 months ago

(La branche est basée sur le 0001 de #65412.)

#4

Updated by Benjamin Dauvergne 5 months ago

  • Status changed from Solution proposée to En cours

En fait il faut remonter le atomic() (on ne peut verrouiller que dans une transaction, les tests sont trompeurs car ils tournent déjà dans une transaction, ici la méthode link() est cassée je pense) plus haut dans handle_authorization_response et y gérer le lock ici, il sera unique.

Ensuit dans link, on peut maintenant faire un check explicite de l'existence du sub ou pas (et donc faudrait faire un lock aussi dans le code de suppression d'une liaison, mais là c'est du fignolage), plutôt qu'utiliser IntegrityError comme indication de ce fait.

Dès qu'on s'asssure d'avoir le lock dans tous les chemins, tous ces chemins peuvent être simplifiés en supposant qu'aucune exécution concurrente n'est possible, donc on peut faire tranquillement des SELECT sans se demander si les choses ne vont pas bouger sous nos pieds. C'est en ça que ça simplifie le code.

#5

Updated by Paul Marillonnet 5 months ago

Ok, pour la création de compte FC le try: … except IntegrityError était là aussi pour attraper les violations de contrainte d’unicité sur le doublet (user, order) concernant l’objet FcAccount que l’on crée. Je ne me souviens plus pourquoi on tolère plusieurs liaisons FC pour un même utilisateurs, mais puisque c’est le cas il faut aussi un lock sur l’email de l’usager concerné pour rétablir l’ordre des comptes déjà présents et créer la nouvelle liaison avec un order:=0.

#6

Updated by Paul Marillonnet 5 months ago

  • Status changed from Solution proposée to En cours

(Et il manque aussi la simplification sur le link(), je regarde.)

#7

Updated by Paul Marillonnet 5 months ago

Paul Marillonnet a écrit :

(Et il manque aussi la simplification sur le link(), je regarde.)

Ici, dans cette nouvelle version du patche.

Edit: et une correction pylint sur un import mort, et le retrait d’un commentaire obsolète "As we intercept IntegrityError […]", présents dans la branche.

#8

Updated by Benjamin Dauvergne 3 months ago

  • Status changed from Solution proposée to En cours

Paul Marillonnet a écrit :

Ok, pour la création de compte FC le try: … except IntegrityError était là aussi pour attraper les violations de contrainte d’unicité sur le doublet (user, order) concernant l’objet FcAccount que l’on crée. Je ne me souviens plus pourquoi on tolère plusieurs liaisons FC pour un même utilisateurs, mais puisque c’est le cas il faut aussi un lock sur l’email de l’usager concerné pour rétablir l’ordre des comptes déjà présents et créer la nouvelle liaison avec un order:=0.

On ne tolère pas plusieurs liaison, le order a été introduit pour conserver des liaisons existantes en double et pouvoir migrer vers la situation actuelle d'une seule liaison par sub et par utilisateur sans avoir à en supprimer au moment de cette migration. Donc plusieurs soucis :


+            if not models.FcAccount.objects.filter(sub=self.sub).exclude(user=request.user).count():
+                self.fc_account, created = models.FcAccount.objects.get_or_create(
+                    sub=self.sub,
+                    user=request.user,
+                    order=0,
+                    defaults={
+                        'token': json.dumps(self.token),
+                        'user_info': json.dumps(self.user_info),
+                    },
+                )

Ici la condition devrait être not FcAcount.objects.filter(Q(sub=sub) | Q(user=user)), pour justement éviter les doublons d'user ou de sub.

Ensuite on ne doit pas créer de liaison avec order>0 :

+            # reshuffle existing accounts order
+            Lock.lock_email(email=user.email)
+            for account in models.FcAccount.objects.filter(user=user).order_by('-order'):
+                account.order += 1
+                account.save()

Aussi tu introduis un filter_by_email() qui n'a pas de rapport avec ce ticket je pense.

Also available in: Atom PDF