Produits Entr'ouvert » Authentic 2

Il ne sert à rien le paramètre mandatory, c'est toujours le cas (sinon c'est un autre type d'erreur), il faut juste ajouter le nom lors du raise, et ce n'est pas traduit parce que ces erreurs n'arrivent pas normalement (un annuaire avec des gens sans email et l'idp qui ne renvoie rien, on y peut rien), uniquement durant la configuration mais on peut effectivement améliorer les messages si les gens ne comprennent pas (ça parait évident que les clients ne lisent pas les screenshots, même si on améliore les messages d'erreur, ils nous renverront toujours la balle avant de faire quoi que ce soit). Dans ce cas je dirai d'adapter le code ainsi :

class MappingError(Exception):
    def __init__(self, msg, *args):
        assert (msg % args)
        self.msg = msg
        super().__init(*args)

    def message(self):
        return ugettext(self.msg) % self.args

    def __str__(self):
        return self.msg % self.args
...
except MappingError as e:
    logging.warning('blaba: %s', e)
    messages.warning(request, e.message())
...
raise MappingError(N_('message %s'), saml_attribute/whatever)

Ainsi on pourra tout traduire facilement.

Benjamin Dauvergne a écrit :

Il ne sert à rien le paramètre mandatory, c'est toujours le cas (sinon c'est un autre type d'erreur).

• paramétrage:
  

  ...
  raise MappingError('invalid A2_ATTRIBUTE_MAPPING')
  ...
  raise MappingError('invalid mapping action', details={'mapping': mapping})
  

• fonctionnel:
  

  ...
              if len(value) == 0:
                  raise MappingError('no value')
              if len(value) > 1:
                  raise MappingError('too many values', details={'value': value})
  ...
  

et pour un paramètre obligatoire manquant:

...
            except MappingError as e:
                e.details['mapping'] = mapping
                if mandatory:
                    # it's mandatory, provisionning should fail completely
                    raise e
...

Je pensais rajouter une nouvelle exception uniquement pour les paramètres obligatoires manquants mais finalement opté pour mon patch.

Je vais voir pour suivre ta proposition.

Serghei Mihai a écrit :

Benjamin Dauvergne a écrit :

Il ne sert à rien le paramètre mandatory, c'est toujours le cas (sinon c'est un autre type d'erreur).

J'ai m'impression que non car MappingError est générique pour tout type d'erreur

Ce que je voulais dire, c'est que si un message est affiché, c'est que la règle était "mandatory", le code fait déjà la distinction sans avoir besoin de mettre un attribut mandatory sur MappingError, il manque juste du code pour traduire ces messages, je ne vois pas autre chose à changer.

On peut bien sûr garder la partie "details" pour les logs techniques, mais que tout ça vient de ticket où il m'était demandé d'aider le debug à la mise en place pour nous pas pour les clients, le cas nominal étant que ça marche tout le temps une fois mis en place et si ça ne marche pas il est probable qu'on nous ouvrira toujours un ticket quelque soit le temps passé à écrire des messages les plus clairs possibles. L'espoir de Fred d'améliorer le message pour que le client n'ouvre pas un ticket est à mon avis peine perdue, mais c'est toujours plus sympa d'avoir un message en français, sûr.

Serghei Mihai a écrit :

Benjamin Dauvergne a écrit :

Il ne sert à rien le paramètre mandatory, c'est toujours le cas (sinon c'est un autre type d'erreur).

J'ai m'impression que non car MappingError est générique pour tout type d'erreur:

• paramétrage:
  [...]

• fonctionnel:
  [...]

Il n'y a rien de fonctionnel, tout est technique ici, un IdP doit servir les attributs attendus par le SP, sinon il est mal configuré, dans les métadonnées de Renater par exemple on voit bien la liste des attributs obligatoires et ceux qui ne le sont pas, je ne sais pas si LemonLDAP peut-être configuré de la même manière mais l'IdP est sensé bloquer les choses avant.

Mais on peut changer le message pour dire que « la connexion est impossible: votre fournisseur d'identité est mal configuré car vous ne disposez pas d'un attribut "email" » et ne plus parler de "saml_attribute" et pour les autres messages dire « la configuration locale du fournisseur d'identité est erronée à cause de "xyz" ». Tout ces problèmes sont techniques mais des fois le problème est local et des fois il est distant.

Automatic expiration