django-saml2-auth icon indicating copy to clipboard operation
django-saml2-auth copied to clipboard

Username SAML attribute not passing to User object

Open heydonovan opened this issue 2 years ago • 6 comments

We have forked this library with a few tweaks, wanted to run this by you on adding these to the main branch:

user.py

-def create_new_user(email: str, firstname: str, lastname: str) -> Type[Model]:
+def create_new_user(username: str, email: str, firstname: str, lastname: str) -> Type[Model]:

-        user = user_model.objects.create_user(email, first_name=firstname, last_name=lastname)
+        user = user_model.objects.create_user(username=username, email=email, first_name=firstname, last_name=lastname)

-            target_user = create_new_user(get_user_id(user), user["first_name"], user["last_name"])
+            target_user = create_new_user(user["username"], user["email"], user["first_name"], user["last_name"])

We were originally passing all 4 attributes, and the user was being created, but with a blank email field. Now we are only passing three (email, first, last) and inferring the username below:

saml.py

+    # Infer username from email
+    user["username"] = user["email"].split("@")[0]

To add the username attribute is a non-trivial task with our idP provider. Only one line in Python. Could make a INFER_USERNAME_FROM_EMAIL flag for use in the SAML2_AUTH map.

heydonovan avatar Apr 21 '22 05:04 heydonovan

Hey @heydonovan,

Thanks for the suggestion. I originally tried to deduct the username/email field from the get_user_model().USERNAME_FIELD. Using USERNAME_FIELD ensures that either the email field email or the username land in the correct field on the model, yet you are trying to handle both, am I correct?

I am trying to understand what you're trying to solve, as you can always opt for optional arguments to the create_user. The signature of create_user is:

def create_user(self, username, email=None, password=None, **extra_fields):

And the username parameter will work based on the USERNAME_FIELD property of the model, so if you pass either of them, it'll work just fine. I suppose you want to pass both?

mostafa avatar Apr 21 '22 10:04 mostafa

You are correct! The original issue was our idP was sending 3 attributes (email, first_name, and last_name). This threw an error on the django side:

"msg": "Attribute Statement: <ns0:AttributeStatement xmlns:ns0=\"urn:oasis:names:tc:SAML:2.0:assertion\" xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"><ns0:Attribute Name=\"email\" NameFormat=\"urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified\"><ns0:AttributeValue xsi:type=\"xs:anyType\" xmlns:xs=\"http://www.w3.org/2001/XMLSchema\">[email protected]</ns0:AttributeValue></ns0:Attribute><ns0:Attribute Name=\"first_name\" NameFormat=\"urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified\"><ns0:AttributeValue xsi:type=\"xs:anyType\" xmlns:xs=\"http://www.w3.org/2001/XMLSchema\">Donovan</ns0:AttributeValue></ns0:Attribute><ns0:Attribute Name=\"last_name\" NameFormat=\"urn:oasis:names:tc:SAML:2.0:attrname-format:unspecified\"><ns0:AttributeValue xsi:type=\"xs:anyType\" xmlns:xs=\"http://www.w3.org/2001/XMLSchema\">Hernandez</ns0:AttributeValue></ns0:Attribute></ns0:AttributeStatement>"
 "msg": "'NoneType' object has no attribute 'lower'"
 "msg": "Internal Server Error: /sso/acs/"

If I send in 4 attributes, and update the SAML2_AUTH map to match, it creates the user. However, the email address field is left blank. It might be because username and email are the same value. That's why I opted to push the logic on the python side, since changing that username attribute to even test if the same result occurs is a non-trivial task.

heydonovan avatar Apr 21 '22 16:04 heydonovan

@heydonovan I also map username to email in the IdP, and it is pretty straightforward. But if you think having this would improve the overall UX of this library, then let's be it! Please create PR so that we can discuss it there.

mostafa avatar Apr 21 '22 16:04 mostafa

@mostafa Let's say the only SAML attribute I'm sending is email set to [email protected] and "CREATE_USER": False is set. I've created a django superuser with username set to username and email set to [email protected]. Shouldn't that be enough to validate the user? That was the main reason I started digging into this.

I've also tried setting the email/username SAML attribute to both be [email protected] but that doesn't work either (probably because the username isn't actually set to the email on the django side). All our pre-existing users have a username that is different than email, so we would prefer to look up a user only by email.

Does it only work with sending a username?

heydonovan avatar Apr 21 '22 18:04 heydonovan

@heydonovan, I think that's reasonable. Would you create a PR and add a few tests so that we don't break the old functionality by adding this?

mostafa avatar Apr 28 '22 09:04 mostafa

@heydonovan Any updates here?

mostafa avatar May 06 '22 09:05 mostafa

I'll close this due to inactivity. Feel free to re-open it if you still have this issue/question.

mostafa avatar Sep 02 '22 13:09 mostafa