-
Notifications
You must be signed in to change notification settings - Fork 139
Update logout command to revoke token server-side #1733
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -201,6 +201,8 @@ def login(args: "Namespace"): | |
|
|
||
|
|
||
| def logout(local: bool = False): | ||
| from datachain.remote.studio import get_studio_env_variable | ||
|
|
||
| level = ConfigLevel.LOCAL if local else ConfigLevel.GLOBAL | ||
| with Config(level).edit() as conf: | ||
| token = conf.get("studio", {}).get("token") | ||
|
|
@@ -209,6 +211,37 @@ def logout(local: bool = False): | |
| "Not logged in to Studio. Log in with 'datachain auth login'." | ||
| ) | ||
|
|
||
| studio_url = ( | ||
| conf.get("studio", {}).get("url") | ||
| or get_studio_env_variable("URL") | ||
| or STUDIO_URL | ||
| ) | ||
|
|
||
| try: | ||
| response = requests.post( | ||
| f"{studio_url.rstrip('/')}/api/token-logout", | ||
| headers={"Authorization": f"token {token}"}, | ||
| timeout=10, | ||
| ) | ||
| if response.status_code == 401: | ||
| print( | ||
| "Token was already revoked or is invalid on Studio.", | ||
| file=sys.stderr, | ||
| ) | ||
| elif not response.ok: | ||
| print( | ||
| f"Warning: Unexpected response from Studio " | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here - don't proceed then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reasoning above. |
||
| f"(HTTP {response.status_code}).", | ||
| file=sys.stderr, | ||
| ) | ||
| except requests.RequestException: | ||
| print( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's be strict then - don't proceed with logout at all
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem with that is in case user deletes the token from UI, then tries to clear it from cli too, it will raise error and user can't remove the token from the config. It will be a pain point for user and blocks further process.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's handle not found properly then (assuming it was deleted already)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider cleaning up tokens from Studio before deleting them locally. Abort the logout process with an error if the Studio logout request fails, giving users the responsibility to re-try. Studio should be able to emit distinct responses for "this token is well-formed but no longer valid" (logout can continue and local token can be deleted) versus anything else (server error, network error, ...) |
||
| "Warning: Could not reach Studio to revoke the token. " | ||
| "The token has been removed locally but may still be valid " | ||
| "on the server.", | ||
| file=sys.stderr, | ||
| ) | ||
|
|
||
| del conf["studio"]["token"] | ||
|
|
||
| print("Logged out from Studio. (you can log back in with 'datachain auth login')") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,15 +94,57 @@ def test_studio_login_arguments(mocker): | |
|
|
||
| def test_studio_logout(): | ||
| with Config(ConfigLevel.GLOBAL).edit() as conf: | ||
| conf["studio"] = {"token": "isat_access_token"} | ||
| conf["studio"] = {"token": "isat_access_token", "url": STUDIO_URL} | ||
|
|
||
| with requests_mock.mock() as m: | ||
| m.post( | ||
| f"{STUDIO_URL}/api/token-logout", | ||
| json={"detail": "Token revoked successfully"}, | ||
| ) | ||
| assert main(["auth", "logout"]) == 0 | ||
|
Comment on lines
95
to
+104
|
||
| assert m.called | ||
| assert m.last_request.headers["Authorization"] == "token isat_access_token" | ||
|
|
||
| assert main(["auth", "logout"]) == 0 | ||
| config = Config(ConfigLevel.GLOBAL).read() | ||
| assert "token" not in config["studio"] | ||
|
|
||
| assert main(["auth", "logout"]) == 1 | ||
|
|
||
|
|
||
| def test_studio_logout_token_already_revoked(capsys): | ||
| with Config(ConfigLevel.GLOBAL).edit() as conf: | ||
| conf["studio"] = {"token": "isat_access_token", "url": STUDIO_URL} | ||
|
|
||
| with requests_mock.mock() as m: | ||
| m.post( | ||
| f"{STUDIO_URL}/api/token-logout", | ||
| json={"detail": "Invalid token"}, | ||
| status_code=401, | ||
| ) | ||
| assert main(["auth", "logout"]) == 0 | ||
|
|
||
| config = Config(ConfigLevel.GLOBAL).read() | ||
| assert "token" not in config["studio"] | ||
|
|
||
| err = capsys.readouterr().err | ||
| assert "already revoked or is invalid" in err | ||
|
|
||
|
|
||
| def test_studio_logout_custom_url(): | ||
| custom_url = "https://custom-studio.example.com" | ||
| with Config(ConfigLevel.GLOBAL).edit() as conf: | ||
| conf["studio"] = {"token": "isat_access_token", "url": custom_url} | ||
|
|
||
| with requests_mock.mock() as m: | ||
| m.post( | ||
| f"{custom_url}/api/token-logout", | ||
| json={"detail": "Token revoked successfully"}, | ||
| ) | ||
| assert main(["auth", "logout"]) == 0 | ||
| assert m.called | ||
| assert m.last_request.url == f"{custom_url}/api/token-logout" | ||
|
|
||
|
|
||
| def test_studio_token(capsys): | ||
| with Config(ConfigLevel.GLOBAL).edit() as conf: | ||
| conf["studio"] = {"token": "isat_access_token"} | ||
|
|
||
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.
logout()resolvesstudio_urlfrom config before checkingDATACHAIN_STUDIO_URL(get_studio_env_variable("URL")). Elsewhere, Studio URL resolution prioritizes the env var (e.g.,StudioClient.urlandlogin()), so whenDATACHAIN_STUDIO_URLis set this can revoke the token against a different host than the rest of the CLI uses. Consider aligning the precedence (env var first) or reusing a shared URL-resolution helper to keep behavior consistent across commands.