Produits Entr'ouvert » Authentic 2

La "même" lors d'une mise à jour migration plouf,

  File "/usr/lib/python2.7/dist-packages/django/db/backends/utils.py", line 64, in execute                                                      
    return self.cursor.execute(sql, params)                                                                                                     
  File "/usr/lib/python2.7/dist-packages/django/db/utils.py", line 94, in __exit__                                                              
    six.reraise(dj_exc_type, dj_exc_value, traceback)                                                                                           
  File "/usr/lib/python2.7/dist-packages/django/db/backends/utils.py", line 64, in execute                                                      
    return self.cursor.execute(sql, params)                                                                                                     
django.db.utils.IntegrityError: duplicate key value violates unique constraint "a2_rbac_role_admin_scope_ct_id_632506495f341476_uniq"           
DETAIL:  Key (admin_scope_ct_id, admin_scope_id)=(36, 1014) already exists.                                                                     

À vue de nez après avoir regardé la trace, c'est parce qu'on a unicité sur admin_scope_ct et admin_scope_id alors que le lookup pour le get_or_create lors du

        assert role.pk == administered_role.get_admin_role().pk

Ok d'ac.

Toujours pas; as tu besoin que je relise la branche ou ça va aller ?

Ça ne va pas tu as laissé l'ou dans les critères de recherche des rôles :

        if ou:
            if update_ou:
                update_fields['ou'] = ou
            else:
                kwargs['ou'] = ou <- ici
        else:
            kwargs['ou__isnull'] = True <- et là
        if create:
            role, created = self.prefetch_related('permissions').get_or_create(
            role, _ = self.prefetch_related('permissions').update_or_create(
                admin_scope_ct=ct,
                admin_scope_id=instance.pk,
                defaults={
                    'name': name,
                    'slug': slug,
                },
                defaults=update_fields,
                **kwargs)

si on ne met pas à jour l'ou on ne doit juste pas s'en préoccuper, mais mon avis c'est que pour l'instant on doit toujours mettre à jour l'OU quoi qu'il arrive.

Concernant les permissions il va falloir ne plus y stocker l'OU, tant qu'on y stockera l'OU, sur changement d'OU sur un rôle la permission va changer et l'ancienne devenir orpheline, on ne devrait mettre l'OU sur la permission que si instance est un ContentType et le paramètre ou est défini, pareil ici update_ou n'a pas de sens. Pour récupérer de la situation actuelle il va falloir passer par mon script.

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

si on ne met pas à jour l'ou on ne doit juste pas s'en préoccuper, mais mon avis c'est que pour l'instant on doit toujours mettre à jour l'OU quoi qu'il arrive.

Ok, mais :

• ce n'est pas ce que j'avais compris quand on discutait ici de rajouter explicitement un paramètre update_ou.

En fait on a pas le choix, le rôle de gestion doit suivre son objet; faudrait pas qu'en supprimant l'OU on supprime le rôle de gestion d'un objet dans une autre OU, c'est le fait de rendre ça optionnel qui me chagrine pas de le faire.

• on est bien d'accord que la fonction update_or_create va aller mettre à jour l'objet trouvé en fonction du dictionnaire qu'on lui file en paramètre default, et donc qu'on met à jour l'ou ?

Oui ça c'est ok.

• il me semblait que le code dans son état actuel appelait à relâcher les contraintes d'unicité :
  [...]
  et donc qu'il faut se préoccuper de l'ou dans tous les cas, sinon c'est s'exposer au risque de MultipleObjectsReturned.

C'est pas possible d'avoir plusieurs objets avec le même admin_scope puisque c'est une contrainte.

Concernant les permissions il va falloir ne plus y stocker l'OU, tant qu'on y stockera l'OU, sur changement d'OU sur un rôle la permission va changer et l'ancienne devenir orpheline, on ne devrait mettre l'OU sur la permission que si instance est un ContentType et le paramètre ou est défini, pareil ici update_ou n'a pas de sens. Pour récupérer de la situation actuelle il va falloir passer par mon script.

Ah oui, ça me sautait pas aux yeux à la lecture du code, mais je comprends maintenant cette histoire de permission orpheline.

Faut croiser ça avec la migration post_migrate de Valentin, voir si elle va bien récupérer une permission sans OU (mais j'ai surtout l'impression qu'elle vise le rôle avant la permission, il faudrait faire l'inverse).

Réponse de Benj sur jabber :

Pour les histoires permissions / rôles, on a un cycle pour les rôles d'administration, le rôle pointe vers la permission via admin_scope_ct et admin_scope_id et la permission pointe sur le rôle via target_ct et target_id.
Donc on peut partir de l'un ou l'autre pour retrouver le couple.
Dès qu'on arrêtera de mettre l'OU sur la permission ça deviendra stable.

(mais le test est rouge)

• on passe de count > 1 à count != 2 ? Elle vient d'où cette nouvelle permission d'administration ? Je rate quelque chose, j’ai pas l'impression que cet ajout soit introduit dans 0001 ni dans 0002.
• on a un admin_permission.target.get_admin_role() dans un if qui n'a pas l'air lié à cette instruction (le if vérifie que la permission en elle-même n’est pas dans une OU) ?

Paul Marillonnet a écrit :

Pourquoi dans 0002 :

• on passe de count > 1 à count != 2 ? Elle vient d'où cette nouvelle permission d'administration ? Je rate quelque chose, j’ai pas l'impression que cet ajout soit introduit dans 0001 ni dans 0002.

Déjà 2 c'est bien supérieur à 1, donc déjà c'est cohérent :) Un rôle d'administration d'un autre rôle a deux permissions d'administration de rôle : une sur le rôle supervisé et une autre sur lui même, ça fait 2, exemple sur villeurbanne :

villeurbannetest=# select r.id, r.slug, o.slug as operation_slug, p.target_ct_id, p.target_id from a2_rbac_role as r, a2_rbac_role_permissions as rp, a2_rbac_permission as p, django_rbac_operation o where r.id = rp.role_id and rp.permission_id = p.id and p.operation_id = o.id and r.slug like '%managers-of-role%' order by r.slug;
  id   |                                                  slug                                                   | operation_slug | target_ct_id | target_id 
-------+---------------------------------------------------------------------------------------------------------+----------------+--------------+-----------
 20371 | _a2-managers-of-role-administrateur-fonctionnel                                                         | manage_members |           31 |     18602
 20371 | _a2-managers-of-role-administrateur-fonctionnel                                                         | view           |            4 |         5
 20371 | _a2-managers-of-role-administrateur-fonctionnel                                                         | manage_members |           31 |     20371
 19976 | _a2-managers-of-role-agent                                                                              | manage_members |           31 |     19976
 19976 | _a2-managers-of-role-agent                                                                              | view           |            4 |         5
 19976 | _a2-managers-of-role-agent                                                                              | manage_members |           31 |     18604

Mais en vrai j'y vais un peu au pif là.

• on a un admin_permission.target.get_admin_role() dans un if qui n'a pas l'air lié à cette instruction (le if vérifie que la permission en elle-même n’est pas dans une OU) ?

J'utilise l'effet de bord qui fait que get_admin_role() va virer l'OU de la permission via .update_or_create(..., defaults={'ou': None}) dans le cas du rôle d'administration d'un rôle; ce qui va corriger le problème.

Benjamin Dauvergne a écrit :

Déjà 2 c'est bien supérieur à 1, donc déjà c'est cohérent :) Un rôle d'administration d'un autre rôle a deux permissions d'administration de rôle : une sur le rôle supervisé et une autre sur lui même, ça fait 2, exemple sur villeurbanne :

[...]

Mais en vrai j'y vais un peu au pif là.

Ok, merci c’est plus clair. Simplement je ne comprends pas comment le elif count > 1: self.warning(…) n'a pas été à l'origine de faux positifs jusque là à l'exécution de la commande. Je vais regarder.

J'utilise l'effet de bord qui fait que get_admin_role() va virer l'OU de la permission via .update_or_create(..., defaults={'ou': None}) dans le cas du rôle d'administration d'un rôle; ce qui va corriger le problème.

D'ac ok, j'ai pas l'impression que ce comportement correctif soit testé alors je vais relire et dérouler à tête reposée pour qu'on soit sûr de notre coup, et peut-être je vais ajouter un assert dans les tests aussi.

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Déjà 2 c'est bien supérieur à 1, donc déjà c'est cohérent :) Un rôle d'administration d'un autre rôle a deux permissions d'administration de rôle : une sur le rôle supervisé et une autre sur lui même, ça fait 2, exemple sur villeurbanne :

[...]

Mais en vrai j'y vais un peu au pif là.

Ok, merci c’est plus clair. Simplement je ne comprends pas comment le elif count > 1: self.warning(…) n'a pas été à l'origine de faux positifs jusque là à l'exécution de la commande. Je vais regarder.

Ah si plein mais comme c'est juste des warnings qui ne corrigent rien, ce n'était pas bien grave.

J'utilise l'effet de bord qui fait que get_admin_role() va virer l'OU de la permission via .update_or_create(..., defaults={'ou': None}) dans le cas du rôle d'administration d'un rôle; ce qui va corriger le problème.

D'ac ok, j'ai pas l'impression que ce comportement correctif soit testé alors je vais relire et dérouler à tête reposée pour qu'on soit sûr de notre coup, et peut-être je vais ajouter un assert dans les tests aussi.

Mais tu testes ta modif ou check-and-repair ? Personne utilise check-and-repair à part moi pour l'instant.

Benjamin Dauvergne a écrit :

Mais tu testes ta modif ou check-and-repair ? Personne utilise check-and-repair à part moi pour l'instant.

Iirc on avait quand même ajouté des tests à cette commande check-and-repair. C'est là que j'aurais imaginé ajouter un assert pour vérifier ce comportement sur l'OU de la permission.

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Mais tu testes ta modif ou check-and-repair ? Personne utilise check-and-repair à part moi pour l'instant.

Iirc on avait quand même ajouté des tests à cette commande check-and-repair. C'est là que j'aurais imaginé ajouter un assert pour vérifier ce comportement sur l'OU de la permission.

D'ac je vais ajouter ça.

Paul Marillonnet a écrit :

Ack […]

Par contre le bogue n’est plus critique maintenant que les instances problématiques sont passées à la moulinette du check-and-repair. Je ne verrais pas ça poussé avant vendredi.

Benjamin Dauvergne a écrit :

Voilà.

PS: comme convenu je pousserai demain.

Top nickel.