-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(appstore): Don't double-send requests for SMS codes during 2FA auth #29500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ | |
| import logging | ||
| from collections import namedtuple | ||
| from http import HTTPStatus | ||
| from typing import List, NewType, Optional | ||
| from typing import List, NewType, Optional, Tuple | ||
|
|
||
| import requests | ||
| import sentry_sdk | ||
|
|
@@ -144,6 +144,9 @@ def __init__(self, service_key: Optional[ITunesServiceKey] = None): | |
| # The scnt header as populated by :meth:`start_login_sequence`. | ||
| self._scnt: Optional[str] = None | ||
|
|
||
| # The trusted phone info, set by :meth:`_request_trusted_phone_info`. | ||
| self._trusted_phone: Optional[TrustedPhoneInfo] = None | ||
|
|
||
| @property | ||
| def session_id(self) -> str: | ||
| """The session ID, if client already has one (after :meth:start_login_sequence). | ||
|
|
@@ -191,6 +194,9 @@ def to_json(self) -> json.JSONData: | |
| context["session_id"] = self.session_id | ||
| if self._scnt is not None: | ||
| context["scnt"] = self.scnt | ||
| if self._trusted_phone is not None: | ||
| context["phone_id"] = self._trusted_phone.id | ||
| context["phone_push_mode"] = self._trusted_phone.push_mode | ||
| if self.state is ClientState.AUTHENTICATED: | ||
| context["session_cookie"] = self.session_cookie() | ||
| return context | ||
|
|
@@ -215,6 +221,10 @@ def from_json(cls, context: json.JSONData) -> "ITunesClient": | |
| ]: | ||
| obj._session_id = context["session_id"] | ||
| obj._scnt = context["scnt"] | ||
| if obj.state is ClientState.SMS_AUTH_REQUESTED: | ||
| obj._trusted_phone = TrustedPhoneInfo( | ||
| id=context["phone_id"], push_mode=context["phone_push_mode"] | ||
| ) | ||
| if obj.state in [ClientState.AUTHENTICATED, ClientState.EXPIRED]: | ||
| obj.load_session_cookie(context["session_cookie"]) | ||
| return obj | ||
|
|
@@ -308,7 +318,7 @@ def two_factor_code(self, code: str) -> None: | |
| else: | ||
| self.state = ClientState.AUTHENTICATED | ||
|
|
||
| def _request_trusted_phone_info(self) -> TrustedPhoneInfo: | ||
| def _request_trusted_phone_info(self) -> Tuple[TrustedPhoneInfo, bool]: | ||
| """Requests the trusted phone info for the account.""" | ||
| url = "https://idmsa.apple.com/appleauth/auth" | ||
| logger.debug("GET %s", url) | ||
|
|
@@ -322,13 +332,15 @@ def _request_trusted_phone_info(self) -> TrustedPhoneInfo: | |
| }, | ||
| timeout=REQUEST_TIMEOUT, | ||
| ) | ||
|
|
||
| if response.status_code == HTTPStatus.LOCKED: | ||
| raise SmsBlockedError | ||
| if not response.ok: | ||
| raise ITunesError(f"Unexpected response status: {response.status_code}") | ||
|
|
||
| try: | ||
| info = response.json()["trustedPhoneNumber"] | ||
| info = response.json() | ||
| trusted_phone_info = info["trustedPhoneNumber"] | ||
| except ValueError: | ||
| raise ITunesError( | ||
| f"Received unexpected response content, response status: {response.status_code}" | ||
|
|
@@ -337,9 +349,18 @@ def _request_trusted_phone_info(self) -> TrustedPhoneInfo: | |
| raise ITunesError( | ||
| f"Trusted phone info missing from response with status: {response.status_code}" | ||
| ) | ||
| return TrustedPhoneInfo( | ||
| id=info["id"], | ||
| push_mode=info["pushMode"], | ||
|
|
||
| # The code has already been sent to the only trusted phone number | ||
| sms_automatically_sent = len(info.get("trustedPhoneNumbers", [])) == 1 and info.get( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this logic is basically pulled from fastlane's implementation. they also have more complex logic to handle other cases, such as when a user has multiple trusted phone numbers, etc. in the same file, they also skip hitting |
||
| "noTrustedDevices", False | ||
| ) | ||
|
|
||
| return ( | ||
| TrustedPhoneInfo( | ||
| id=trusted_phone_info["id"], | ||
| push_mode=trusted_phone_info["pushMode"], | ||
| ), | ||
| sms_automatically_sent, | ||
| ) | ||
|
|
||
| def request_sms_auth(self) -> None: | ||
|
|
@@ -353,7 +374,14 @@ def request_sms_auth(self) -> None: | |
| ClientState.AUTH_REQUESTED, | ||
| ClientState.SMS_AUTH_REQUESTED, | ||
| ], f"Actual client state: {self.state}" | ||
| trusted_phone = self._request_trusted_phone_info() | ||
|
|
||
| trusted_phone, sms_automatically_sent = self._request_trusted_phone_info() | ||
|
|
||
| if sms_automatically_sent: | ||
| self.state = ClientState.SMS_AUTH_REQUESTED | ||
| self._trusted_phone = trusted_phone | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a bit odd together with the new assert on line 378?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry about that, this is a little strange. i'm pretty sure this assert is supposed to sit inside of |
||
| return | ||
|
|
||
| url = "https://idmsa.apple.com/appleauth/auth/verify/phone" | ||
| logger.debug("PUT %s", url) | ||
| response = self.session.put( | ||
|
|
@@ -375,23 +403,26 @@ def request_sms_auth(self) -> None: | |
| raise SmsBlockedError | ||
| if response.status_code != HTTPStatus.OK: | ||
| raise ITunesError(f"Unexpected response status: {response.status_code}") | ||
|
|
||
| self.state = ClientState.SMS_AUTH_REQUESTED | ||
| self._trusted_phone = trusted_phone | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can't we be already sure this is already set by now? |
||
|
|
||
| def sms_code(self, code: str) -> None: | ||
| """Sends the SMS auth code, completing authentication. | ||
|
|
||
| :raises InvalidSmsAuthError: | ||
| """ | ||
| assert self.state is ClientState.SMS_AUTH_REQUESTED, f"Actual client state: {self.state}" | ||
| assert self._trusted_phone is not None | ||
|
|
||
| url = "https://idmsa.apple.com/appleauth/auth/verify/phone/securitycode" | ||
| logger.debug("PUT %s", url) | ||
| trusted_phone = self._request_trusted_phone_info() | ||
| response = self.session.post( | ||
| url, | ||
| json={ | ||
| "securityCode": {"code": code}, | ||
| "phoneNumber": {"id": trusted_phone.id}, | ||
| "mode": trusted_phone.push_mode, | ||
| "phoneNumber": {"id": self._trusted_phone.id}, | ||
| "mode": self._trusted_phone.push_mode, | ||
| }, | ||
| headers={ | ||
| "scnt": self.scnt, | ||
|
|
@@ -401,6 +432,7 @@ def sms_code(self, code: str) -> None: | |
| }, | ||
| timeout=REQUEST_TIMEOUT, | ||
| ) | ||
|
|
||
| if response.status_code != HTTPStatus.OK: | ||
| # TODO: Make invalid code distinguishable from generic error. | ||
| raise InvalidAuthCodeError | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not particularly married to this return type.