Produits Entr'ouvert » Authentic 2

Je ne suis pas entièrement convaincu, dans quel cas on voudra requêter ces objets ? Si c'est juste parce qu'un modèle te donne la génération automatique des formulaires ça ne me semble pas super utile.

Benjamin Dauvergne a écrit :

Je ne suis pas entièrement convaincu, dans quel cas on voudra requêter ces objets ? Si c'est juste parce qu'un modèle te donne la génération automatique des formulaires ça ne me semble pas super utile.

C'est vraiment nul comme interface d'avoir à taper du JSON, se rappeler de la syntaxe bizarre, ne pas faire de faute de frappe.

On y gagnera aussi en terme d'affichage des infos, de validation, de facilité d'évolutions et de clarification du code qui parse ça.

Valentin Deniaud a écrit :

Benjamin Dauvergne a écrit :

Je ne suis pas entièrement convaincu, dans quel cas on voudra requêter ces objets ? Si c'est juste parce qu'un modèle te donne la génération automatique des formulaires ça ne me semble pas super utile.

C'est vraiment nul comme interface d'avoir à taper du JSON, se rappeler de la syntaxe bizarre, ne pas faire de faute de frappe.

C'est là que je ne comprends pas, c'est quoi le rapport entre taper du JSON et stocker en JSON ? Tu peux faire des formulaires qui produisent du JSON c'est pas un souci.

PS: des trucs simples comme ça (pseudocode, je copie vaguement du code de la doc des FormSet de Django)

class LookupByAttributes(Form):
    x = forms.CharField()
    y = forms.CharField()

def edit_lookup_by_attributes(request, pk):
    authenticator = get_object_or_404(SAMLAuthenticator, pk=pk)
    LookupByAttributesFormSet = forms.formset_factory(SAMLLookupByAttributes)
    formset = LookupByAttributesFormSet(data=request.GET)
    if formset.is_valid():
        authenticator.lookup_by_attributes = list(formset.cleaned_data)
        authenticator.save()        
       return redirect('authenticator-details', kwargs={'pk': pk})
    return render(request, 'edit.html', context={'formset': formset})

Benjamin Dauvergne a écrit :

C'est là que je ne comprends pas, c'est quoi le rapport entre taper du JSON et stocker en JSON ? Tu peux faire des formulaires qui produisent du JSON c'est pas un souci.

OK je n'avais pas compris que ta remarque portait sur avoir des objets et pas sur avoir des formulaires.

Stocker du JSON c'est quand même moins dans les clous que de stocker un objet.

PS: des trucs simples comme ça (pseudocode, je copie vaguement du code de la doc des FormSet de Django) [...]

Oui et tu prends LOOKUP_BY_ATTRIBUTES qui est simple, par contre A2_ATTRIBUTE_MAPPING c'est un bordel qui contient à la fois le mapping et aussi des actions pour renommer des attributs et ajouter des rôles. Je voudrais une séparation claire dans l'interface entre les 3, or garder du JSON ça voudrait dire bricoler un gros formulaire qui permette de configurer tout, on ne pourrait pas avoir 3 formulaires séparés notamment parce que l'ordre des actions dans le JSON importe.

Bref si l'argument c'est l'inutilité de passer du temps à migrer ces JSONField dans des objets, je suis convaincu que je vais passer moins de temps à le faire que ce que tu me proposes.

Valentin Deniaud a écrit :

PS: des trucs simples comme ça (pseudocode, je copie vaguement du code de la doc des FormSet de Django) [...]

Oui et tu prends LOOKUP_BY_ATTRIBUTES qui est simple, par contre A2_ATTRIBUTE_MAPPING c'est un bordel qui contient à la fois le mapping et aussi des actions pour renommer des attributs et ajouter des rôles. Je voudrais une séparation claire dans l'interface entre les 3, or garder du JSON ça voudrait dire bricoler un gros formulaire qui permette de configurer tout, on ne pourrait pas avoir 3 formulaires séparés notamment parce que l'ordre des actions dans le JSON importe.

La façon dont c'est stocké n'a aucune influence sur la façon dont c'est présenté dans l'IHM ni dans la façon dont c'est transmis à django-mellon, coté w.c.s. on a tout un workflow dans un seul objet pickle qu'on pourrait apparenter à un document JSON et ça n'empêche pas d'éditer ça comme on veut.

Si tu ne veux pas utiliser des FormSet mais des formulaires indépendants pour éditer et ajouter un nouvel "ATTRIBUTE_MAPPING" rien ne t'en empêche, si tu trouves plus simples d'avoir un identifiant par ligne pour l'édition tu peux l'ajouter. Tu peux vraiment écrire ça de la même manière que tu l'aurais fait avec des modèles.

Idem si tu veux séparer les actions en plusieurs groupes avec des formulaires différents tu peux aussi.

Ce que je voulais dire c'est que

A2_ATTRIBUTE_MAPPING = [{'action': 'rename', ...}, {'action': 'set-attribute', ...}]

Mon argument c'est vraiment qu'on gagnera à structurer les choses sans stockage JSON, le code sera plus explicite, plus lisible, et le monde plus beau.

Valentin Deniaud a écrit :

Ce que je voulais dire c'est que
[...]

Tu réécris ça en @A2_ATTRIBUTE_MAPPING = [{"uuid": "abcde...", }, {"uuid": "12345", ...}] et magie tu peux adresser chacun d'entre eux sans te soucier de l'ordre qui sera conservé via

for row in model.a2_attribute_mapping:
    if row['uuid'] == uuid:
        row.clear()
        row.update(new_row)
model.save()

C'est exactement ce qui est fait pour les champs des formulaires ou le statuts dans w.c.s. et personne n'y trouve d'inconvénient (modulo l'allocation des id qui a été longtemps foireuse, partir sur de l'uuid c'est être certain de n'avoir jamais de souci).

si je veux avoir un formulaire spécialisé pour ajouter/éditer une action renommage, ok je peux lire/écrire dans le JSONField mais il va falloir faire attention à l'ordre vu qu'un set-attribute dépend du rename, ça reviendra à mettre toujours les renames en début de liste

Je ne comprends pas pourquoi tu penses cela, le formulaire affiché dépend de la valeur de row['action'] c'est à peu près tout, pour réordonner tu postes la listes des UUIDs dans l'ordre, idem pour supprimer, y a vraiment aucun souci.

Mon argument c'est vraiment qu'on gagnera à structurer les choses sans stockage JSON, le code sera plus explicite, plus lisible, et le monde plus beau.

C'est déjà structuré, re-structurer autrement c'est un boulot inutile, faut juste présenter les choses.

Dans le code de migration :

                attribute = get_key(obj, 'attribute', 32)
                saml_attribute = get_key(obj, 'saml_attribute', 128)

• de pousser à 1024 partout. Même pas peur, il y aura peu de lignes. Ainsi, l'assurance à 99,9999% de n'avoir jamais de coupure. Si c'est jugé abusif, 512, allez.
• si possible d'émettre un warning/trace dans la migration get_key quand ça dépasse (mais franchement ça n'arrivera pas, donc si on sait pas faire de warning)

(Désolé, j'arrête ici, courte relecture)

• je considère que le mandatory sur AddRoleAction est inutile (et inutilisable). Si la condition plante, ne donnons pas le rôle. Retirer donc ce flag.
• Sur condition, j'ajouterais un hint, comme pense-bête, pour rappeler que si la condition n'est pas respectée alors le rôle est retiré.
• et en vérité je serais pour retirer complétement "condition", qui n'a jamais été utilisé... et ne le sera jamais.

Désolé, relecture hachée...

• Sur le SAMLAttributeLookup ça serait bien d'avoir un choices au niveau du user_field (ou une ForeignKey, mais je doute qu'on puisse).
• Juste par esthétique, mettre le modèle RenameAttributeAction au dessus de SAMLAttributeLookup pour se rappeler qu'on fait d'abord le rename avec le lookup.
• À ce sujet, pourquoi avoir nommé SAMLAttributeLookup et pas SAMLAttributeMapping ? (vraiment sans importance, juste de la curiosité perso).

Plus loin, le « wrap_mapping_errors » donne du code plus court mais... c'est moins facile à lire pour quelqu'un comme moi. Et j'ai l'impression qu'on va perdre dans cette bataille les erreurs explicites « missing saml_attribute key » et « missing attribute key » et c'est très dommage. Comme je propose plus haut de flinguer le mandatory sur AddRoleAction, je pense que tu pourrais revoir l'affaire sans ce contextmanager.

Sur le formulaire AddRoleAction, en cas de multicollectivité, il faudrait que la liste des rôles ajoute l'OU de chacun entre parenthèse (ou autre).

C'est très très mal de ne pas nommer les migrations :)

SAMLAuthenticatorEventsMixin gagnerait dans mon estime à être renommé SAMLAuthenticatorEventsBase ou SAMLAuthenticatorEvents

Voilà, c'est tout.

Ensuite, je pense qu'après avoir retiré la gestion de condition du AddRoleAction, on peut aussi supprimer l'action de renommage des attributs SAML, qui ne servira plus. Mais c'est plutôt pour un autre ticket "simplification"...

Automatic expiration