Fix apple and saml backend filling user_details with `None` values.

[chdinesh1089]

Proposed changes

Fixes apple and saml backends filling user_details with values containing None. This project generally uses '' instead of None. Most other backends use ''.

Types of changes

Please check the type of change your PR introduces:

• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Code style update (PEP8, lint, formatting, renaming, etc)
• [ ] Refactoring (no functional changes, no api changes)
• [x] Breaking change (fix or feature that would cause existing functionality to not work as expected)
• [ ] Build related changes (build process, tests runner, etc)
• [ ] Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• [ ] Lint and unit tests pass locally with my changes
• [ ] I have added tests that prove my fix is effective or that my feature works

Other information

Any other information that is important to this PR such as screenshots of how the component looks before and after the change.

[chdinesh1089]

@maiiku This PR reverts your change in #465 to have None for firstName and lastName in apple backend when it does not get name details. This could break things in your project/s if it depends on firstName and lastName to be None.

[maiiku]

@chdinesh1089 this breaks things in another way, that is if your project has those details this will override First and Last Names to empty strings on next login. This is why I changed it to None in the first place, as None is omitted in update process by social-auth. If you wish to do it this way you should also modify modify relevant code for user updating to ignore empty strings.

However I think it is your project that should handle None values for first nad last name in the first place, as this is dependent on your project nad social-auth, and that's why I proposed previous solution as opposed to changing user updating process in social-auth.

[chdinesh1089]

@maiiku Since most other backends in social-core give empty strings for such cases, the projects supporting multiple authentication methods dependent on social-core will have code to handle empty strings and not None. Making it None would require everyone to handle the possibility of None just for apple and empty strings for all other backends. This inconsistency is not great.

Also, it is mentioned in this project's code to avoid None values. https://github.com/python-social-auth/social-core/blob/master/social_core/backends/base.py#L176

[mateuszmandera]

I think @maiiku is right here, looking at this code from social_core.pipeline.user.user_details:

    field_mapping = strategy.setting('USER_FIELD_MAPPING', {}, backend)
    for name, value in details.items():
        # Convert to existing user field if mapping exists
        name = field_mapping.get(name, name)
        if value is None or not hasattr(user, name) or name in protected:
            continue

it seems that indeed the intent is that if the field (so first_name and last_name in our apple case) should be considered as "no value sent" and shouldn't overwrite the existing data on the user, then the value should be set to None. So it seems right to have the Nones in the Apple backend.

The question would be what should be the right behavior for SAML.

[maiiku]

@chdinesh1089 then as I said, if you wish to change this you should also propose a change to user_details in social_core/pipeline/user.py which currently only skips None values:

    for name, value in details.items():
        # Convert to existing user field if mapping exists
        name = field_mapping.get(name, name)
        if value is None or not hasattr(user, name) or name in protected:
            continue

        current_value = getattr(user, name, None)
        if current_value == value:
            continue

        changed = True
        setattr(user, name, value)

as this actively overwrites in your database user details. Perhaps other backends do not return Nones on a second and next logins (as apple does) and hence the problem is not as visible.

get_user_names you mentioned was specifically coded to handle None values, not as a warning against them

[mateuszmandera]

It seems to me that user_details should not overwrite user attributes with empty strings, so that might be the right change to make. But I don't know if the current behavior isn't being relied on by other projects/backends, so I don't feel confident in this opinion.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.