Project

General

Profile

Development #27540

idp_oidc: déterminer si claims dont la valeur est absente doivent être absent ou à null

Added by Benjamin Dauvergne 8 months ago. Updated 3 months ago.

Status:
Solution déployée
Priority:
Bas
Category:
-
Target version:
-
Start date:
24 Oct 2018
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

C'est une chose dont je me suis aperçu en ajoutant le test OIDC pour le nouveau type d'attribut image (#26022), si un claim pointe sur un attribut absent du dictionnaire produit par attributes_ng alors je ne produit pas le claim:

# src/authentic2_idp_oidc/utils.py

def create_user_info(client, user, scope_set, id_token=False):
    '''Create user info dictionnary'''
    user_info = {
        'sub': make_sub(client, user)
    }
    attributes = get_attributes({
        'user': user, 'request': None, 'service': client,
        '__wanted_attributes': client.get_wanted_attributes()})
    for claim in client.oidcclaim_set.filter(name__isnull=False):
        if not set(claim.get_scopes()).intersection(scope_set):
            continue
        if not claim.value in attributes: # <-- ICI
            continue
        user_info[claim.name] = normalize_claim_values(attributes[claim.value])
        # check if attribute is verified
        if claim.value + ':verified' in attributes:
            user_info[claim.value + '_verified'] = True
    hooks.call_hooks('idp_oidc_modify_user_info', client, user, scope_set, user_info)
    return user_info

Je me demande si ce ne serait pas plus agréable pour les consommateurs que le claim soit toujours présent (déjà ça rend mon test plus propre, au lieu d'un .get() je peux faire un []) mais avec une valeur None/null.

0001-idp_oidc-use-force_bytes-text-and-six.text_type-inst.patch View (2.79 KB) Benjamin Dauvergne, 22 Nov 2018 12:48 PM

0002-idp_oidc-export-claim-even-if-source-attribute-is-ab.patch View (2.31 KB) Benjamin Dauvergne, 22 Nov 2018 12:48 PM

0001-idp_oidc-use-force_bytes-text-and-six.text_type-inst.patch View (2.79 KB) Benjamin Dauvergne, 18 Mar 2019 11:01 AM

0002-idp_oidc-export-claim-even-if-source-attribute-is-ab.patch View (2.31 KB) Benjamin Dauvergne, 18 Mar 2019 11:01 AM

Associated revisions

Revision ce1b7964 (diff)
Added by Benjamin Dauvergne 3 months ago

idp_oidc: use force_bytes/text and six.text_type instead of smart_bytes and unicode (#27540)

Just some cleaning, not really related to #27540.

Revision 6964b0cc (diff)
Added by Benjamin Dauvergne 3 months ago

idp_oidc: export claim even if source attribute is absent (fixes #27540)

History

#1 Updated by Benjamin Dauvergne 8 months ago

  • Tracker changed from Support to Development

#2 Updated by Benjamin Dauvergne 8 months ago

  • Priority changed from Normal to Bas

#3 Updated by Benjamin Dauvergne 8 months ago

  • Assignee deleted (Benjamin Dauvergne)

#4 Updated by Benjamin Dauvergne 7 months ago

  • Assignee set to Benjamin Dauvergne

#5 Updated by Benjamin Dauvergne 7 months ago

Tous les claims déclarés sont désormais à null aucune valeur n'est trouvée via les attributs.

Si plusieurs mapping génère des valeurs on garde la dernière différente de None.

J'ai du modifier un test provenant de #23643 qui testait systématiquement
l'absence de preferred_username car ce n'était pas le but de ce test (le but
étant que cela fonctionne pour un utilisateur sans username, ce qui est
toujours le cas).

#7 Updated by Paul Marillonnet 3 months ago

  • Project changed from Authentic 2 to Site Publik
  • Status changed from Solution proposée to Information nécessaire

Hmm, on s'éloigne des recommandations dans les spécs :/

Cf https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse

If a Claim is not returned, that Claim Name SHOULD be omitted from the JSON object representing the Claims; it SHOULD NOT be present with a null or empty string value.

Et c'est vrai qu'on aurait plus de moyen de discerner un attribut présent mais valant null, et l'absence de cet attribut.
Si c'est simplement pour pouvoir faire un dict[key] à la place de dict.get(key) dans les tests je ne pense pas que ça vaille la peine d'enfreindre cette recommandation.

Est-ce que je passe à côté de quelque chose d'important sur ce patch ?

#8 Updated by Paul Marillonnet 3 months ago

  • Project changed from Site Publik to Authentic 2

(oups, mauvaise manip' de ma part, je replace le ticket dans le projet a2)

#9 Updated by Benjamin Dauvergne 3 months ago

Tu as raison, je modifie.

#10 Updated by Benjamin Dauvergne 3 months ago

  • Status changed from Information nécessaire to Solution proposée

Benjamin Dauvergne a écrit :

Tu as raison, je modifie.

Je n'ai pas la même lecture finalement, cette phrase fait suite à celle-ci :

For privacy reasons, OpenID Providers MAY elect to not return values for some requested Claims.

Et donc "not returned" ça ne veut pas dire absence du claim, ça veut dire refus de l'IdP de renvoyer le claim et ça c'est pris en compte; en fait on ne refuse jamais actuellement, on va refuser l'autorisation complète, donc le cas n'arrivant la phrase que tu cites ne s'applique pas pour moi. Ainsi le RP peut faire la différence entre un refus et une absence.

#11 Updated by Paul Marillonnet 3 months ago

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

Oui, complètement, mauvaise lecture de ma part.
Ack.

#12 Updated by Benjamin Dauvergne 3 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 6964b0cc8283381528f3c92a58db12e041d5b3c4
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Thu Nov 22 12:37:29 2018 +0100

    idp_oidc: export claim even if source attribute is absent (fixes #27540)

commit ce1b796473cbdabcf025e5be9266e999ef17f5b1
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Thu Nov 22 12:34:11 2018 +0100

    idp_oidc: use force_bytes/text and six.text_type instead of smart_bytes and unicode (#27540)

    Just some cleaning, not really related to #27540.

#13 Updated by Benjamin Dauvergne 3 months ago

  • % Done changed from 0 to 100

#14 Updated by Benjamin Dauvergne 3 months ago

  • % Done changed from 100 to 0
  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF