chore: refactor a bunch of auth-related python [MLG-215] [MLG-1321] #8347

rb-determined-ai · 2023-11-06T22:13:26Z

Remove the Authentication object.

Authentication was both a dataclass containing username and token, as
well as a complex login function disguised as an init.

Now the dataclass is called UsernameTokenPair and the complex login
function is called login() or login_with_cache().
Make Session the core of API calls, not the api.request() function.

The Session has a cache of authentication information and cert
customization, and is easy to reuse. The api.request() function
encourages the use of singletons and creates really confusing
configure-singleton-in-every-entrypoint behaviors.
Introduce an UnauthSession which doesn't have an authentication token,
for those few calls that we need to make which are not authenticated.
Add a detproc helper module to the e2e tests, so even tests requiring
the cli can be based on explicit sessions.
Remove cli_auth and cli_cert singletons.
Delete all singleton-based @authentication.required decorators from
the cli; just create a session instead.
Remove all api.{get,post,whatever} calls.
Refactor the Profiler a bit to accept a Session object. It used to
depend on the cli_auth as well.
Add some Session helpers to the e2e tests, with much better caching.
Redesign the e2e auth to be Session-centric rather than cached-login
centric, in particular:
- redesign api_utils.create_test_user()
- remove test_users.logged_in_user()
- remove test_users.log_in_cli()
- remove test_users.login_admin()
- replace any call to Determined() with Determined._from_session()
Introduce detproc module in e2e tests for session-centric CLI calls,
replacing various adhoc helpers:
- test_user.det_spawn()
- test_user.det_run()
- test_exp_continue.det_cmd()
- test_groups.det_cmd()
- test_groups.det_cmd_json()
- test_groupsdet_cmd_expect_error()

netlify · 2023-11-06T22:13:31Z

✅ Deploy Preview for determined-ui canceled.

Name	Link
🔨 Latest commit	`1eef9fe`
🔍 Latest deploy log	https://app.netlify.com/sites/determined-ui/deploys/65d54a60b7acb0000879b88d

harness/determined/common/api/_session.py

harness/determined/common/api/authentication.py

harness/determined/common/api/__init__.py

harness/determined/common/api/request.py

harness/determined/profiler.py

wes-turner · 2024-02-09T00:46:56Z

harness/determined/core/_context.py

-    session = api.Session(
-        info.master_url, None, None, cert, max_retries=util.get_max_retries_config()
-    )
+    utp = authentication.login_with_cache(info.master_url, cert=cert)


This looks good. The magic is more explicit after this change. I think it's a good sign for the login part of your patch.

It'd be nice it could be done away with entirely, but that's outside the scope of this refactor.

Also note here that Session is a natural choice in this context for a login to return.

harness/determined/exec/harness.py

harness/determined/experimental/client.py

harness/determined/common/experimental/determined.py

harness/determined/common/api/authentication.py

wes-turner · 2024-02-14T20:10:34Z

harness/determined/cli/user.py

-    token_store.set_token(username, token)
-    token_store.set_active(username)
+    token_store = authentication.TokenStore(args.master)
+    utp = authentication.login(args.master, username, password, cli.cert)


Not your bug (exists both before this PR and after), but I just noticed that if you aren't logged in and you try to log in with a bad password you get

Failed to log in user: Unauthenticated: Please use 'det user login <username>' for password login, or for Enterprise users logging in with an SSO provider, use 'det auth login --provider=<provider>'.

Just in case you're interested in fixing it here.

Internet says:

Response to the User: A 401 error urges the user to log in or provide valid credentials. In contrast, a 403 error informs the user that access is forbidden, regardless of their authentication status.

I am not interested in fixing it here, sorry.

https://hpe-aiatscale.atlassian.net/browse/MLG-1576

docs/release-notes/8437-auth-refactor.rst

harness/determined/common/api/authentication.py

wes-turner · 2024-02-20T21:14:06Z

harness/determined/cli/_util.py

-    retry = api.default_retry()
-
-    return api.Session(master_url, args.user, authentication.cli_auth, cert, retry)
+    utp = authentication.login_with_cache(


I like how this refactor has landed for the CLI. I'm not asking for anything from this PR, in this comment.

I think this is a place where this refactor is a potential stepping-stone to a later state where we've set up a login path that is only for this particular use case. Curious if you think the same and can express it in a sentence or two for the wiki.

I'm not sure I understand what you mean

wes-turner · 2024-02-20T23:45:15Z

harness/determined/cli/_util.py

-            resp = bindings.get_GetMaster(setup_session(args))
-            if not resp.to_json().get("rbacEnabled"):
+            resp = bindings.get_GetMaster(unauth_session(args))
+            if not resp.rbacEnabled:


I can't tell if you wanted feature_flag to be a parameter or if you wanted it rbacEnabled only.

pre-existing bug

(I only changed the code from weirdly switching from strongly typed bindings objects to weakly typed json dicts)

wes-turner · 2024-02-20T23:47:19Z

harness/determined/cli/version.py

 from determined.cli import render
 from determined.common import api
 from determined.common.declarative_argparse import Cmd


-def get_version(host: str) -> Dict[str, Any]:
-    client_info = {"version": determined.__version__}
+def get_version(sess: api.BaseSession, host: str) -> Dict[str, Any]:


I think including host might be an unnecessary footgun.

yeah, this is a carryover from when I had Session leaving certain of its attributes as private. It doesn't really make sense anymore.

Fixed.

- Remove the Authentication object. Authentication was both a dataclass containing username and token, as well as a complex login function disguised as an __init__. Now the dataclass is called UsernameTokenPair and the complex login function is called `login()` or `login_with_cache()`. - Make Session the core of API calls, not the api.request() function. The Session has a cache of authentication information and cert customization, and is easy to reuse. The api.request() function encourages the use of singletons and creates really confusing configure-singleton-in-every-entrypoint behaviors. - Introduce an UnauthSession which doesn't have an authentication token, for those few calls that we need to make which are not authenticated. - Add a detproc helper module to the e2e tests, so even tests requiring the cli can be based on explicit sessions. - Remove cli_auth and cli_cert singletons. - Delete all singleton-based @authentication.required decorators from the cli; just create a session instead. - Remove all api.{get,post,whatever} calls. - Refactor the Profiler a bit to accept a Session object. It used to depend on the cli_auth as well. - Add some Session helpers to the e2e tests, with much better caching. - Redesign the e2e auth to be Session-centric rather than cached-login centric, in particular: - redesign api_utils.create_test_user() - remove test_users.logged_in_user() - remove test_users.log_in_cli() - remove test_users.login_admin() - replace any call to Determined() with Determined._from_session() - Introduce detproc module in e2e tests for session-centric CLI calls, replacing various adhoc helpers: - test_user.det_spawn() - test_user.det_run() - test_exp_continue.det_cmd() - test_groups.det_cmd() - test_groups.det_cmd_json() - test_groups.det_cmd_expect_error() BREAKING CHANGE: This refactor removes the Session object from all public API namespaces, and also adds the new requirement that all tasks have a writable HOME directory. Previously, a small subset of functionality existed for tasks which had no writable HOME directory.

dannysauer

LGTM

- Remove the Authentication object. Authentication was both a dataclass containing username and token, as well as a complex login function disguised as an __init__. Now the dataclass is called UsernameTokenPair and the complex login function is called `login()` or `login_with_cache()`. - Make Session the core of API calls, not the api.request() function. The Session has a cache of authentication information and cert customization, and is easy to reuse. The api.request() function encourages the use of singletons and creates really confusing configure-singleton-in-every-entrypoint behaviors. - Introduce an UnauthSession which doesn't have an authentication token, for those few calls that we need to make which are not authenticated. - Add a detproc helper module to the e2e tests, so even tests requiring the cli can be based on explicit sessions. - Remove cli_auth and cli_cert singletons. - Delete all singleton-based @authentication.required decorators from the cli; just create a session instead. - Remove all api.{get,post,whatever} calls. - Refactor the Profiler a bit to accept a Session object. It used to depend on the cli_auth as well. - Add some Session helpers to the e2e tests, with much better caching. - Redesign the e2e auth to be Session-centric rather than cached-login centric, in particular: - redesign api_utils.create_test_user() - remove test_users.logged_in_user() - remove test_users.log_in_cli() - remove test_users.login_admin() - replace any call to Determined() with Determined._from_session() - Introduce detproc module in e2e tests for session-centric CLI calls, replacing various adhoc helpers: - test_user.det_spawn() - test_user.det_run() - test_exp_continue.det_cmd() - test_groups.det_cmd() - test_groups.det_cmd_json() - test_groups.det_cmd_expect_error() BREAKING CHANGE: This refactor removes the Session object from all public API namespaces, and also adds the new requirement that all tasks have a writable HOME directory. Previously, a small subset of functionality existed for tasks which had no writable HOME directory.

rb-determined-ai requested a review from wes-turner November 6, 2023 22:13

rb-determined-ai self-assigned this Nov 6, 2023

cla-bot bot added the cla-signed label Nov 6, 2023

rb-determined-ai force-pushed the rb/boil-oceans branch 3 times, most recently from aeec327 to f404228 Compare November 9, 2023 17:30

rb-determined-ai commented Nov 9, 2023

View reviewed changes

harness/determined/common/api/_session.py Show resolved Hide resolved

rb-determined-ai commented Nov 9, 2023

View reviewed changes

harness/determined/common/api/_session.py Outdated Show resolved Hide resolved

rb-determined-ai commented Nov 9, 2023

View reviewed changes

harness/determined/common/api/_session.py Show resolved Hide resolved

rb-determined-ai force-pushed the rb/boil-oceans branch 6 times, most recently from 78e76e9 to 8f340ab Compare November 13, 2023 23:46

rb-determined-ai force-pushed the rb/boil-oceans branch from 8f340ab to 08d859f Compare November 17, 2023 21:42

wes-turner reviewed Nov 28, 2023

View reviewed changes

harness/determined/common/api/_session.py Outdated Show resolved Hide resolved

wes-turner reviewed Nov 30, 2023

View reviewed changes

harness/determined/common/api/_session.py Outdated Show resolved Hide resolved

rb-determined-ai force-pushed the rb/boil-oceans branch 2 times, most recently from 02ae907 to 0b9ebda Compare December 14, 2023 18:20

rb-determined-ai force-pushed the rb/boil-oceans branch 5 times, most recently from 73424bd to 9611dd4 Compare January 3, 2024 00:58

dzhu force-pushed the main branch from 1d216fb to eea7ca5 Compare January 3, 2024 16:52

rb-determined-ai force-pushed the rb/boil-oceans branch 3 times, most recently from 89f9335 to db43335 Compare January 3, 2024 21:11

wes-turner reviewed Feb 8, 2024

View reviewed changes

harness/determined/common/api/_session.py Show resolved Hide resolved

wes-turner reviewed Feb 8, 2024

View reviewed changes

harness/determined/common/api/authentication.py Show resolved Hide resolved

wes-turner reviewed Feb 8, 2024

View reviewed changes

harness/determined/common/api/__init__.py Show resolved Hide resolved

wes-turner reviewed Feb 8, 2024

View reviewed changes

harness/determined/common/api/request.py Show resolved Hide resolved

wes-turner reviewed Feb 9, 2024

View reviewed changes

harness/determined/profiler.py Show resolved Hide resolved

wes-turner reviewed Feb 9, 2024

View reviewed changes

harness/determined/exec/harness.py Show resolved Hide resolved

wes-turner reviewed Feb 9, 2024

View reviewed changes

harness/determined/experimental/client.py Show resolved Hide resolved

wes-turner reviewed Feb 9, 2024

View reviewed changes

harness/determined/common/experimental/determined.py Show resolved Hide resolved

wes-turner reviewed Feb 13, 2024

View reviewed changes

harness/determined/common/api/authentication.py Show resolved Hide resolved

wes-turner reviewed Feb 14, 2024

View reviewed changes

rb-determined-ai force-pushed the rb/boil-oceans branch from d192bb4 to 7c081a3 Compare February 15, 2024 00:36

wes-turner reviewed Feb 15, 2024

View reviewed changes

docs/release-notes/8437-auth-refactor.rst Show resolved Hide resolved

wes-turner reviewed Feb 15, 2024

View reviewed changes

harness/determined/common/api/authentication.py Show resolved Hide resolved

rb-determined-ai force-pushed the rb/boil-oceans branch 2 times, most recently from 289a04e to 334ae52 Compare February 16, 2024 23:42

wes-turner reviewed Feb 20, 2024

View reviewed changes

rb-determined-ai force-pushed the rb/boil-oceans branch from 334ae52 to 62c3b0b Compare February 20, 2024 23:57

wes-turner approved these changes Feb 21, 2024

View reviewed changes

rb-determined-ai force-pushed the rb/boil-oceans branch 2 times, most recently from 42a02fc to 9a867e9 Compare February 21, 2024 00:41

rb-determined-ai force-pushed the rb/boil-oceans branch from 9a867e9 to 1eef9fe Compare February 21, 2024 00:57

dannysauer approved these changes Feb 21, 2024

View reviewed changes

rb-determined-ai merged commit 76ec233 into main Feb 21, 2024
102 of 103 checks passed

rb-determined-ai deleted the rb/boil-oceans branch February 21, 2024 03:37

hilvi mentioned this pull request Aug 22, 2024

🐛[bug] Experiments fails after running for a week #9856

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor a bunch of auth-related python [MLG-215] [MLG-1321] #8347

chore: refactor a bunch of auth-related python [MLG-215] [MLG-1321] #8347

rb-determined-ai commented Nov 6, 2023 •

edited

Loading

netlify bot commented Nov 6, 2023 •

edited

Loading

wes-turner Feb 9, 2024

wes-turner Feb 14, 2024 •

edited

Loading

rb-determined-ai Feb 16, 2024

wes-turner Feb 20, 2024

wes-turner Feb 20, 2024

rb-determined-ai Feb 20, 2024

wes-turner Feb 20, 2024

rb-determined-ai Feb 21, 2024 •

edited

Loading

wes-turner Feb 20, 2024

rb-determined-ai Feb 21, 2024

dannysauer left a comment

chore: refactor a bunch of auth-related python [MLG-215] [MLG-1321] #8347

chore: refactor a bunch of auth-related python [MLG-215] [MLG-1321] #8347

Conversation

rb-determined-ai commented Nov 6, 2023 • edited Loading

netlify bot commented Nov 6, 2023 • edited Loading

✅ Deploy Preview for determined-ui canceled.

Choose a reason for hiding this comment

wes-turner Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rb-determined-ai Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannysauer left a comment

Choose a reason for hiding this comment

rb-determined-ai commented Nov 6, 2023 •

edited

Loading

netlify bot commented Nov 6, 2023 •

edited

Loading

wes-turner Feb 14, 2024 •

edited

Loading

rb-determined-ai Feb 21, 2024 •

edited

Loading