From b717755f2062d691444e1c29ac0f5273ca092aa7 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Mon, 11 May 2020 17:47:09 +0200 Subject: [PATCH 1/7] Add "Sign in with another account" button in AddCloudPrintersView There are cases where Cura and the browser fall out of sync when it comes to accounts. In such cases, you may be logged in cura with an account that has no cloud printers and in the browser with an account that has printers. So when you press the "Add cloud printer" button, you are redirected to mycloud and you see cloud printers that are not detected by Cura (because Cura is in a different acconut). In such cases, the user can now press the "Sign in with a different account" link in the "Waiting for cloud response" page, which will log him/her out in Cura AND in the browser, and then reinitiate the whole authorization flow, to make sure the accounts are in sync. CURA-7427 --- cura/API/Account.py | 12 +++++++ cura/OAuth2/AuthorizationService.py | 17 ++++++--- .../qml/WelcomePages/AddCloudPrintersView.qml | 35 +++++++++++++++++-- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index 7273479de4..acf507e23f 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -96,6 +96,18 @@ class Account(QObject): return self._authorization_service.startAuthorizationFlow() + @pyqtSlot() + def loginWithForcedLogout(self) -> None: + """ + Forces a logout from Cura and then initiates the authorization flow with the force_logout_from_mycloud variable + as true, to sync the accounts in Cura and in the browser. + + :return: None + """ + if self._logged_in: + self.logout() + self._authorization_service.startAuthorizationFlow(True) + @pyqtProperty(str, notify=loginStateChanged) def userName(self): user_profile = self._authorization_service.getUserProfile() diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 2f865456b6..ee1cdff19d 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -4,7 +4,7 @@ import json from datetime import datetime, timedelta from typing import Optional, TYPE_CHECKING -from urllib.parse import urlencode +from urllib.parse import urlencode, quote_plus import requests.exceptions from PyQt5.QtCore import QUrl @@ -142,7 +142,7 @@ class AuthorizationService: self.onAuthStateChanged.emit(logged_in = False) ## Start the flow to become authenticated. This will start a new webbrowser tap, prompting the user to login. - def startAuthorizationFlow(self) -> None: + def startAuthorizationFlow(self, force_logout_from_mycloud = False) -> None: Logger.log("d", "Starting new OAuth2 flow...") # Create the tokens needed for the code challenge (PKCE) extension for OAuth2. @@ -173,8 +173,17 @@ class AuthorizationService: title=i18n_catalog.i18nc("@info:title", "Warning")).show() return - # Open the authorization page in a new browser window. - QDesktopServices.openUrl(QUrl("{}?{}".format(self._auth_url, query_string))) + # Open the authorization page in a new browser window. If a force logout is requested during the authorization + # flow, the "mycloud logoff" link will be prepended to the authorization url to make sure that the user will be + # logged off from the browser before being redirected to login again. This case is used to sync the accounts + # between Cura and the browser. + auth_url = "{}?{}".format(self._auth_url, query_string) + if force_logout_from_mycloud: + mycloud_logoff_link = "https://mycloud.ultimaker.com/logoff" + logoff_auth_url = "{}?next={}".format(mycloud_logoff_link, quote_plus(auth_url)) + QDesktopServices.openUrl(QUrl(logoff_auth_url)) + else: + QDesktopServices.openUrl(QUrl(auth_url)) ## Callback method for the authentication flow. diff --git a/resources/qml/WelcomePages/AddCloudPrintersView.qml b/resources/qml/WelcomePages/AddCloudPrintersView.qml index f97d68f776..511328b0c8 100644 --- a/resources/qml/WelcomePages/AddCloudPrintersView.qml +++ b/resources/qml/WelcomePages/AddCloudPrintersView.qml @@ -51,11 +51,11 @@ Item } // Component that contains a busy indicator and a message, while it waits for Cura to discover a cloud printer - Rectangle + Item { id: waitingContent width: parent.width - height: waitingIndicator.height + waitingLabel.height + height: childrenRect.height anchors.verticalCenter: parent.verticalCenter anchors.horizontalCenter: parent.horizontalCenter BusyIndicator @@ -74,6 +74,37 @@ Item font: UM.Theme.getFont("large") renderType: Text.NativeRendering } + Label + { + id: noPrintersFoundLabel + anchors.top: waitingLabel.bottom + anchors.topMargin: 2 * UM.Theme.getSize("wide_margin").height + anchors.horizontalCenter: parent.horizontalCenter + horizontalAlignment: Text.AlignHCenter + text: catalog.i18nc("@label", "No printers found in your account?") + font: UM.Theme.getFont("medium") + } + Label + { + text: "Sign in with a different account" + anchors.top: noPrintersFoundLabel.bottom + anchors.horizontalCenter: parent.horizontalCenter + font: UM.Theme.getFont("medium") + color: UM.Theme.getColor("text_link") + MouseArea { + anchors.fill: parent; + onClicked: Cura.API.account.loginWithForcedLogout() + hoverEnabled: true + onEntered: + { + parent.font.underline = true + } + onExited: + { + parent.font.underline = false + } + } + } visible: discoveredCloudPrintersModel.count == 0 } From e3e767f4b974209414fc9db843ed716198a13f1f Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Tue, 12 May 2020 12:23:42 +0200 Subject: [PATCH 2/7] Simplify the link in AddNetworkOrLocalPrinterContent CURA-7427 --- resources/qml/WelcomePages/AddNetworkOrLocalPrinterContent.qml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/resources/qml/WelcomePages/AddNetworkOrLocalPrinterContent.qml b/resources/qml/WelcomePages/AddNetworkOrLocalPrinterContent.qml index 9e892e5521..b6f715aa0b 100644 --- a/resources/qml/WelcomePages/AddNetworkOrLocalPrinterContent.qml +++ b/resources/qml/WelcomePages/AddNetworkOrLocalPrinterContent.qml @@ -75,7 +75,7 @@ Item } else { - Qt.openUrlExternally("https://mycloud.ultimaker.com/app/manage/printers") + Qt.openUrlExternally("https://mycloud.ultimaker.com/") } } From 96387ef2aa4459ac3891f2830143ace064282c4e Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Tue, 12 May 2020 13:06:47 +0200 Subject: [PATCH 3/7] Add tests for loginWithForcedLogout() in account CURA-7427 --- cura/API/Account.py | 2 +- tests/API/TestAccount.py | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index acf507e23f..f87f613382 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -99,7 +99,7 @@ class Account(QObject): @pyqtSlot() def loginWithForcedLogout(self) -> None: """ - Forces a logout from Cura and then initiates the authorization flow with the force_logout_from_mycloud variable + Forces a logout from Cura and then initiates the authorization flow with the force_browser_logout variable as true, to sync the accounts in Cura and in the browser. :return: None diff --git a/tests/API/TestAccount.py b/tests/API/TestAccount.py index 4c6141e782..1b23947cc0 100644 --- a/tests/API/TestAccount.py +++ b/tests/API/TestAccount.py @@ -23,7 +23,7 @@ def test_login(): account.login() mocked_auth_service.startAuthorizationFlow.assert_called_once_with() - # Fake a sucesfull login + # Fake a successful login account._onLoginStateChanged(True) # Attempting to log in again shouldn't change anything. @@ -31,6 +31,26 @@ def test_login(): mocked_auth_service.startAuthorizationFlow.assert_called_once_with() +def test_loginWithForcedLogout(): + account = Account(MagicMock()) + mocked_auth_service = MagicMock() + account._authorization_service = mocked_auth_service + account.logout = MagicMock() + + # Fake a successful login + account._onLoginStateChanged(True) + account.loginWithForcedLogout() + # Make sure logout is called once + account.logout.assert_called_once_with() + mocked_auth_service.startAuthorizationFlow.assert_called_once_with(True) + + account._onLoginStateChanged(False) + account.loginWithForcedLogout() + # If we are not logged in previously, logout shouldn't be called again + account.logout.assert_called_once_with() + assert mocked_auth_service.startAuthorizationFlow.call_count == 2 + + def test_initialize(): account = Account(MagicMock()) mocked_auth_service = MagicMock() From d3fb002d9be0d3ffa86ee9df4a46911ca09f17c8 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Tue, 12 May 2020 13:07:39 +0200 Subject: [PATCH 4/7] Transfer the generation of the auth link into its own function The authentication link should be prepended with a logoff link from mycloud, if it is requested. In order to make this process testable this commit separates the generation of the authentication link, based on whether it requests for a browser logoff first, into its own function. This commit also adds tests for this function. CURA-7427 --- cura/OAuth2/AuthorizationService.py | 40 +++++++++++++++++------------ tests/TestOAuth2.py | 18 ++++++++++++- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index ee1cdff19d..5d7263280e 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 +from typing import Optional, TYPE_CHECKING, Dict from urllib.parse import urlencode, quote_plus import requests.exceptions @@ -24,6 +24,7 @@ if TYPE_CHECKING: from cura.OAuth2.Models import UserProfile, OAuth2Settings from UM.Preferences import Preferences +MYCLOUD_LOGOFF = "https://mycloud.ultimaker.com/logoff" ## The authorization service is responsible for handling the login flow, # storing user credentials and providing account information. @@ -142,7 +143,7 @@ class AuthorizationService: self.onAuthStateChanged.emit(logged_in = False) ## Start the flow to become authenticated. This will start a new webbrowser tap, prompting the user to login. - def startAuthorizationFlow(self, force_logout_from_mycloud = False) -> None: + def startAuthorizationFlow(self, force_browser_logout: bool = False) -> None: Logger.log("d", "Starting new OAuth2 flow...") # Create the tokens needed for the code challenge (PKCE) extension for OAuth2. @@ -153,8 +154,8 @@ class AuthorizationService: state = AuthorizationHelpers.generateVerificationCode() - # Create the query string needed for the OAuth2 flow. - query_string = urlencode({ + # Create the query dict needed for the OAuth2 flow. + query_parameters_dict = { "client_id": self._settings.CLIENT_ID, "redirect_uri": self._settings.CALLBACK_URL, "scope": self._settings.CLIENT_SCOPES, @@ -162,7 +163,7 @@ class AuthorizationService: "state": state, # Forever in our Hearts, RIP "(.Y.)" (2018-2020) "code_challenge": challenge_code, "code_challenge_method": "S512" - }) + } # Start a local web server to receive the callback URL on. try: @@ -173,18 +174,25 @@ class AuthorizationService: title=i18n_catalog.i18nc("@info:title", "Warning")).show() return - # Open the authorization page in a new browser window. If a force logout is requested during the authorization - # flow, the "mycloud logoff" link will be prepended to the authorization url to make sure that the user will be - # logged off from the browser before being redirected to login again. This case is used to sync the accounts - # between Cura and the browser. - auth_url = "{}?{}".format(self._auth_url, query_string) - if force_logout_from_mycloud: - mycloud_logoff_link = "https://mycloud.ultimaker.com/logoff" - logoff_auth_url = "{}?next={}".format(mycloud_logoff_link, quote_plus(auth_url)) - QDesktopServices.openUrl(QUrl(logoff_auth_url)) - else: - QDesktopServices.openUrl(QUrl(auth_url)) + auth_url = self._generate_auth_url(query_parameters_dict, force_browser_logout) + # Open the authorization page in a new browser window. + QDesktopServices.openUrl(QUrl(auth_url)) + def _generate_auth_url(self, query_parameters_dict: Dict[str, str], force_browser_logout: bool) -> str: + """ + Generates the authentications url based on the original auth_url and the query_parameters_dict to be included. + If there is a request to force logging out of mycloud in the browser, the link to logoff from mycloud is + prepended in order to force the browser to logoff from mycloud and then redirect to the authentication url to + login again. This case is used to sync the accounts between Cura and the browser. + :param query_parameters_dict: + :param force_browser_logout: + :return: + """ + auth_url = "{}?{}".format(self._auth_url, urlencode(query_parameters_dict)) + if force_browser_logout: + # The url after '?next=' should be urlencoded + auth_url = "{}?next={}".format(MYCLOUD_LOGOFF, quote_plus(auth_url)) + return auth_url ## Callback method for the authentication flow. def _onAuthStateChanged(self, auth_response: AuthenticationResponse) -> None: diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index 1e305c6549..159c482128 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -7,7 +7,7 @@ from PyQt5.QtGui import QDesktopServices from UM.Preferences import Preferences from cura.OAuth2.AuthorizationHelpers import AuthorizationHelpers, TOKEN_TIMESTAMP_FORMAT -from cura.OAuth2.AuthorizationService import AuthorizationService +from cura.OAuth2.AuthorizationService import AuthorizationService, MYCLOUD_LOGOFF from cura.OAuth2.LocalAuthorizationServer import LocalAuthorizationServer from cura.OAuth2.Models import OAuth2Settings, AuthenticationResponse, UserProfile @@ -226,3 +226,19 @@ def test_wrongServerResponses() -> None: with patch.object(AuthorizationHelpers, "parseJWT", return_value=UserProfile()): 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) + query_parameters_dict = { + "client_id": "", + "redirect_uri": OAUTH_SETTINGS.CALLBACK_URL, + "scope": OAUTH_SETTINGS.CLIENT_SCOPES, + "response_type": "code" + } + auth_url = authorization_service._generate_auth_url(query_parameters_dict, force_browser_logout = False) + assert MYCLOUD_LOGOFF + "?next=" not in auth_url + + auth_url = authorization_service._generate_auth_url(query_parameters_dict, force_browser_logout = True) + assert MYCLOUD_LOGOFF + "?next=" in auth_url From 32efb8d7bbc6417a60068db8a99fd5184b187841 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Tue, 12 May 2020 13:17:18 +0200 Subject: [PATCH 5/7] Fix description comment of _generate_auth_url() CURA-7427 --- cura/OAuth2/AuthorizationService.py | 13 ++++++++----- tests/TestOAuth2.py | 6 +++--- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/cura/OAuth2/AuthorizationService.py b/cura/OAuth2/AuthorizationService.py index 5d7263280e..b7bc025ec1 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -24,7 +24,7 @@ if TYPE_CHECKING: from cura.OAuth2.Models import UserProfile, OAuth2Settings from UM.Preferences import Preferences -MYCLOUD_LOGOFF = "https://mycloud.ultimaker.com/logoff" +MYCLOUD_LOGOFF_URL = "https://mycloud.ultimaker.com/logoff" ## The authorization service is responsible for handling the login flow, # storing user credentials and providing account information. @@ -184,14 +184,17 @@ class AuthorizationService: If there is a request to force logging out of mycloud in the browser, the link to logoff from mycloud is prepended in order to force the browser to logoff from mycloud and then redirect to the authentication url to login again. This case is used to sync the accounts between Cura and the browser. - :param query_parameters_dict: - :param force_browser_logout: - :return: + + :param query_parameters_dict: A dictionary with the query parameters to be url encoded and added to the + authentication link + :param force_browser_logout: If True, Cura will prepend the MYCLOUD_LOGOFF_URL link before the authentication + link to force the a browser logout from mycloud.ultimaker.com + :return: The authentication URL, properly formatted and encoded """ auth_url = "{}?{}".format(self._auth_url, urlencode(query_parameters_dict)) if force_browser_logout: # The url after '?next=' should be urlencoded - auth_url = "{}?next={}".format(MYCLOUD_LOGOFF, quote_plus(auth_url)) + auth_url = "{}?next={}".format(MYCLOUD_LOGOFF_URL, quote_plus(auth_url)) return auth_url ## Callback method for the authentication flow. diff --git a/tests/TestOAuth2.py b/tests/TestOAuth2.py index 159c482128..6b7e28917f 100644 --- a/tests/TestOAuth2.py +++ b/tests/TestOAuth2.py @@ -7,7 +7,7 @@ from PyQt5.QtGui import QDesktopServices from UM.Preferences import Preferences from cura.OAuth2.AuthorizationHelpers import AuthorizationHelpers, TOKEN_TIMESTAMP_FORMAT -from cura.OAuth2.AuthorizationService import AuthorizationService, MYCLOUD_LOGOFF +from cura.OAuth2.AuthorizationService import AuthorizationService, MYCLOUD_LOGOFF_URL from cura.OAuth2.LocalAuthorizationServer import LocalAuthorizationServer from cura.OAuth2.Models import OAuth2Settings, AuthenticationResponse, UserProfile @@ -238,7 +238,7 @@ def test__generate_auth_url() -> None: "response_type": "code" } auth_url = authorization_service._generate_auth_url(query_parameters_dict, force_browser_logout = False) - assert MYCLOUD_LOGOFF + "?next=" not in auth_url + assert MYCLOUD_LOGOFF_URL + "?next=" not in auth_url auth_url = authorization_service._generate_auth_url(query_parameters_dict, force_browser_logout = True) - assert MYCLOUD_LOGOFF + "?next=" in auth_url + assert MYCLOUD_LOGOFF_URL + "?next=" in auth_url From 898ca852f0e4c2a65822e3591ea823342ed1a741 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Tue, 12 May 2020 13:40:27 +0200 Subject: [PATCH 6/7] Fix mypy complaint about Optional type CURA-7427 --- 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 b7bc025ec1..47e6c139b8 100644 --- a/cura/OAuth2/AuthorizationService.py +++ b/cura/OAuth2/AuthorizationService.py @@ -178,7 +178,7 @@ class AuthorizationService: # Open the authorization page in a new browser window. QDesktopServices.openUrl(QUrl(auth_url)) - def _generate_auth_url(self, query_parameters_dict: Dict[str, str], force_browser_logout: bool) -> str: + def _generate_auth_url(self, query_parameters_dict: Dict[str, Optional[str]], force_browser_logout: bool) -> str: """ Generates the authentications url based on the original auth_url and the query_parameters_dict to be included. If there is a request to force logging out of mycloud in the browser, the link to logoff from mycloud is From eac4d3e463c659f9f18ca3a3d7aff603f2561933 Mon Sep 17 00:00:00 2001 From: Kostas Karmas Date: Tue, 12 May 2020 16:54:41 +0200 Subject: [PATCH 7/7] Make login parametrized with a force_logout boolean Instead of using a separate function to force logging out before login, login now has a boolean parameter that instructs it to logout before loging in again, if the user is alread logged in. It then starts the authorization with a force browser logout first. CURA-7427 --- cura/API/Account.py | 24 ++++++++--------- .../qml/WelcomePages/AddCloudPrintersView.qml | 2 +- tests/API/TestAccount.py | 26 +++++-------------- 3 files changed, 20 insertions(+), 32 deletions(-) diff --git a/cura/API/Account.py b/cura/API/Account.py index f87f613382..7f15ad7efb 100644 --- a/cura/API/Account.py +++ b/cura/API/Account.py @@ -90,23 +90,23 @@ class Account(QObject): self.loginStateChanged.emit(logged_in) @pyqtSlot() - def login(self) -> None: - if self._logged_in: - # Nothing to do, user already logged in. - return - self._authorization_service.startAuthorizationFlow() - - @pyqtSlot() - def loginWithForcedLogout(self) -> None: + @pyqtSlot(bool) + def login(self, force_logout_before_login: bool = False) -> None: """ - Forces a logout from Cura and then initiates the authorization flow with the force_browser_logout variable - as true, to sync the accounts in Cura and in the browser. + Initializes the login process. If the user is logged in already and force_logout_before_login is true, Cura will + logout from the account before initiating the authorization flow. If the user is logged in and + force_logout_before_login is false, the function will return, as there is nothing to do. + :param force_logout_before_login: Optional boolean parameter :return: None """ if self._logged_in: - self.logout() - self._authorization_service.startAuthorizationFlow(True) + if force_logout_before_login: + self.logout() + else: + # Nothing to do, user already logged in. + return + self._authorization_service.startAuthorizationFlow(force_logout_before_login) @pyqtProperty(str, notify=loginStateChanged) def userName(self): diff --git a/resources/qml/WelcomePages/AddCloudPrintersView.qml b/resources/qml/WelcomePages/AddCloudPrintersView.qml index 511328b0c8..ccaaa6908f 100644 --- a/resources/qml/WelcomePages/AddCloudPrintersView.qml +++ b/resources/qml/WelcomePages/AddCloudPrintersView.qml @@ -93,7 +93,7 @@ Item color: UM.Theme.getColor("text_link") MouseArea { anchors.fill: parent; - onClicked: Cura.API.account.loginWithForcedLogout() + onClicked: Cura.API.account.login(true) hoverEnabled: true onEntered: { diff --git a/tests/API/TestAccount.py b/tests/API/TestAccount.py index 1b23947cc0..09091ba7e0 100644 --- a/tests/API/TestAccount.py +++ b/tests/API/TestAccount.py @@ -19,35 +19,23 @@ def test_login(): account = Account(MagicMock()) mocked_auth_service = MagicMock() account._authorization_service = mocked_auth_service + account.logout = MagicMock() account.login() - mocked_auth_service.startAuthorizationFlow.assert_called_once_with() + mocked_auth_service.startAuthorizationFlow.assert_called_once_with(False) # Fake a successful login account._onLoginStateChanged(True) # Attempting to log in again shouldn't change anything. account.login() - mocked_auth_service.startAuthorizationFlow.assert_called_once_with() + mocked_auth_service.startAuthorizationFlow.assert_called_once_with(False) - -def test_loginWithForcedLogout(): - account = Account(MagicMock()) - mocked_auth_service = MagicMock() - account._authorization_service = mocked_auth_service - account.logout = MagicMock() - - # Fake a successful login - account._onLoginStateChanged(True) - account.loginWithForcedLogout() - # Make sure logout is called once - account.logout.assert_called_once_with() - mocked_auth_service.startAuthorizationFlow.assert_called_once_with(True) - - account._onLoginStateChanged(False) - account.loginWithForcedLogout() - # If we are not logged in previously, logout shouldn't be called again + # Attempting to log in with force_logout_before_login as True should call the logout before calling the + # startAuthorizationFlow(True). + account.login(force_logout_before_login=True) account.logout.assert_called_once_with() + mocked_auth_service.startAuthorizationFlow.assert_called_with(True) assert mocked_auth_service.startAuthorizationFlow.call_count == 2