Trim username's spaces when user authenticates via CAS.#500
Conversation
|
Merci ! |
| def populateUser(tree): | ||
| username_element = tree.find( | ||
| './/{http://www.yale.edu/tp/cas}%s' % AUTH_CAS_USER_SEARCH) | ||
| username_element.text = username_element.text.strip() |
There was a problem hiding this comment.
@ZephOne peut-on modifier username plutôt que username_element ?
ex:
username = username_element.text.strip()There was a problem hiding this comment.
Si l'on fait ça, à la ligne 97 :
user, user_created = User.objects.get_or_create(username=username)il y a bien création de l'utilisateur avec les potentiels espaces rognés.
Par contre la variable tree est réutilisé plus tard par la bibliothèque django-cas-client. L'usage de la bibliothèque est d'également de récupérer le username de l'utilisateur via le xml tree. Ensuite la bibliothèque django-cas-client essaye de récupérer l'utilisateur correspondant s'il existe ou le crée s'il n'existe pas lors de l'authentification de l'utilisateur.
Ainsi dans le cas où le xml tree n'a pas été modifié, le nom d'utilisateur récupéré dans la fonction d'authentification aura gardé ses potentiels espaces ce qui provoquera la création d'un nouvel utilisateur dont le nom d'utilisateur comportera des espaces avant et/ou après le véritable nom d'utilisateur.
There was a problem hiding this comment.
Modifier seulement username au lieu de username_element est possible si cette pull request est acceptée dans django-cas-client.
Cependant, les mainteneurs n'ont plus l'air d'accepter des pull requests.
There was a problem hiding this comment.
@ZephOne mais du coups cette modification risque de créer un nouvel utilisateur egalement non?
à la ligne 93, on modifie l'attribut text de username_element pour ensuite stocker cette meme attribut dans username et à la ligne 97, on get ou create le user avec le username (qui est modifié, sans espace)
There was a problem hiding this comment.
ma proposition est de combiner la ligne 93 et 94 si cela n'a pas de déconvenue.
There was a problem hiding this comment.
Pour bien comprendre je me permet de paraphraser :
ma proposition est de combiner la ligne 93 et 94 si cela n'a pas de déconvenue
Donc le patch suivant :
--- a/pod/authentication/populatedCASbackend.py
+++ b/pod/authentication/populatedCASbackend.py
@@ -90,7 +90,7 @@ SUBTREE = 'SUBTREE'
def populateUser(tree):
username_element = tree.find(
'.//{http://www.yale.edu/tp/cas}%s' % AUTH_CAS_USER_SEARCH)
- username = username_element.text
+ username = username_element.text.strip()
if CAS_FORCE_LOWERCASE_USERNAME:
username = username.lower()
user, user_created = User.objects.get_or_create(username=username)C'est bien ça @meschac38700 ?
@ZephOne mais du coups cette modification risque de créer un nouvel utilisateur egalement non?
Je ne comprends pas pourquoi ? J'ai peut-être loupé quelque chose. Peux tu m'expliquer @meschac38700 ?
There was a problem hiding this comment.
Je comprends que ca puisse être compliquer alors je me rééxplique:
Ta Pull Request propose cette ligne en plus [L93]:
username_element.text = username_element.text.strip()et juste en dessous on a cec [L94]i:
username = username_element.textalors moi, je te propose qu'on raccourcir ces 2 lignes en ceci:
username = username_element.text.strip()There was a problem hiding this comment.
Ok donc j'avais bien compris ce que tu me disais.
Si on fait ça, le code :
user, user_created = User.objects.get_or_create(username=username)ne créera pas de nouvel utilisateur car ce que tu proposes auras permis de rogner les espaces potentiels préfixant et/ou suffixant le nom de l'utilisateur. C'est bien le fonctionnement attendu.
Toutefois, la variable tree qui est utilisé pour récupéré le nom de l'utilisateur (ligne 91) a été passée à cette fonction populateUser par django-cas-client. La bibliothèque django-cas-client via une variable de configuration propose de customiser la fonction utilisant les données de retour de l'authentification CAS : https://github.com/kstateome/django-cas/#cas-response-callbacks.
Dans le cas de Pod, la fonction custom est populateUser.
Une fois que la fonction populateUser est appelée par la bibliothèque django-cas-client en lui passant le tree xml en argument, la bibliothèque django-cas-client continue l'authentification de l'utilisateur, via la méthode authenticate de la classe CASBackend.
Je poursuis. Dans la méthode authenticate, l'appel (dans le cas de Pod) à la fonction populateUser se fait au travers de la fonction _verify qui pointe vers la fonction _verify_cas3 qui appelle directement la fonction _internal_verify_cas. C'est précisement dans cette fonction _internal_verify_cas qu[est appelée la fonction populateUser de Pod. Cette fonction _internal_verify_cas retourne le username de l'utilisateur. Pour trouver le username de l'utilisateur, elle utilise le tree xml récupéré lors de la réponse envoyé par le serveur CAS. Ce tree s'il n'a pas été modifié entre temps aura toujours un username avec des espaces en préfixe et/ou en suffixe. Dans ce cas la fonction authenticate récupère un username avec des espaces en préfixe et/ou en suffixe.
Après avoir récupéré le username la fonction authenticate va exécuter ce block try-except qui fait à peu près la même chose que la méthode User.objects.get_or_create. Et dans le cas où le tree xml n'a pas été changé et que c'est toujours un utilisateur avec un nom d'utilisateur prefixé et/ou suffixé d'espaces, un nouvel utilisateur est créé. Ce n'est pas ce que l'on veut.
Ce qu'il faut comprendre est qu'il y a deux opération de get_or_create d'utilisateur qui interviennent dans l'authentification CAS :
- Une dans la fonction
populateUserde Pod - Une dans la fonction
authenticatededjango-cas-client
Il faut s'assurer que pour ces deux opérations, leusernameest bien rogné de ces espaces.
La variable qui est utilisé lors de ces deux opérations pour récupérer leusernameesttree. C'est pour ça que je change la valeur de l'attributtextde l'élément XML qui contient leusernamede l'utilisateur.
Si l'on fait seulement ce que tu propose, on récupère bien unusernamerogné de ses espaces pour l'opération deget_or_createdanspopulateUser mais le tree XML reste inchangé. Lorsque ce dernier est à nouveau utilisé par la bibliothèquedjango-cas-client`, cela provoque la création d'un nouvel utilisateur.
Est ce que je suis clair dans mes explications @meschac38700 ?
|
I set this PR as draft in order to implement test so that test coverage does not decreased. |
|
Finally I unset the draft state as testing the modification would imply testing the |
| def populateUser(tree): | ||
| username_element = tree.find( | ||
| './/{http://www.yale.edu/tp/cas}%s' % AUTH_CAS_USER_SEARCH) | ||
| username_element.text = username_element.text.strip() |
There was a problem hiding this comment.
@ZephOne mais du coups cette modification risque de créer un nouvel utilisateur egalement non?
à la ligne 93, on modifie l'attribut text de username_element pour ensuite stocker cette meme attribut dans username et à la ligne 97, on get ou create le user avec le username (qui est modifié, sans espace)
| def populateUser(tree): | ||
| username_element = tree.find( | ||
| './/{http://www.yale.edu/tp/cas}%s' % AUTH_CAS_USER_SEARCH) | ||
| username_element.text = username_element.text.strip() |
There was a problem hiding this comment.
ma proposition est de combiner la ligne 93 et 94 si cela n'a pas de déconvenue.
| def populateUser(tree): | ||
| username_element = tree.find( | ||
| './/{http://www.yale.edu/tp/cas}%s' % AUTH_CAS_USER_SEARCH) | ||
| username_element.text = username_element.text.strip() |
There was a problem hiding this comment.
Je comprends que ca puisse être compliquer alors je me rééxplique:
Ta Pull Request propose cette ligne en plus [L93]:
username_element.text = username_element.text.strip()et juste en dessous on a cec [L94]i:
username = username_element.textalors moi, je te propose qu'on raccourcir ces 2 lignes en ceci:
username = username_element.text.strip()
|
J'ai changé la basepour passer en dev, il y a un conflit avec le fichier populatedCASbackend.py. @ZephOne est-ce possible de le résoudre que je puisse merger ? Merci |
Oui bien sûr. |
ZephOne
force-pushed
the
cas-sso-strip-spaces-around-username
branch
from
9d54599 to
9dc77af
Compare
|
Je vois que vous utilisez black pour le formattage du code. Quel est la version utilisée comme ça je peux le passer sur la nouvelle ligne ajoutée. Même si je suis quasiment sûr que ça ne va rien apporter. |
https://github.com/EsupPortail/Esup-Pod/blob/dev/.github/workflows/code_formatting.yml#L27 |
Ok la dernière, dans le doute c'est celle que j'ai utilisée. |
ceci dit, tu n'as pas à te préoccuper du formattage, celui-ci est automatisé lors du merge request de la branch en Dev |
3 participants
This pull request assumes that there is no case needing username with spaces when CAS authentication is used. So this behavior is not configurable. However if you feel it should be, I can make it configurable.