Produits Entr'ouvert » Authentic 2

(and a command to produce an appropriate data dump).

{
    "attributes": [
        {
            "asked_on_registration": true,
            "description": "",
            "kind": "string",
            "label": "Pr\u00e9nom",
            "name": "first_name",
            "required": true,
            "user_editable": true,
            "user_visible": true,
            "value": "CECILE" 
        },

looks like a merge of Attribute and AttributeValue while I'd simple expect a dump of AttributeValue:

{
    "attributes": {
        "first_name": "CECILE",
        ...

(I'm not even sure it needs to be in a "attributes" key)

Also, users export has:

    "ou": {
        "name": "Fabr\u00e8gues",
        "uuid": "3bcd7488577748e7a1e63887a3972e67" 
    },

while roles export has:

    "ou__name": "Fabr\u00e8gues", 
    "ou__slug": "fabregues", 
    "ou__uuid": "3bcd7488577748e7a1e63887a3972e67", 

This is the same data structure, why two different ways to represent it?

And of course, the attached dumps cover one user and and one role while the command is expected to dump all of them.

Frédéric Péters a écrit :

[...]

looks like a merge of Attribute and AttributeValue while I'd simple expect a dump of AttributeValue:

Yes indeed. My bad, i forgot that Attribute's type would already be set up. Thus we only need to set their values.

[...]

(I'm not even sure it needs to be in a "attributes" key).

Also, users export has:

[...]

while roles export has:

[...]

This is the same data structure, why two different ways to represent it?

+1

And of course, the attached dumps cover one user and and one role while the command is expected to dump all of them.

I did it on purpose. I didn't want to upload the whole files.

I wrote:

    "attributes": {
        "first_name": "CECILE",

but this doesn't work with additional attribute details such as the current "verified" or future "locked"; it would be better like this:

    "attributes": {
        "first_name": {"value": "CECILE"},

Tests are missing; and please indent the files in test/data/cmd/import/, it will make it easier later to see changes.

If there's a singe organizational unit, I would totally skip the "ou" part: this is not something that needs to be advertised. Thus things like "# get user's ou or crash" shouldn't happen, it should use the default ou instead. Also related to organizational units, an "import-users" management command could take a --ou parameter to give the specified to ou to imported users.

Missing import-users command?

No references to password?

Role = get_role_model() and friends, they depend on settings so they cannot be done on module load time, it needs to happen within the relevant functions.

• corriger get_or_none() pour utiliser ou__isnull=True si "ou is None"
• corriger

          # get role's ou or crash

• tu dois exporter le service associé à un rôle (via son name et son slug)
• lors de l'import des attributs de rôle, prévoir le cas created is False et dans ce cas mettre à jour l'attribut
• corriger

          # get user's ou or crash

• pour les attributs des rôles j'exporterai de cette manière, je n'aime pas trop les dictionnaires avec clé variables:
  

  'attributes': [
    {
       'name': ...,
       'value': ...,
    },
  ],
  

• ajouter un export de UserExternalId pour l'import/export d'utilisateurs reliés à un LDAP
• ne pas exporter les parents des rôles dont l'utilisateur est membre, uniquement les rôles dont il est membre direct (user.roles.all()).
• importer les rôles techniques dont l'utilisateur est membre
• comme pour l'import des rôles pour retrouver le rôle d'un utilisateur il faut sont service

Dans l'import des rôles (pas regardé le reste),

if all(role.get('service', {}).values()):
    if all(role['service'].get('ou', {}).values()):
        service = ...
else:
    service = None

Il y a une situation là où service ne va pas être défini.

            # update attribute's value if not created
            if not created:
                attribute.value = attr['value']

Manque un .save(), non ?

• manque licence/copyright en haut des fichiers
• else: pass : quel sens à cette structure inutile ?
• except(ModelDoesNotExist,): mauvais style
• etc.

--include-a2, sérieusement, ça semble une bonne idée de nom d'option ?

Je continue le etc.

• En l'absence de fichier de destination, sortir sur la sortie standard, plutôt qu'aller écraser un fichier roles.json.

• Indenter les fichiers produits, parce qu'on le vaut bien.

J'ai appliqué le patch, j'ai fait un export d'un de mes sites locaux, j'ai fait l'import dans un autre qui n'avait aucun rôle, ça a crashé.

  File "/home/fred/src/eo/authentic/src/authentic2/import_export_utils.py", line 173, in import_roles
    obj = get_object_by_main_fields_or_create('role', uuid=uuid, slug=slug, name=name, ou=ou, service=service)
  File "/home/fred/src/eo/authentic/src/authentic2/import_export_utils.py", line 45, in get_object_by_main_fields_or_create
    Model = ContentType.objects.get(model=model_name).model_class()
  File "/home/fred/src/eo/venv/local/lib/python2.7/site-packages/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/fred/src/eo/venv/local/lib/python2.7/site-packages/django/db/models/query.py", line 338, in get
    (self.model._meta.object_name, num)
django.contrib.contenttypes.models.MultipleObjectsReturned: get() returned more than one ContentType -- it returned 2!

• c'est parce que avec le nom "role" il y a aussi bien a2_rbac.role que common.role (de hobo.agent.common).

• faire quelque chose du flag verbose, genre afficher les infos du rôle importé.

Bon le patch en l'état est pas du tout acceptable, c'est beaucoup trop gros et le code est illisible. Il faudrait que le code ressemble un peu plus au code dans l'import Passerelle:

• un peu plus DRY et de separation of concerns: chaque modèle doit savoir comme s'importe/s'exporter, avec des méthodes du genre export_json/import_json, prenant en argument le bout de JSON concerné et un objet context précisant le paramétrage de l'import/export (dans passerelle j'ai un argument par option possible mais avec le recul autant passer un objet context, ça évite d'ajouter des paramètres partout).
• il faut mieux exposer les choix de politique d'import/export possible avec l'objet Context:
  • doit-on arrêter complètement un import si une donnée est manquante ? doit-on créer la donnée ?, ex: si un utilisateur précise un rôle et celui-ci n'est pas présent que faire ? arrêter l'import ? créer le rôle à partir des données succinctes qui sont données ? idem pour une ou.
  • doit-on écraser tout (supprimer tous les rôles, tous les utilisateurs, toutes les ou et repartir de zéro) ? écraser que ce qui est donné dans l'export ? uniquement ajouter des membres aux rôles sans écraser ? ne pas toucher à ce qui existe déjà...

Il faut vraiment partir d'une liste restreintes de cas d'usage, les mettre en commentaire dans le code et expliquer en quoi le code aide à leur implémentation.

L'export/import doit-être un peu plus "model based", i.e. on exporte implicitement la plupart des champs basés sur leur type (voir comment c'est fait dans passerelle), si on ajoute un champ booléen à un modèle on a pas envie de venir modifier le code d'import/export à chaque fois. L'attribut Role.external_id n'est ici juste pas pris en compte par exemple.

Des méthodes comme get_ou_or_default, ça encode directement une politique qui serait que si on ne trouve pas une OU alors on en affecte de facto une autre. Je préfère que par défaut on abandonne dans ce cas avec un message précisant le problème. Par contre on pourra avoir dans le contexte une OU soit à forcer soit par défaut (si forcé, alors on l'affecte à tous les utilisateurs et rôles, si par défaut uniquement si on ne trouve pas).

On ne devrait jamais utiliser ContentType dans ce code, c'est inutile, tout doit passer par les classes elle mêmes (on exporte 5 classes, OU, User, Role, RoleParenting, AttributeValue et RoleAttribute).

Pour moi l'affectation utilisateur-rôles pourrait être un objet "à part", sinon on ne trouvera jamais le bon endroit pour le mettre (dans les utilisateurs ? dans les rôles? et si on veut juste exporter les affectations sans créer de nouveaux rôles ou utilisateurs).

La recherche par identifiant LDAP des utilisateurs n'y est pas (par UserExternalId).

• export complet des utilisateurs, ou, rôles non techniques, graphe des rôles, affectation des utilisateurs aux rôles avec écrasement complet (sauf rôles techniques), si un rôle technique est manquant dans le référencement on abandonne (on doit avoir les mêmes noms de rôles technique en prod et recette), c'est le cas prod -> recette ou recette -> première prod
• export/import d'un ou plusieurs rôles avec liens de parentalité, les affectations ou pas aux utilisateurs, arrêt si les liens de parentalité ne peuvent être reproduits ou ou manquant, et bloquage ou pas si un utilisateur n'est pas trouvé

Pour ne pas faire exploser le nombre d'options dans les commandes d'import/export je dirai qu'il faudrait un paramètre --option qui s'utiliserait comme ceci:

--option no-create-user
--option stop-on-absent-ou
--option stop-on-absent-parent

Ça poserait un flag booléen du même nom sur l'objet ImportContext/ExportContext (à False si l'option commence par no- sinon à True).

Si un attribut n'existe pas on ne doit pas bloquer l'import. Il faut une exception ImportError pour faire remonter ces erreurs proprement aux codes appelant (en essayant de conserver un hint sur l'emplacement de l'erreur dans l'export JSON + un message d'explication). Pour des warning on peut avoir un objet ImportWarning qu'on accrocherait sur l'objet Context au fur à mesure de l'import (genre context.add_warning(Warning(json_path, "user was found using LDAP identifier with a different UUID")), etc..

Désolé c'est un peu long mais je pense que ce ticket le mérite. Tout ça n'a pas à faire partie du patch V1, mais il faut que le code soit plus simple pour permettre chacune des évolutions sans demander une refonte totale de la structure.

Je viens de créer #16514 pour laisser ce ticket sur la partie "utilisateurs "et avoir la partie "rôles" indépendante, qui serait déjà bien utile et il me semble la partie la plus simple.

Cas d'usage : exporter/importer uniquement les rôles, pas les utilisateurs.

D'un chat avec Pierre : "e peux t'en filer un prochain à Lille ouais :
en recette on a créé des users et des rôles
en prod on sera raccordé au LDAP mais on veut récupérer les rôles de la recette"