From f1c763ad9f9ea0fa309df5d04f280c219b6e1760 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Wed, 17 Nov 2021 14:32:53 +0100 Subject: [PATCH 01/40] Use HttpRequestManager to acquire new tokens This is a re-write from a previous attempt. Instead of requests, which doesn't properly use SSL certificates installed on the computer among other things, we'll now use the HttpRequestManager which uses QNetworkManager under the hood and properly uses system settings. The QNetworkManager is asynchronous which would normally be very nice, but due to the nature of this call we want to make it synchronous so we'll use a lock here. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 83 +++++++++++++++++++---------- 1 file changed, 55 insertions(+), 28 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index d6f4980fe4..57af58493c 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -1,16 +1,19 @@ # Copyright (c) 2021 Ultimaker B.V. # Cura is released under the terms of the LGPLv3 or higher. -from datetime import datetime -import json -import secrets -from hashlib import sha512 from base64 import b64encode +from datetime import datetime +from hashlib import sha512 +from PyQt5.QtNetwork import QNetworkReply +import secrets +from threading import Lock from typing import Optional import requests +import urllib.parse from UM.i18n import i18nCatalog from UM.Logger import Logger +from UM.TaskManagement.HttpRequestManager import HttpRequestManager # To download log-in tokens. from cura.OAuth2.Models import AuthenticationResponse, UserProfile, OAuth2Settings catalog = i18nCatalog("cura") @@ -23,6 +26,8 @@ class AuthorizationHelpers: def __init__(self, settings: "OAuth2Settings") -> None: self._settings = settings self._token_url = "{}/token".format(self._settings.OAUTH_SERVER_URL) + self._request_lock = Lock() + self._auth_response = None # type: Optional[AuthenticationResponse] @property def settings(self) -> "OAuth2Settings": @@ -46,10 +51,19 @@ class AuthorizationHelpers: "code_verifier": verification_code, "scope": self._settings.CLIENT_SCOPES if self._settings.CLIENT_SCOPES is not None else "", } - try: - return self.parseTokenResponse(requests.post(self._token_url, data = data)) # type: ignore - except requests.exceptions.ConnectionError as connection_error: - return AuthenticationResponse(success = False, err_message = f"Unable to connect to remote server: {connection_error}") + headers = {"Content-type": "application/x-www-form-urlencoded"} + self._request_lock.acquire() + HttpRequestManager.getInstance().post( + self._token_url, + data = urllib.parse.urlencode(data).encode("UTF-8"), + headers_dict = headers, + callback = self.parseTokenResponse + ) + self._request_lock.acquire(timeout = 60) # Block until the request is completed. 1 minute timeout. + response = self._auth_response + self._auth_response = None + self._request_lock.release() + return response def getAccessTokenUsingRefreshToken(self, refresh_token: str) -> "AuthenticationResponse": """Request the access token from the authorization server using a refresh token. @@ -66,15 +80,21 @@ class AuthorizationHelpers: "refresh_token": refresh_token, "scope": self._settings.CLIENT_SCOPES if self._settings.CLIENT_SCOPES is not None else "", } - try: - return self.parseTokenResponse(requests.post(self._token_url, data = data)) # type: ignore - except requests.exceptions.ConnectionError: - return AuthenticationResponse(success = False, err_message = "Unable to connect to remote server") - except OSError as e: - return AuthenticationResponse(success = False, err_message = "Operating system is unable to set up a secure connection: {err}".format(err = str(e))) + headers = {"Content-type": "application/x-www-form-urlencoded"} + self._request_lock.acquire() + HttpRequestManager.getInstance().post( + self._token_url, + data = urllib.parse.urlencode(data).encode("UTF-8"), + headers_dict = headers, + callback = self.parseTokenResponse + ) + self._request_lock.acquire(timeout = 60) # Block until the request is completed. 1 minute timeout. + response = self._auth_response + self._auth_response = None + self._request_lock.release() + return response - @staticmethod - def parseTokenResponse(token_response: requests.models.Response) -> "AuthenticationResponse": + def parseTokenResponse(self, token_response: QNetworkReply) -> None: """Parse the token response from the authorization server into an AuthenticationResponse object. :param token_response: The JSON string data response from the authorization server. @@ -82,25 +102,32 @@ class AuthorizationHelpers: """ token_data = None + http = HttpRequestManager.getInstance() try: - token_data = json.loads(token_response.text) + token_data = http.readJSON(token_response) except ValueError: - Logger.log("w", "Could not parse token response data: %s", token_response.text) + Logger.log("w", "Could not parse token response data: %s", http.readText(token_response)) if not token_data: - return AuthenticationResponse(success = False, err_message = catalog.i18nc("@message", "Could not read response.")) + self._auth_response = AuthenticationResponse(success = False, err_message = catalog.i18nc("@message", "Could not read response.")) + self._request_lock.release() + return - if token_response.status_code not in (200, 201): - return AuthenticationResponse(success = False, err_message = token_data["error_description"]) + if token_response.error() != QNetworkReply.NetworkError.NoError: + self._auth_response = AuthenticationResponse(success = False, err_message = token_data["error_description"]) + self._request_lock.release() + return - return AuthenticationResponse(success=True, - token_type=token_data["token_type"], - access_token=token_data["access_token"], - refresh_token=token_data["refresh_token"], - expires_in=token_data["expires_in"], - scope=token_data["scope"], - received_at=datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT)) + self._auth_response = AuthenticationResponse(success = True, + token_type = token_data["token_type"], + access_token = token_data["access_token"], + refresh_token = token_data["refresh_token"], + expires_in = token_data["expires_in"], + scope = token_data["scope"], + received_at = datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT)) + self._request_lock.release() + return def parseJWT(self, access_token: str) -> Optional["UserProfile"]: """Calls the authentication API endpoint to get the token data. From a9990eaa7567746a97e211a7cb30eaa36feeee0d Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Thu, 18 Nov 2021 17:11:27 +0100 Subject: [PATCH 02/40] Make parseJWT asynchronous This involves returning the user profile via a callback. No longer use the Requests library, which doesn't properly use the SSL certificates locally on the computer like the QNetworkManager does. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 84 ++++++++++++++++------------- 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 57af58493c..229e7a3f14 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -7,7 +7,7 @@ from hashlib import sha512 from PyQt5.QtNetwork import QNetworkReply import secrets from threading import Lock -from typing import Optional +from typing import Callable, Optional import requests import urllib.parse @@ -100,15 +100,7 @@ class AuthorizationHelpers: :param token_response: The JSON string data response from the authorization server. :return: An AuthenticationResponse object. """ - - token_data = None - http = HttpRequestManager.getInstance() - - try: - token_data = http.readJSON(token_response) - except ValueError: - Logger.log("w", "Could not parse token response data: %s", http.readText(token_response)) - + token_data = HttpRequestManager.readJSON(token_response) if not token_data: self._auth_response = AuthenticationResponse(success = False, err_message = catalog.i18nc("@message", "Could not read response.")) self._request_lock.release() @@ -129,39 +121,57 @@ class AuthorizationHelpers: self._request_lock.release() return - def parseJWT(self, access_token: str) -> Optional["UserProfile"]: + def checkToken(self, access_token: str, callback: Optional[Callable[[UserProfile], None]] = None) -> None: """Calls the authentication API endpoint to get the token data. + The API is called asynchronously. When a response is given, the callback is called with the user's profile. :param access_token: The encoded JWT token. - :return: Dict containing some profile data. + :param callback: When a response is given, this function will be called with a user profile. If None, there will + not be a callback. If the token failed to give/parse a user profile, the callback will not be called either. """ - - try: - check_token_url = "{}/check-token".format(self._settings.OAUTH_SERVER_URL) - Logger.log("d", "Checking the access token for [%s]", check_token_url) - token_request = requests.get(check_token_url, headers = { - "Authorization": "Bearer {}".format(access_token) - }) - except (requests.exceptions.ConnectionError, requests.exceptions.Timeout): - # Connection was suddenly dropped. Nothing we can do about that. - Logger.logException("w", "Something failed while attempting to parse the JWT token") - return None - if token_request.status_code not in (200, 201): - Logger.log("w", "Could not retrieve token data from auth server: %s", token_request.text) - return None - user_data = token_request.json().get("data") - if not user_data or not isinstance(user_data, dict): - Logger.log("w", "Could not parse user data from token: %s", user_data) - return None - - return UserProfile( - user_id = user_data["user_id"], - username = user_data["username"], - profile_image_url = user_data.get("profile_image_url", ""), - organization_id = user_data.get("organization", {}).get("organization_id"), - subscriptions = user_data.get("subscriptions", []) + self._user_profile = None + check_token_url = "{}/check-token".format(self._settings.OAUTH_SERVER_URL) + Logger.log("d", "Checking the access token for [%s]", check_token_url) + headers = { + "Authorization": f"Bearer {access_token}" + } + HttpRequestManager.getInstance().get( + check_token_url, + headers_dict = headers, + callback = lambda reply: self._parseUserProfile(reply, callback) ) + def _parseUserProfile(self, reply: QNetworkReply, callback: Optional[Callable[[UserProfile], None]]) -> None: + """ + Parses the user profile from a reply to /check-token. + + If the response is valid, the callback will be called to return the user profile to the caller. + :param reply: A network reply to a request to the /check-token URL. + :param callback: A function to call once a user profile was successfully obtained. + """ + if reply.error() != QNetworkReply.NetworkError.NoError: + Logger.warning(f"Could not access account information. QNetworkError {reply.errorString()}") + return + + profile_data = HttpRequestManager.getInstance().readJSON(reply) + if profile_data is None or "data" not in profile_data: + Logger.warning("Could not parse user data from token.") + return + profile_data = profile_data["data"] + + required_fields = {"user_id", "username"} + if "user_id" not in profile_data or "username" not in profile_data: + Logger.warning(f"User data missing required field(s): {required_fields - set(profile_data.keys())}") + return + + callback(UserProfile( + user_id = profile_data["user_id"], + username = profile_data["username"], + profile_image_url = profile_data.get("profile_image_url", ""), + organization_id = profile_data.get("organization", {}).get("organization_id"), + subscriptions = profile_data.get("subscriptions", []) + )) + @staticmethod def generateVerificationCode(code_length: int = 32) -> str: """Generate a verification code of arbitrary length. From aff0764c1da05642a6f1a566a93d786f743d6a14 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Thu, 18 Nov 2021 17:19:05 +0100 Subject: [PATCH 03/40] Add failed callback for when the request fails Maybe we should add a parameter to this to respond with an error message. But maybe later. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 229e7a3f14..841bb66990 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -121,13 +121,14 @@ class AuthorizationHelpers: self._request_lock.release() return - def checkToken(self, access_token: str, callback: Optional[Callable[[UserProfile], None]] = None) -> None: + def checkToken(self, access_token: str, success_callback: Optional[Callable[[UserProfile], None]] = None, failed_callback: Optional[Callable[[], None]] = None) -> None: """Calls the authentication API endpoint to get the token data. The API is called asynchronously. When a response is given, the callback is called with the user's profile. :param access_token: The encoded JWT token. - :param callback: When a response is given, this function will be called with a user profile. If None, there will - not be a callback. If the token failed to give/parse a user profile, the callback will not be called either. + :param success_callback: When a response is given, this function will be called with a user profile. If None, + there will not be a callback. + :param failed_callback: When the request failed or the response didn't parse, this function will be called. """ self._user_profile = None check_token_url = "{}/check-token".format(self._settings.OAUTH_SERVER_URL) @@ -138,33 +139,38 @@ class AuthorizationHelpers: HttpRequestManager.getInstance().get( check_token_url, headers_dict = headers, - callback = lambda reply: self._parseUserProfile(reply, callback) + callback = lambda reply: self._parseUserProfile(reply, success_callback, failed_callback), + error_callback = lambda _: failed_callback() ) - def _parseUserProfile(self, reply: QNetworkReply, callback: Optional[Callable[[UserProfile], None]]) -> None: + def _parseUserProfile(self, reply: QNetworkReply, success_callback: Optional[Callable[[UserProfile], None]], failed_callback: Optional[Callable[[], None]] = None) -> None: """ Parses the user profile from a reply to /check-token. If the response is valid, the callback will be called to return the user profile to the caller. :param reply: A network reply to a request to the /check-token URL. - :param callback: A function to call once a user profile was successfully obtained. + :param success_callback: A function to call once a user profile was successfully obtained. + :param failed_callback: A function to call if parsing the profile failed. """ if reply.error() != QNetworkReply.NetworkError.NoError: Logger.warning(f"Could not access account information. QNetworkError {reply.errorString()}") + failed_callback() return profile_data = HttpRequestManager.getInstance().readJSON(reply) if profile_data is None or "data" not in profile_data: Logger.warning("Could not parse user data from token.") + failed_callback() return profile_data = profile_data["data"] required_fields = {"user_id", "username"} if "user_id" not in profile_data or "username" not in profile_data: Logger.warning(f"User data missing required field(s): {required_fields - set(profile_data.keys())}") + failed_callback() return - callback(UserProfile( + success_callback(UserProfile( user_id = profile_data["user_id"], username = profile_data["username"], profile_image_url = profile_data.get("profile_image_url", ""), From 920d9b9d44858e15160d4824a807747142f7acbe Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Thu, 18 Nov 2021 17:33:39 +0100 Subject: [PATCH 04/40] Make _parseJWT asynchronous as well Now it calls checkToken correctly. However now this _parseJWT is not called correctly since there are things calling this one in an attempt to be synchronous again. There is the additional issue that we can't call getAccessTokenUsingRefreshToken synchronously now, because this runs on the main thread and it will then block the main thread until the request is made, which is never because the request is also done on the main thread via Qt's event loop. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 62 ++++++++++++++--------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 291845fd78..bf853ed6f1 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -3,7 +3,7 @@ import json from datetime import datetime, timedelta -from typing import Optional, TYPE_CHECKING, Dict +from typing import Callable, Dict, Optional, TYPE_CHECKING from urllib.parse import urlencode, quote_plus import requests.exceptions @@ -89,42 +89,42 @@ class AuthorizationService: return self._user_profile - def _parseJWT(self) -> Optional["UserProfile"]: - """Tries to parse the JWT (JSON Web Token) data, which it does if all the needed data is there. - - :return: UserProfile if it was able to parse, None otherwise. + def _parseJWT(self, callback: Callable[[Optional[UserProfile]], None]) -> None: + """ + Tries to parse the JWT (JSON Web Token) data, which it does if all the needed data is there. + :param callback: A function to call asynchronously once the user profile has been obtained. It will be called + with `None` if it failed to obtain a user profile. """ if not self._auth_data or self._auth_data.access_token is None: # If no auth data exists, we should always log in again. - Logger.log("d", "There was no auth data or access token") - return None + Logger.debug("There was no auth data or access token") + callback(None) - try: - user_data = self._auth_helpers.parseJWT(self._auth_data.access_token) - except AttributeError: - # THis might seem a bit double, but we get crash reports about this (CURA-2N2 in sentry) - Logger.log("d", "There was no auth data or access token") - return None + # When we checked the token we may get a user profile. This callback checks if that is a valid one and tries to refresh the token if it's not. + def check_user_profile(user_profile): + if user_profile: + # If the profile was found, we call it back immediately. + callback(user_profile) + return + # The JWT was expired or invalid and we should request a new one. + if self._auth_data.refresh_token is None: + Logger.warning("There was no refresh token in the auth data.") + callback(None) + return + self._auth_data = self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token) # TODO: Blocks main thread, preventing HttpRequestManager from functioning? + if not self._auth_data or self._auth_data.access_token is None: + Logger.warning("Unable to use the refresh token to get a new access token.") + callback(None) + return + # Ensure it gets stored as otherwise we only have it in memory. The stored refresh token has been deleted + # from the server already. Do not store the auth_data if we could not get new auth_data (e.g. due to a + # network error), since this would cause an infinite loop trying to get new auth-data. + if self._auth_data.success: + self._storeAuthData(self._auth_data) + self._auth_helpers.checkToken(self._auth_data.access_token, callback, lambda: callback(None)) - if user_data: - # If the profile was found, we return it immediately. - return user_data - # The JWT was expired or invalid and we should request a new one. - if self._auth_data.refresh_token is None: - Logger.log("w", "There was no refresh token in the auth data.") - return None - self._auth_data = self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token) - if not self._auth_data or self._auth_data.access_token is None: - Logger.log("w", "Unable to use the refresh token to get a new access token.") - # The token could not be refreshed using the refresh token. We should login again. - return None - # Ensure it gets stored as otherwise we only have it in memory. The stored refresh token has been deleted - # from the server already. Do not store the auth_data if we could not get new auth_data (eg due to a - # network error), since this would cause an infinite loop trying to get new auth-data - if self._auth_data.success: - self._storeAuthData(self._auth_data) - return self._auth_helpers.parseJWT(self._auth_data.access_token) + self._auth_helpers.checkToken(self._auth_data.access_token, check_user_profile) def getAccessToken(self) -> Optional[str]: """Get the access token as provided by the response data.""" From 591a2f89b8490d0397287d1f810c62fcc3e4f169 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 15:29:29 +0100 Subject: [PATCH 05/40] Make getUserProfile return profile asynchronously All via callbacks. Quite a mess. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 42 ++++++++++++++--------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index bf853ed6f1..77f53f208a 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -62,34 +62,34 @@ class AuthorizationService: if self._preferences: self._preferences.addPreference(self._settings.AUTH_DATA_PREFERENCE_KEY, "{}") - def getUserProfile(self) -> Optional["UserProfile"]: - """Get the user profile as obtained from the JWT (JSON Web Token). + def getUserProfile(self, callback: Callable[["UserProfile"], None]) -> None: + """ + Get the user profile as obtained from the JWT (JSON Web Token). - If the JWT is not yet parsed, calling this will take care of that. - - :return: UserProfile if a user is logged in, None otherwise. + If the JWT is not yet checked and parsed, calling this will take care of that. + :param callback: Once the user profile is obtained, this function will be called with the given user profile. If + the profile fails to be obtained, this will never be called. See also: :py:method:`cura.OAuth2.AuthorizationService.AuthorizationService._parseJWT` """ + if self._user_profile: + # We already obtained the profile. No need to make another request for it. + callback(self._user_profile) + return - if not self._user_profile: - # If no user profile was stored locally, we try to get it from JWT. - try: - self._user_profile = self._parseJWT() - except requests.exceptions.ConnectionError: - # Unable to get connection, can't login. - Logger.logException("w", "Unable to validate user data with the remote server.") - return None + # If no user profile was stored locally, we try to get it from JWT. + def store_profile(profile: Optional["UserProfile"]): + if profile is not None: + self._user_profile = profile + callback(profile) + elif self._auth_data: + # If there is no user profile from the JWT, we have to log in again. + Logger.warning("The user profile could not be loaded. The user must log in again!") + self.deleteAuthData() - if not self._user_profile and self._auth_data: - # If there is still no user profile from the JWT, we have to log in again. - Logger.log("w", "The user profile could not be loaded. The user must log in again!") - self.deleteAuthData() - return None + self._parseJWT(callback = store_profile) - return self._user_profile - - def _parseJWT(self, callback: Callable[[Optional[UserProfile]], None]) -> None: + def _parseJWT(self, callback: Callable[[Optional["UserProfile"]], None]) -> None: """ Tries to parse the JWT (JSON Web Token) data, which it does if all the needed data is there. :param callback: A function to call asynchronously once the user profile has been obtained. It will be called From 92437a920d6d6ee0ee107efa43f1899af75b7f2b Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 16:02:25 +0100 Subject: [PATCH 06/40] Store account information asynchronously when completing request And make the PyQt properties listen to the signal of completion. Contributes to issue CURA-8539. --- cura/API/Account.py | 43 ++++++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 19 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index 2d4b204333..9a4dceb3c9 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -1,15 +1,15 @@ -# Copyright (c) 2018 Ultimaker B.V. +# Copyright (c) 2021 Ultimaker B.V. # Cura is released under the terms of the LGPLv3 or higher. -from datetime import datetime -from typing import Any, Optional, Dict, TYPE_CHECKING, Callable +from datetime import datetime from PyQt5.QtCore import QObject, pyqtSignal, pyqtSlot, pyqtProperty, QTimer, Q_ENUMS +from typing import Any, Optional, Dict, TYPE_CHECKING, Callable from UM.Logger import Logger from UM.Message import Message from UM.i18n import i18nCatalog from cura.OAuth2.AuthorizationService import AuthorizationService -from cura.OAuth2.Models import OAuth2Settings +from cura.OAuth2.Models import OAuth2Settings, UserProfile from cura.UltimakerCloud import UltimakerCloudConstants if TYPE_CHECKING: @@ -46,6 +46,9 @@ class Account(QObject): loginStateChanged = pyqtSignal(bool) """Signal emitted when user logged in or out""" + userProfileChanged = pyqtSignal() + """Signal emitted when new account information is available.""" + additionalRightsChanged = pyqtSignal("QVariantMap") """Signal emitted when a users additional rights change""" @@ -73,6 +76,7 @@ class Account(QObject): self._error_message = None # type: Optional[Message] self._logged_in = False + self._user_profile = None self._additional_rights: Dict[str, Any] = {} self._sync_state = SyncState.IDLE self._manual_sync_enabled = False @@ -196,12 +200,17 @@ class Account(QObject): self._logged_in = logged_in self.loginStateChanged.emit(logged_in) if logged_in: + self._authorization_service.getUserProfile(self._onProfileChanged) self._setManualSyncEnabled(False) self._sync() else: if self._update_timer.isActive(): self._update_timer.stop() + def _onProfileChanged(self, profile: UserProfile): + self._user_profile = profile + self.userProfileChanged.emit(profile) + def _sync(self) -> None: """Signals all sync services to start syncing @@ -243,32 +252,28 @@ class Account(QObject): return self._authorization_service.startAuthorizationFlow(force_logout_before_login) - @pyqtProperty(str, notify=loginStateChanged) + @pyqtProperty(str, notify = userProfileChanged) def userName(self): - user_profile = self._authorization_service.getUserProfile() - if not user_profile: - return None - return user_profile.username + if not self._user_profile: + return "" + return self._user_profile.username - @pyqtProperty(str, notify = loginStateChanged) + @pyqtProperty(str, notify = userProfileChanged) def profileImageUrl(self): - user_profile = self._authorization_service.getUserProfile() - if not user_profile: - return None - return user_profile.profile_image_url + if not self._user_profile: + return "" + return self._user_profile.profile_image_url @pyqtProperty(str, notify=accessTokenChanged) def accessToken(self) -> Optional[str]: return self._authorization_service.getAccessToken() - @pyqtProperty("QVariantMap", notify = loginStateChanged) + @pyqtProperty("QVariantMap", notify = userProfileChanged) def userProfile(self) -> Optional[Dict[str, Optional[str]]]: """None if no user is logged in otherwise the logged in user as a dict containing containing user_id, username and profile_image_url """ - - user_profile = self._authorization_service.getUserProfile() - if not user_profile: + if not self._user_profile: return None - return user_profile.__dict__ + return self._user_profile.__dict__ @pyqtProperty(str, notify=lastSyncDateTimeChanged) def lastSyncDateTime(self) -> str: From bfb8440a04db95d3aa4ef2ceb6d8302c1c044bb7 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 16:20:00 +0100 Subject: [PATCH 07/40] Get user profile asynchronously when restoring from preferences Do not stop the start-up process for it. Let the callback handle the updating if necessary. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 36 +++++++++++++++-------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 77f53f208a..3cb85c009e 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -62,13 +62,13 @@ class AuthorizationService: if self._preferences: self._preferences.addPreference(self._settings.AUTH_DATA_PREFERENCE_KEY, "{}") - def getUserProfile(self, callback: Callable[["UserProfile"], None]) -> None: + def getUserProfile(self, callback: Callable[[Optional["UserProfile"]], None] = None) -> None: """ Get the user profile as obtained from the JWT (JSON Web Token). If the JWT is not yet checked and parsed, calling this will take care of that. :param callback: Once the user profile is obtained, this function will be called with the given user profile. If - the profile fails to be obtained, this will never be called. + the profile fails to be obtained, this function will be called with None. See also: :py:method:`cura.OAuth2.AuthorizationService.AuthorizationService._parseJWT` """ @@ -86,6 +86,7 @@ class AuthorizationService: # If there is no user profile from the JWT, we have to log in again. Logger.warning("The user profile could not be loaded. The user must log in again!") self.deleteAuthData() + callback(None) self._parseJWT(callback = store_profile) @@ -244,21 +245,22 @@ class AuthorizationService: preferences_data = json.loads(self._preferences.getValue(self._settings.AUTH_DATA_PREFERENCE_KEY)) if preferences_data: self._auth_data = AuthenticationResponse(**preferences_data) - # Also check if we can actually get the user profile information. - user_profile = self.getUserProfile() - if user_profile is not None: - self.onAuthStateChanged.emit(logged_in = True) - Logger.log("d", "Auth data was successfully loaded") - else: - if self._unable_to_get_data_message is not None: - self._unable_to_get_data_message.hide() - self._unable_to_get_data_message = Message(i18n_catalog.i18nc("@info", - "Unable to reach the Ultimaker account server."), - title = i18n_catalog.i18nc("@info:title", "Warning"), - message_type = Message.MessageType.ERROR) - Logger.log("w", "Unable to load auth data from preferences") - self._unable_to_get_data_message.show() + # Also check if we can actually get the user profile information. + def callback(profile: Optional[UserProfile]): + if profile is not None: + self.onAuthStateChanged.emit(logged_in = True) + Logger.debug("Auth data was successfully loaded") + else: + if self._unable_to_get_data_message is not None: + self._unable_to_get_data_message.show() + else: + self._unable_to_get_data_message = Message(i18n_catalog.i18nc("@info", "Unable to reach the Ultimaker account server."), + title = i18n_catalog.i18nc("@info:title", "Log-in failed"), + message_type = Message.MessageType.ERROR) + Logger.warning("Unable to get user profile using auth data from preferences.") + self._unable_to_get_data_message.show() + self.getUserProfile(callback) except (ValueError, TypeError): Logger.logException("w", "Could not load auth data from preferences") @@ -272,7 +274,7 @@ class AuthorizationService: self._auth_data = auth_data if auth_data: - self._user_profile = self.getUserProfile() + self.getUserProfile() self._preferences.setValue(self._settings.AUTH_DATA_PREFERENCE_KEY, json.dumps(auth_data.dump())) else: Logger.log("d", "Clearing the user profile") From a5202b61d2a82de1c8985417d983405ec3f80d94 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 16:22:41 +0100 Subject: [PATCH 08/40] Don't emit with new profile The signal can't accept it because it's not a QObject. Contributes to issue CURA-8539. --- cura/API/Account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index 9a4dceb3c9..bf8a883c1a 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -209,7 +209,7 @@ class Account(QObject): def _onProfileChanged(self, profile: UserProfile): self._user_profile = profile - self.userProfileChanged.emit(profile) + self.userProfileChanged.emit() def _sync(self) -> None: """Signals all sync services to start syncing From acbbf83510a8daee4e0e42317cd1901f1f8d4220 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 16:23:48 +0100 Subject: [PATCH 09/40] Deal with absence of callback function It may be None, so then we don't need to call back. The consumer may just take it from self._user_profile. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 3cb85c009e..c49055a82c 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -74,19 +74,22 @@ class AuthorizationService: """ if self._user_profile: # We already obtained the profile. No need to make another request for it. - callback(self._user_profile) + if callback is not None: + callback(self._user_profile) return # If no user profile was stored locally, we try to get it from JWT. def store_profile(profile: Optional["UserProfile"]): if profile is not None: self._user_profile = profile - callback(profile) + if callback is not None: + callback(profile) elif self._auth_data: # If there is no user profile from the JWT, we have to log in again. Logger.warning("The user profile could not be loaded. The user must log in again!") self.deleteAuthData() - callback(None) + if callback is not None: + callback(None) self._parseJWT(callback = store_profile) From 9b55ae6dda094e1d5568f2ff60b36e78c3e51f49 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 16:25:50 +0100 Subject: [PATCH 10/40] Remove unused imports of requests This was the objective, really. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 1 - cura/OAuth2/AuthorizationService.py | 1 - 2 files changed, 2 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 841bb66990..04dccaa226 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -8,7 +8,6 @@ from PyQt5.QtNetwork import QNetworkReply import secrets from threading import Lock from typing import Callable, Optional -import requests import urllib.parse from UM.i18n import i18nCatalog diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index c49055a82c..1da94094f1 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -6,7 +6,6 @@ from datetime import datetime, timedelta from typing import Callable, Dict, Optional, TYPE_CHECKING from urllib.parse import urlencode, quote_plus -import requests.exceptions from PyQt5.QtCore import QUrl from PyQt5.QtGui import QDesktopServices From 7091c5f3aa96fd91d5753247a52959f9cf8a2b8f Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 16:55:45 +0100 Subject: [PATCH 11/40] Make getAuthenticationTokenUsingXYZ asynchronous As a result, the local webserver now needs to synchronise that with a lock. Otherwise the do_GET function would no longer block, and wouldn't properly be able to return the correct redirect URL. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 55 +++++++--------------- cura/OAuth2/AuthorizationRequestHandler.py | 19 ++++++-- cura/OAuth2/AuthorizationService.py | 2 +- 3 files changed, 33 insertions(+), 43 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 04dccaa226..516fcde932 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -6,15 +6,14 @@ from datetime import datetime from hashlib import sha512 from PyQt5.QtNetwork import QNetworkReply import secrets -from threading import Lock from typing import Callable, Optional import urllib.parse +from cura.OAuth2.Models import AuthenticationResponse, UserProfile, OAuth2Settings from UM.i18n import i18nCatalog from UM.Logger import Logger from UM.TaskManagement.HttpRequestManager import HttpRequestManager # To download log-in tokens. -from cura.OAuth2.Models import AuthenticationResponse, UserProfile, OAuth2Settings catalog = i18nCatalog("cura") TOKEN_TIMESTAMP_FORMAT = "%Y-%m-%d %H:%M:%S" @@ -25,8 +24,6 @@ class AuthorizationHelpers: def __init__(self, settings: "OAuth2Settings") -> None: self._settings = settings self._token_url = "{}/token".format(self._settings.OAUTH_SERVER_URL) - self._request_lock = Lock() - self._auth_response = None # type: Optional[AuthenticationResponse] @property def settings(self) -> "OAuth2Settings": @@ -34,14 +31,13 @@ class AuthorizationHelpers: return self._settings - def getAccessTokenUsingAuthorizationCode(self, authorization_code: str, verification_code: str) -> "AuthenticationResponse": - """Request the access token from the authorization server. - + def getAccessTokenUsingAuthorizationCode(self, authorization_code: str, verification_code: str, callback: Callable[[AuthenticationResponse], None]) -> None: + """ + Request the access token from the authorization server. :param authorization_code: The authorization code from the 1st step. :param verification_code: The verification code needed for the PKCE extension. - :return: An AuthenticationResponse object. + :param callback: Once the token has been obtained, this function will be called with the response. """ - data = { "client_id": self._settings.CLIENT_ID if self._settings.CLIENT_ID is not None else "", "redirect_uri": self._settings.CALLBACK_URL if self._settings.CALLBACK_URL is not None else "", @@ -51,26 +47,19 @@ class AuthorizationHelpers: "scope": self._settings.CLIENT_SCOPES if self._settings.CLIENT_SCOPES is not None else "", } headers = {"Content-type": "application/x-www-form-urlencoded"} - self._request_lock.acquire() HttpRequestManager.getInstance().post( self._token_url, data = urllib.parse.urlencode(data).encode("UTF-8"), headers_dict = headers, - callback = self.parseTokenResponse + callback = lambda response: self.parseTokenResponse(response, callback) ) - self._request_lock.acquire(timeout = 60) # Block until the request is completed. 1 minute timeout. - response = self._auth_response - self._auth_response = None - self._request_lock.release() - return response - def getAccessTokenUsingRefreshToken(self, refresh_token: str) -> "AuthenticationResponse": - """Request the access token from the authorization server using a refresh token. - - :param refresh_token: - :return: An AuthenticationResponse object. + def getAccessTokenUsingRefreshToken(self, refresh_token: str, callback: Callable[[AuthenticationResponse], None]) -> None: + """ + Request the access token from the authorization server using a refresh token. + :param refresh_token: A long-lived token used to refresh the authentication token. + :param callback: Once the token has been obtained, this function will be called with the response. """ - Logger.log("d", "Refreshing the access token for [%s]", self._settings.OAUTH_SERVER_URL) data = { "client_id": self._settings.CLIENT_ID if self._settings.CLIENT_ID is not None else "", @@ -80,20 +69,14 @@ class AuthorizationHelpers: "scope": self._settings.CLIENT_SCOPES if self._settings.CLIENT_SCOPES is not None else "", } headers = {"Content-type": "application/x-www-form-urlencoded"} - self._request_lock.acquire() HttpRequestManager.getInstance().post( self._token_url, data = urllib.parse.urlencode(data).encode("UTF-8"), headers_dict = headers, - callback = self.parseTokenResponse + callback = lambda response: self.parseTokenResponse(response, callback) ) - self._request_lock.acquire(timeout = 60) # Block until the request is completed. 1 minute timeout. - response = self._auth_response - self._auth_response = None - self._request_lock.release() - return response - def parseTokenResponse(self, token_response: QNetworkReply) -> None: + def parseTokenResponse(self, token_response: QNetworkReply, callback: Callable[[AuthenticationResponse], None]) -> None: """Parse the token response from the authorization server into an AuthenticationResponse object. :param token_response: The JSON string data response from the authorization server. @@ -101,23 +84,20 @@ class AuthorizationHelpers: """ token_data = HttpRequestManager.readJSON(token_response) if not token_data: - self._auth_response = AuthenticationResponse(success = False, err_message = catalog.i18nc("@message", "Could not read response.")) - self._request_lock.release() + callback(AuthenticationResponse(success = False, err_message = catalog.i18nc("@message", "Could not read response."))) return if token_response.error() != QNetworkReply.NetworkError.NoError: - self._auth_response = AuthenticationResponse(success = False, err_message = token_data["error_description"]) - self._request_lock.release() + callback(AuthenticationResponse(success = False, err_message = token_data["error_description"])) return - self._auth_response = AuthenticationResponse(success = True, + callback(AuthenticationResponse(success = True, token_type = token_data["token_type"], access_token = token_data["access_token"], refresh_token = token_data["refresh_token"], expires_in = token_data["expires_in"], scope = token_data["scope"], - received_at = datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT)) - self._request_lock.release() + received_at = datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT))) return def checkToken(self, access_token: str, success_callback: Optional[Callable[[UserProfile], None]] = None, failed_callback: Optional[Callable[[], None]] = None) -> None: @@ -129,7 +109,6 @@ class AuthorizationHelpers: there will not be a callback. :param failed_callback: When the request failed or the response didn't parse, this function will be called. """ - self._user_profile = None check_token_url = "{}/check-token".format(self._settings.OAUTH_SERVER_URL) Logger.log("d", "Checking the access token for [%s]", check_token_url) headers = { diff --git a/cura/OAuth2/AuthorizationRequestHandler.py b/cura/OAuth2/AuthorizationRequestHandler.py index c7ce9b6faf..ff01969c50 100644 --- a/cura/OAuth2/AuthorizationRequestHandler.py +++ b/cura/OAuth2/AuthorizationRequestHandler.py @@ -2,6 +2,7 @@ # Cura is released under the terms of the LGPLv3 or higher. from http.server import BaseHTTPRequestHandler +from threading import Lock # To turn an asynchronous call synchronous. from typing import Optional, Callable, Tuple, Dict, Any, List, TYPE_CHECKING from urllib.parse import parse_qs, urlparse @@ -70,13 +71,23 @@ class AuthorizationRequestHandler(BaseHTTPRequestHandler): if state != self.state: token_response = AuthenticationResponse( success = False, - err_message=catalog.i18nc("@message", - "The provided state is not correct.") + err_message = catalog.i18nc("@message", "The provided state is not correct.") ) elif code and self.authorization_helpers is not None and self.verification_code is not None: + token_response = AuthenticationResponse( + success = False, + err_message = catalog.i18nc("@message", "Timeout when authenticating with the account server.") + ) # If the code was returned we get the access token. - token_response = self.authorization_helpers.getAccessTokenUsingAuthorizationCode( - code, self.verification_code) + lock = Lock() + lock.acquire() + + def callback(response: AuthenticationResponse) -> None: + nonlocal token_response + token_response = response + lock.release() + self.authorization_helpers.getAccessTokenUsingAuthorizationCode(code, self.verification_code, callback) + lock.acquire(timeout = 60) # Block thread until request is completed (which releases the lock). If not acquired, the timeout message stays. elif self._queryGet(query, "error_code") == "user_denied": # Otherwise we show an error message (probably the user clicked "Deny" in the auth dialog). diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 1da94094f1..cf1b306657 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -249,7 +249,7 @@ class AuthorizationService: self._auth_data = AuthenticationResponse(**preferences_data) # Also check if we can actually get the user profile information. - def callback(profile: Optional[UserProfile]): + def callback(profile: Optional["UserProfile"]): if profile is not None: self.onAuthStateChanged.emit(logged_in = True) Logger.debug("Auth data was successfully loaded") From 9895015235d6195efb4a73833fad7998a2818160 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 17:04:37 +0100 Subject: [PATCH 12/40] Call getAccessTokenUsingRefreshToken asynchronously Getting into nested inline functions into inline functions here. Let's see what the reviewer thinks of all this! Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 42 ++++++++++++++++------------- 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index cf1b306657..797ebe2c38 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -115,17 +115,20 @@ class AuthorizationService: Logger.warning("There was no refresh token in the auth data.") callback(None) return - self._auth_data = self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token) # TODO: Blocks main thread, preventing HttpRequestManager from functioning? - if not self._auth_data or self._auth_data.access_token is None: - Logger.warning("Unable to use the refresh token to get a new access token.") - callback(None) - return - # Ensure it gets stored as otherwise we only have it in memory. The stored refresh token has been deleted - # from the server already. Do not store the auth_data if we could not get new auth_data (e.g. due to a - # network error), since this would cause an infinite loop trying to get new auth-data. - if self._auth_data.success: - self._storeAuthData(self._auth_data) - self._auth_helpers.checkToken(self._auth_data.access_token, callback, lambda: callback(None)) + + def process_auth_data(auth_data: AuthenticationResponse): + if auth_data.access_token is None: + Logger.warning("Unable to use the refresh token to get a new access token.") + callback(None) + return + # Ensure it gets stored as otherwise we only have it in memory. The stored refresh token has been + # deleted from the server already. Do not store the auth_data if we could not get new auth_data (e.g. + # due to a network error), since this would cause an infinite loop trying to get new auth-data. + if auth_data.success: + self._storeAuthData(auth_data) + self._auth_helpers.checkToken(auth_data.access_token, callback, lambda: callback(None)) + + self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token, process_auth_data) self._auth_helpers.checkToken(self._auth_data.access_token, check_user_profile) @@ -152,13 +155,16 @@ class AuthorizationService: if self._auth_data is None or self._auth_data.refresh_token is None: Logger.log("w", "Unable to refresh access token, since there is no refresh token.") return - response = self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token) - if response.success: - self._storeAuthData(response) - self.onAuthStateChanged.emit(logged_in = True) - else: - Logger.log("w", "Failed to get a new access token from the server.") - self.onAuthStateChanged.emit(logged_in = False) + + def process_auth_data(response: AuthenticationResponse): + if response.success: + self._storeAuthData(response) + self.onAuthStateChanged.emit(logged_in = True) + else: + Logger.warning("Failed to get a new access token from the server.") + self.onAuthStateChanged.emit(logged_in = False) + + self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token, process_auth_data) def deleteAuthData(self) -> None: """Delete the authentication data that we have stored locally (eg; logout)""" From ffb891fb69ef6b06dffaa46f25dccef048b5f7aa Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Fri, 19 Nov 2021 17:09:33 +0100 Subject: [PATCH 13/40] Fix call to failed_callback It provides 2 arguments (reply and error) which we both ignore and proceed to call failed_callback. And failed_callback may also be None in which case we don't call anything. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 516fcde932..d84da46c5f 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -118,7 +118,7 @@ class AuthorizationHelpers: check_token_url, headers_dict = headers, callback = lambda reply: self._parseUserProfile(reply, success_callback, failed_callback), - error_callback = lambda _: failed_callback() + error_callback = lambda _, _2: failed_callback() if failed_callback is not None else None ) def _parseUserProfile(self, reply: QNetworkReply, success_callback: Optional[Callable[[UserProfile], None]], failed_callback: Optional[Callable[[], None]] = None) -> None: From 1636cca601a993619f52f7a713dbc22cf5051f49 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 10:34:39 +0100 Subject: [PATCH 14/40] Add missing return We're calling back that there is no auth data, so we should stop here and not try to obtain a user profile. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 1 + 1 file changed, 1 insertion(+) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 797ebe2c38..c9f60ddf2d 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -103,6 +103,7 @@ class AuthorizationService: # If no auth data exists, we should always log in again. Logger.debug("There was no auth data or access token") callback(None) + return # When we checked the token we may get a user profile. This callback checks if that is a valid one and tries to refresh the token if it's not. def check_user_profile(user_profile): From 30d19844f2cd24c0b1640dfa66837c82634310ce Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 10:45:01 +0100 Subject: [PATCH 15/40] Always callback if there is a callback, even if no auth data Because not having auth data is a reason to return no profile too. Otherwise the other side might wait for a response for a long time. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index c9f60ddf2d..fb2ba40c71 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -89,6 +89,9 @@ class AuthorizationService: self.deleteAuthData() if callback is not None: callback(None) + else: + if callback is not None: + callback(None) self._parseJWT(callback = store_profile) From 9dd251975d2fbf339021976cf9f202c8a545c13c Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 10:48:45 +0100 Subject: [PATCH 16/40] Test for result of callback with a callable In this case the callback should get called immediately (no wait) so it is safe to test for this callback, albeit slightly implementation-defined. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index 2c039b296a..24cfe50921 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -1,5 +1,5 @@ from datetime import datetime -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, Mock, patch import requests @@ -53,7 +53,11 @@ def test_cleanAuthService() -> None: # Ensure that when setting up an AuthorizationService, no data is set. authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() - assert authorization_service.getUserProfile() is None + + mock_callback = Mock() + authorization_service.getUserProfile(mock_callback) + mock_callback.assert_called_once_with(None) + assert authorization_service.getAccessToken() is None From f0cbdeb90311794936463b7aebf37aeab803bdc0 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 10:50:06 +0100 Subject: [PATCH 17/40] Use docstring format for this documentation Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index 24cfe50921..226e7437a3 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -50,7 +50,9 @@ MALFORMED_AUTH_RESPONSE = AuthenticationResponse(success=False) def test_cleanAuthService() -> None: - # Ensure that when setting up an AuthorizationService, no data is set. + """ + Ensure that when setting up an AuthorizationService, no data is set. + """ authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() From c36863da56260ab82dbd4bd6aadff12d23da8c5e Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 11:59:23 +0100 Subject: [PATCH 18/40] Only call failed_callback if provided Otherwise we'd crash because NoneType can't be called. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index d84da46c5f..28b07cf17c 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -132,20 +132,23 @@ class AuthorizationHelpers: """ if reply.error() != QNetworkReply.NetworkError.NoError: Logger.warning(f"Could not access account information. QNetworkError {reply.errorString()}") - failed_callback() + if failed_callback is not None: + failed_callback() return profile_data = HttpRequestManager.getInstance().readJSON(reply) if profile_data is None or "data" not in profile_data: Logger.warning("Could not parse user data from token.") - failed_callback() + if failed_callback is not None: + failed_callback() return profile_data = profile_data["data"] required_fields = {"user_id", "username"} if "user_id" not in profile_data or "username" not in profile_data: Logger.warning(f"User data missing required field(s): {required_fields - set(profile_data.keys())}") - failed_callback() + if failed_callback is not None: + failed_callback() return success_callback(UserProfile( From c8aff57bfe6844379a1caad5d866044be0981975 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 12:11:45 +0100 Subject: [PATCH 19/40] Actually mock a reply from the auth server The reply is not really relevant. The reply is mocked through readJSON. So it turns out that so far, our tests have been making actual requests to the authentication server, and depended on it being online. Not good. Mock those external dependencies! Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index 226e7437a3..a57dd87991 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -4,6 +4,7 @@ from unittest.mock import MagicMock, Mock, patch import requests from PyQt5.QtGui import QDesktopServices +from PyQt5.QtNetwork import QNetworkReply from UM.Preferences import Preferences from cura.OAuth2.AuthorizationHelpers import AuthorizationHelpers, TOKEN_TIMESTAMP_FORMAT @@ -76,10 +77,32 @@ def test_refreshAccessTokenSuccess(): def test__parseJWTNoRefreshToken(): + """ + Tests parsing the user profile if there is no refresh token stored, but there is a normal authentication token. + + The request for the user profile using the authentication token should still work normally. + """ authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) with patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()): authorization_service._storeAuthData(NO_REFRESH_AUTH_RESPONSE) - assert authorization_service._parseJWT() is None + + mock_callback = Mock() # To log the final profile response. + mock_reply = Mock() # The user profile that the service should respond with. + mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.NoError) + + http_mock = Mock() + def mock_get(url, headers_dict, callback, error_callback): + nonlocal mock_reply + callback(mock_reply) + http_mock.get = mock_get + http_mock.readJSON = Mock(return_value = {"data": {"user_id": "id_ego_or_superego", "username": "Ghostkeeper"}}) + + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + authorization_service._parseJWT(mock_callback) + mock_callback.assert_called_once() + profile_reply = mock_callback.call_args_list[0][0][0] + assert profile_reply.user_id == "id_ego_or_superego" + assert profile_reply.username == "Ghostkeeper" def test__parseJWTFailOnRefresh(): From 937d48a4e8f5bfe6791abacf2fe6049e3423a2f0 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 13:24:06 +0100 Subject: [PATCH 20/40] Add missing failed_callback If it fails to check the token, respond through the callback with None as well. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index fb2ba40c71..290fc5c651 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -134,7 +134,7 @@ class AuthorizationService: self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token, process_auth_data) - self._auth_helpers.checkToken(self._auth_data.access_token, check_user_profile) + self._auth_helpers.checkToken(self._auth_data.access_token, check_user_profile, lambda: callback(None)) def getAccessToken(self) -> Optional[str]: """Get the access token as provided by the response data.""" From 3b6ff15d603d15ca813cca204dbc0e2ebdb8a397 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 13:25:15 +0100 Subject: [PATCH 21/40] Rewrite test to mock HttpRequestManager with authentication failure That's the expected outcome of the premise of this test. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index a57dd87991..afcfb59091 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -63,7 +63,6 @@ def test_cleanAuthService() -> None: assert authorization_service.getAccessToken() is None - def test_refreshAccessTokenSuccess(): authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() @@ -75,7 +74,6 @@ def test_refreshAccessTokenSuccess(): authorization_service.refreshAccessToken() assert authorization_service.onAuthStateChanged.emit.called_with(True) - def test__parseJWTNoRefreshToken(): """ Tests parsing the user profile if there is no refresh token stored, but there is a normal authentication token. @@ -91,10 +89,7 @@ def test__parseJWTNoRefreshToken(): mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.NoError) http_mock = Mock() - def mock_get(url, headers_dict, callback, error_callback): - nonlocal mock_reply - callback(mock_reply) - http_mock.get = mock_get + http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) http_mock.readJSON = Mock(return_value = {"data": {"user_id": "id_ego_or_superego", "username": "Ghostkeeper"}}) with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): @@ -104,15 +99,23 @@ def test__parseJWTNoRefreshToken(): assert profile_reply.user_id == "id_ego_or_superego" assert profile_reply.username == "Ghostkeeper" - def test__parseJWTFailOnRefresh(): + """ + Tries to refresh the authentication token using an invalid refresh token. The request should fail. + """ authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) with patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()): authorization_service._storeAuthData(SUCCESSFUL_AUTH_RESPONSE) - with patch.object(AuthorizationHelpers, "getAccessTokenUsingRefreshToken", return_value=FAILED_AUTH_RESPONSE): - assert authorization_service._parseJWT() is None + mock_callback = Mock() # To log the final profile response. + mock_reply = Mock() # The response that the request should give, containing an error about it failing to authenticate. + mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.AuthenticationRequiredError) # The reply is 403: Authentication required, meaning the server responded with a "Can't do that, Dave". + http_mock = Mock() + http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + authorization_service._parseJWT(mock_callback) + mock_callback.assert_called_once_with(None) def test__parseJWTSucceedOnRefresh(): authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) @@ -125,7 +128,6 @@ def test__parseJWTSucceedOnRefresh(): authorization_service._parseJWT() mocked_parseJWT.assert_called_with("beep") - def test_initialize(): original_preference = MagicMock() initialize_preferences = MagicMock() @@ -134,7 +136,6 @@ def test_initialize(): initialize_preferences.addPreference.assert_called_once_with("test/auth_data", "{}") original_preference.addPreference.assert_not_called() - def test_refreshAccessTokenFailed(): authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() @@ -145,7 +146,6 @@ def test_refreshAccessTokenFailed(): authorization_service.refreshAccessToken() assert authorization_service.onAuthStateChanged.emit.called_with(False) - def test_refreshAccesTokenWithoutData(): authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() @@ -153,14 +153,12 @@ def test_refreshAccesTokenWithoutData(): authorization_service.refreshAccessToken() authorization_service.onAuthStateChanged.emit.assert_not_called() - def test_userProfileException(): authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() authorization_service._parseJWT = MagicMock(side_effect=requests.exceptions.ConnectionError) assert authorization_service.getUserProfile() is None - def test_failedLogin() -> None: authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.onAuthenticationError.emit = MagicMock() @@ -180,7 +178,6 @@ def test_failedLogin() -> None: assert authorization_service.getUserProfile() is None assert authorization_service.getAccessToken() is None - @patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()) def test_storeAuthData(get_user_profile) -> None: preferences = Preferences() @@ -199,7 +196,6 @@ def test_storeAuthData(get_user_profile) -> None: second_auth_service.loadAuthDataFromPreferences() assert second_auth_service.getAccessToken() == SUCCESSFUL_AUTH_RESPONSE.access_token - @patch.object(LocalAuthorizationServer, "stop") @patch.object(LocalAuthorizationServer, "start") @patch.object(QDesktopServices, "openUrl") @@ -217,7 +213,6 @@ def test_localAuthServer(QDesktopServices_openUrl, start_auth_server, stop_auth_ # Ensure that it stopped the server. assert stop_auth_server.call_count == 1 - def test_loginAndLogout() -> None: preferences = Preferences() authorization_service = AuthorizationService(OAUTH_SETTINGS, preferences) @@ -248,7 +243,6 @@ def test_loginAndLogout() -> None: # Ensure the data is gone after we logged out. assert preferences.getValue("test/auth_data") == "{}" - def test_wrongServerResponses() -> None: authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() @@ -256,7 +250,6 @@ def test_wrongServerResponses() -> None: authorization_service._onAuthStateChanged(MALFORMED_AUTH_RESPONSE) assert authorization_service.getUserProfile() is None - def test__generate_auth_url() -> None: preferences = Preferences() authorization_service = AuthorizationService(OAUTH_SETTINGS, preferences) From 70924f17aac63800fb2d1464123bec8732c3cc93 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 13:44:46 +0100 Subject: [PATCH 22/40] Mock HttpRequestManager getting the profile with refresh token I'm not sure the refresh token is actually used though. I might want to try to guarantee that. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index afcfb59091..3ac79b8917 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -81,7 +81,7 @@ def test__parseJWTNoRefreshToken(): The request for the user profile using the authentication token should still work normally. """ authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) - with patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()): + with patch.object(AuthorizationService, "getUserProfile", return_value = UserProfile()): authorization_service._storeAuthData(NO_REFRESH_AUTH_RESPONSE) mock_callback = Mock() # To log the final profile response. @@ -104,7 +104,7 @@ def test__parseJWTFailOnRefresh(): Tries to refresh the authentication token using an invalid refresh token. The request should fail. """ authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) - with patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()): + with patch.object(AuthorizationService, "getUserProfile", return_value = UserProfile()): authorization_service._storeAuthData(SUCCESSFUL_AUTH_RESPONSE) mock_callback = Mock() # To log the final profile response. @@ -118,15 +118,28 @@ def test__parseJWTFailOnRefresh(): mock_callback.assert_called_once_with(None) def test__parseJWTSucceedOnRefresh(): + """ + Tries to refresh the authentication token using a valid refresh token. The request should succeed. + """ authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() - with patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()): + with patch.object(AuthorizationService, "getUserProfile", return_value = UserProfile()): authorization_service._storeAuthData(SUCCESSFUL_AUTH_RESPONSE) - with patch.object(AuthorizationHelpers, "getAccessTokenUsingRefreshToken", return_value=SUCCESSFUL_AUTH_RESPONSE): - with patch.object(AuthorizationHelpers, "parseJWT", MagicMock(return_value = None)) as mocked_parseJWT: - authorization_service._parseJWT() - mocked_parseJWT.assert_called_with("beep") + mock_callback = Mock() # To log the final profile response. + mock_reply = Mock() # The response that the request should give, containing an error about it failing to authenticate. + mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.NoError) + http_mock = Mock() + http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) + http_mock.readJSON = Mock(return_value = {"data": {"user_id": "user_idea", "username": "Ghostkeeper"}}) + + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + authorization_service._parseJWT(mock_callback) + + mock_callback.assert_called_once() + profile_reply = mock_callback.call_args_list[0][0][0] + assert profile_reply.user_id == "user_idea" + assert profile_reply.username == "Ghostkeeper" def test_initialize(): original_preference = MagicMock() From e8f9e92f02a25994307c785b1cbe0f1cdc313856 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 14:26:15 +0100 Subject: [PATCH 23/40] Simulate an actual expired authentication token The test should trigger the refresh token to be used to get a new authentication token. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index 3ac79b8917..bc5dc21553 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -40,6 +40,14 @@ SUCCESSFUL_AUTH_RESPONSE = AuthenticationResponse( success = True ) +EXPIRED_AUTH_RESPONSE = AuthenticationResponse( + access_token = "expired", + refresh_token = "beep?", + received_at = datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT), + expires_in = 300, # 5 minutes should be more than enough for testing + success = True +) + NO_REFRESH_AUTH_RESPONSE = AuthenticationResponse( access_token = "beep", received_at = datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT), @@ -124,17 +132,27 @@ def test__parseJWTSucceedOnRefresh(): authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() with patch.object(AuthorizationService, "getUserProfile", return_value = UserProfile()): - authorization_service._storeAuthData(SUCCESSFUL_AUTH_RESPONSE) + authorization_service._storeAuthData(EXPIRED_AUTH_RESPONSE) mock_callback = Mock() # To log the final profile response. - mock_reply = Mock() # The response that the request should give, containing an error about it failing to authenticate. - mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.NoError) + mock_reply_success = Mock() # The reply should be a failure when using the expired access token, but succeed when using the refresh token. + mock_reply_success.error = Mock(return_value = QNetworkReply.NetworkError.NoError) + mock_reply_failure = Mock() + mock_reply_failure.error = Mock(return_value = QNetworkReply.NetworkError.AuthenticationRequiredError) http_mock = Mock() - http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) + def mock_get(url, headers_dict, callback, error_callback): + if(headers_dict == {"Authorization": "Bearer beep"}): + callback(mock_reply_success) + else: + callback(mock_reply_failure) + http_mock.get = mock_get http_mock.readJSON = Mock(return_value = {"data": {"user_id": "user_idea", "username": "Ghostkeeper"}}) + def mock_refresh(self, refresh_token, callback): # Refreshing gives a valid token. + callback(SUCCESSFUL_AUTH_RESPONSE) - with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): - authorization_service._parseJWT(mock_callback) + with patch("cura.OAuth2.AuthorizationHelpers.AuthorizationHelpers.getAccessTokenUsingRefreshToken", mock_refresh): + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + authorization_service._parseJWT(mock_callback) mock_callback.assert_called_once() profile_reply = mock_callback.call_args_list[0][0][0] From 8ea8cc752fe3addb9e0eb8a13ec205e8d3a51c0a Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 14:44:52 +0100 Subject: [PATCH 24/40] Also call processing functions for error callbacks Otherwise the appropriate callbacks might not get called. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 3 ++- cura/OAuth2/AuthorizationService.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 28b07cf17c..ac16ea9c9f 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -73,7 +73,8 @@ class AuthorizationHelpers: self._token_url, data = urllib.parse.urlencode(data).encode("UTF-8"), headers_dict = headers, - callback = lambda response: self.parseTokenResponse(response, callback) + callback = lambda response: self.parseTokenResponse(response, callback), + error_callback = lambda response: self.parseTokenResponse(response, callback) ) def parseTokenResponse(self, token_response: QNetworkReply, callback: Callable[[AuthenticationResponse], None]) -> None: diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 290fc5c651..98c09c6873 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -134,7 +134,7 @@ class AuthorizationService: self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token, process_auth_data) - self._auth_helpers.checkToken(self._auth_data.access_token, check_user_profile, lambda: callback(None)) + self._auth_helpers.checkToken(self._auth_data.access_token, check_user_profile, lambda: check_user_profile(None)) def getAccessToken(self) -> Optional[str]: """Get the access token as provided by the response data.""" From 3236be1c2002d4cea2d1452ec3c3d4bb6f712f58 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 14:45:44 +0100 Subject: [PATCH 25/40] Also mock POST command and a failed response Otherwise it'll end up using actual internet connections anyway. Not what we want. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index bc5dc21553..9993440ed4 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -120,9 +120,11 @@ def test__parseJWTFailOnRefresh(): mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.AuthenticationRequiredError) # The reply is 403: Authentication required, meaning the server responded with a "Can't do that, Dave". http_mock = Mock() http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) + http_mock.post = lambda url, data, headers_dict, callback, error_callback: callback(mock_reply) - with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): - authorization_service._parseJWT(mock_callback) + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.readJSON", Mock(return_value = {"error_description": "Mock a failed request!"})): + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + authorization_service._parseJWT(mock_callback) mock_callback.assert_called_once_with(None) def test__parseJWTSucceedOnRefresh(): From 41393504963c9e34f2273954d9614d3d29e09c6e Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 15:11:35 +0100 Subject: [PATCH 26/40] Fix test checking for failure of refresh token to reset auth It was previously mocking some return values that should now get returned via callbacks. And it was previously relying on a web service which might not connect at all. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index 9993440ed4..d39dc35258 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -170,14 +170,27 @@ def test_initialize(): original_preference.addPreference.assert_not_called() def test_refreshAccessTokenFailed(): + """ + Test if the authentication is reset once the refresh token fails to refresh access. + """ authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() - with patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()): - authorization_service._storeAuthData(SUCCESSFUL_AUTH_RESPONSE) - authorization_service.onAuthStateChanged.emit = MagicMock() - with patch.object(AuthorizationHelpers, "getAccessTokenUsingRefreshToken", return_value=FAILED_AUTH_RESPONSE): - authorization_service.refreshAccessToken() - assert authorization_service.onAuthStateChanged.emit.called_with(False) + + def mock_refresh(self, refresh_token, callback): # Refreshing gives a valid token. + callback(FAILED_AUTH_RESPONSE) + mock_reply = Mock() # The response that the request should give, containing an error about it failing to authenticate. + mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.AuthenticationRequiredError) # The reply is 403: Authentication required, meaning the server responded with a "Can't do that, Dave". + http_mock = Mock() + http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) + http_mock.post = lambda url, data, headers_dict, callback, error_callback: callback(mock_reply) + + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.readJSON", Mock(return_value = {"error_description": "Mock a failed request!"})): + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + authorization_service._storeAuthData(SUCCESSFUL_AUTH_RESPONSE) + authorization_service.onAuthStateChanged.emit = MagicMock() + with patch("cura.OAuth2.AuthorizationHelpers.AuthorizationHelpers.getAccessTokenUsingRefreshToken", mock_refresh): + authorization_service.refreshAccessToken() + assert authorization_service.onAuthStateChanged.emit.called_with(False) def test_refreshAccesTokenWithoutData(): authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) From a8a41381cb8493ff2254054c5c740051f1dd4131 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 15:14:05 +0100 Subject: [PATCH 27/40] Remove test simulating request error in user profile We're no longer generating that error. We're generating a QNetworkReply with a built-in error code and those errors are handled the same way as the failed requests tested above. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index d39dc35258..cb28d8625c 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -199,12 +199,6 @@ def test_refreshAccesTokenWithoutData(): authorization_service.refreshAccessToken() authorization_service.onAuthStateChanged.emit.assert_not_called() -def test_userProfileException(): - authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) - authorization_service.initialize() - authorization_service._parseJWT = MagicMock(side_effect=requests.exceptions.ConnectionError) - assert authorization_service.getUserProfile() is None - def test_failedLogin() -> None: authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.onAuthenticationError.emit = MagicMock() From c5e22c53cca2c0896bced64d306825a67ee16fcd Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 15:48:29 +0100 Subject: [PATCH 28/40] Fix checking return values in log-in/log-out flow test Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index cb28d8625c..dcb3af6877 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -95,7 +95,6 @@ def test__parseJWTNoRefreshToken(): mock_callback = Mock() # To log the final profile response. mock_reply = Mock() # The user profile that the service should respond with. mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.NoError) - http_mock = Mock() http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) http_mock.readJSON = Mock(return_value = {"data": {"user_id": "id_ego_or_superego", "username": "Ghostkeeper"}}) @@ -260,8 +259,14 @@ def test_loginAndLogout() -> None: authorization_service.onAuthStateChanged.emit = MagicMock() authorization_service.initialize() + mock_reply = Mock() # The user profile that the service should respond with. + mock_reply.error = Mock(return_value = QNetworkReply.NetworkError.NoError) + http_mock = Mock() + http_mock.get = lambda url, headers_dict, callback, error_callback: callback(mock_reply) + http_mock.readJSON = Mock(return_value = {"data": {"user_id": "di_resu", "username": "Emanresu"}}) + # Let the service think there was a successful response - with patch.object(AuthorizationHelpers, "parseJWT", return_value=UserProfile()): + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): authorization_service._onAuthStateChanged(SUCCESSFUL_AUTH_RESPONSE) # Ensure that the error signal was not triggered @@ -269,7 +274,10 @@ def test_loginAndLogout() -> None: # Since we said that it went right this time, validate that we got a signal. assert authorization_service.onAuthStateChanged.emit.call_count == 1 - assert authorization_service.getUserProfile() is not None + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + def callback(profile): + assert profile is not None + authorization_service.getUserProfile(callback) assert authorization_service.getAccessToken() == "beep" # Check that we stored the authentication data, so next time the user won't have to log in again. @@ -278,7 +286,10 @@ def test_loginAndLogout() -> None: # We're logged in now, also check if logging out works authorization_service.deleteAuthData() assert authorization_service.onAuthStateChanged.emit.call_count == 2 - assert authorization_service.getUserProfile() is None + with patch("UM.TaskManagement.HttpRequestManager.HttpRequestManager.getInstance", MagicMock(return_value = http_mock)): + def callback(profile): + assert profile is None + authorization_service.getUserProfile(callback) # Ensure the data is gone after we logged out. assert preferences.getValue("test/auth_data") == "{}" From fbbf1427b3d46a2fa1e2598782e609907ebb58bf Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 15:52:24 +0100 Subject: [PATCH 29/40] Fix patching of getUserProfile The actual profile is not necessary for this test. But this function always returns None and we shouldn't patch it to make it return something else. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index dcb3af6877..f34d5dcf5f 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -217,7 +217,7 @@ def test_failedLogin() -> None: assert authorization_service.getUserProfile() is None assert authorization_service.getAccessToken() is None -@patch.object(AuthorizationService, "getUserProfile", return_value=UserProfile()) +@patch.object(AuthorizationService, "getUserProfile") def test_storeAuthData(get_user_profile) -> None: preferences = Preferences() authorization_service = AuthorizationService(OAUTH_SETTINGS, preferences) From 4b5d698325e4ef9249c8df4ce74bba0a8aba55fa Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 15:54:42 +0100 Subject: [PATCH 30/40] Fix assertion of resulting profile via callback Needs to work differently with the asynchronous workflow. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index f34d5dcf5f..cc6dc175f1 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -297,9 +297,11 @@ def test_loginAndLogout() -> None: def test_wrongServerResponses() -> None: authorization_service = AuthorizationService(OAUTH_SETTINGS, Preferences()) authorization_service.initialize() - with patch.object(AuthorizationHelpers, "parseJWT", return_value=UserProfile()): - authorization_service._onAuthStateChanged(MALFORMED_AUTH_RESPONSE) - assert authorization_service.getUserProfile() is None + authorization_service._onAuthStateChanged(MALFORMED_AUTH_RESPONSE) + + def callback(profile): + assert profile is None + authorization_service.getUserProfile(callback) def test__generate_auth_url() -> None: preferences = Preferences() From 595a6580f5c24780b5a3fd8c3769e3f3ba3c3411 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 16:17:30 +0100 Subject: [PATCH 31/40] Use error callback and with correct number of parameters It gives two parameters, additionally an error code. However our callback wrapper gets the error code from the reply itself so it doesn't need it. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index ac16ea9c9f..7107d4df6a 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -51,7 +51,8 @@ class AuthorizationHelpers: self._token_url, data = urllib.parse.urlencode(data).encode("UTF-8"), headers_dict = headers, - callback = lambda response: self.parseTokenResponse(response, callback) + callback = lambda response: self.parseTokenResponse(response, callback), + error_callback = lambda response, _: self.parseTokenResponse(response, callback) ) def getAccessTokenUsingRefreshToken(self, refresh_token: str, callback: Callable[[AuthenticationResponse], None]) -> None: @@ -74,7 +75,7 @@ class AuthorizationHelpers: data = urllib.parse.urlencode(data).encode("UTF-8"), headers_dict = headers, callback = lambda response: self.parseTokenResponse(response, callback), - error_callback = lambda response: self.parseTokenResponse(response, callback) + error_callback = lambda response, _: self.parseTokenResponse(response, callback) ) def parseTokenResponse(self, token_response: QNetworkReply, callback: Callable[[AuthenticationResponse], None]) -> None: From 1ee9f73075a0752f8b3198b5bdd6733fa2c6b3b5 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 16:20:37 +0100 Subject: [PATCH 32/40] Only call success_callback if it's not None It could also be called as a fire-and-forget update thing. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationHelpers.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 7107d4df6a..7b7f58e74f 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -153,13 +153,14 @@ class AuthorizationHelpers: failed_callback() return - success_callback(UserProfile( - user_id = profile_data["user_id"], - username = profile_data["username"], - profile_image_url = profile_data.get("profile_image_url", ""), - organization_id = profile_data.get("organization", {}).get("organization_id"), - subscriptions = profile_data.get("subscriptions", []) - )) + if success_callback is not None: + success_callback(UserProfile( + user_id = profile_data["user_id"], + username = profile_data["username"], + profile_image_url = profile_data.get("profile_image_url", ""), + organization_id = profile_data.get("organization", {}).get("organization_id"), + subscriptions = profile_data.get("subscriptions", []) + )) @staticmethod def generateVerificationCode(code_length: int = 32) -> str: From 0196fd54ea1129f74426c3b4ade726f2e3a56cd1 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 16:24:22 +0100 Subject: [PATCH 33/40] Add a few missing types The return type missing caused the type checker to think it returned Any, which is clearly not true. And the type missing from _user_profile caused it to think it always had to be None, which is nonsense in any application. Contributes to issue CURA-8539. --- cura/API/Account.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index bf8a883c1a..94b28e7b64 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -76,7 +76,7 @@ class Account(QObject): self._error_message = None # type: Optional[Message] self._logged_in = False - self._user_profile = None + self._user_profile = None # type: Optional[UserProfile] self._additional_rights: Dict[str, Any] = {} self._sync_state = SyncState.IDLE self._manual_sync_enabled = False @@ -207,7 +207,7 @@ class Account(QObject): if self._update_timer.isActive(): self._update_timer.stop() - def _onProfileChanged(self, profile: UserProfile): + def _onProfileChanged(self, profile: UserProfile) -> None: self._user_profile = profile self.userProfileChanged.emit() From 1cdeb7d56b01cb1a5997874801628aa7195513fd Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 16:30:03 +0100 Subject: [PATCH 34/40] Typing: When the user profile changes, it may also become None This happens when logging out. Contributes to issue CURA-8539. --- cura/API/Account.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index 94b28e7b64..fa42c3c44d 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -207,7 +207,7 @@ class Account(QObject): if self._update_timer.isActive(): self._update_timer.stop() - def _onProfileChanged(self, profile: UserProfile) -> None: + def _onProfileChanged(self, profile: Optional[UserProfile]) -> None: self._user_profile = profile self.userProfileChanged.emit() From 608cce491d82be9b29440e4da7a6a58b5c80d65f Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 18:45:04 +0100 Subject: [PATCH 35/40] Remove tests checking for data in UserProfile The UserProfile is being stored in the account with a simple assignment, and these are just some property getters with a fallback. I don't think we need to test that. The actual getting of a user profile (and whether that returns correctly when logged out and such) is already tested as part of the OAuth flow where that code lives. Contributes to issue CURA-8539. --- tests/API/TestAccount.py | 40 ---------------------------------------- 1 file changed, 40 deletions(-) diff --git a/tests/API/TestAccount.py b/tests/API/TestAccount.py index 6780e50b81..1ad73462c2 100644 --- a/tests/API/TestAccount.py +++ b/tests/API/TestAccount.py @@ -80,46 +80,6 @@ def test_errorLoginState(application): account._onLoginStateChanged(False, "OMGZOMG!") account.loginStateChanged.emit.called_with(False) - -def test_userName(user_profile): - account = Account(MagicMock()) - mocked_auth_service = MagicMock() - account._authorization_service = mocked_auth_service - mocked_auth_service.getUserProfile = MagicMock(return_value = user_profile) - - assert account.userName == "username!" - - mocked_auth_service.getUserProfile = MagicMock(return_value=None) - assert account.userName is None - - -def test_profileImageUrl(user_profile): - account = Account(MagicMock()) - mocked_auth_service = MagicMock() - account._authorization_service = mocked_auth_service - mocked_auth_service.getUserProfile = MagicMock(return_value = user_profile) - - assert account.profileImageUrl == "profile_image_url!" - - mocked_auth_service.getUserProfile = MagicMock(return_value=None) - assert account.profileImageUrl is None - - -def test_userProfile(user_profile): - account = Account(MagicMock()) - mocked_auth_service = MagicMock() - account._authorization_service = mocked_auth_service - mocked_auth_service.getUserProfile = MagicMock(return_value=user_profile) - - returned_user_profile = account.userProfile - assert returned_user_profile["username"] == "username!" - assert returned_user_profile["profile_image_url"] == "profile_image_url!" - assert returned_user_profile["user_id"] == "user_id!" - - mocked_auth_service.getUserProfile = MagicMock(return_value=None) - assert account.userProfile is None - - def test_sync_success(): account = Account(MagicMock()) From 3f92b46ac8fad124aeff09abec65a8a3402bd7a3 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 18:52:43 +0100 Subject: [PATCH 36/40] Don't restart refresh token while it's already processing If two requests to the API occur at the same time, they will both see at the same time that they need an access token, and if it is expired they will both see that it needs refreshing. The server then sees two refreshes, both with the same refresh token. This is not allowed. The second one then gets a failure to refresh the token, which causes the user to log out. Instead, we'll stop one of the refresh requests. They were fire-and-forget anyway, so it's not needed to actually continue the request. Contributes to issue CURA-8539. --- cura/OAuth2/AuthorizationService.py | 6 ++++++ plugins/Toolbox/src/CloudSync/CloudPackageChecker.py | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 98c09c6873..0884ec8901 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -46,6 +46,7 @@ class AuthorizationService: self._user_profile = None # type: Optional["UserProfile"] self._preferences = preferences self._server = LocalAuthorizationServer(self._auth_helpers, self._onAuthStateChanged, daemon=True) + self._currently_refreshing_token = False # Whether we are currently in the process of refreshing auth. Don't make new requests while busy. self._unable_to_get_data_message = None # type: Optional[Message] @@ -168,6 +169,10 @@ class AuthorizationService: Logger.warning("Failed to get a new access token from the server.") self.onAuthStateChanged.emit(logged_in = False) + if self._currently_refreshing_token: + Logger.debug("Was already busy refreshing token. Do not start a new request.") + return + self._currently_refreshing_token = True self._auth_helpers.getAccessTokenUsingRefreshToken(self._auth_data.refresh_token, process_auth_data) def deleteAuthData(self) -> None: @@ -285,6 +290,7 @@ class AuthorizationService: return self._auth_data = auth_data + self._currently_refreshing_token = False if auth_data: self.getUserProfile() self._preferences.setValue(self._settings.AUTH_DATA_PREFERENCE_KEY, json.dumps(auth_data.dump())) diff --git a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py index 4c1818e4ee..6d2ed1dcbd 100644 --- a/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py +++ b/plugins/Toolbox/src/CloudSync/CloudPackageChecker.py @@ -66,7 +66,7 @@ class CloudPackageChecker(QObject): self._application.getHttpRequestManager().get(url, callback = self._onUserPackagesRequestFinished, error_callback = self._onUserPackagesRequestFinished, - timeout=10, + timeout = 10, scope = self._scope) def _onUserPackagesRequestFinished(self, reply: "QNetworkReply", error: Optional["QNetworkReply.NetworkError"] = None) -> None: From 677b64f06877df9d60d86413b8ccbacd40090931 Mon Sep 17 00:00:00 2001 From: Ghostkeeper Date: Mon, 22 Nov 2021 19:08:17 +0100 Subject: [PATCH 37/40] Remove unnecessary import We're not using this library any more now. Not even in the tests. Contributes to issue CURA-8539. --- tests/TestOAuth2.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index cc6dc175f1..7d0a4bc5c4 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -1,8 +1,9 @@ +# Copyright (c) 2021 Ultimaker B.V. +# Cura is released under the terms of the LGPLv3 or higher. + from datetime import datetime from unittest.mock import MagicMock, Mock, patch -import requests - from PyQt5.QtGui import QDesktopServices from PyQt5.QtNetwork import QNetworkReply From 70504f1b0461c50bb0f499f7f8d002eeb907a528 Mon Sep 17 00:00:00 2001 From: Jelle Spijker Date: Tue, 23 Nov 2021 10:30:23 +0100 Subject: [PATCH 38/40] Modernize typing usage Some boyscouting. --- cura/API/Account.py | 8 ++++---- cura/OAuth2/AuthorizationRequestHandler.py | 8 ++++---- cura/OAuth2/AuthorizationService.py | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index fa42c3c44d..9f1184a0a0 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -74,14 +74,14 @@ class Account(QObject): self._application = application self._new_cloud_printers_detected = False - self._error_message = None # type: Optional[Message] + self._error_message: Optional[Message] = None self._logged_in = False - self._user_profile = None # type: Optional[UserProfile] + self._user_profile: Optional[UserProfile] = None self._additional_rights: Dict[str, Any] = {} self._sync_state = SyncState.IDLE self._manual_sync_enabled = False self._update_packages_enabled = False - self._update_packages_action = None # type: Optional[Callable] + self._update_packages_action: Optional[Callable] = None self._last_sync_str = "-" self._callback_port = 32118 @@ -107,7 +107,7 @@ class Account(QObject): self._update_timer.setSingleShot(True) self._update_timer.timeout.connect(self.sync) - self._sync_services = {} # type: Dict[str, int] + self._sync_services: Dict[str, int] = {} """contains entries "service_name" : SyncState""" def initialize(self) -> None: diff --git a/cura/OAuth2/AuthorizationRequestHandler.py b/cura/OAuth2/AuthorizationRequestHandler.py index ff01969c50..59b3bafa98 100644 --- a/cura/OAuth2/AuthorizationRequestHandler.py +++ b/cura/OAuth2/AuthorizationRequestHandler.py @@ -25,11 +25,11 @@ class AuthorizationRequestHandler(BaseHTTPRequestHandler): super().__init__(request, client_address, server) # These values will be injected by the HTTPServer that this handler belongs to. - self.authorization_helpers = None # type: Optional[AuthorizationHelpers] - self.authorization_callback = None # type: Optional[Callable[[AuthenticationResponse], None]] - self.verification_code = None # type: Optional[str] + self.authorization_helpers: Optional[AuthorizationHelpers] = None + self.authorization_callback: Optional[Callable[[AuthenticationResponse], None]] = None + self.verification_code: Optional[str] = None - self.state = None # type: Optional[str] + self.state: Optional[str] = None # CURA-6609: Some browser seems to issue a HEAD instead of GET request as the callback. def do_HEAD(self) -> None: diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 0884ec8901..03d1640659 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -42,13 +42,13 @@ class AuthorizationService: self._settings = settings self._auth_helpers = AuthorizationHelpers(settings) self._auth_url = "{}/authorize".format(self._settings.OAUTH_SERVER_URL) - self._auth_data = None # type: Optional[AuthenticationResponse] - self._user_profile = None # type: Optional["UserProfile"] + self._auth_data: Optional[AuthenticationResponse] = None + self._user_profile: Optional["UserProfile"] = None self._preferences = preferences self._server = LocalAuthorizationServer(self._auth_helpers, self._onAuthStateChanged, daemon=True) self._currently_refreshing_token = False # Whether we are currently in the process of refreshing auth. Don't make new requests while busy. - self._unable_to_get_data_message = None # type: Optional[Message] + self._unable_to_get_data_message: Optional[Message] = None self.onAuthStateChanged.connect(self._authChanged) From 0a43366ffca39bd13cd4867dc8bf90519866ea83 Mon Sep 17 00:00:00 2001 From: Jelle Spijker Date: Tue, 23 Nov 2021 11:09:40 +0100 Subject: [PATCH 39/40] Fixed some styling and typing issues Some boyscouting. Contributes to: CURA-8539 --- cura/OAuth2/AuthorizationHelpers.py | 12 ++++++------ cura/OAuth2/AuthorizationRequestHandler.py | 1 + cura/OAuth2/AuthorizationService.py | 16 +++++++++------- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/cura/OAuth2/AuthorizationHelpers.py b/cura/OAuth2/AuthorizationHelpers.py index 7b7f58e74f..77e3c66c11 100644 --- a/cura/OAuth2/AuthorizationHelpers.py +++ b/cura/OAuth2/AuthorizationHelpers.py @@ -94,12 +94,12 @@ class AuthorizationHelpers: return callback(AuthenticationResponse(success = True, - token_type = token_data["token_type"], - access_token = token_data["access_token"], - refresh_token = token_data["refresh_token"], - expires_in = token_data["expires_in"], - scope = token_data["scope"], - received_at = datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT))) + token_type = token_data["token_type"], + access_token = token_data["access_token"], + refresh_token = token_data["refresh_token"], + expires_in = token_data["expires_in"], + scope = token_data["scope"], + received_at = datetime.now().strftime(TOKEN_TIMESTAMP_FORMAT))) return def checkToken(self, access_token: str, success_callback: Optional[Callable[[UserProfile], None]] = None, failed_callback: Optional[Callable[[], None]] = None) -> None: diff --git a/cura/OAuth2/AuthorizationRequestHandler.py b/cura/OAuth2/AuthorizationRequestHandler.py index 59b3bafa98..f303980e3c 100644 --- a/cura/OAuth2/AuthorizationRequestHandler.py +++ b/cura/OAuth2/AuthorizationRequestHandler.py @@ -15,6 +15,7 @@ if TYPE_CHECKING: catalog = i18nCatalog("cura") + class AuthorizationRequestHandler(BaseHTTPRequestHandler): """This handler handles all HTTP requests on the local web server. diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 03d1640659..38ecd1e823 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -25,6 +25,7 @@ if TYPE_CHECKING: MYCLOUD_LOGOFF_URL = "https://account.ultimaker.com/logoff?utm_source=cura&utm_medium=software&utm_campaign=change-account-before-adding-printers" + class AuthorizationService: """The authorization service is responsible for handling the login flow, storing user credentials and providing account information. @@ -62,7 +63,7 @@ class AuthorizationService: if self._preferences: self._preferences.addPreference(self._settings.AUTH_DATA_PREFERENCE_KEY, "{}") - def getUserProfile(self, callback: Callable[[Optional["UserProfile"]], None] = None) -> None: + def getUserProfile(self, callback: Optional[Callable[[Optional["UserProfile"]], None]] = None) -> None: """ Get the user profile as obtained from the JWT (JSON Web Token). @@ -79,7 +80,7 @@ class AuthorizationService: return # If no user profile was stored locally, we try to get it from JWT. - def store_profile(profile: Optional["UserProfile"]): + def store_profile(profile: Optional["UserProfile"]) -> None: if profile is not None: self._user_profile = profile if callback is not None: @@ -110,7 +111,7 @@ class AuthorizationService: return # When we checked the token we may get a user profile. This callback checks if that is a valid one and tries to refresh the token if it's not. - def check_user_profile(user_profile): + def check_user_profile(user_profile: Optional["UserProfile"]) -> None: if user_profile: # If the profile was found, we call it back immediately. callback(user_profile) @@ -121,7 +122,7 @@ class AuthorizationService: callback(None) return - def process_auth_data(auth_data: AuthenticationResponse): + def process_auth_data(auth_data: AuthenticationResponse) -> None: if auth_data.access_token is None: Logger.warning("Unable to use the refresh token to get a new access token.") callback(None) @@ -161,7 +162,7 @@ class AuthorizationService: Logger.log("w", "Unable to refresh access token, since there is no refresh token.") return - def process_auth_data(response: AuthenticationResponse): + def process_auth_data(response: AuthenticationResponse) -> None: if response.success: self._storeAuthData(response) self.onAuthStateChanged.emit(logged_in = True) @@ -264,7 +265,7 @@ class AuthorizationService: self._auth_data = AuthenticationResponse(**preferences_data) # Also check if we can actually get the user profile information. - def callback(profile: Optional["UserProfile"]): + def callback(profile: Optional["UserProfile"]) -> None: if profile is not None: self.onAuthStateChanged.emit(logged_in = True) Logger.debug("Auth data was successfully loaded") @@ -272,7 +273,8 @@ class AuthorizationService: if self._unable_to_get_data_message is not None: self._unable_to_get_data_message.show() else: - self._unable_to_get_data_message = Message(i18n_catalog.i18nc("@info", "Unable to reach the Ultimaker account server."), + self._unable_to_get_data_message = Message(i18n_catalog.i18nc("@info", + "Unable to reach the Ultimaker account server."), title = i18n_catalog.i18nc("@info:title", "Log-in failed"), message_type = Message.MessageType.ERROR) Logger.warning("Unable to get user profile using auth data from preferences.") From 36e28a245cb3ab14c20cc185356e93ce5fab3e03 Mon Sep 17 00:00:00 2001 From: Jelle Spijker Date: Tue, 23 Nov 2021 13:18:42 +0100 Subject: [PATCH 40/40] Fixed typing issues Contributes to: CURA-8539 --- cura/OAuth2/AuthorizationService.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 38ecd1e823..0343af68a8 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -3,7 +3,7 @@ import json from datetime import datetime, timedelta -from typing import Callable, Dict, Optional, TYPE_CHECKING +from typing import Callable, Dict, Optional, TYPE_CHECKING, Union from urllib.parse import urlencode, quote_plus from PyQt5.QtCore import QUrl @@ -15,7 +15,7 @@ from UM.Signal import Signal from UM.i18n import i18nCatalog from cura.OAuth2.AuthorizationHelpers import AuthorizationHelpers, TOKEN_TIMESTAMP_FORMAT from cura.OAuth2.LocalAuthorizationServer import LocalAuthorizationServer -from cura.OAuth2.Models import AuthenticationResponse +from cura.OAuth2.Models import AuthenticationResponse, BaseModel i18n_catalog = i18nCatalog("cura") @@ -117,7 +117,7 @@ class AuthorizationService: callback(user_profile) return # The JWT was expired or invalid and we should request a new one. - if self._auth_data.refresh_token is None: + if self._auth_data is None or self._auth_data.refresh_token is None: Logger.warning("There was no refresh token in the auth data.") callback(None) return