Double update of main attribute when SAML_USE_NAME_ID_AS_USERNAME is False

[BRONSOLO]

I noticed that when SAML_USE_NAME_ID_AS_USERNAME is False, whatever attribute is determined to be the main attribute here https://github.com/knaperek/djangosaml2/blob/fd6a8237ed7b2b2308de97b11773c1860139b048/djangosaml2/backends.py#L88-L90 gets applied to the user both on creation and on update of the attributes.

This double update does not seem necessary and can cause problems if a cleaning is defined on the main attribute that is separate from the cleaning of the attributes.

Example:

1. Configure an email attribute to be sent from the IdP and be used as the main attribute / set to be the username within Django

2. Define a cleaning on this main attribute that converts @ to _ (e.g., user@company within IdP ---> user_company Django user)

3. When a user is created here, the username will be user_company as expected

4. Once the user is created, the attributes will be updated here and the username will change to the attribute value of user@company, which is not desired.

Any suggestions / feedback on the above? I may be missing a best practice configuration here. Thanks!

[peppelinux]

Hi @BRONSOLO
As I can see you found a bug!
Probably you're in the flow, enough for push a PR with a unit test if you agree.
consider me available for a quick code merge 👌

[BRONSOLO]

Thanks @peppelinux! I'm happy to have a go at a fix but I'm not sure when I'll have the time to get to it.

[peppelinux]

Ok don't worry, Just Need to ask you the code/example to reproduce exactly what you described.

[BRONSOLO]

I haven't tested this but implementing the following should reproduce the issue:

1. override clean_user_main_attribute to include some modification to the main_attribute:

def clean_user_main_attribute(self, main_attribute):
    return main_attribute + 'test'

2. Within the django app settings, use:

SAML_USE_NAME_ID_AS_USERNAME=False
SAML_ATTRIBUTE_MAPPING = { SOME_ATTRIBUTE: ('username', ) }

3. Have the IdP return SOME_ATTRIBUTE to the SP to be used as the django user username.

4. log in as a user to your django app, say USER1

5. confirm that the user is incorrectly created / saved with a username of USER1

expected: the user should be created / saved as USER1test

[peppelinux]

I think that this issue and other two should be fixed in next release 1.3.0, I put it in this milestone.
Would you like to take this task and purpose a PR that solves this issue?

[BRONSOLO]

Thanks @peppelinux! I'm really swamped at the moment : / Unfortunately I won't be able to tackle a fix + PR for this.

[peppelinux]

if I were not swamped too believe me that I would have already done this correction!
thank you for the quick reply anyway 👍

[jaap3]

Would adding if attr == self._user_lookup_attribute(): continue right before

djangosaml2/djangosaml2/backends.py

Line 164 in fd6a823

if hasattr(user, attr):

solve this issue?

That way _update_user would never update the field that was just used to create/find the user it's updating right?

[peppelinux]

@jaap3 I think that you're the man
I'd Be Happy to read you PR related on that with some of your awesome tests

You're in the flow, please don't stop the 🧠