-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
studio: Add Studio authentication to DVC #10074
Conversation
Codecov ReportAll modified and coverable lines are covered by tests β
Additional details and impacted files@@ Coverage Diff @@
## main #10074 +/- ##
==========================================
+ Coverage 90.57% 90.59% +0.02%
==========================================
Files 494 496 +2
Lines 37654 37754 +100
Branches 5483 5490 +7
==========================================
+ Hits 34105 34205 +100
Misses 2910 2910
Partials 639 639 β View full report in Codecov by Sentry. |
Minor, but can we name it When I try it, it always asks me for a device code. Should |
Sure. Will update it .
It is pending in https://github.com/iterative/studio/pull/8147 . For now, you can copy the device code from the url params as well.
User has that option from UI as well. Should we show the message instead? |
What message? Not sure I follow what you mean. |
I meant, showing the link to remove the token from UI or show some help message saying you can remove it as well. I think if user used dvc cli to authenticate and used the token else where, do we want to invalidate that as well? |
Makes sense, thanks! |
Seems like the internals for this may belong in https://github.com/iterative/dvc-studio-client instead of |
This commit introduces two new methods, 'start_device_login' and 'check_token_authorization', in the auth.py file. These methods initiate the device login process for Studio and check the token authorization using a device code. Used the urllib, requests, and os modules, and defined the necessary exceptions. This new module will be used in iterative/dvc#10074
Moved those utils to dvc-studio-client. PTAL |
@pmrowla Could you take a look at how it is now, please? |
* Add authorization methods in auth.py This commit introduces two new methods, 'start_device_login' and 'check_token_authorization', in the auth.py file. These methods initiate the device login process for Studio and check the token authorization using a device code. Used the urllib, requests, and os modules, and defined the necessary exceptions. This new module will be used in iterative/dvc#10074 * Update auth tests to cover DeviceLoginResponse * Corrected the type hint for scopes in auth.py The syntax for type hinting a list of strings has been corrected from 'list[str]' to '[str]'. * Update type annotation and formatting in auth and test_auth files * The scopes parameter type has been adjusted to use the Optional[] annotation instead of the "|" operator in the auth.py file. * Change '|' operator to Optional in token authorization check
@skshetry Could you please review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a brief look here. The code overall looks good, but I don't want to deal with a lot of details here and should be best handled in dvc-studio-client
.
Ideally, there should be a single call to dvc-studio-client
that returns the token, and/or error message with proper message.
Again, code looks good, but we need to figure out how to organize this and where. :)
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.
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.
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.
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.
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.
@shcheklein, @dberenbaum, could you PTAL as well? |
dvc/commands/studio.py
Outdated
token_name=name, | ||
hostname=hostname, | ||
scopes=scopes, | ||
use_device_code=self.args.use_device_code, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wait for a user input before opening a browser? @shcheklein
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one that I remember is az login
, and I think it was not waiting for any input. We can check a few other tools. I don't have a strong opinion on this (means probably I would err to keep it simpler and faster unless we have some security concern, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need proper review of the messages, help text, etc. Authorize vs authenticate, things like dvc vs DVC, things like "command helps to" (can be simplified). Also seems we need to spend a bit of time to cleanup Auth stuff in code.
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.
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.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming DVC has reviewed and approved it. Thanks @amritghimire
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.
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.
token_name=name, | ||
hostname=hostname, | ||
scopes=scopes, | ||
use_device_code=self.args.no_open, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider doing the same upstream.
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Adds a new authentication command to DVC that authorizes dvc with Studio.
Commands added are:
dvc studio login
dvc studio logout
dvc studio token
Studio command
Login
To login, you can call
dvc studio login
which will open the Studio with a device code prefilled in your browser. Once the user authorizes the code, the token is saved to global config.Example usages:
Normal flow
On running the command
dvc studio login
, following screen is presented in CLI:A webbrowser is opened with the device code in Studio.
Once user authorizes the token in the screen above, the token is saved to the user's global config.
Additional message as below will be shown in the CLI and will exit.
Authentication successful. The token will be available as risen-geum in Studio profile.
Device login flow
In case, you are running this in remote machine or where you are unable to open web browser, the following screen is presented.
Every other flow is same.
Logout
Token
TODO:
dvc studio status
to check the status and scopes of existing token. This requires changes in Studio first.Fixes #9265