apple_auth: Remove decode_id_token override and update social-auth-core #15553

chdinesh1089 · 2020-06-24T16:39:08Z

Testing Plan:
https://circleci.com/gh/chdinesh1089/zulip/tree/apple_auth

zproject/backends.py

mateuszmandera · 2020-06-24T17:35:29Z

zproject/computed_settings.py

@@ -1030,6 +1031,7 @@ def zulip_path(path: str) -> str:
 # SERVICES_ID to make things more readable in the configuration
 # and our own custom backend code.
 SOCIAL_AUTH_APPLE_CLIENT = SOCIAL_AUTH_APPLE_SERVICES_ID
+SOCIAL_AUTH_APPLE_AUDIENCE = [SOCIAL_AUTH_APPLE_SERVICES_ID, SOCIAL_AUTH_APPLE_BUNDLE_ID]


I think this is why you're getting the mypy error for test_extra_settings. The way you're defining this AUDIENCE list here means it might end up being something like ["some.services.id", None]. That's not what we want. If BUNDLE_ID isn't specified, AUDIENCE should only have the services id

gnprice

Thanks @chdinesh1089 ! One comment below, apart from the spots Mateusz has mentioned.

docs/production/authentication-methods.md

version.py

mateuszmandera · 2020-06-27T15:35:09Z

zproject/backends.py

@@ -1209,7 +1209,7 @@ def social_associate_user_helper(backend: BaseAuth, return_data: Dict[str, Any],
        # In SAML authentication, the IdP may support only sending
        # the first and last name as separate attributes - in that case
        # we construct the full name from them.
-        return_data["full_name"] = f"{first_name} {last_name}".strip()  # strip removes the unnecessary ' '
+        return_data["full_name"] = f"{first_name or ''} {last_name or ''}".strip()  # strip removes the unnecessary ' '


Now that it's not just SAML using this code, we should update the comment above.

Do these or '' do anything? They’re already looked up with a default of '' above:

first_name = kwargs['details'].get('first_name', '') last_name = kwargs['details'].get('last_name', '')

when the name details aren't sent by apple, python-social-auth sets first_name and last_name to None. Because of that return_data["full_name"] is getting the value "None None". Those or '' are used to avoid that.

In general this whole structure is kind of weird:

if full_name is None: if not first_name and not last_name: # We need custom code here for any social auth backends # that don't provide name details feature. if (backend.name == 'apple'): # Apple authentication provides the user's name only # the very first time a user tries to login. So if # the user aborts login or otherwise is doing this the # second time, we won't have any name data. We handle # this by setting full_name to be the empty string. full_name = "" else: raise AssertionError("Social auth backend doesn't provide name") if full_name: return_data["full_name"] = full_name else: # In SAML authentication, the IdP may support only sending # the first and last name as separate attributes - in that case # we construct the full name from them. return_data["full_name"] = f"{first_name} {last_name}".strip() # strip removes the unnecessary ' ' return_data["full_name"] = f"{first_name or ''} {last_name or ''}".strip() # strip removes the unnecessary ' '

The block if (backend.name == 'apple'): does nothing, because then we fall into the else condition of if full_name / else. I think this needs to be cleaned up.

mateuszmandera · 2020-06-27T15:37:45Z

zproject/test_extra_settings.py

@@ -175,6 +175,7 @@ def set_loglevel(logger_name: str, level: str) -> None:
 SOCIAL_AUTH_APPLE_SERVICES_ID = 'com.zulip.chat'
 SOCIAL_AUTH_APPLE_BUNDLE_ID = 'com.zulip.bundle.id'
 SOCIAL_AUTH_APPLE_CLIENT = 'com.zulip.chat'
+SOCIAL_AUTH_APPLE_AUDIENCE: List[Optional[str]] = [SOCIAL_AUTH_APPLE_BUNDLE_ID, SOCIAL_AUTH_APPLE_SERVICES_ID]


This is not correct yet, having a None in this list is not something we want. The type should be List[str] and we should make mypy happy with it (by correctly defining this list in computed_settings - in a way the ensures mypy there are no None in there).

mateuszmandera · 2020-06-27T15:40:59Z

The changes that are related to the package update should be in the same commit as the update. The other things are good as separate commits.

andersk · 2020-06-27T20:36:45Z

requirements/common.in

@@ -144,7 +144,7 @@ py3dns

 # Install Python Social Auth
 social-auth-app-django
-social-auth-core[azuread,saml]
+https://github.com/python-social-auth/social-core/archive/3.4.0.zip/#egg=social-auth-core[azuread,saml]==3.4.0


Have you asked upstream why this hasn’t been released to PyPI?

No, just opened an issue asking. python-social-auth/social-core#485

andersk · 2020-06-27T20:36:49Z

zproject/test_extra_settings.py

 SOCIAL_AUTH_APPLE_CLIENT = 'com.zulip.chat'
+SOCIAL_AUTH_APPLE_AUDIENCE: List[Optional[str]] = [SOCIAL_AUTH_APPLE_APP_ID, SOCIAL_AUTH_APPLE_SERVICES_ID]


Why Optional? Is None really a valid element of this list, or are you just missing a SOCIAL_AUTH_APPLE_CLIENT is not None check in computed_settings?

mypy gets that None from having SOCIAL_AUTH_APPLE_SERVICES_ID = get_secret('social_auth_apple_services_id', development_only=True) in default_settings.py where get_secret returns Optional[str].

Fixed with adding a SOCIAL_AUTH_APPLE_CLIENT is not None check.

andersk · 2020-06-27T20:41:03Z

zproject/backends.py

@@ -1209,7 +1209,7 @@ def social_associate_user_helper(backend: BaseAuth, return_data: Dict[str, Any],
        # In SAML authentication, the IdP may support only sending
        # the first and last name as separate attributes - in that case
        # we construct the full name from them.
-        return_data["full_name"] = f"{first_name} {last_name}".strip()  # strip removes the unnecessary ' '
+        return_data["full_name"] = f"{first_name or ''} {last_name or ''}".strip()  # strip removes the unnecessary ' '


Do these or '' do anything? They’re already looked up with a default of '' above:

first_name = kwargs['details'].get('first_name', '') last_name = kwargs['details'].get('last_name', '')

mateuszmandera · 2020-06-29T16:42:09Z

zproject/backends.py

-                full_name = ""
-            else:
-                raise AssertionError("Social auth backend doesn't provide name")
+    if all(name is None for name in [full_name, first_name, last_name]) and backend.name != "apple":


this won't work correctly if e.g. first_name is the empty string rather than None. We want not name I think.

Ohh, I used is None thinking we'll restrict full_name = '' case with not name.
Can we change

first_name = kwargs['details'].get('first_name', '') last_name = kwargs['details'].get('last_name', '')

to

first_name = kwargs['details'].get('first_name') last_name = kwargs['details'].get('last_name')

so that this nice if line will still be correct?

chdinesh1089 · 2020-06-30T19:37:00Z

@timabbott I think this is ready for review. Could you take a look?

timabbott · 2020-07-01T05:40:37Z

I don't understand the end state with first_name and last_name -- is the problem in part that the details object can contain None values for those properties now?

chdinesh1089 · 2020-07-01T07:04:43Z

Yes, Only for apple, details object can contain first_name and last_name as None, because python social auth makes it None when apple doesn't send us name details because of user trying to re-attempt a failed registration/login(apple only sends name details the very first time user tries to log in).

timabbott · 2020-07-01T18:28:13Z

OK; we should look at reporting an issue with python-social-auth upstream for that; it's not the convention that project uses for other details objects AFAIK. @mateuszmandera I suppose should confirm.

chdinesh1089 · 2020-07-03T17:30:09Z

yes, looks like it isn't the convention that the project uses, but that was an intentional change introduced in python-social-auth/social-core#465 The author of PR says

if there's no first/last name in the response return None so that the User model is not updated to empty strings if user_details is used in the pipeline

Python social auth says to avoid None in https://github.com/python-social-auth/social-core/blob/9d93069564a60495e0ebd697b33e16fcff14195b/social_core/backends/base.py#L176

@mateuszmandera Could you confirm if it is a mistake? (I'm not sure if it's a mistake as it got merged as some sort of a fix)

mateuszmandera · 2020-07-05T11:03:44Z

It does seem like perhaps the original intent was to use '' rather than None in PSA backends and the convention is generally respected, but for instance SAML doesn't follow it and can return Nones:

    # Attributes processing:
    def get_user_details(self, attributes):
        """
        Given the SAML attributes extracted from the SSO response, get
        the user data like name.
        """
        return {
            'fullname': self.get_attr(attributes, 'attr_full_name',
                                      OID_COMMON_NAME),
            'first_name': self.get_attr(attributes, 'attr_first_name',
                                        OID_GIVEN_NAME),
            'last_name': self.get_attr(attributes, 'attr_last_name',
                                       OID_SURNAME),
            'username': self.get_attr(attributes, 'attr_username',
                                      OID_USERID),
            'email': self.get_attr(attributes, 'attr_email',
                                   OID_MAIL),
        }

    def get_attr(self, attributes, conf_key, default_attribute):
        """
        Internal helper method.
        Get the attribute 'default_attribute' out of the attributes,
        unless self.conf[conf_key] overrides the default by specifying
        another attribute to use.
        """
        key = self.conf.get(conf_key, default_attribute)
        value = attributes[key] if key in attributes else None
        if isinstance(value, list):
            if len(value):
                value = value[0]
            else:
                value = None
        return value

Perhaps we should submit PRs to upstream for apple and SAML to fix this, but regardless I don't think we can rely on this convention being followed and we cannot be investigating if it's not suddenly being broken on every PSA version bump - so I'd say in our code we should handle the None possibility.

mateuszmandera · 2020-07-05T11:04:41Z

@chdinesh1089 Do you want to take care of submitting the PR to upstream?

chdinesh1089 · 2020-07-07T11:43:15Z

Opened python-social-auth/social-core#490.

timabbott · 2020-07-25T00:30:09Z

What's the status of this PR? Are we blocked on upstream? And if not, @mateuszmandera are you happy with it?

mateuszmandera · 2020-07-25T13:52:56Z

@timabbott I don't believe we're blocked. The answer to the #15553 (comment) concern is that None is a permissible value for first_name and others, and the None value is the right one as explained on Dinesh's upstream PR discussion. So we need to handle it.

One controversy might that the new PSA version isn't released to pypi due to Travis failing, but as explained by python-social-auth/social-core#485 (comment) it seems safe to upgrade.

The commits LGTM, so should be ready for your review.

@chdinesh1089 Can you fix the conflict and post a link to the discussion on renaming the certificate file as reference since it seems useful to havae it here.

Uses git release as this version 3.4.0 is not released to pypi. This is required for removing some overriden functions of apple auth backend class AppleAuthBackend. With the update we also make following changes: * Fix full name being populated as "None None". c5c74f27dd that's included in update assigns first_name and last_name to None when no name is provided by apple. Due to this our code is filling return_data['full_name'] to 'None None'. This commit fixes it by making first and last name strings empty. * Remove decode_id_token override. Python social auth merged the PR we sent including the changes we made to decode_id_token function. So, now there is no necessity for the override. * Add _AUDIENCE setting in computed_settings.py. `decode_id_token` is dependent on this setting.

Changes to a better name apple-auth-key.p8 and removes the extra directory apple.

The apple developer webapp consistently refers this App ID. So, this clears any confusion that can occur. Since python social auth only requires us to include App ID in _AUDIENCE(a list), we do that in computed settings making it easier for server admin and we make it much clear by having it set to APP_ID instead of BUNDLE_ID.

Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>

Apple has some other obligatory settings other than key and secret. To handle that this commit adds a function check_config() similar to that of SAML.

chdinesh1089 · 2020-07-25T16:53:17Z

Rebased.
Link to discussion about renaming private key file: https://chat.zulip.org/#narrow/stream/3-backend/topic/apple.20auth/near/912392

zulipbot · 2020-07-27T18:12:27Z

Heads up @chdinesh1089, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

timabbott · 2020-07-29T00:17:19Z

Merged as the series ending with 9583554 after rebasing. One thing to note is the PROVISION_VERSION bump here was wrong; 91.3 should increase to 92.0, not 92.3.

zulipbot added the size: L label Jun 24, 2020

mateuszmandera reviewed Jun 24, 2020

View reviewed changes

zproject/backends.py Outdated Show resolved Hide resolved

mateuszmandera reviewed Jun 24, 2020

View reviewed changes

chdinesh1089 force-pushed the apple_auth branch 2 times, most recently from 4279341 to 22146b1 Compare June 25, 2020 12:53

zulipbot added the has conflicts label Jun 25, 2020

gnprice reviewed Jun 26, 2020

View reviewed changes

docs/production/authentication-methods.md Outdated Show resolved Hide resolved

chdinesh1089 force-pushed the apple_auth branch from 22146b1 to e3e276e Compare June 26, 2020 10:51

zulipbot removed the has conflicts label Jun 26, 2020

chdinesh1089 force-pushed the apple_auth branch from e3e276e to 111a1c6 Compare June 26, 2020 10:53

chdinesh1089 marked this pull request as ready for review June 26, 2020 10:54

mateuszmandera reviewed Jun 27, 2020

View reviewed changes

andersk reviewed Jun 27, 2020

View reviewed changes

chdinesh1089 force-pushed the apple_auth branch 5 times, most recently from e185bc2 to 38817be Compare June 28, 2020 14:45

zulipbot added size: XL and removed size: L labels Jun 28, 2020

chdinesh1089 force-pushed the apple_auth branch from 6c87463 to 9ebce36 Compare June 28, 2020 16:12

mateuszmandera reviewed Jun 29, 2020

View reviewed changes

chdinesh1089 force-pushed the apple_auth branch from 9ebce36 to 33ad9be Compare June 30, 2020 16:30

zulipbot added the has conflicts label Jul 3, 2020

zulipbot removed the has conflicts label Jul 4, 2020

chdinesh1089 force-pushed the apple_auth branch 2 times, most recently from e278dc1 to 3d8206a Compare July 4, 2020 19:00

zulipbot added the has conflicts label Jul 6, 2020

chdinesh1089 force-pushed the apple_auth branch from 3d8206a to 8d573de Compare July 7, 2020 06:47

zulipbot removed the has conflicts label Jul 7, 2020

chdinesh1089 force-pushed the apple_auth branch from 8d573de to 855bc39 Compare July 7, 2020 08:21

zulipbot added the has conflicts label Jul 10, 2020

chdinesh1089 force-pushed the apple_auth branch from 855bc39 to 84c0f20 Compare July 22, 2020 13:42

zulipbot added has conflicts and removed has conflicts labels Jul 22, 2020

chdinesh1089 and others added 5 commits July 25, 2020 22:00

apple_auth: Change where private key is stored.

14c1ae1

Changes to a better name apple-auth-key.p8 and removes the extra directory apple.

auth: Restructure code handling no name data sent by auth backend.

44a1916

Co-authored-by: Mateusz Mandera <mateusz.mandera@zulip.com>

auth: Add check_config for apple auth.

2560dd0

Apple has some other obligatory settings other than key and secret. To handle that this commit adds a function check_config() similar to that of SAML.

chdinesh1089 force-pushed the apple_auth branch from 84c0f20 to 2560dd0 Compare July 25, 2020 16:39

zulipbot removed the has conflicts label Jul 25, 2020

zulipbot added the has conflicts label Jul 27, 2020

timabbott closed this Jul 29, 2020

chdinesh1089 deleted the apple_auth branch July 29, 2020 07:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apple_auth: Remove decode_id_token override and update social-auth-core #15553

apple_auth: Remove decode_id_token override and update social-auth-core #15553

chdinesh1089 commented Jun 24, 2020

mateuszmandera Jun 24, 2020

gnprice left a comment

mateuszmandera Jun 27, 2020

andersk Jun 27, 2020

chdinesh1089 Jun 28, 2020

mateuszmandera Jun 28, 2020

mateuszmandera Jun 27, 2020

mateuszmandera commented Jun 27, 2020

andersk Jun 27, 2020

chdinesh1089 Jun 28, 2020

andersk Jun 27, 2020

chdinesh1089 Jun 28, 2020

chdinesh1089 Jun 28, 2020

andersk Jun 27, 2020

mateuszmandera Jun 29, 2020

chdinesh1089 Jun 29, 2020 •

edited

chdinesh1089 commented Jun 30, 2020

timabbott commented Jul 1, 2020

chdinesh1089 commented Jul 1, 2020 •

edited

timabbott commented Jul 1, 2020

chdinesh1089 commented Jul 3, 2020

mateuszmandera commented Jul 5, 2020

mateuszmandera commented Jul 5, 2020

chdinesh1089 commented Jul 7, 2020

timabbott commented Jul 25, 2020

mateuszmandera commented Jul 25, 2020

chdinesh1089 commented Jul 25, 2020

zulipbot commented Jul 27, 2020

timabbott commented Jul 29, 2020

		SOCIAL_AUTH_APPLE_CLIENT = 'com.zulip.chat'
		SOCIAL_AUTH_APPLE_AUDIENCE: List[Optional[str]] = [SOCIAL_AUTH_APPLE_APP_ID, SOCIAL_AUTH_APPLE_SERVICES_ID]

apple_auth: Remove decode_id_token override and update social-auth-core #15553

apple_auth: Remove decode_id_token override and update social-auth-core #15553

Conversation

chdinesh1089 commented Jun 24, 2020

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszmandera commented Jun 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chdinesh1089 Jun 29, 2020 • edited

Choose a reason for hiding this comment

chdinesh1089 commented Jun 30, 2020

timabbott commented Jul 1, 2020

chdinesh1089 commented Jul 1, 2020 • edited

timabbott commented Jul 1, 2020

chdinesh1089 commented Jul 3, 2020

mateuszmandera commented Jul 5, 2020

mateuszmandera commented Jul 5, 2020

chdinesh1089 commented Jul 7, 2020

timabbott commented Jul 25, 2020

mateuszmandera commented Jul 25, 2020

chdinesh1089 commented Jul 25, 2020

zulipbot commented Jul 27, 2020

timabbott commented Jul 29, 2020

chdinesh1089 Jun 29, 2020 •

edited

chdinesh1089 commented Jul 1, 2020 •

edited