Produits Entr'ouvert » Authentic 2

Et donc j’imagine qu’il manque dans la migration un AlterField sur la FK côté mapping, avant le AlterModelBase défini à l’occasion, quelque chose du genre :

diff --git a/src/authentic2_auth_oidc/migrations/0011_auto_20220413_1632.py b/src/authentic2_auth_oidc/migrations/0011_auto_20220413_1632.py
index 713fe5071..b418f7a99 100644
--- a/src/authentic2_auth_oidc/migrations/0011_auto_20220413_1632.py
+++ b/src/authentic2_auth_oidc/migrations/0011_auto_20220413_1632.py
@@ -61,5 +61,16 @@ class Migration(migrations.Migration):
                 to='authenticators.BaseAuthenticator',
             ),
         ),
+        migrations.AlterField(
+            model_name='oidcclaimmapping',
+            name='provider',
+            field=models.ForeignKey(
+                related_name='claim_mappings',
+                verbose_name='provider',
+                to='authentic2_auth_oidc.OIDCProvider',
+                to_field='baseauthenticator_ptr',
+                on_delete=models.CASCADE,
+            ),
+        ),
         AlterModelBase('oidcprovider', 'authenticators.baseauthenticator'),
     ]
diff --git a/src/authentic2_auth_oidc/models.py b/src/authentic2_auth_oidc/models.py
index 944f40270..8b97630fe 100644
--- a/src/authentic2_auth_oidc/models.py
+++ b/src/authentic2_auth_oidc/models.py
@@ -208,7 +208,11 @@ class OIDCClaimMapping(models.Model):
     ]

     provider = models.ForeignKey(
-        to='OIDCProvider', verbose_name=_('provider'), related_name='claim_mappings', on_delete=models.CASCADE
+        to='OIDCProvider',
+        to_field='baseauthenticator_ptr',
+        verbose_name=_('provider'),
+        related_name='claim_mappings',
+        on_delete=models.CASCADE,
     )
     claim = models.CharField(max_length=128, verbose_name=_('claim'), validators=[validate_template])
     attribute = models.CharField(max_length=64, verbose_name=_('attribute'))

Et donc effectivement, logiquement après application de 0011, en base la colonne id de la table des fournisseurs oidc a disparu.
La colonne provider_id de la tables oidcclaimmapping pointe vers un id maintenant inexistant.
En l’absence du alterfield(to_field="baseauthenticator_ptr_id") mentionné dans la première note de ce ticket, je ne crois pas que vider la table des mappings pour les récréer à nouveau fonctionne.
Je suis à court d’idées pour trouver une solution (autre qu’un git-revert de cette migration 0011) qui à la fois corrige les confs oidc en erreur, et qui empêche 0011 en l’état actuel de s’appliquer (par exemple lors de la màj des prod de jeudi prochain ☺) et de casser définitivement les FK existantes.

Si quelqu’un a une idée, je suis preneur.

J'ai voulu vérifier si on avait des incohérences en base (test.saas.entrouvert.org) et j'observe plusieurs schémas sur lesquels baseauthenticator_ptr_id n'existe pas. Sur ceux où la migration a été faite, on n'a pour le moment pas d'incohérence visible (on pourrait mettre une FK vers baseauthenticator_ptr_id sans que ça casse).

Paul Marillonnet a écrit :

Et donc effectivement, logiquement après application de 0011, en base la colonne id de la table des fournisseurs oidc a disparu.
La colonne provider_id de la tables oidcclaimmapping pointe vers un id maintenant inexistant.
En l’absence du alterfield(to_field="baseauthenticator_ptr_id") mentionné dans la première note de ce ticket, je ne crois pas que vider la table des mappings pour les récréer à nouveau fonctionne.

Il y aurait un problème à ajouter ça ? Ça ne plait pas à django ? Côte SQL ça me choque pas en tout cas.

Je suis à court d’idées pour trouver une solution (autre qu’un git-revert de cette migration 0011) qui à la fois corrige les confs oidc en erreur, et qui empêche 0011 en l’état actuel de s’appliquer (par exemple lors de la màj des prod de jeudi prochain ☺) et de casser définitivement les FK existantes.

Quelles sont les confs en erreur ?

Pierre Ducroquet a écrit :

Il y aurait un problème à ajouter ça ? Ça ne plait pas à django ? Côte SQL ça me choque pas en tout cas.

Je notais que ce AlterField aurait dû avoir lieu avant l’opération custom AlterModelBase. Maintenant que c’est fait (et que la colonne id de la table des fournisseur a sauté) je n’ai pas de solution qui va dans ce sens.

Quelles sont les confs en erreur ?

Sur la recette HDS, le fournisseur oidc de l’instance de l’académie de versailles : https://connexion-ac-versailles.test.entrouvert.org/admin/authentic2_auth_oidc/oidcprovider/7/change/
Sur la recette GL (grandlyon-cut-test-sso-1), le fournisseur oidc (qui il me semble est un autre authentic à part) : https://moncompte-rec.grandlyon.com/admin/authentic2_auth_oidc/oidcprovider/96/change/

Paul Marillonnet a écrit :

Sur la recette GL (grandlyon-cut-test-sso-1), le fournisseur oidc (qui il me semble est un autre authentic à part) : https://moncompte-rec.grandlyon.com/admin/authentic2_auth_oidc/oidcprovider/96/change/

Sur GLC l'authentification OIDC n'est plus utilisé on peut juste virer ça.

Paul Marillonnet a écrit :

Pierre Ducroquet a écrit :

Il y aurait un problème à ajouter ça ? Ça ne plait pas à django ? Côte SQL ça me choque pas en tout cas.

Je notais que ce AlterField aurait dû avoir lieu avant l’opération custom AlterModelBase. Maintenant que c’est fait (et que la colonne id de la table des fournisseur a sauté) je n’ai pas de solution qui va dans ce sens.

Je peux faire un script qui va corriger toutes les bases qui ont subi cette migration pour y ajouter la FK.
Si l'ajout de la FK échoue, ça indiquera qu'il y a une erreur, du coup j'aurai la liste précise.
Comme ça la migration corrigée pourra tourner sur la prod, et le delta avec les autres envs sera minime, modulo les erreurs à corriger.
Ça irait comme ça ?

Rapide investigation sur GLC, ce sont les ids qui ont bougé (excusez moi si je dis des trucs déjà su, j'essaye de comprendre le problème) :

In [8]: OIDCProvider.objects.get().pk
Out[8]: 96

In [9]: set(OIDCClaimMapping.objects.values_list('provider_id', flat=True))
Out[9]: {1}

Ici c'est simple y a qu'un provider, un :

In [10]: OIDCClaimMapping.objects.update(provider_id=96)
Out[10]: 1

Ce qui tombe bien c'est que la contrainte de foreignkey de ClaimMapping.provider_id vers Provider.id a sauté toute seule, je ne sais pas trop ce qui a fait ça. Après correction il faudra éventuellement faire une miggration fk -> integer puis integer -> fk pour la rétablir.

PS: fk -> integer risque de planter si il essaye de drop la contrainte qui n'existe déjà plus... il faudra s'en prémunir.

Benjamin Dauvergne a écrit :

Ce qui tombe bien c'est que la contrainte de foreignkey de ClaimMapping.provider_id vers Provider.id a sauté toute seule, je ne sais pas trop ce qui a fait ça. Après correction il faudra éventuellement faire une miggration fk -> integer puis integer -> fk pour la rétablir.

PS: fk -> integer risque de planter si il essaye de drop la contrainte qui n'existe déjà plus... il faudra s'en prémunir.

Je ne comprends pas trop cette phrase, à moins que ça ne soit un "djangoïsme": une FK est une contrainte attachée à une colonne, donc une migration fk vers autre chose... Ben ça n'a pas trop de sens.

Benjamin Dauvergne a écrit :

Rapide investigation sur GLC, ce sont les ids qui ont bougé (excusez moi si je dis des trucs déjà su, j'essaye de comprendre le problème) :
[...]

Ici c'est simple y a qu'un provider, un :
[...]
m'a suffit à corriger les choses.

Perso côté AC Versailles j’ai tapé en SQL dans les FK concernées la nouvelle valeur de la clé primaire de l’unique fournisseur. C’est revenu aussi.

Ce qui tombe bien c'est que la contrainte de foreignkey de ClaimMapping.provider_id vers Provider.id a sauté toute seule, je ne sais pas trop ce qui a fait ça.

C’est un RemoveField de la migration auth_oidc 0011 dont je ne comprends pas la nécessité.

Ici c'est simple y a qu'un provider, un :

Sur Nantes en recette il y en a 2 :
https://connexion-nantes-metropole.test.entrouvert.org/admin/authentic2_auth_oidc/oidcprovider/
Étrangement la configuration n'a pas sauté.

Ok, PierreD me confirme mon intuition, c'est bien le RemoveField('id') qui a supprimé la contrainte de FK, mais le code migration Django ne le sait pas vu qu'on lui a remis une primarykey sous le nez via le AlterModelBase.

Je propose d'ajouter à la migration 0009 en fin de migration deux alterfield sur oidcclaimmapping.provider_id et oidcaccount.provider_id, les transformant en integerfield (en plus c'est nécessaire pour que 0010 marche sinon il va refuser de modifier provider_id sir la contrainte de FK vers provider.id n'est pas respectée), le bonus c'est que Django sait maintenant que la contrainte de FK a disparu.

Ensuite on ajoute un 0012 qui refait l'opération inverse, alterfield de provider_id vers une ForeignKey.

Benjamin Dauvergne a écrit :

en plus c'est nécessaire pour que 0010 marche sinon il va refuser de modifier provider_id sir la contrainte de FK vers provider.id n'est pas respectée

Yep et le test passe complètement à côté parce que d'abord initialisation de la db avec toutes les migrations dont celle qui fait sauter la contrainte, puis rollback à 0008 mais ça ne restaure pas la contrainte :/

Ça marche bien seulement sur une nouvelle base, par contre sur une instance déjà migrée 0012 plante parce qu'on a renommé la colonne dans 0009.

Je vais tester les migrations de master sur versailles puis passer sur la branche, ensuite refaire la même chose directement sur la branche.

Automatic expiration