From 77d802a4a2e54cda04cdc20191bf7e7972777e8c Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Mon, 6 Nov 2023 20:34:19 +0545 Subject: [PATCH 01/24] Add Studio authentication to DVC Implemented a new feature to authenticate DVC with Iterative Studio. This includes generating an access token, starting device login, checking if user code is authorized, and handling any request exceptions. Furthermore, the commit creates a new authentication command for DVC. This feature was added to allow DVC users to authenticate with Studio, which is necessary for functionality such as sharing live experiments and notifying Studio about pushed experiments. --- dvc/cli/parser.py | 2 + dvc/commands/auth.py | 145 +++++++++++++++++++++++++++++++++++++++++++ dvc/utils/studio.py | 92 ++++++++++++++++++++++----- 3 files changed, 225 insertions(+), 14 deletions(-) create mode 100644 dvc/commands/auth.py diff --git a/dvc/cli/parser.py b/dvc/cli/parser.py index 88a60d1c2a..551c498fbd 100644 --- a/dvc/cli/parser.py +++ b/dvc/cli/parser.py @@ -7,6 +7,7 @@ from dvc.commands import ( add, artifacts, + auth, cache, check_ignore, checkout, @@ -93,6 +94,7 @@ check_ignore, data, artifacts, + auth, ] diff --git a/dvc/commands/auth.py b/dvc/commands/auth.py new file mode 100644 index 0000000000..99cd8646cc --- /dev/null +++ b/dvc/commands/auth.py @@ -0,0 +1,145 @@ +import argparse +import logging +import os +import sys + +from dvc.cli.command import CmdBase +from dvc.cli.utils import append_doc_link, fix_subparsers + +logger = logging.getLogger(__name__) + +DEFAULT_SCOPES = "live,dvc_experiment,view_url,dql,download_model" +AVAILABLE_SCOPES = ["live", "dvc_experiment", "view_url", "dql", "download_model"] + + +class CmdAuthLogin(CmdBase): + def run(self): + from dvc.env import DVC_STUDIO_URL + from dvc.repo.experiments.utils import gen_random_name + from dvc.ui import ui + from dvc.utils.studio import STUDIO_URL, check_token_authorization + + scopes = self.args.scopes or DEFAULT_SCOPES + name = self.args.name or gen_random_name() + hostname = self.args.hostname or os.environ.get(DVC_STUDIO_URL) or STUDIO_URL + + data = {"client_name": "dvc", "token_name": name, "scopes": scopes} + device_code, token_uri = self.initiate_authorization(hostname, data) + + access_token = check_token_authorization(uri=token_uri, device_code=device_code) + if not access_token: + ui.write( + "failed to authenticate: This 'device_code' has expired.(expired_token)" + ) + sys.exit(1) + + self.save_config(hostname, access_token) + ui.write( + f"Authentication successful. The token will be" + f"available as {name} in Studio profile." + ) + + def initiate_authorization(self, hostname, data): + import webbrowser + + from dvc.ui import ui + from dvc.utils.studio import start_device_login + + response = start_device_login(data=data, base_url=hostname) + verification_uri = response["verification_uri"] + user_code = response["user_code"] + device_code = response["device_code"] + token_uri = response["token_uri"] + + opened = False + if not self.args.use_device_code: + ui.write( + f"A web browser has been opened at \n{verification_uri}.\n" + f"Please continue the login in the web browser.\n" + f"If no web browser is available or if the web browser fails to open,\n" + f"use device code flow with `dvc auth login --use-device-code`." + ) + url = f"{verification_uri}?code={user_code}" + opened = webbrowser.open(url) + + if not opened: + ui.write( + f"Please open the following url in your browser.\n{verification_uri}" + ) + ui.write(f"And enter the user code below {user_code} to authorize.") + return device_code, token_uri + + def save_config(self, hostname, token): + with self.config.edit("global") as conf: + conf["studio"]["token"] = token + conf["studio"]["url"] = hostname + + +def add_parser(subparsers, parent_parser): + AUTH_HELP = "Authenticate dvc with Iterative Studio" + AUTH_DESCRIPTION = ( + "Authorize dvc with Studio and set the token. When this is\n" + "set, DVC uses this to share live experiments and notify\n" + "Studio about pushed experiments." + ) + + auth_parser = subparsers.add_parser( + "auth", + parents=[parent_parser], + description=append_doc_link(AUTH_DESCRIPTION, "auth"), + help=AUTH_HELP, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + auth_subparsers = auth_parser.add_subparsers( + dest="cmd", + help="Use `dvc auth CMD --help` to display command-specific help.", + ) + fix_subparsers(auth_subparsers) + + AUTH_LOGIN_HELP = "Authenticate DVC with Studio host" + AUTH_LOGIN_DESCRIPTION = ( + "By default, this command authorize dvc with Studio with\n" + " default scopes and a random name as token name." + ) + login_parser = auth_subparsers.add_parser( + "login", + parents=[parent_parser], + description=append_doc_link(AUTH_LOGIN_DESCRIPTION, "auth/login"), + help=AUTH_LOGIN_HELP, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + + login_parser.add_argument( + "-H", + "--hostname", + action="store", + default=None, + help="The hostname of the Studio instance to authenticate with.", + ) + login_parser.add_argument( + "-s", + "--scopes", + action="store", + default=None, + help="The scopes for the authentication token. ", + ) + + login_parser.add_argument( + "-n", + "--name", + action="store", + default=None, + help="The name of the authentication token. It will be used to\n" + "identify token shown in Studio profile.", + ) + + login_parser.add_argument( + "-d", + "--use-device-code", + action="store_true", + default=False, + help="Use authentication flow based on user code.\n" + "You will be presented with user code to enter in browser.\n" + "DVC will also use this if it cannot launch browser on your behalf.", + ) + login_parser.set_defaults(func=CmdAuthLogin) diff --git a/dvc/utils/studio.py b/dvc/utils/studio.py index 2530db78b8..a47cc188b6 100644 --- a/dvc/utils/studio.py +++ b/dvc/utils/studio.py @@ -35,7 +35,7 @@ def post( logger.trace("Sending %s to %s", data, url) - headers = {"Authorization": f"token {token}"} + headers = {"Authorization": f"token {token}"} if token else None r = session.post( url, json=data, headers=headers, timeout=timeout, allow_redirects=False ) @@ -66,19 +66,7 @@ def notify_refs( try: r = post("webhook/dvc", token, data, base_url=base_url) except requests.RequestException as e: - logger.trace("", exc_info=True) - - msg = str(e) - if e.response is None: - logger.warning("failed to notify Studio: %s", msg.lower()) - return {} - - r = e.response - d = ignore(Exception, default={})(r.json)() - status = r.status_code - if detail := d.get("detail"): - msg = f"{detail} ({status=})" - logger.warning("failed to notify Studio: %s", msg.lower()) + _handle_request_exception(e) else: d = r.json() @@ -87,6 +75,82 @@ def notify_refs( return d +def start_device_login( + *, + data, + base_url=STUDIO_URL, +): + logger.debug( + "Starting device login for Studio%s", + f" ({base_url})" if base_url else "", + ) + + try: + r = post("api/device-login", "", data=data, base_url=base_url) + except requests.RequestException as e: + _handle_request_exception(e) + raise + else: + d = r.json() + + if d: + logger.trace( # type: ignore[attr-defined] + "received response: %s (status=%r)", d, r.status_code + ) + return d + + +def check_token_authorization(*, uri, device_code): + import time + + logger.debug("Polling to find if the user code is authorized") + + data = {"code": device_code} + session = requests.Session() + session.mount(uri, HTTPAdapter(max_retries=3)) + + logger.debug("Checking with %s to %s", device_code, uri) + + counter = 1 + while True: + try: + logger.debug("Polling attempt #%s", counter) + r = session.post(uri, json=data, timeout=5, allow_redirects=False) + counter += 1 + if r.status_code == 400: + d = ignore(Exception, default={})(r.json)() + detail = d.get("detail") + if detail == "authorization_pending": + # Wait 5 seconds before retrying. + time.sleep(5) + continue + if detail == "authorization_expired": + return + + r.raise_for_status() + + return r.json()["access_token"] + except requests.RequestException as e: + _handle_request_exception(e) + raise + + +def _handle_request_exception(e): + logger.trace("", exc_info=True) # type: ignore[attr-defined] + + msg = str(e) + if e.response is None: + logger.warning("failed to contact Studio: %s", msg.lower()) + return {} + + r = e.response + d = ignore(Exception, default={})(r.json)() + status = r.status_code + if detail := d.get("detail"): + msg = f"{detail} ({status=})" + logger.warning("failed to contact Studio: %s", msg.lower()) + + def config_to_env(config: Dict[str, Any]) -> Dict[str, Any]: env = {} if "offline" in config: From 450e7f3e13e6fa772d8d73322a05239de2c3bd60 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 7 Nov 2023 13:59:13 +0545 Subject: [PATCH 02/24] Add authentication validation and error handling to DVC Studio This commit introduces validation for scopes used in DVC Studio's authentication process. If an unsupported scope is passed, the user will receive an error message listing the invalid scopes and the program will return an error status. Additionally, this commit changes the program's exit behavior when device code authorization fails or authentication process is successful, replacing 'sys.exit' with appropriate returned values. --- dvc/commands/auth.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/dvc/commands/auth.py b/dvc/commands/auth.py index 99cd8646cc..b55803a6a1 100644 --- a/dvc/commands/auth.py +++ b/dvc/commands/auth.py @@ -1,7 +1,6 @@ import argparse import logging import os -import sys from dvc.cli.command import CmdBase from dvc.cli.utils import append_doc_link, fix_subparsers @@ -20,6 +19,14 @@ def run(self): from dvc.utils.studio import STUDIO_URL, check_token_authorization scopes = self.args.scopes or DEFAULT_SCOPES + if invalid_scopes := list( + filter(lambda s: s not in AVAILABLE_SCOPES, scopes.split(",")) + ): + ui.error_write( + f"Following scopes are not valid: {', '.join(invalid_scopes)}" + ) + return 1 + name = self.args.name or gen_random_name() hostname = self.args.hostname or os.environ.get(DVC_STUDIO_URL) or STUDIO_URL @@ -31,13 +38,14 @@ def run(self): ui.write( "failed to authenticate: This 'device_code' has expired.(expired_token)" ) - sys.exit(1) + return 1 self.save_config(hostname, access_token) ui.write( - f"Authentication successful. The token will be" + f"Authentication successful. The token will be " f"available as {name} in Studio profile." ) + return 0 def initiate_authorization(self, hostname, data): import webbrowser From 29ed14b180c06b203c32f573d21e061ad34fcccb Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 7 Nov 2023 14:12:08 +0545 Subject: [PATCH 03/24] Add logout functionality to DVC Studio commands This commit introduces the logout command for DVC Studio. It allows users to manually log out from DVC Studio via the command line interface. The logout command only removes the user's token if it exists, otherwise, it will print an error message and return an error status. This adds an extra layer of control for the users and enhances the security for the usage of DVC Studio. --- dvc/commands/auth.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/dvc/commands/auth.py b/dvc/commands/auth.py index b55803a6a1..dba02f0d83 100644 --- a/dvc/commands/auth.py +++ b/dvc/commands/auth.py @@ -2,6 +2,8 @@ import logging import os +from funcy import get_in + from dvc.cli.command import CmdBase from dvc.cli.utils import append_doc_link, fix_subparsers @@ -83,6 +85,21 @@ def save_config(self, hostname, token): conf["studio"]["url"] = hostname +class CmdAuthLogout(CmdBase): + def run(self): + from dvc.ui import ui + + with self.config.edit("global") as conf: + if not get_in(conf, ["studio", "token"]): + ui.error_write("Not logged in to Studio.") + return 1 + + del conf["studio"]["token"] + + ui.write("Logged out from Studio") + return 0 + + def add_parser(subparsers, parent_parser): AUTH_HELP = "Authenticate dvc with Iterative Studio" AUTH_DESCRIPTION = ( @@ -151,3 +168,16 @@ def add_parser(subparsers, parent_parser): "DVC will also use this if it cannot launch browser on your behalf.", ) login_parser.set_defaults(func=CmdAuthLogin) + + AUTH_LOGOUT_HELP = "Logout user from Studio" + AUTH_LOGOUT_DESCRIPTION = "This command helps to log out user from DVC Studio.\n" + + logout_parser = auth_subparsers.add_parser( + "logout", + parents=[parent_parser], + description=append_doc_link(AUTH_LOGOUT_DESCRIPTION, "auth/logout"), + help=AUTH_LOGOUT_HELP, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + + logout_parser.set_defaults(func=CmdAuthLogout) From 941764a09549a00a1e7a7000b87d6ed7f4430f5d Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 7 Nov 2023 14:41:06 +0545 Subject: [PATCH 04/24] Improve User Authentication process for DVC Studio Implemented a new 'token' command facilitating users to check their authentication token while operating with DVC Studio. This implementation enhances user experience by providing the ability to validate logged-in status. If not logged in, it rightly prompts an error, ensuring secure access. --- dvc/commands/auth.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/dvc/commands/auth.py b/dvc/commands/auth.py index dba02f0d83..7fa5076b0a 100644 --- a/dvc/commands/auth.py +++ b/dvc/commands/auth.py @@ -100,6 +100,20 @@ def run(self): return 0 +class CmdAuthToken(CmdBase): + def run(self): + from dvc.ui import ui + + conf = self.config.read("global") + token = get_in(conf, ["studio", "token"]) + if not token: + ui.error_write("Not logged in to Studio.") + return 1 + + ui.write(token) + return 0 + + def add_parser(subparsers, parent_parser): AUTH_HELP = "Authenticate dvc with Iterative Studio" AUTH_DESCRIPTION = ( @@ -181,3 +195,17 @@ def add_parser(subparsers, parent_parser): ) logout_parser.set_defaults(func=CmdAuthLogout) + + AUTH_TOKEN_HELP = ( + "View the token dvc uses to contact Studio" # noqa: S105 # nosec B105 + ) + + logout_parser = auth_subparsers.add_parser( + "token", + parents=[parent_parser], + description=append_doc_link(AUTH_TOKEN_HELP, "auth/token"), + help=AUTH_TOKEN_HELP, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + + logout_parser.set_defaults(func=CmdAuthToken) From bca15104cb23249103348b9d88b684bd15c8a6a3 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 7 Nov 2023 19:41:53 +0545 Subject: [PATCH 05/24] Add unit tests for auth commands Unit tests are added for the authentication commands used in DVC Studio. The modifications include tests for login with invalid scope, standard login, logout, and token commands. The tests verify successful execution and appropriate responses for different user authentication scenarios, thereby increasing the robustness and reliability of the auth module. --- tests/unit/command/test_auth.py | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 tests/unit/command/test_auth.py diff --git a/tests/unit/command/test_auth.py b/tests/unit/command/test_auth.py new file mode 100644 index 0000000000..f2688f5975 --- /dev/null +++ b/tests/unit/command/test_auth.py @@ -0,0 +1,54 @@ +from unittest import mock + +from dvc.cli import main +from dvc.utils.studio import STUDIO_URL + + +def test_auth_login_invalid_scope(): + assert main(["auth", "login", "--scopes", "invalid!"]) == 1 + + +@mock.patch("dvc.utils.studio.check_token_authorization") +@mock.patch("dvc.utils.studio.start_device_login") +def test_auth_login(mock_start_device_login, mock_check_token_authorization, dvc): + mock_response = { + "verification_uri": STUDIO_URL + "/auth/device-login", + "user_code": "MOCKCODE", + "device_code": "random-value", + "token_uri": STUDIO_URL + "/api/device-login/token", + } + mock_start_device_login.return_value = mock_response + mock_check_token_authorization.return_value = None + + assert main(["auth", "login"]) == 1 + + mock_check_token_authorization.return_value = "isat_access_token" + assert main(["auth", "login"]) == 0 + config = dvc.config.load_one("global") + assert config["studio"]["token"] == "isat_access_token" + assert config["studio"]["url"] == STUDIO_URL + + +def test_auth_logout(dvc): + with dvc.config.edit("global") as conf: + conf["studio"]["token"] = "isat_access_token" + + assert main(["auth", "logout"]) == 0 + config = dvc.config.load_one("global") + assert "token" not in config["studio"] + + assert main(["auth", "logout"]) == 1 + + +@mock.patch("dvc.ui.ui.write") +def test_auth_token(mock_write, dvc): + with dvc.config.edit("global") as conf: + conf["studio"]["token"] = "isat_access_token" + + assert main(["auth", "token"]) == 0 + mock_write.assert_called_with("isat_access_token") + + with dvc.config.edit("global") as conf: + del conf["studio"]["token"] + + assert main(["auth", "token"]) == 1 From 0db01817035b1dfd9b3a136fffc05bf4b9b9c218 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 7 Nov 2023 20:01:00 +0545 Subject: [PATCH 06/24] Update unit tests for auth commands Unit tests for DVC Studio authentication commands have been updated. These checks verify successful execution and appropriate responses under various authentication scenarios, including login with invalid scope, standard login, logout, and token commands in order to enhance the robustness and reliability of the auth module. --- tests/unit/command/test_auth.py | 81 +++++++++++++++++++++++++++++---- 1 file changed, 73 insertions(+), 8 deletions(-) diff --git a/tests/unit/command/test_auth.py b/tests/unit/command/test_auth.py index f2688f5975..15d8d77bd4 100644 --- a/tests/unit/command/test_auth.py +++ b/tests/unit/command/test_auth.py @@ -1,8 +1,16 @@ from unittest import mock from dvc.cli import main +from dvc.commands.auth import DEFAULT_SCOPES from dvc.utils.studio import STUDIO_URL +MOCK_RESPONSE = { + "verification_uri": STUDIO_URL + "/auth/device-login", + "user_code": "MOCKCODE", + "device_code": "random-value", + "token_uri": STUDIO_URL + "/api/device-login/token", +} + def test_auth_login_invalid_scope(): assert main(["auth", "login", "--scopes", "invalid!"]) == 1 @@ -10,25 +18,82 @@ def test_auth_login_invalid_scope(): @mock.patch("dvc.utils.studio.check_token_authorization") @mock.patch("dvc.utils.studio.start_device_login") -def test_auth_login(mock_start_device_login, mock_check_token_authorization, dvc): - mock_response = { - "verification_uri": STUDIO_URL + "/auth/device-login", - "user_code": "MOCKCODE", - "device_code": "random-value", - "token_uri": STUDIO_URL + "/api/device-login/token", - } - mock_start_device_login.return_value = mock_response +def test_auth_login_token_check_failed( + mock_start_device_login, mock_check_token_authorization, dvc +): + mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = None assert main(["auth", "login"]) == 1 + +@mock.patch("dvc.utils.studio.check_token_authorization") +@mock.patch("dvc.utils.studio.start_device_login") +def test_auth_login_success( + mock_start_device_login, mock_check_token_authorization, dvc, M +): + mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = "isat_access_token" + assert main(["auth", "login"]) == 0 + assert mock_start_device_login.call_args.kwargs == { + "base_url": STUDIO_URL, + "data": {"client_name": "dvc", "scopes": DEFAULT_SCOPES, "token_name": M.any}, + } + mock_check_token_authorization.assert_called_with( + uri=MOCK_RESPONSE["token_uri"], device_code=MOCK_RESPONSE["device_code"] + ) + config = dvc.config.load_one("global") assert config["studio"]["token"] == "isat_access_token" assert config["studio"]["url"] == STUDIO_URL +@mock.patch("dvc.utils.studio.check_token_authorization") +@mock.patch("dvc.utils.studio.start_device_login") +def test_auth_login_arguments( + mock_start_device_login, mock_check_token_authorization, dvc, M +): + mock_start_device_login.return_value = MOCK_RESPONSE + mock_check_token_authorization.return_value = "isat_access_token" + + assert ( + main( + [ + "auth", + "login", + "--name", + "token_name", + "--hostname", + "https://example.com", + "--scopes", + "live", + ] + ) + == 0 + ) + + mock_start_device_login.assert_called_with( + data={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, + base_url="https://example.com", + ) + mock_check_token_authorization.assert_called() + + +@mock.patch("dvc.utils.studio.check_token_authorization") +@mock.patch("dvc.utils.studio.start_device_login") +@mock.patch("webbrowser.open") +def test_auth_device_code( + mock_webbrowser_open, mock_start_device_login, mock_check_token_authorization +): + mock_start_device_login.return_value = MOCK_RESPONSE + mock_check_token_authorization.return_value = "isat_access_token" + + assert main(["auth", "login", "--use-device-code"]) == 0 + + mock_webbrowser_open.assert_not_called() + + def test_auth_logout(dvc): with dvc.config.edit("global") as conf: conf["studio"]["token"] = "isat_access_token" From 7e214edce8e73fcc45426808317e6a30de6c0c2a Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Wed, 8 Nov 2023 17:12:00 +0545 Subject: [PATCH 07/24] Add unit tests and modify authentication function in studio utils This commit adds several new unit tests in test_studio.py. Tests for start_device_login and check_token_authorization functionality are included to verify the correct behavior of these methods. Moreover, it also modifies the authentication function in studio.py for improving the robustness. Changes in the function aimed to handle unexpected error scenarios and make the code more resilient. --- dvc/utils/studio.py | 1 + tests/unit/utils/test_studio.py | 114 +++++++++++++++++++++++++++++++- 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/dvc/utils/studio.py b/dvc/utils/studio.py index a47cc188b6..5e1833feaf 100644 --- a/dvc/utils/studio.py +++ b/dvc/utils/studio.py @@ -63,6 +63,7 @@ def notify_refs( ) data = {"repo_url": repo_url, "client": "dvc", "refs": refs} + d = None try: r = post("webhook/dvc", token, data, base_url=base_url) except requests.RequestException as e: diff --git a/tests/unit/utils/test_studio.py b/tests/unit/utils/test_studio.py index 68dcfed77d..da1a63d717 100644 --- a/tests/unit/utils/test_studio.py +++ b/tests/unit/utils/test_studio.py @@ -1,6 +1,7 @@ from urllib.parse import urljoin import pytest +import requests from requests import Response from dvc.env import ( @@ -9,11 +10,17 @@ DVC_STUDIO_TOKEN, DVC_STUDIO_URL, ) -from dvc.utils.studio import STUDIO_URL, config_to_env, env_to_config, notify_refs +from dvc.utils.studio import ( + STUDIO_URL, + check_token_authorization, + config_to_env, + env_to_config, + notify_refs, + start_device_login, +) CONFIG = {"offline": True, "repo_url": "repo_url", "token": "token", "url": "url"} - ENV = { DVC_STUDIO_OFFLINE: True, DVC_STUDIO_REPO_URL: "repo_url", @@ -67,3 +74,106 @@ def test_config_to_env(): def test_env_to_config(): assert env_to_config(ENV) == CONFIG + + +def test_start_device_login(mocker): + response = Response() + response.status_code = 200 + mocker.patch.object(response, "json", side_effect=[{"user_code": "MOCKCODE"}]) + + mock_post = mocker.patch("requests.Session.post", return_value=response) + + start_device_login( + data={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, + base_url="https://example.com", + ) + assert mock_post.called + assert mock_post.call_args == mocker.call( + "https://example.com/api/device-login", + json={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, + headers=None, + timeout=5, + allow_redirects=False, + ) + + +def test_check_token_authorization_expired(mocker): + mock_post = mocker.patch( + "requests.Session.post", + side_effect=[ + _mock_response(mocker, 400, {"detail": "authorization_pending"}), + _mock_response(mocker, 400, {"detail": "authorization_expired"}), + ], + ) + + assert ( + check_token_authorization( + uri="https://example.com/token_uri", device_code="random_device_code" + ) + is None + ) + + assert mock_post.call_count == 2 + assert mock_post.call_args == mocker.call( + "https://example.com/token_uri", + json={"code": "random_device_code"}, + timeout=5, + allow_redirects=False, + ) + + +def test_check_token_authorization_error(mocker): + mock_post = mocker.patch( + "requests.Session.post", + side_effect=[ + _mock_response(mocker, 400, {"detail": "authorization_pending"}), + _mock_response(mocker, 500, {"detail": "unexpected_error"}), + ], + ) + + with pytest.raises(requests.RequestException): + check_token_authorization( + uri="https://example.com/token_uri", device_code="random_device_code" + ) + + assert mock_post.call_count == 2 + assert mock_post.call_args == mocker.call( + "https://example.com/token_uri", + json={"code": "random_device_code"}, + timeout=5, + allow_redirects=False, + ) + + +def test_check_token_authorization_success(mocker): + mock_post = mocker.patch( + "requests.Session.post", + side_effect=[ + _mock_response(mocker, 400, {"detail": "authorization_pending"}), + _mock_response(mocker, 400, {"detail": "authorization_pending"}), + _mock_response(mocker, 200, {"access_token": "isat_token"}), + ], + ) + + assert ( + check_token_authorization( + uri="https://example.com/token_uri", device_code="random_device_code" + ) + == "isat_token" + ) + + assert mock_post.call_count == 3 + assert mock_post.call_args == mocker.call( + "https://example.com/token_uri", + json={"code": "random_device_code"}, + timeout=5, + allow_redirects=False, + ) + + +def _mock_response(mocker, status_code, json): + response = Response() + response.status_code = status_code + mocker.patch.object(response, "json", side_effect=[json]) + + return response From 7a956c44f0187fbf7e3d38bb0f728673388ba10c Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Wed, 8 Nov 2023 17:44:04 +0545 Subject: [PATCH 08/24] Refactor mock_response usage in tests and add test_auth.py This commit refactors the way mock responses are used in test_studio.py to follow DRY principles, changing the private _mock_response to a public reusable function. It also adds a new file, test_auth.py, that provides functional tests for authorisation methods, ensuring their functionality. The patches in test_auth.py helps test different cases like expired authorization and successful authorization. Furthermore, test_auth.py also tests if correct configuration is set on successful authorization. Modifications on the method test_auth_login_arguments in test_auth.py were made to refine test cases, thus making the tests more precise and straightforward. --- tests/func/test_auth.py | 89 +++++++++++++++++++++++++++++++++ tests/unit/command/test_auth.py | 6 +-- tests/unit/utils/test_studio.py | 28 ++++++----- 3 files changed, 106 insertions(+), 17 deletions(-) create mode 100644 tests/func/test_auth.py diff --git a/tests/func/test_auth.py b/tests/func/test_auth.py new file mode 100644 index 0000000000..e9bda76314 --- /dev/null +++ b/tests/func/test_auth.py @@ -0,0 +1,89 @@ +from dvc.cli import main +from dvc.commands.auth import DEFAULT_SCOPES +from dvc.utils.studio import STUDIO_URL +from tests.unit.command.test_auth import MOCK_RESPONSE +from tests.unit.utils.test_studio import mock_response + + +def test_auth_expired(mocker, M): + mock_post = mocker.patch( + "requests.Session.post", + side_effect=[ + mock_response(mocker, 200, MOCK_RESPONSE), + mock_response(mocker, 400, {"detail": "authorization_expired"}), + ], + ) + + assert main(["auth", "login"]) == 1 + + assert mock_post.call_count == 2 + assert mock_post.call_args_list == [ + mocker.call( + f"{STUDIO_URL}/api/device-login", + json={"client_name": "dvc", "token_name": M.any, "scopes": DEFAULT_SCOPES}, + headers=None, + timeout=5, + allow_redirects=False, + ), + mocker.call( + f"{STUDIO_URL}/api/device-login/token", + json={"code": "random-value"}, + timeout=5, + allow_redirects=False, + ), + ] + + +def test_auth_success(mocker, dvc): + mocker.patch("time.sleep") + mock_post = mocker.patch( + "requests.Session.post", + side_effect=[ + mock_response(mocker, 200, MOCK_RESPONSE), + mock_response(mocker, 400, {"detail": "authorization_pending"}), + mock_response(mocker, 200, {"access_token": "isat_access_token"}), + ], + ) + + assert ( + main( + [ + "auth", + "login", + "--name", + "token_name", + "--hostname", + "https://example.com", + "--scopes", + "live", + ] + ) + == 0 + ) + + assert mock_post.call_count == 3 + assert mock_post.call_args_list == [ + mocker.call( + "https://example.com/api/device-login", + json={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, + headers=None, + timeout=5, + allow_redirects=False, + ), + mocker.call( + f"{STUDIO_URL}/api/device-login/token", + json={"code": "random-value"}, + timeout=5, + allow_redirects=False, + ), + mocker.call( + f"{STUDIO_URL}/api/device-login/token", + json={"code": "random-value"}, + timeout=5, + allow_redirects=False, + ), + ] + + config = dvc.config.load_one("global") + assert config["studio"]["token"] == "isat_access_token" + assert config["studio"]["url"] == "https://example.com" diff --git a/tests/unit/command/test_auth.py b/tests/unit/command/test_auth.py index 15d8d77bd4..8d8ed70b74 100644 --- a/tests/unit/command/test_auth.py +++ b/tests/unit/command/test_auth.py @@ -19,7 +19,7 @@ def test_auth_login_invalid_scope(): @mock.patch("dvc.utils.studio.check_token_authorization") @mock.patch("dvc.utils.studio.start_device_login") def test_auth_login_token_check_failed( - mock_start_device_login, mock_check_token_authorization, dvc + mock_start_device_login, mock_check_token_authorization ): mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = None @@ -51,9 +51,7 @@ def test_auth_login_success( @mock.patch("dvc.utils.studio.check_token_authorization") @mock.patch("dvc.utils.studio.start_device_login") -def test_auth_login_arguments( - mock_start_device_login, mock_check_token_authorization, dvc, M -): +def test_auth_login_arguments(mock_start_device_login, mock_check_token_authorization): mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = "isat_access_token" diff --git a/tests/unit/utils/test_studio.py b/tests/unit/utils/test_studio.py index da1a63d717..5f75acdcc3 100644 --- a/tests/unit/utils/test_studio.py +++ b/tests/unit/utils/test_studio.py @@ -77,11 +77,10 @@ def test_env_to_config(): def test_start_device_login(mocker): - response = Response() - response.status_code = 200 - mocker.patch.object(response, "json", side_effect=[{"user_code": "MOCKCODE"}]) - - mock_post = mocker.patch("requests.Session.post", return_value=response) + mock_post = mocker.patch( + "requests.Session.post", + return_value=mock_response(mocker, 200, {"user_code": "MOCKCODE"}), + ) start_device_login( data={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, @@ -98,11 +97,12 @@ def test_start_device_login(mocker): def test_check_token_authorization_expired(mocker): + mocker.patch("time.sleep") mock_post = mocker.patch( "requests.Session.post", side_effect=[ - _mock_response(mocker, 400, {"detail": "authorization_pending"}), - _mock_response(mocker, 400, {"detail": "authorization_expired"}), + mock_response(mocker, 400, {"detail": "authorization_pending"}), + mock_response(mocker, 400, {"detail": "authorization_expired"}), ], ) @@ -123,11 +123,12 @@ def test_check_token_authorization_expired(mocker): def test_check_token_authorization_error(mocker): + mocker.patch("time.sleep") mock_post = mocker.patch( "requests.Session.post", side_effect=[ - _mock_response(mocker, 400, {"detail": "authorization_pending"}), - _mock_response(mocker, 500, {"detail": "unexpected_error"}), + mock_response(mocker, 400, {"detail": "authorization_pending"}), + mock_response(mocker, 500, {"detail": "unexpected_error"}), ], ) @@ -146,12 +147,13 @@ def test_check_token_authorization_error(mocker): def test_check_token_authorization_success(mocker): + mocker.patch("time.sleep") mock_post = mocker.patch( "requests.Session.post", side_effect=[ - _mock_response(mocker, 400, {"detail": "authorization_pending"}), - _mock_response(mocker, 400, {"detail": "authorization_pending"}), - _mock_response(mocker, 200, {"access_token": "isat_token"}), + mock_response(mocker, 400, {"detail": "authorization_pending"}), + mock_response(mocker, 400, {"detail": "authorization_pending"}), + mock_response(mocker, 200, {"access_token": "isat_token"}), ], ) @@ -171,7 +173,7 @@ def test_check_token_authorization_success(mocker): ) -def _mock_response(mocker, status_code, json): +def mock_response(mocker, status_code, json): response = Response() response.status_code = status_code mocker.patch.object(response, "json", side_effect=[json]) From c57a673ecf4eb5c7cb5f082c24c3b76dfcc9eb81 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Wed, 8 Nov 2023 18:21:25 +0545 Subject: [PATCH 09/24] Refactor error handling in `dvc/utils/studio.py` Refactored error handling by removing redundant code blocks and introducing more detailed logging for request exceptions. This makes the failure cases handled explicitly and streamlines the traceability. Also, tests for authorization have been refined and more specific cases like 'expired authorization' have been included. --- dvc/utils/studio.py | 90 +++++++++++++++++++++------------------------ 1 file changed, 41 insertions(+), 49 deletions(-) diff --git a/dvc/utils/studio.py b/dvc/utils/studio.py index 5e1833feaf..3b1fcf62e5 100644 --- a/dvc/utils/studio.py +++ b/dvc/utils/studio.py @@ -63,11 +63,22 @@ def notify_refs( ) data = {"repo_url": repo_url, "client": "dvc", "refs": refs} - d = None try: r = post("webhook/dvc", token, data, base_url=base_url) except requests.RequestException as e: - _handle_request_exception(e) + logger.trace("", exc_info=True) # type: ignore[attr-defined] + + msg = str(e) + if e.response is None: + logger.warning("failed to notify Studio: %s", msg.lower()) + return {} + + r = e.response + d = ignore(Exception, default={})(r.json)() + status = r.status_code + if detail := d.get("detail"): + msg = f"{detail} ({status=})" + logger.warning("failed to notify Studio: %s", msg.lower()) else: d = r.json() @@ -86,18 +97,19 @@ def start_device_login( f" ({base_url})" if base_url else "", ) - try: - r = post("api/device-login", "", data=data, base_url=base_url) - except requests.RequestException as e: - _handle_request_exception(e) - raise - else: - d = r.json() - - if d: - logger.trace( # type: ignore[attr-defined] - "received response: %s (status=%r)", d, r.status_code + r = post("api/device-login", "", data=data, base_url=base_url) + if r.status_code == 400: + logger.error( + "Failed to start authentication with Studio: %s", r.json().get("detail") ) + return + + r.raise_for_status() + d = r.json() + + logger.trace( # type: ignore[attr-defined] + "received response: %s (status=%r)", d, r.status_code + ) return d @@ -114,42 +126,22 @@ def check_token_authorization(*, uri, device_code): counter = 1 while True: - try: - logger.debug("Polling attempt #%s", counter) - r = session.post(uri, json=data, timeout=5, allow_redirects=False) - counter += 1 - if r.status_code == 400: - d = ignore(Exception, default={})(r.json)() - detail = d.get("detail") - if detail == "authorization_pending": - # Wait 5 seconds before retrying. - time.sleep(5) - continue - if detail == "authorization_expired": - return - - r.raise_for_status() - - return r.json()["access_token"] - except requests.RequestException as e: - _handle_request_exception(e) - raise - - -def _handle_request_exception(e): - logger.trace("", exc_info=True) # type: ignore[attr-defined] - - msg = str(e) - if e.response is None: - logger.warning("failed to contact Studio: %s", msg.lower()) - return {} - - r = e.response - d = ignore(Exception, default={})(r.json)() - status = r.status_code - if detail := d.get("detail"): - msg = f"{detail} ({status=})" - logger.warning("failed to contact Studio: %s", msg.lower()) + logger.debug("Polling attempt #%s", counter) + r = session.post(uri, json=data, timeout=5, allow_redirects=False) + counter += 1 + if r.status_code == 400: + d = ignore(Exception, default={})(r.json)() + detail = d.get("detail") + if detail == "authorization_pending": + # Wait 5 seconds before retrying. + time.sleep(5) + continue + if detail == "authorization_expired": + return + + r.raise_for_status() + + return r.json()["access_token"] def config_to_env(config: Dict[str, Any]) -> Dict[str, Any]: From b94e858627a34b1cd43e13470d4db1e85cfbf786 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Thu, 9 Nov 2023 15:39:04 +0545 Subject: [PATCH 10/24] Replace `auth` module with `studio` in DVC Renamed all instances of `auth` to `studio`, as we are moving the authentication process into a new module `studio`. This includes changes to command names, classes, and test file names to reflect the updated module. It improves the organization of the authentication features related to DVC Studio, separating them from other types of authorization. --- dvc/cli/parser.py | 4 +- dvc/commands/{auth.py => studio.py} | 60 +++++++++---------- tests/func/{test_auth.py => test_studio.py} | 10 ++-- .../command/{test_auth.py => test_studio.py} | 36 +++++------ 4 files changed, 56 insertions(+), 54 deletions(-) rename dvc/commands/{auth.py => studio.py} (78%) rename tests/func/{test_auth.py => test_studio.py} (92%) rename tests/unit/command/{test_auth.py => test_studio.py} (79%) diff --git a/dvc/cli/parser.py b/dvc/cli/parser.py index 551c498fbd..31e68ad4d0 100644 --- a/dvc/cli/parser.py +++ b/dvc/cli/parser.py @@ -7,7 +7,6 @@ from dvc.commands import ( add, artifacts, - auth, cache, check_ignore, checkout, @@ -43,6 +42,7 @@ repro, root, stage, + studio, unprotect, update, version, @@ -94,7 +94,7 @@ check_ignore, data, artifacts, - auth, + studio, ] diff --git a/dvc/commands/auth.py b/dvc/commands/studio.py similarity index 78% rename from dvc/commands/auth.py rename to dvc/commands/studio.py index 7fa5076b0a..0cd8d9a558 100644 --- a/dvc/commands/auth.py +++ b/dvc/commands/studio.py @@ -13,7 +13,7 @@ AVAILABLE_SCOPES = ["live", "dvc_experiment", "view_url", "dql", "download_model"] -class CmdAuthLogin(CmdBase): +class CmdStudioLogin(CmdBase): def run(self): from dvc.env import DVC_STUDIO_URL from dvc.repo.experiments.utils import gen_random_name @@ -67,7 +67,7 @@ def initiate_authorization(self, hostname, data): f"A web browser has been opened at \n{verification_uri}.\n" f"Please continue the login in the web browser.\n" f"If no web browser is available or if the web browser fails to open,\n" - f"use device code flow with `dvc auth login --use-device-code`." + f"use device code flow with `dvc studio login --use-device-code`." ) url = f"{verification_uri}?code={user_code}" opened = webbrowser.open(url) @@ -85,7 +85,7 @@ def save_config(self, hostname, token): conf["studio"]["url"] = hostname -class CmdAuthLogout(CmdBase): +class CmdStudioLogout(CmdBase): def run(self): from dvc.ui import ui @@ -100,7 +100,7 @@ def run(self): return 0 -class CmdAuthToken(CmdBase): +class CmdStudioToken(CmdBase): def run(self): from dvc.ui import ui @@ -115,36 +115,36 @@ def run(self): def add_parser(subparsers, parent_parser): - AUTH_HELP = "Authenticate dvc with Iterative Studio" - AUTH_DESCRIPTION = ( + STUDIO_HELP = "Authenticate dvc with Iterative Studio" + STUDIO_DESCRIPTION = ( "Authorize dvc with Studio and set the token. When this is\n" "set, DVC uses this to share live experiments and notify\n" "Studio about pushed experiments." ) - auth_parser = subparsers.add_parser( - "auth", + studio_parser = subparsers.add_parser( + "studio", parents=[parent_parser], - description=append_doc_link(AUTH_DESCRIPTION, "auth"), - help=AUTH_HELP, + description=append_doc_link(STUDIO_DESCRIPTION, "studio"), + help=STUDIO_HELP, formatter_class=argparse.RawDescriptionHelpFormatter, ) - auth_subparsers = auth_parser.add_subparsers( + studio_subparser = studio_parser.add_subparsers( dest="cmd", - help="Use `dvc auth CMD --help` to display command-specific help.", + help="Use `dvc studio CMD --help` to display command-specific help.", ) - fix_subparsers(auth_subparsers) + fix_subparsers(studio_subparser) - AUTH_LOGIN_HELP = "Authenticate DVC with Studio host" - AUTH_LOGIN_DESCRIPTION = ( + STUDIO_LOGIN_HELP = "Authenticate DVC with Studio host" + STUDIO_LOGIN_DESCRIPTION = ( "By default, this command authorize dvc with Studio with\n" " default scopes and a random name as token name." ) - login_parser = auth_subparsers.add_parser( + login_parser = studio_subparser.add_parser( "login", parents=[parent_parser], - description=append_doc_link(AUTH_LOGIN_DESCRIPTION, "auth/login"), - help=AUTH_LOGIN_HELP, + description=append_doc_link(STUDIO_LOGIN_DESCRIPTION, "studio/login"), + help=STUDIO_LOGIN_HELP, formatter_class=argparse.RawDescriptionHelpFormatter, ) @@ -181,31 +181,31 @@ def add_parser(subparsers, parent_parser): "You will be presented with user code to enter in browser.\n" "DVC will also use this if it cannot launch browser on your behalf.", ) - login_parser.set_defaults(func=CmdAuthLogin) + login_parser.set_defaults(func=CmdStudioLogin) - AUTH_LOGOUT_HELP = "Logout user from Studio" - AUTH_LOGOUT_DESCRIPTION = "This command helps to log out user from DVC Studio.\n" + STUDIO_LOGOUT_HELP = "Logout user from Studio" + STUDIO_LOGOUT_DESCRIPTION = "This command helps to log out user from DVC Studio.\n" - logout_parser = auth_subparsers.add_parser( + logout_parser = studio_subparser.add_parser( "logout", parents=[parent_parser], - description=append_doc_link(AUTH_LOGOUT_DESCRIPTION, "auth/logout"), - help=AUTH_LOGOUT_HELP, + description=append_doc_link(STUDIO_LOGOUT_DESCRIPTION, "studio/logout"), + help=STUDIO_LOGOUT_HELP, formatter_class=argparse.RawDescriptionHelpFormatter, ) - logout_parser.set_defaults(func=CmdAuthLogout) + logout_parser.set_defaults(func=CmdStudioLogout) - AUTH_TOKEN_HELP = ( + STUDIO_TOKEN_HELP = ( "View the token dvc uses to contact Studio" # noqa: S105 # nosec B105 ) - logout_parser = auth_subparsers.add_parser( + logout_parser = studio_subparser.add_parser( "token", parents=[parent_parser], - description=append_doc_link(AUTH_TOKEN_HELP, "auth/token"), - help=AUTH_TOKEN_HELP, + description=append_doc_link(STUDIO_TOKEN_HELP, "studio/token"), + help=STUDIO_TOKEN_HELP, formatter_class=argparse.RawDescriptionHelpFormatter, ) - logout_parser.set_defaults(func=CmdAuthToken) + logout_parser.set_defaults(func=CmdStudioToken) diff --git a/tests/func/test_auth.py b/tests/func/test_studio.py similarity index 92% rename from tests/func/test_auth.py rename to tests/func/test_studio.py index e9bda76314..870d70d75c 100644 --- a/tests/func/test_auth.py +++ b/tests/func/test_studio.py @@ -1,7 +1,7 @@ from dvc.cli import main -from dvc.commands.auth import DEFAULT_SCOPES +from dvc.commands.studio import DEFAULT_SCOPES from dvc.utils.studio import STUDIO_URL -from tests.unit.command.test_auth import MOCK_RESPONSE +from tests.unit.command.test_studio import MOCK_RESPONSE from tests.unit.utils.test_studio import mock_response @@ -14,7 +14,7 @@ def test_auth_expired(mocker, M): ], ) - assert main(["auth", "login"]) == 1 + assert main(["studio", "login"]) == 1 assert mock_post.call_count == 2 assert mock_post.call_args_list == [ @@ -34,7 +34,7 @@ def test_auth_expired(mocker, M): ] -def test_auth_success(mocker, dvc): +def test_studio_success(mocker, dvc): mocker.patch("time.sleep") mock_post = mocker.patch( "requests.Session.post", @@ -48,7 +48,7 @@ def test_auth_success(mocker, dvc): assert ( main( [ - "auth", + "studio", "login", "--name", "token_name", diff --git a/tests/unit/command/test_auth.py b/tests/unit/command/test_studio.py similarity index 79% rename from tests/unit/command/test_auth.py rename to tests/unit/command/test_studio.py index 8d8ed70b74..d7edd747fe 100644 --- a/tests/unit/command/test_auth.py +++ b/tests/unit/command/test_studio.py @@ -1,7 +1,7 @@ from unittest import mock from dvc.cli import main -from dvc.commands.auth import DEFAULT_SCOPES +from dvc.commands.studio import DEFAULT_SCOPES from dvc.utils.studio import STUDIO_URL MOCK_RESPONSE = { @@ -12,30 +12,30 @@ } -def test_auth_login_invalid_scope(): - assert main(["auth", "login", "--scopes", "invalid!"]) == 1 +def test_studio_login_invalid_scope(): + assert main(["studio", "login", "--scopes", "invalid!"]) == 1 @mock.patch("dvc.utils.studio.check_token_authorization") @mock.patch("dvc.utils.studio.start_device_login") -def test_auth_login_token_check_failed( +def test_studio_login_token_check_failed( mock_start_device_login, mock_check_token_authorization ): mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = None - assert main(["auth", "login"]) == 1 + assert main(["studio", "login"]) == 1 @mock.patch("dvc.utils.studio.check_token_authorization") @mock.patch("dvc.utils.studio.start_device_login") -def test_auth_login_success( +def test_studio_login_success( mock_start_device_login, mock_check_token_authorization, dvc, M ): mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = "isat_access_token" - assert main(["auth", "login"]) == 0 + assert main(["studio", "login"]) == 0 assert mock_start_device_login.call_args.kwargs == { "base_url": STUDIO_URL, "data": {"client_name": "dvc", "scopes": DEFAULT_SCOPES, "token_name": M.any}, @@ -51,14 +51,16 @@ def test_auth_login_success( @mock.patch("dvc.utils.studio.check_token_authorization") @mock.patch("dvc.utils.studio.start_device_login") -def test_auth_login_arguments(mock_start_device_login, mock_check_token_authorization): +def test_studio_login_arguments( + mock_start_device_login, mock_check_token_authorization +): mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = "isat_access_token" assert ( main( [ - "auth", + "studio", "login", "--name", "token_name", @@ -81,37 +83,37 @@ def test_auth_login_arguments(mock_start_device_login, mock_check_token_authoriz @mock.patch("dvc.utils.studio.check_token_authorization") @mock.patch("dvc.utils.studio.start_device_login") @mock.patch("webbrowser.open") -def test_auth_device_code( +def test_studio_device_code( mock_webbrowser_open, mock_start_device_login, mock_check_token_authorization ): mock_start_device_login.return_value = MOCK_RESPONSE mock_check_token_authorization.return_value = "isat_access_token" - assert main(["auth", "login", "--use-device-code"]) == 0 + assert main(["studio", "login", "--use-device-code"]) == 0 mock_webbrowser_open.assert_not_called() -def test_auth_logout(dvc): +def test_studio_logout(dvc): with dvc.config.edit("global") as conf: conf["studio"]["token"] = "isat_access_token" - assert main(["auth", "logout"]) == 0 + assert main(["studio", "logout"]) == 0 config = dvc.config.load_one("global") assert "token" not in config["studio"] - assert main(["auth", "logout"]) == 1 + assert main(["studio", "logout"]) == 1 @mock.patch("dvc.ui.ui.write") -def test_auth_token(mock_write, dvc): +def test_studio_token(mock_write, dvc): with dvc.config.edit("global") as conf: conf["studio"]["token"] = "isat_access_token" - assert main(["auth", "token"]) == 0 + assert main(["studio", "token"]) == 0 mock_write.assert_called_with("isat_access_token") with dvc.config.edit("global") as conf: del conf["studio"]["token"] - assert main(["auth", "token"]) == 1 + assert main(["studio", "token"]) == 1 From c5b08e4ce814b1124c0031a06b290e598bc715aa Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Fri, 10 Nov 2023 19:24:49 +0545 Subject: [PATCH 11/24] Remove device login methods in utils/studio Removed methods related to device login and token authorization in `utils/studio.py` and its corresponding test cases. These functionalities have been moved to the `dvc-studio-client` module to decouple the login process and make the code maintainable. Updated the dependencies in `pyproject.toml` to use the new `dvc-studio-client` module. Also, updated the command/studio to use the `dvc-studio-client` module for auth functionalities. --- dvc/commands/studio.py | 34 +++++---- dvc/utils/studio.py | 57 --------------- pyproject.toml | 4 +- tests/func/test_studio.py | 65 ++++++++++------- tests/unit/command/test_studio.py | 24 ++++--- tests/unit/utils/test_studio.py | 115 +----------------------------- 6 files changed, 78 insertions(+), 221 deletions(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index 0cd8d9a558..5398e532ed 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -15,25 +15,21 @@ class CmdStudioLogin(CmdBase): def run(self): + from dvc_studio_client.auth import check_token_authorization + from dvc.env import DVC_STUDIO_URL from dvc.repo.experiments.utils import gen_random_name from dvc.ui import ui - from dvc.utils.studio import STUDIO_URL, check_token_authorization - - scopes = self.args.scopes or DEFAULT_SCOPES - if invalid_scopes := list( - filter(lambda s: s not in AVAILABLE_SCOPES, scopes.split(",")) - ): - ui.error_write( - f"Following scopes are not valid: {', '.join(invalid_scopes)}" - ) - return 1 + from dvc.utils.studio import STUDIO_URL name = self.args.name or gen_random_name() hostname = self.args.hostname or os.environ.get(DVC_STUDIO_URL) or STUDIO_URL - data = {"client_name": "dvc", "token_name": name, "scopes": scopes} - device_code, token_uri = self.initiate_authorization(hostname, data) + try: + device_code, token_uri = self.initiate_authorization(name, hostname) + except ValueError as e: + ui.error_write(str(e)) + return 1 access_token = check_token_authorization(uri=token_uri, device_code=device_code) if not access_token: @@ -49,13 +45,21 @@ def run(self): ) return 0 - def initiate_authorization(self, hostname, data): + def initiate_authorization(self, name, hostname): import webbrowser + from dvc_studio_client.auth import start_device_login + from dvc.ui import ui - from dvc.utils.studio import start_device_login - response = start_device_login(data=data, base_url=hostname) + scopes = self.args.scopes or DEFAULT_SCOPES + + response = start_device_login( + client_name="dvc", + base_url=hostname, + token_name=name, + scopes=scopes.split(","), + ) verification_uri = response["verification_uri"] user_code = response["user_code"] device_code = response["device_code"] diff --git a/dvc/utils/studio.py b/dvc/utils/studio.py index 3b1fcf62e5..2df0bfced4 100644 --- a/dvc/utils/studio.py +++ b/dvc/utils/studio.py @@ -87,63 +87,6 @@ def notify_refs( return d -def start_device_login( - *, - data, - base_url=STUDIO_URL, -): - logger.debug( - "Starting device login for Studio%s", - f" ({base_url})" if base_url else "", - ) - - r = post("api/device-login", "", data=data, base_url=base_url) - if r.status_code == 400: - logger.error( - "Failed to start authentication with Studio: %s", r.json().get("detail") - ) - return - - r.raise_for_status() - d = r.json() - - logger.trace( # type: ignore[attr-defined] - "received response: %s (status=%r)", d, r.status_code - ) - return d - - -def check_token_authorization(*, uri, device_code): - import time - - logger.debug("Polling to find if the user code is authorized") - - data = {"code": device_code} - session = requests.Session() - session.mount(uri, HTTPAdapter(max_retries=3)) - - logger.debug("Checking with %s to %s", device_code, uri) - - counter = 1 - while True: - logger.debug("Polling attempt #%s", counter) - r = session.post(uri, json=data, timeout=5, allow_redirects=False) - counter += 1 - if r.status_code == 400: - d = ignore(Exception, default={})(r.json)() - detail = d.get("detail") - if detail == "authorization_pending": - # Wait 5 seconds before retrying. - time.sleep(5) - continue - if detail == "authorization_expired": - return - - r.raise_for_status() - - return r.json()["access_token"] - - def config_to_env(config: Dict[str, Any]) -> Dict[str, Any]: env = {} if "offline" in config: diff --git a/pyproject.toml b/pyproject.toml index 694fd180ec..bc352b5486 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,9 @@ dependencies = [ "dvc-data>=2.22.0,<2.23.0", "dvc-http>=2.29.0", "dvc-render>=0.3.1,<1", - "dvc-studio-client>=0.13.0,<1", + # TODO: To update this once the PR at https://github.com/iterative/dvc-studio-client/pull/113 is merged. + "dvc-studio-client@git+https://github.com/amritghimire/dvc-studio-client@auth", + # "dvc-studio-client>=0.13.0,<1", "dvc-task>=0.3.0,<1", "flatten_dict<1,>=0.4.1", # https://github.com/iterative/dvc/issues/9654 diff --git a/tests/func/test_studio.py b/tests/func/test_studio.py index 870d70d75c..7e56365d88 100644 --- a/tests/func/test_studio.py +++ b/tests/func/test_studio.py @@ -1,30 +1,36 @@ +from requests import Response + from dvc.cli import main from dvc.commands.studio import DEFAULT_SCOPES from dvc.utils.studio import STUDIO_URL from tests.unit.command.test_studio import MOCK_RESPONSE -from tests.unit.utils.test_studio import mock_response def test_auth_expired(mocker, M): - mock_post = mocker.patch( + mock_login_post = mocker.patch( + "requests.post", return_value=_mock_response(mocker, 200, MOCK_RESPONSE) + ) + mock_poll_post = mocker.patch( "requests.Session.post", side_effect=[ - mock_response(mocker, 200, MOCK_RESPONSE), - mock_response(mocker, 400, {"detail": "authorization_expired"}), + _mock_response(mocker, 400, {"detail": "authorization_expired"}), ], ) assert main(["studio", "login"]) == 1 - assert mock_post.call_count == 2 - assert mock_post.call_args_list == [ - mocker.call( - f"{STUDIO_URL}/api/device-login", - json={"client_name": "dvc", "token_name": M.any, "scopes": DEFAULT_SCOPES}, - headers=None, - timeout=5, - allow_redirects=False, - ), + assert mock_login_post.call_args == mocker.call( + url=f"{STUDIO_URL}/api/device-login", + json={ + "client_name": "dvc", + "scopes": DEFAULT_SCOPES.split(","), + "token_name": M.any, + }, + headers={"Content-type": "application/json"}, + timeout=5, + ) + + assert mock_poll_post.call_args_list == [ mocker.call( f"{STUDIO_URL}/api/device-login/token", json={"code": "random-value"}, @@ -36,12 +42,14 @@ def test_auth_expired(mocker, M): def test_studio_success(mocker, dvc): mocker.patch("time.sleep") - mock_post = mocker.patch( + mock_login_post = mocker.patch( + "requests.post", return_value=_mock_response(mocker, 200, MOCK_RESPONSE) + ) + mock_poll_post = mocker.patch( "requests.Session.post", side_effect=[ - mock_response(mocker, 200, MOCK_RESPONSE), - mock_response(mocker, 400, {"detail": "authorization_pending"}), - mock_response(mocker, 200, {"access_token": "isat_access_token"}), + _mock_response(mocker, 400, {"detail": "authorization_pending"}), + _mock_response(mocker, 200, {"access_token": "isat_access_token"}), ], ) @@ -61,15 +69,16 @@ def test_studio_success(mocker, dvc): == 0 ) - assert mock_post.call_count == 3 - assert mock_post.call_args_list == [ + assert mock_login_post.call_args_list == [ mocker.call( - "https://example.com/api/device-login", - json={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, - headers=None, + url="https://example.com/api/device-login", + json={"client_name": "dvc", "token_name": "token_name", "scopes": ["live"]}, + headers={"Content-type": "application/json"}, timeout=5, - allow_redirects=False, - ), + ) + ] + assert mock_poll_post.call_count == 2 + assert mock_poll_post.call_args_list == [ mocker.call( f"{STUDIO_URL}/api/device-login/token", json={"code": "random-value"}, @@ -87,3 +96,11 @@ def test_studio_success(mocker, dvc): config = dvc.config.load_one("global") assert config["studio"]["token"] == "isat_access_token" assert config["studio"]["url"] == "https://example.com" + + +def _mock_response(mocker, status_code, json): + response = Response() + response.status_code = status_code + mocker.patch.object(response, "json", side_effect=[json]) + + return response diff --git a/tests/unit/command/test_studio.py b/tests/unit/command/test_studio.py index d7edd747fe..294889d16f 100644 --- a/tests/unit/command/test_studio.py +++ b/tests/unit/command/test_studio.py @@ -16,8 +16,8 @@ def test_studio_login_invalid_scope(): assert main(["studio", "login", "--scopes", "invalid!"]) == 1 -@mock.patch("dvc.utils.studio.check_token_authorization") -@mock.patch("dvc.utils.studio.start_device_login") +@mock.patch("dvc_studio_client.auth.check_token_authorization") +@mock.patch("dvc_studio_client.auth.start_device_login") def test_studio_login_token_check_failed( mock_start_device_login, mock_check_token_authorization ): @@ -27,8 +27,8 @@ def test_studio_login_token_check_failed( assert main(["studio", "login"]) == 1 -@mock.patch("dvc.utils.studio.check_token_authorization") -@mock.patch("dvc.utils.studio.start_device_login") +@mock.patch("dvc_studio_client.auth.check_token_authorization") +@mock.patch("dvc_studio_client.auth.start_device_login") def test_studio_login_success( mock_start_device_login, mock_check_token_authorization, dvc, M ): @@ -38,7 +38,9 @@ def test_studio_login_success( assert main(["studio", "login"]) == 0 assert mock_start_device_login.call_args.kwargs == { "base_url": STUDIO_URL, - "data": {"client_name": "dvc", "scopes": DEFAULT_SCOPES, "token_name": M.any}, + "client_name": "dvc", + "scopes": DEFAULT_SCOPES.split(","), + "token_name": M.any, } mock_check_token_authorization.assert_called_with( uri=MOCK_RESPONSE["token_uri"], device_code=MOCK_RESPONSE["device_code"] @@ -49,8 +51,8 @@ def test_studio_login_success( assert config["studio"]["url"] == STUDIO_URL -@mock.patch("dvc.utils.studio.check_token_authorization") -@mock.patch("dvc.utils.studio.start_device_login") +@mock.patch("dvc_studio_client.auth.check_token_authorization") +@mock.patch("dvc_studio_client.auth.start_device_login") def test_studio_login_arguments( mock_start_device_login, mock_check_token_authorization ): @@ -74,14 +76,16 @@ def test_studio_login_arguments( ) mock_start_device_login.assert_called_with( - data={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, + client_name="dvc", base_url="https://example.com", + token_name="token_name", + scopes=["live"], ) mock_check_token_authorization.assert_called() -@mock.patch("dvc.utils.studio.check_token_authorization") -@mock.patch("dvc.utils.studio.start_device_login") +@mock.patch("dvc_studio_client.auth.check_token_authorization") +@mock.patch("dvc_studio_client.auth.start_device_login") @mock.patch("webbrowser.open") def test_studio_device_code( mock_webbrowser_open, mock_start_device_login, mock_check_token_authorization diff --git a/tests/unit/utils/test_studio.py b/tests/unit/utils/test_studio.py index 5f75acdcc3..7c0a99f85c 100644 --- a/tests/unit/utils/test_studio.py +++ b/tests/unit/utils/test_studio.py @@ -1,7 +1,6 @@ from urllib.parse import urljoin import pytest -import requests from requests import Response from dvc.env import ( @@ -10,14 +9,7 @@ DVC_STUDIO_TOKEN, DVC_STUDIO_URL, ) -from dvc.utils.studio import ( - STUDIO_URL, - check_token_authorization, - config_to_env, - env_to_config, - notify_refs, - start_device_login, -) +from dvc.utils.studio import STUDIO_URL, config_to_env, env_to_config, notify_refs CONFIG = {"offline": True, "repo_url": "repo_url", "token": "token", "url": "url"} @@ -74,108 +66,3 @@ def test_config_to_env(): def test_env_to_config(): assert env_to_config(ENV) == CONFIG - - -def test_start_device_login(mocker): - mock_post = mocker.patch( - "requests.Session.post", - return_value=mock_response(mocker, 200, {"user_code": "MOCKCODE"}), - ) - - start_device_login( - data={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, - base_url="https://example.com", - ) - assert mock_post.called - assert mock_post.call_args == mocker.call( - "https://example.com/api/device-login", - json={"client_name": "dvc", "token_name": "token_name", "scopes": "live"}, - headers=None, - timeout=5, - allow_redirects=False, - ) - - -def test_check_token_authorization_expired(mocker): - mocker.patch("time.sleep") - mock_post = mocker.patch( - "requests.Session.post", - side_effect=[ - mock_response(mocker, 400, {"detail": "authorization_pending"}), - mock_response(mocker, 400, {"detail": "authorization_expired"}), - ], - ) - - assert ( - check_token_authorization( - uri="https://example.com/token_uri", device_code="random_device_code" - ) - is None - ) - - assert mock_post.call_count == 2 - assert mock_post.call_args == mocker.call( - "https://example.com/token_uri", - json={"code": "random_device_code"}, - timeout=5, - allow_redirects=False, - ) - - -def test_check_token_authorization_error(mocker): - mocker.patch("time.sleep") - mock_post = mocker.patch( - "requests.Session.post", - side_effect=[ - mock_response(mocker, 400, {"detail": "authorization_pending"}), - mock_response(mocker, 500, {"detail": "unexpected_error"}), - ], - ) - - with pytest.raises(requests.RequestException): - check_token_authorization( - uri="https://example.com/token_uri", device_code="random_device_code" - ) - - assert mock_post.call_count == 2 - assert mock_post.call_args == mocker.call( - "https://example.com/token_uri", - json={"code": "random_device_code"}, - timeout=5, - allow_redirects=False, - ) - - -def test_check_token_authorization_success(mocker): - mocker.patch("time.sleep") - mock_post = mocker.patch( - "requests.Session.post", - side_effect=[ - mock_response(mocker, 400, {"detail": "authorization_pending"}), - mock_response(mocker, 400, {"detail": "authorization_pending"}), - mock_response(mocker, 200, {"access_token": "isat_token"}), - ], - ) - - assert ( - check_token_authorization( - uri="https://example.com/token_uri", device_code="random_device_code" - ) - == "isat_token" - ) - - assert mock_post.call_count == 3 - assert mock_post.call_args == mocker.call( - "https://example.com/token_uri", - json={"code": "random_device_code"}, - timeout=5, - allow_redirects=False, - ) - - -def mock_response(mocker, status_code, json): - response = Response() - response.status_code = status_code - mocker.patch.object(response, "json", side_effect=[json]) - - return response From fb03545ca273aa46010b06e426e9ece4a8598f32 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 10 Nov 2023 13:44:11 +0000 Subject: [PATCH 12/24] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- dvc/commands/studio.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index 5398e532ed..a5c0ac7719 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -200,9 +200,7 @@ def add_parser(subparsers, parent_parser): logout_parser.set_defaults(func=CmdStudioLogout) - STUDIO_TOKEN_HELP = ( - "View the token dvc uses to contact Studio" # noqa: S105 # nosec B105 - ) + STUDIO_TOKEN_HELP = "View the token dvc uses to contact Studio" # noqa: S105 # nosec B105 logout_parser = studio_subparser.add_parser( "token", From 7d5315d902bd2335a13791296fc7af840bbd2eba Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 21 Nov 2023 17:25:28 +0545 Subject: [PATCH 13/24] Update dvc-studio-client dependency in pyproject.toml The "dvc-studio-client" dependency in pyproject.toml is updated from a specific git commit to v0.16.0, removing the TODO comment. This change was made because the necessary features from the PR linked in the TODO comment have been merged and released in v0.16.0, making the git commit unnecessary. Now the code is cleaner and easier to track with versioned dependency. --- pyproject.toml | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index bc352b5486..1892fb7252 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,9 +37,7 @@ dependencies = [ "dvc-data>=2.22.0,<2.23.0", "dvc-http>=2.29.0", "dvc-render>=0.3.1,<1", - # TODO: To update this once the PR at https://github.com/iterative/dvc-studio-client/pull/113 is merged. - "dvc-studio-client@git+https://github.com/amritghimire/dvc-studio-client@auth", - # "dvc-studio-client>=0.13.0,<1", + "dvc-studio-client>=0.16.0,<1", "dvc-task>=0.3.0,<1", "flatten_dict<1,>=0.4.1", # https://github.com/iterative/dvc/issues/9654 From 0be99a8d3400607663e29fc2253610510edd77a8 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Thu, 23 Nov 2023 15:10:18 +0545 Subject: [PATCH 14/24] Update DVC Studio components and simplify authentication process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit modifies several parts of the ‘dvc/commands/studio.py’ and updates DVC Studio Client's version in 'pyproject.toml' for compatibility with the changes. Primarily, the authentication process has been simplified. The unnecessary generation of a random token name is removed while initiating authorization in the run method of CmdStudioLogin class, enhancing the code quality. Changes made in 'dvc/commands/studio.py' are mainly restructuring the code to adapt to the newer version of DVC Studio Client, including changes to the scopes and methods used from this client. The corresponding changes were also reflected in unit tests. The test files 'tests/func/test_studio.py' and 'tests/unit/command/test_studio.py' are updated in accordance with the approach now used in 'dvc/commands/studio.py'. --- dvc/commands/studio.py | 75 +++++++------------------------ pyproject.toml | 2 +- tests/func/test_studio.py | 1 - tests/unit/command/test_studio.py | 5 ++- 4 files changed, 22 insertions(+), 61 deletions(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index a5c0ac7719..6ef2c1eeb8 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -1,95 +1,54 @@ import argparse -import logging import os from funcy import get_in -from dvc.cli.command import CmdBase from dvc.cli.utils import append_doc_link, fix_subparsers +from dvc.commands.config import CmdConfig +from dvc.log import logger -logger = logging.getLogger(__name__) +logger = logger.getChild(__name__) DEFAULT_SCOPES = "live,dvc_experiment,view_url,dql,download_model" -AVAILABLE_SCOPES = ["live", "dvc_experiment", "view_url", "dql", "download_model"] -class CmdStudioLogin(CmdBase): +class CmdStudioLogin(CmdConfig): def run(self): - from dvc_studio_client.auth import check_token_authorization + from dvc_studio_client.auth import StudioAuthError, initiate_authorization from dvc.env import DVC_STUDIO_URL - from dvc.repo.experiments.utils import gen_random_name from dvc.ui import ui from dvc.utils.studio import STUDIO_URL - name = self.args.name or gen_random_name() + name = self.args.name hostname = self.args.hostname or os.environ.get(DVC_STUDIO_URL) or STUDIO_URL + scopes = self.args.scopes or DEFAULT_SCOPES try: - device_code, token_uri = self.initiate_authorization(name, hostname) - except ValueError as e: - ui.error_write(str(e)) - return 1 - - access_token = check_token_authorization(uri=token_uri, device_code=device_code) - if not access_token: - ui.write( - "failed to authenticate: This 'device_code' has expired.(expired_token)" + token_name, access_token = initiate_authorization( + name=name, + hostname=hostname, + scopes=scopes, + use_device_code=self.args.use_device_code, ) + except StudioAuthError as e: + ui.error_write(str(e)) return 1 self.save_config(hostname, access_token) ui.write( f"Authentication successful. The token will be " - f"available as {name} in Studio profile." + f"available as {token_name} in Studio profile." ) return 0 - def initiate_authorization(self, name, hostname): - import webbrowser - - from dvc_studio_client.auth import start_device_login - - from dvc.ui import ui - - scopes = self.args.scopes or DEFAULT_SCOPES - - response = start_device_login( - client_name="dvc", - base_url=hostname, - token_name=name, - scopes=scopes.split(","), - ) - verification_uri = response["verification_uri"] - user_code = response["user_code"] - device_code = response["device_code"] - token_uri = response["token_uri"] - - opened = False - if not self.args.use_device_code: - ui.write( - f"A web browser has been opened at \n{verification_uri}.\n" - f"Please continue the login in the web browser.\n" - f"If no web browser is available or if the web browser fails to open,\n" - f"use device code flow with `dvc studio login --use-device-code`." - ) - url = f"{verification_uri}?code={user_code}" - opened = webbrowser.open(url) - - if not opened: - ui.write( - f"Please open the following url in your browser.\n{verification_uri}" - ) - ui.write(f"And enter the user code below {user_code} to authorize.") - return device_code, token_uri - def save_config(self, hostname, token): with self.config.edit("global") as conf: conf["studio"]["token"] = token conf["studio"]["url"] = hostname -class CmdStudioLogout(CmdBase): +class CmdStudioLogout(CmdConfig): def run(self): from dvc.ui import ui @@ -104,7 +63,7 @@ def run(self): return 0 -class CmdStudioToken(CmdBase): +class CmdStudioToken(CmdConfig): def run(self): from dvc.ui import ui diff --git a/pyproject.toml b/pyproject.toml index 1892fb7252..56eb56476f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,7 @@ dependencies = [ "dvc-data>=2.22.0,<2.23.0", "dvc-http>=2.29.0", "dvc-render>=0.3.1,<1", - "dvc-studio-client>=0.16.0,<1", + "dvc-studio-client>=0.16.1,<1", "dvc-task>=0.3.0,<1", "flatten_dict<1,>=0.4.1", # https://github.com/iterative/dvc/issues/9654 diff --git a/tests/func/test_studio.py b/tests/func/test_studio.py index 7e56365d88..1d35ccf5c2 100644 --- a/tests/func/test_studio.py +++ b/tests/func/test_studio.py @@ -24,7 +24,6 @@ def test_auth_expired(mocker, M): json={ "client_name": "dvc", "scopes": DEFAULT_SCOPES.split(","), - "token_name": M.any, }, headers={"Content-type": "application/json"}, timeout=5, diff --git a/tests/unit/command/test_studio.py b/tests/unit/command/test_studio.py index 294889d16f..cecce5e01a 100644 --- a/tests/unit/command/test_studio.py +++ b/tests/unit/command/test_studio.py @@ -1,5 +1,7 @@ from unittest import mock +from dvc_studio_client.auth import AuthorizationExpired + from dvc.cli import main from dvc.commands.studio import DEFAULT_SCOPES from dvc.utils.studio import STUDIO_URL @@ -9,6 +11,7 @@ "user_code": "MOCKCODE", "device_code": "random-value", "token_uri": STUDIO_URL + "/api/device-login/token", + "token_name": "random-name", } @@ -22,7 +25,7 @@ def test_studio_login_token_check_failed( mock_start_device_login, mock_check_token_authorization ): mock_start_device_login.return_value = MOCK_RESPONSE - mock_check_token_authorization.return_value = None + mock_check_token_authorization.side_effect = AuthorizationExpired assert main(["studio", "login"]) == 1 From a2fe144b7275eb632a07d7463846ec3b0bb28fbe Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Mon, 27 Nov 2023 11:39:59 +0545 Subject: [PATCH 15/24] Remove unnecessary tests and update StudioClient methods Removed functional test file `test_studio.py`from DVC to move it to studio client. The new mock response no longer needs a full testing file, simplifying our test environment. In `dvc/commands/studio.py`, adjusted function imports to align with new StudioClient function names, which better indicate their purpose: `initiate_authorization` changed to `get_access_token`. Additionally, `DEFAULT_SCOPES` was removed from both `dvc/commands/studio.py` and `tests/unit/command/test_studio.py`, simplifying the token acquisition process and removing unnecessary global variable. The `--scopes` command, if needed, can be passed directly without requiring a default fallback. The corresponding changes were reflected in unit tests : the mock functions in `tests/unit/command/test_studio.py` were changed to align with the new authentication method, and updated the usage of the `get_access_token` function. --- dvc/commands/studio.py | 11 ++-- dvc/utils/studio.py | 2 +- pyproject.toml | 2 +- tests/func/test_studio.py | 105 ------------------------------ tests/unit/command/test_studio.py | 71 +++++--------------- tests/unit/utils/test_studio.py | 1 + 6 files changed, 26 insertions(+), 166 deletions(-) delete mode 100644 tests/func/test_studio.py diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index 6ef2c1eeb8..81d2762816 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -9,12 +9,10 @@ logger = logger.getChild(__name__) -DEFAULT_SCOPES = "live,dvc_experiment,view_url,dql,download_model" - class CmdStudioLogin(CmdConfig): def run(self): - from dvc_studio_client.auth import StudioAuthError, initiate_authorization + from dvc_studio_client.auth import StudioAuthError, get_access_token from dvc.env import DVC_STUDIO_URL from dvc.ui import ui @@ -22,14 +20,15 @@ def run(self): name = self.args.name hostname = self.args.hostname or os.environ.get(DVC_STUDIO_URL) or STUDIO_URL - scopes = self.args.scopes or DEFAULT_SCOPES + scopes = self.args.scopes try: - token_name, access_token = initiate_authorization( - name=name, + token_name, access_token = get_access_token( + token_name=name, hostname=hostname, scopes=scopes, use_device_code=self.args.use_device_code, + client_name="dvc", ) except StudioAuthError as e: ui.error_write(str(e)) diff --git a/dvc/utils/studio.py b/dvc/utils/studio.py index 2df0bfced4..2e0b1fc95f 100644 --- a/dvc/utils/studio.py +++ b/dvc/utils/studio.py @@ -66,7 +66,7 @@ def notify_refs( try: r = post("webhook/dvc", token, data, base_url=base_url) except requests.RequestException as e: - logger.trace("", exc_info=True) # type: ignore[attr-defined] + logger.trace("", exc_info=True) msg = str(e) if e.response is None: diff --git a/pyproject.toml b/pyproject.toml index 56eb56476f..0d6b113339 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,7 @@ dependencies = [ "dvc-data>=2.22.0,<2.23.0", "dvc-http>=2.29.0", "dvc-render>=0.3.1,<1", - "dvc-studio-client>=0.16.1,<1", + "dvc-studio-client>=0.17.0,<1", "dvc-task>=0.3.0,<1", "flatten_dict<1,>=0.4.1", # https://github.com/iterative/dvc/issues/9654 diff --git a/tests/func/test_studio.py b/tests/func/test_studio.py deleted file mode 100644 index 1d35ccf5c2..0000000000 --- a/tests/func/test_studio.py +++ /dev/null @@ -1,105 +0,0 @@ -from requests import Response - -from dvc.cli import main -from dvc.commands.studio import DEFAULT_SCOPES -from dvc.utils.studio import STUDIO_URL -from tests.unit.command.test_studio import MOCK_RESPONSE - - -def test_auth_expired(mocker, M): - mock_login_post = mocker.patch( - "requests.post", return_value=_mock_response(mocker, 200, MOCK_RESPONSE) - ) - mock_poll_post = mocker.patch( - "requests.Session.post", - side_effect=[ - _mock_response(mocker, 400, {"detail": "authorization_expired"}), - ], - ) - - assert main(["studio", "login"]) == 1 - - assert mock_login_post.call_args == mocker.call( - url=f"{STUDIO_URL}/api/device-login", - json={ - "client_name": "dvc", - "scopes": DEFAULT_SCOPES.split(","), - }, - headers={"Content-type": "application/json"}, - timeout=5, - ) - - assert mock_poll_post.call_args_list == [ - mocker.call( - f"{STUDIO_URL}/api/device-login/token", - json={"code": "random-value"}, - timeout=5, - allow_redirects=False, - ), - ] - - -def test_studio_success(mocker, dvc): - mocker.patch("time.sleep") - mock_login_post = mocker.patch( - "requests.post", return_value=_mock_response(mocker, 200, MOCK_RESPONSE) - ) - mock_poll_post = mocker.patch( - "requests.Session.post", - side_effect=[ - _mock_response(mocker, 400, {"detail": "authorization_pending"}), - _mock_response(mocker, 200, {"access_token": "isat_access_token"}), - ], - ) - - assert ( - main( - [ - "studio", - "login", - "--name", - "token_name", - "--hostname", - "https://example.com", - "--scopes", - "live", - ] - ) - == 0 - ) - - assert mock_login_post.call_args_list == [ - mocker.call( - url="https://example.com/api/device-login", - json={"client_name": "dvc", "token_name": "token_name", "scopes": ["live"]}, - headers={"Content-type": "application/json"}, - timeout=5, - ) - ] - assert mock_poll_post.call_count == 2 - assert mock_poll_post.call_args_list == [ - mocker.call( - f"{STUDIO_URL}/api/device-login/token", - json={"code": "random-value"}, - timeout=5, - allow_redirects=False, - ), - mocker.call( - f"{STUDIO_URL}/api/device-login/token", - json={"code": "random-value"}, - timeout=5, - allow_redirects=False, - ), - ] - - config = dvc.config.load_one("global") - assert config["studio"]["token"] == "isat_access_token" - assert config["studio"]["url"] == "https://example.com" - - -def _mock_response(mocker, status_code, json): - response = Response() - response.status_code = status_code - mocker.patch.object(response, "json", side_effect=[json]) - - return response diff --git a/tests/unit/command/test_studio.py b/tests/unit/command/test_studio.py index cecce5e01a..98546572a3 100644 --- a/tests/unit/command/test_studio.py +++ b/tests/unit/command/test_studio.py @@ -3,64 +3,33 @@ from dvc_studio_client.auth import AuthorizationExpired from dvc.cli import main -from dvc.commands.studio import DEFAULT_SCOPES from dvc.utils.studio import STUDIO_URL -MOCK_RESPONSE = { - "verification_uri": STUDIO_URL + "/auth/device-login", - "user_code": "MOCKCODE", - "device_code": "random-value", - "token_uri": STUDIO_URL + "/api/device-login/token", - "token_name": "random-name", -} - def test_studio_login_invalid_scope(): assert main(["studio", "login", "--scopes", "invalid!"]) == 1 -@mock.patch("dvc_studio_client.auth.check_token_authorization") -@mock.patch("dvc_studio_client.auth.start_device_login") -def test_studio_login_token_check_failed( - mock_start_device_login, mock_check_token_authorization -): - mock_start_device_login.return_value = MOCK_RESPONSE - mock_check_token_authorization.side_effect = AuthorizationExpired +@mock.patch("dvc_studio_client.auth.get_access_token") +def test_studio_login_token_check_failed(mock_get_access_token): + mock_get_access_token.side_effect = AuthorizationExpired assert main(["studio", "login"]) == 1 -@mock.patch("dvc_studio_client.auth.check_token_authorization") -@mock.patch("dvc_studio_client.auth.start_device_login") -def test_studio_login_success( - mock_start_device_login, mock_check_token_authorization, dvc, M -): - mock_start_device_login.return_value = MOCK_RESPONSE - mock_check_token_authorization.return_value = "isat_access_token" - +@mock.patch("dvc_studio_client.auth.get_access_token") +def test_studio_login_success(mock_get_access_token, dvc): + mock_get_access_token.return_value = ("token_name", "isat_access_token") assert main(["studio", "login"]) == 0 - assert mock_start_device_login.call_args.kwargs == { - "base_url": STUDIO_URL, - "client_name": "dvc", - "scopes": DEFAULT_SCOPES.split(","), - "token_name": M.any, - } - mock_check_token_authorization.assert_called_with( - uri=MOCK_RESPONSE["token_uri"], device_code=MOCK_RESPONSE["device_code"] - ) config = dvc.config.load_one("global") assert config["studio"]["token"] == "isat_access_token" assert config["studio"]["url"] == STUDIO_URL -@mock.patch("dvc_studio_client.auth.check_token_authorization") -@mock.patch("dvc_studio_client.auth.start_device_login") -def test_studio_login_arguments( - mock_start_device_login, mock_check_token_authorization -): - mock_start_device_login.return_value = MOCK_RESPONSE - mock_check_token_authorization.return_value = "isat_access_token" +@mock.patch("dvc_studio_client.auth.get_access_token") +def test_studio_login_arguments(mock_get_access_token): + mock_get_access_token.return_value = ("token_name", "isat+access_token") assert ( main( @@ -72,29 +41,25 @@ def test_studio_login_arguments( "--hostname", "https://example.com", "--scopes", - "live", + "experiments", ] ) == 0 ) - mock_start_device_login.assert_called_with( - client_name="dvc", - base_url="https://example.com", + mock_get_access_token.assert_called_with( token_name="token_name", - scopes=["live"], + hostname="https://example.com", + scopes="experiments", + use_device_code=False, + client_name="dvc", ) - mock_check_token_authorization.assert_called() -@mock.patch("dvc_studio_client.auth.check_token_authorization") -@mock.patch("dvc_studio_client.auth.start_device_login") +@mock.patch("dvc_studio_client.auth.get_access_token") @mock.patch("webbrowser.open") -def test_studio_device_code( - mock_webbrowser_open, mock_start_device_login, mock_check_token_authorization -): - mock_start_device_login.return_value = MOCK_RESPONSE - mock_check_token_authorization.return_value = "isat_access_token" +def test_studio_device_code(mock_webbrowser_open, mock_get_access_token): + mock_get_access_token.return_value = ("token_name", "isat+access_token") assert main(["studio", "login", "--use-device-code"]) == 0 diff --git a/tests/unit/utils/test_studio.py b/tests/unit/utils/test_studio.py index 7c0a99f85c..68dcfed77d 100644 --- a/tests/unit/utils/test_studio.py +++ b/tests/unit/utils/test_studio.py @@ -13,6 +13,7 @@ CONFIG = {"offline": True, "repo_url": "repo_url", "token": "token", "url": "url"} + ENV = { DVC_STUDIO_OFFLINE: True, DVC_STUDIO_REPO_URL: "repo_url", From 1781e7d13b8d61794920ceb978cc2771f8479778 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Mon, 27 Nov 2023 11:42:56 +0545 Subject: [PATCH 16/24] Update token success message in dvc/commands/studio.py This commit replaces f-string (formatted string literal) with a regular string for the success message after token authentication. The change was necessary as the variable `token_name` is already being passed as a formatted string, thus no need for f-string in the success message sentence. This makes code cleaner and more consistent. --- dvc/commands/studio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index 81d2762816..3b803d0843 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -36,7 +36,7 @@ def run(self): self.save_config(hostname, access_token) ui.write( - f"Authentication successful. The token will be " + "Authentication successful. The token will be " f"available as {token_name} in Studio profile." ) return 0 From 8776227b80a29890b59d877e8fdb886925c5d511 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Wed, 29 Nov 2023 14:59:02 +0545 Subject: [PATCH 17/24] Refactor tests in `test_studio.py` for better readability --- tests/unit/command/test_studio.py | 50 +++++++++++++------------------ 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/tests/unit/command/test_studio.py b/tests/unit/command/test_studio.py index 98546572a3..3adb835418 100644 --- a/tests/unit/command/test_studio.py +++ b/tests/unit/command/test_studio.py @@ -1,25 +1,23 @@ -from unittest import mock - from dvc_studio_client.auth import AuthorizationExpired from dvc.cli import main from dvc.utils.studio import STUDIO_URL -def test_studio_login_invalid_scope(): - assert main(["studio", "login", "--scopes", "invalid!"]) == 1 - - -@mock.patch("dvc_studio_client.auth.get_access_token") -def test_studio_login_token_check_failed(mock_get_access_token): - mock_get_access_token.side_effect = AuthorizationExpired +def test_studio_login_token_check_failed(mocker): + mocker.patch( + "dvc_studio_client.auth.get_access_token", side_effect=AuthorizationExpired + ) assert main(["studio", "login"]) == 1 -@mock.patch("dvc_studio_client.auth.get_access_token") -def test_studio_login_success(mock_get_access_token, dvc): - mock_get_access_token.return_value = ("token_name", "isat_access_token") +def test_studio_login_success(mocker, dvc): + mocker.patch( + "dvc_studio_client.auth.get_access_token", + return_value=("token_name", "isat_access_token"), + ) + assert main(["studio", "login"]) == 0 config = dvc.config.load_one("global") @@ -27,9 +25,11 @@ def test_studio_login_success(mock_get_access_token, dvc): assert config["studio"]["url"] == STUDIO_URL -@mock.patch("dvc_studio_client.auth.get_access_token") -def test_studio_login_arguments(mock_get_access_token): - mock_get_access_token.return_value = ("token_name", "isat+access_token") +def test_studio_login_arguments(mocker): + mock = mocker.patch( + "dvc_studio_client.auth.get_access_token", + return_value=("token_name", "isat_access_token"), + ) assert ( main( @@ -42,30 +42,21 @@ def test_studio_login_arguments(mock_get_access_token): "https://example.com", "--scopes", "experiments", + "--use-device-code", ] ) == 0 ) - mock_get_access_token.assert_called_with( + mock.assert_called_with( token_name="token_name", hostname="https://example.com", scopes="experiments", - use_device_code=False, + use_device_code=True, client_name="dvc", ) -@mock.patch("dvc_studio_client.auth.get_access_token") -@mock.patch("webbrowser.open") -def test_studio_device_code(mock_webbrowser_open, mock_get_access_token): - mock_get_access_token.return_value = ("token_name", "isat+access_token") - - assert main(["studio", "login", "--use-device-code"]) == 0 - - mock_webbrowser_open.assert_not_called() - - def test_studio_logout(dvc): with dvc.config.edit("global") as conf: conf["studio"]["token"] = "isat_access_token" @@ -77,13 +68,12 @@ def test_studio_logout(dvc): assert main(["studio", "logout"]) == 1 -@mock.patch("dvc.ui.ui.write") -def test_studio_token(mock_write, dvc): +def test_studio_token(dvc, capsys): with dvc.config.edit("global") as conf: conf["studio"]["token"] = "isat_access_token" assert main(["studio", "token"]) == 0 - mock_write.assert_called_with("isat_access_token") + assert capsys.readouterr().out == "isat_access_token\n" with dvc.config.edit("global") as conf: del conf["studio"]["token"] From 3eb26d117f5ff69aa128a570694ea81c2395d023 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Mon, 4 Dec 2023 20:36:53 +0545 Subject: [PATCH 18/24] Updated terminology in `studio.py` and `test_studio.py` Changes terminology from 'authorize' to 'authenticate' to better align with common practices and improve understanding. Also fixed the `get_access_token` function in 'test_studio.py' where 'AuthorizationExpired' was replaced with 'AuthenticationExpired' for better error handling. --- dvc/commands/studio.py | 4 ++-- pyproject.toml | 2 +- tests/unit/command/test_studio.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index 3b803d0843..c04a5d4d4c 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -79,7 +79,7 @@ def run(self): def add_parser(subparsers, parent_parser): STUDIO_HELP = "Authenticate dvc with Iterative Studio" STUDIO_DESCRIPTION = ( - "Authorize dvc with Studio and set the token. When this is\n" + "Authenticate dvc with Studio and set the token. When this is\n" "set, DVC uses this to share live experiments and notify\n" "Studio about pushed experiments." ) @@ -99,7 +99,7 @@ def add_parser(subparsers, parent_parser): STUDIO_LOGIN_HELP = "Authenticate DVC with Studio host" STUDIO_LOGIN_DESCRIPTION = ( - "By default, this command authorize dvc with Studio with\n" + "By default, this command authenticate dvc with Studio with\n" " default scopes and a random name as token name." ) login_parser = studio_subparser.add_parser( diff --git a/pyproject.toml b/pyproject.toml index f8db652d20..d4f8962281 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -37,7 +37,7 @@ dependencies = [ "dvc-data>=2.22.1,<2.23.0", "dvc-http>=2.29.0", "dvc-render>=0.3.1,<1", - "dvc-studio-client>=0.17.0,<1", + "dvc-studio-client>=0.17.1,<1", "dvc-task>=0.3.0,<1", "flatten_dict<1,>=0.4.1", # https://github.com/iterative/dvc/issues/9654 diff --git a/tests/unit/command/test_studio.py b/tests/unit/command/test_studio.py index 3adb835418..df7fc49ed0 100644 --- a/tests/unit/command/test_studio.py +++ b/tests/unit/command/test_studio.py @@ -1,4 +1,4 @@ -from dvc_studio_client.auth import AuthorizationExpired +from dvc_studio_client.auth import AuthenticationExpired from dvc.cli import main from dvc.utils.studio import STUDIO_URL @@ -6,7 +6,7 @@ def test_studio_login_token_check_failed(mocker): mocker.patch( - "dvc_studio_client.auth.get_access_token", side_effect=AuthorizationExpired + "dvc_studio_client.auth.get_access_token", side_effect=AuthenticationExpired ) assert main(["studio", "login"]) == 1 From c269930353ccd34b75ec134fef984c2a8962df02 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Mon, 4 Dec 2023 20:44:27 +0545 Subject: [PATCH 19/24] Change 'dvc' to 'DVC' in `studio.py` Updates all instances of 'dvc' to 'DVC' in `studio.py` for enhanced consistency with the rest of the project. This change aims to standardize the usage of the term across all project files. --- dvc/commands/studio.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index c04a5d4d4c..a882315a15 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -28,7 +28,7 @@ def run(self): hostname=hostname, scopes=scopes, use_device_code=self.args.use_device_code, - client_name="dvc", + client_name="DVC", ) except StudioAuthError as e: ui.error_write(str(e)) @@ -77,9 +77,9 @@ def run(self): def add_parser(subparsers, parent_parser): - STUDIO_HELP = "Authenticate dvc with Iterative Studio" + STUDIO_HELP = "Authenticate DVC with Iterative Studio" STUDIO_DESCRIPTION = ( - "Authenticate dvc with Studio and set the token. When this is\n" + "Authenticate DVC with Studio and set the token. When this is\n" "set, DVC uses this to share live experiments and notify\n" "Studio about pushed experiments." ) @@ -93,13 +93,13 @@ def add_parser(subparsers, parent_parser): ) studio_subparser = studio_parser.add_subparsers( dest="cmd", - help="Use `dvc studio CMD --help` to display command-specific help.", + help="Use `DVC studio CMD --help` to display command-specific help.", ) fix_subparsers(studio_subparser) STUDIO_LOGIN_HELP = "Authenticate DVC with Studio host" STUDIO_LOGIN_DESCRIPTION = ( - "By default, this command authenticate dvc with Studio with\n" + "By default, this command authenticate DVC with Studio with\n" " default scopes and a random name as token name." ) login_parser = studio_subparser.add_parser( From 842315521a1d322eb2220e2eee9c482944d3934c Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Mon, 4 Dec 2023 21:12:07 +0545 Subject: [PATCH 20/24] Improve Studio commands information clarity Updated descriptions in various commands on 'dvc/commands/studio.py' focused on improving clarity and context. Authentication message, command helper, and descriptions have been expanded to provide better understanding. Also, 'dvc' has been changed to 'DVC' in 'studio.py' for consistency with the rest of the project files. --- dvc/commands/studio.py | 23 ++++++++++++++--------- tests/unit/command/test_studio.py | 2 +- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index a882315a15..9324517fd5 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -36,8 +36,9 @@ def run(self): self.save_config(hostname, access_token) ui.write( - "Authentication successful. The token will be " - f"available as {token_name} in Studio profile." + "Authentication has been successfully completed." + "The generated token will now be accessible as" + f" {token_name} in the user's Studio profile." ) return 0 @@ -77,11 +78,13 @@ def run(self): def add_parser(subparsers, parent_parser): - STUDIO_HELP = "Authenticate DVC with Iterative Studio" + STUDIO_HELP = "Commands to authenticate DVC with Iterative Studio" STUDIO_DESCRIPTION = ( - "Authenticate DVC with Studio and set the token. When this is\n" - "set, DVC uses this to share live experiments and notify\n" - "Studio about pushed experiments." + "Authenticate DVC with Studio and set the token." + " Once this token has been properly configured,\n" + " DVC will utilize it for seamlessly sharing live experiments\n" + " and sending notifications to Studio regarding any experiments" + " that have been pushed." ) studio_parser = subparsers.add_parser( @@ -99,8 +102,8 @@ def add_parser(subparsers, parent_parser): STUDIO_LOGIN_HELP = "Authenticate DVC with Studio host" STUDIO_LOGIN_DESCRIPTION = ( - "By default, this command authenticate DVC with Studio with\n" - " default scopes and a random name as token name." + "By default, this command authenticates the DVC with Studio\n" + " using default scopes and assigns a random name as the token name." ) login_parser = studio_subparser.add_parser( "login", @@ -146,7 +149,9 @@ def add_parser(subparsers, parent_parser): login_parser.set_defaults(func=CmdStudioLogin) STUDIO_LOGOUT_HELP = "Logout user from Studio" - STUDIO_LOGOUT_DESCRIPTION = "This command helps to log out user from DVC Studio.\n" + STUDIO_LOGOUT_DESCRIPTION = ( + "This removes the studio token from your global config.\n" + ) logout_parser = studio_subparser.add_parser( "logout", diff --git a/tests/unit/command/test_studio.py b/tests/unit/command/test_studio.py index df7fc49ed0..1c804dd373 100644 --- a/tests/unit/command/test_studio.py +++ b/tests/unit/command/test_studio.py @@ -53,7 +53,7 @@ def test_studio_login_arguments(mocker): hostname="https://example.com", scopes="experiments", use_device_code=True, - client_name="dvc", + client_name="DVC", ) From 98dc4052b8ed7b103e9c041dca0084986495ae15 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 5 Dec 2023 12:41:03 +0545 Subject: [PATCH 21/24] Not relevant anymore --- dvc/utils/studio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/utils/studio.py b/dvc/utils/studio.py index 2e0b1fc95f..2530db78b8 100644 --- a/dvc/utils/studio.py +++ b/dvc/utils/studio.py @@ -35,7 +35,7 @@ def post( logger.trace("Sending %s to %s", data, url) - headers = {"Authorization": f"token {token}"} if token else None + headers = {"Authorization": f"token {token}"} r = session.post( url, json=data, headers=headers, timeout=timeout, allow_redirects=False ) From 935b7e0e66c43ff0369ae71872f3d92cb9501cb0 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 5 Dec 2023 12:47:40 +0545 Subject: [PATCH 22/24] Change argument name in studio login Renamed the argument from "--use-device-code" to "--no-open" in the login function under the Studio authentication. The option's name was changed to make it more intuitive and better align with its actual effect, avoiding potential user confusion. Corresponding changes have also been made in unit tests. --- dvc/commands/studio.py | 4 ++-- tests/unit/command/test_studio.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index 9324517fd5..11f863dcb3 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -27,7 +27,7 @@ def run(self): token_name=name, hostname=hostname, scopes=scopes, - use_device_code=self.args.use_device_code, + use_device_code=self.args.no_open, client_name="DVC", ) except StudioAuthError as e: @@ -139,7 +139,7 @@ def add_parser(subparsers, parent_parser): login_parser.add_argument( "-d", - "--use-device-code", + "--no-open", action="store_true", default=False, help="Use authentication flow based on user code.\n" diff --git a/tests/unit/command/test_studio.py b/tests/unit/command/test_studio.py index 1c804dd373..20d1b4ca0b 100644 --- a/tests/unit/command/test_studio.py +++ b/tests/unit/command/test_studio.py @@ -42,7 +42,7 @@ def test_studio_login_arguments(mocker): "https://example.com", "--scopes", "experiments", - "--use-device-code", + "--no-open", ] ) == 0 From e89314562f18ddd80115ddb3a968e2aa86520467 Mon Sep 17 00:00:00 2001 From: Amrit Ghimire Date: Tue, 5 Dec 2023 12:53:38 +0545 Subject: [PATCH 23/24] Modify flag for Studio login function Updated the argument name from "-d" to "-o" in `studio.py` for the Studio login function. The change makes the argument's intention clearer for users. This is significant in improving user interaction with the feature and mitigating potential user misinterpretation. --- dvc/commands/studio.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index 11f863dcb3..ddffa3a972 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -138,7 +138,7 @@ def add_parser(subparsers, parent_parser): ) login_parser.add_argument( - "-d", + "-o", "--no-open", action="store_true", default=False, From 46c525025fdbff986fb1c61c7e34459c0211918e Mon Sep 17 00:00:00 2001 From: skshetry <18718008+skshetry@users.noreply.github.com> Date: Tue, 5 Dec 2023 16:10:15 +0545 Subject: [PATCH 24/24] Apply suggestions from code review --- dvc/commands/studio.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dvc/commands/studio.py b/dvc/commands/studio.py index ddffa3a972..00b828557a 100644 --- a/dvc/commands/studio.py +++ b/dvc/commands/studio.py @@ -138,7 +138,6 @@ def add_parser(subparsers, parent_parser): ) login_parser.add_argument( - "-o", "--no-open", action="store_true", default=False,