Project

General

Profile

Development #43193

absence de décodage XML des attributs

Added by Frédéric Péters 9 days ago. Updated 3 days ago.

Status:
Solution validée
Priority:
Normal
Target version:
-
Start date:
21 May 2020
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

Si mon prénom est <i>Frédéric</i>, ça se trouve bien dans l'assertion avec l'encodage nécessaire :

<saml:Attribute Name="first_name" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic" FriendlyName=""><saml:AttributeValue>&lt;i&gt;Frédéric&lt;/i&gt;</saml:AttributeValue></saml:Attribute>

mais cette forme encodée persiste à la lecture de l'assertion, dans le saml_attributes passé à provision* etc., exemple dans les logs :

trying to authenticate with attributes {'username': ['fred'], 'first_name': ['&lt;i&gt;Frédéric&lt;/i&gt;'], ...

et c'est cette forme qui se trouve mise en db, et mon nom apparait alors comme <i>Frédéric</i>.

0001-misc-don-t-treat-basic-attributes-as-XML-43193.patch View (1.18 KB) Frédéric Péters, 21 May 2020 07:21 PM

0001-views-ignore-XML-content-in-SAML-attributes-43193.patch View (4.11 KB) Benjamin Dauvergne, 21 May 2020 09:03 PM

0001-views-ignore-XML-content-in-SAML-attributes-43193.patch View (4.08 KB) Benjamin Dauvergne, 21 May 2020 09:05 PM

History

#1 Updated by Frédéric Péters 9 days ago

Ma proposition basique/conservatrice serait de juste prendre le texte tel quel en situation de lasso.SAML2_ATTRIBUTE_NAME_FORMAT_BASIC.

#2 Updated by Benjamin Dauvergne 9 days ago

Le NameFormat n'a pas vraiment de rapport avec le contenu; je vais proposer autre chose.

#3 Updated by Benjamin Dauvergne 9 days ago

  • Assignee set to Benjamin Dauvergne

#4 Updated by Benjamin Dauvergne 9 days ago

  • Assignee deleted (Benjamin Dauvergne)

#5 Updated by Benjamin Dauvergne 9 days ago

  • Assignee set to Benjamin Dauvergne

#6 Updated by Benjamin Dauvergne 9 days ago

#7 Updated by Benjamin Dauvergne 9 days ago

J'ai changé pour logger tout l'attribut en cas d'erreur c'est plus simple.

#8 Updated by Paul Marillonnet 5 days ago

  • Status changed from Solution proposée to Solution validée

Du détail, mais j'aurais bien vu une modif du genre

diff --git a/mellon/views.py b/mellon/views.py
index 899989f..222edc2 100644
--- a/mellon/views.py
+++ b/mellon/views.py
@@ -212,7 +212,7 @@ class LoginView(ProfileMixin, LogMixin, View):
     def get_attribute_value(self, attribute, attribute_value):
         # check attribute_value contains only text
         for node in attribute_value.any:
-            if not isinstance(node, lasso.MiscTextNode) or not node.textChild:
+            if not isinstance(node, lasso.MiscTextNode) or node.content is None:
                 self.log.warning('unsupported attribute %s', attribute.exportToXml())
                 return None
         return ''.join(lasso_decode(node.content) for node in attribute_value.any)

parce qu'après une lecture rapide j'ai l'impression que c'est équivalent, mais aussi parce que ce patch y gagne en lisibilité.

Sinon c'est ok pour moi.

#9 Updated by Benjamin Dauvergne 4 days ago

Paul Marillonnet a écrit :

Du détail, mais j'aurais bien vu une modif du genre
[...]
parce qu'après une lecture rapide j'ai l'impression que c'est équivalent, mais aussi parce que ce patch y gagne en lisibilité.

Je ne crois pas que node.content puisse être à None, il me semble que libxml2 contrairement à ElementTree ne retourne jamais NULL comme contenu texte d'un noeud même s'il est vide (mais je me trompe peut-être, je n'ai pas tenté l'expérience, c'est vague un souvenir sur libxml2 qui est assez cohérent en général).

Si t'as <saml:AttributeValue><trucmuche></trucmuche></saml:AttributeValue> ça donne :

atv.any = [mtn]
mtn.textChild = False
mtn.content = ''
mtn.name = 'trucmuche'

<saml:AttributeValue></saml:AttributeValue> ça donne :
atv.any = [mtn]
mtn.textChild = True
mtn.content = ''
mtn.name = None

le but étant de supporter un contenu mixte (texte/noeuds) qui n'arrive jamais.

#10 Updated by Paul Marillonnet 3 days ago

Benjamin Dauvergne a écrit :

Je ne crois pas que node.content puisse être à None, il me semble que libxml2 contrairement à ElementTree ne retourne jamais NULL comme contenu texte d'un noeud même s'il est vide (mais je me trompe peut-être, je n'ai pas tenté l'expérience, c'est vague un souvenir sur libxml2 qui est assez cohérent en général).

J'ai trituré un peu le patch parce qu'il me semblait d'abord, à tort, qu'avoir le “if not isinstance(node, lasso.MiscTextNode):” était suffisant. Mais dans notre cas au moins, les node qui vérifient “not node.textChild” ont l'attribut content valant None, ce qui casserait le join quelques lignes plus bas si on n'ajoute ni “or not node.textChild” ni “or node.content is None” au if.

#11 Updated by Benjamin Dauvergne 3 days ago

Je ne comprends toujours pas, tu peux me donner un exemple de code et contenu XML qui donnerait .content is None quand .textChild is True ?

#12 Updated by Paul Marillonnet 3 days ago

Benjamin Dauvergne a écrit :

Je ne comprends toujours pas, tu peux me donner un exemple de code et contenu XML qui donnerait .content is None quand .textChild is True ?

Non justement mon impression à la relecture c'était qu'il y a équivalence entre .content is None et not .textChild. Je vais vérifier.

#13 Updated by Benjamin Dauvergne 3 days ago

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Je ne comprends toujours pas, tu peux me donner un exemple de code et contenu XML qui donnerait .content is None quand .textChild is True ?

Non justement mon impression à la relecture c'était qu'il y a équivalence entre .content is None et not .textChild. Je vais vérifier.

Ben non, je viens de le montrer plus haut.

In [5]: mtn = lasso.MiscTextNode.newWithString('coucou')                                                                                                                     

In [6]: mtn.name = 'coin'                                                                                                                                                    

In [7]: mtn.exportToXml()                                                                                                                                                    
Out[7]: '<coin>coucou</coin>'

In [8]: mtn.textChild                                                                                                                                                        
Out[8]: False

In [9]: mtn.content                                                                                                                                                          
Out[9]: 'coucou'

Après je suis d'accord que la classe MiscTextNode est super mal nommée, mais c'est historique ©.

#14 Updated by Paul Marillonnet 3 days ago

Benjamin Dauvergne a écrit :

Ben non, je viens de le montrer plus haut.
[...]

Après je suis d'accord que la classe MiscTextNode est super mal nommée, mais c'est historique ©.

Ah pardon, je n'ai pas vu que c'était ce que tu montrais plus haut. Il faudrait alors s'assurer qu'on n'a pas de cas possible où un node peut vérifier “isinstance(node, lasso.MiscTextNode) and node.textChild and node.content is None", auquel cas le ''.join(…) casserait.

Tu disais “il me semble que libxml2 contrairement à ElementTree ne retourne jamais NULL comme contenu texte d'un noeud même s'il est vide […]” et pourtant en appliquant :

--- a/mellon/views.py
+++ b/mellon/views.py
@@ -212,7 +212,7 @@ class LoginView(ProfileMixin, LogMixin, View):
     def get_attribute_value(self, attribute, attribute_value):
         # check attribute_value contains only text
         for node in attribute_value.any:
-            if not isinstance(node, lasso.MiscTextNode) or not node.textChild:
+            if not isinstance(node, lasso.MiscTextNode):
                 self.log.warning('unsupported attribute %s', attribute.exportToXml())
                 return None
         return ''.join(lasso_decode(node.content) for node in attribute_value.any)>

les tests cassent pour cette raison :
[…]
mellon/views.py:416: in get
    return self.continue_sso_artifact(request, lasso.HTTP_METHOD_ARTIFACT_GET)
mellon/views.py:394: in continue_sso_artifact
    return self.sso_success(request, login)
mellon/views.py:227: in sso_success
    content = self.get_attribute_value(at, attribute_value)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <mellon.views.LoginView object at 0x7f1ed8feca90>, attribute = <lasso.Saml2Attribute object at 0x7f1ed910e7c0>, attribute_value = <lasso.Saml2AttributeValue object at 0x7f1ed910ebb0>

    def get_attribute_value(self, attribute, attribute_value):
        # check attribute_value contains only text
        for node in attribute_value.any:
            if not isinstance(node, lasso.MiscTextNode):
                self.log.warning('unsupported attribute %s', attribute.exportToXml())
                return None
>       return ''.join(lasso_decode(node.content) for node in attribute_value.any)
E       TypeError: sequence item 1: expected str instance, NoneType found

Also available in: Atom PDF