Development #27540
idp_oidc: déterminer si claims dont la valeur est absente doivent être absent ou à null
0%
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.
Fichiers
Révisions associées
idp_oidc: export claim even if source attribute is absent (fixes #27540)
Historique
Mis à jour par Benjamin Dauvergne il y a plus de 5 ans
- Fichier 0001-idp_oidc-use-force_bytes-text-and-six.text_type-inst.patch 0001-idp_oidc-use-force_bytes-text-and-six.text_type-inst.patch ajouté
- Fichier 0002-idp_oidc-export-claim-even-if-source-attribute-is-ab.patch 0002-idp_oidc-export-claim-even-if-source-attribute-is-ab.patch ajouté
- Statut changé de Nouveau à Solution proposée
- Patch proposed changé de Non à Oui
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).
Mis à jour par Benjamin Dauvergne il y a environ 5 ans
- Fichier 0001-idp_oidc-use-force_bytes-text-and-six.text_type-inst.patch 0001-idp_oidc-use-force_bytes-text-and-six.text_type-inst.patch ajouté
- Fichier 0002-idp_oidc-export-claim-even-if-source-attribute-is-ab.patch 0002-idp_oidc-export-claim-even-if-source-attribute-is-ab.patch ajouté
Rebasé.
Mis à jour par Paul Marillonnet il y a environ 5 ans
- Projet changé de Authentic 2 à Site Publik
- Statut changé de Solution proposée à 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 ?
Mis à jour par Paul Marillonnet il y a environ 5 ans
- Projet changé de Site Publik à Authentic 2
(oups, mauvaise manip' de ma part, je replace le ticket dans le projet a2)
Mis à jour par Benjamin Dauvergne il y a environ 5 ans
- Statut changé de Information nécessaire à 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.
Mis à jour par Paul Marillonnet il y a environ 5 ans
- Statut changé de Solution proposée à Solution validée
Oui, complètement, mauvaise lecture de ma part.
Ack.
Mis à jour par Benjamin Dauvergne il y a environ 5 ans
- Statut changé de Solution validée à 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.Mis à jour par Benjamin Dauvergne il y a environ 5 ans
- % réalisé changé de 0 à 100
Appliqué par commit authentic2|6964b0cc8283381528f3c92a58db12e041d5b3c4.
Mis à jour par Benjamin Dauvergne il y a environ 5 ans
- Statut changé de Résolu (à déployer) à Solution déployée
- % réalisé changé de 100 à 0
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.