fix: don't drop active_user on expired tokens [MLG-1494] #8653
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8653 +/- ##
=======================================
Coverage 51.45% 51.46%
=======================================
Files 882 884 +2
Lines 156065 156101 +36
Branches 2085 2085
=======================================
+ Hits 80304 80336 +32
- Misses 74270 74273 +3
- Partials 1491 1492 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
rb-determined-ai
force-pushed
the
rb/dropped-user
branch
from
d7357d7
to
60cd06b
Compare
There was a problem hiding this comment.
Better 1 / better 2, this is clearly better. It's also a nice, small fix.
Extra credit if you want extra credit is embedded in comments.
This fix would have taken me, personally, a long time because I'd first need to figure these things out for myself. This PR didn't cause those problems, but it does make those existing problems marginally worse.
It took me a long time (and your help) to understand how this fix fixes the problem. Not a comment on the fix you provided, but in part a symptom of the complexity in this file.
harness/tests/cli/test_auth.py
Outdated
| @@ -422,6 +423,30 @@ def test_logout(scenario_set: Logout) -> None: | |||
| cli.main(cmd) | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("active_user", ["alice", "bob", None]) | |||
There was a problem hiding this comment.
Request, here, to move those tests that exercise functionality from just one file (and that don't rely on an actual running master) to harness/tests/determined/... per this doc.
There was a problem hiding this comment.
I'll move it to harness/tests/... and change the doc? But the extra /determined/ layer is an unhelpful layer of hierarchy I think.
There was a problem hiding this comment.
Looking at the other tests in this file, these other tests are actually are testing cli logic (at least, as-written). These tests also happen to be the only real coverage of common/api/authentication.py. I'll admit that seems wrong but I don't think moving it is actually any better, unless test_logout() and test_login_scenarios() are re-written to be specific to the authentication module, and separate cli-specific unit tests are written to make sure the cli faithfully passes certain variables into the authentication module.
I think that's a good idea, but I'd prefer to put that off until after my big auth refactor pr, since that pr already had to refactor these tests a bit. Rewriting them in this PR would mean I have to rewrite them again in that PR.
There was a problem hiding this comment.
If you'd like to change the doc from harness/tests/determined/... to harness/tests/..., I support that change. Just when doing it please also move the files currently in harness/tests/determined along with.
There was a problem hiding this comment.
I think that's a good idea...
Your logic has the right shape to me even if I don't fully get the details. The scenarios strike me as tests of auth, not of the CLI. But you know this better than I do.
Anyway, if this is work you'd like the team get done later, maybe worth a ticket? Otherwise good chance we forget.
| @@ -215,6 +215,9 @@ def logout( | |||
| if session_user is None: | |||
| return | |||
|
|
|||
| if session_user == token_store.get_active_user(): | |||
There was a problem hiding this comment.
Extra credit 1:logout is too complicated to not have a docstring that describes its semantics.
There was a problem hiding this comment.
ok I extended the docstring.
| @@ -215,6 +215,9 @@ def logout( | |||
| if session_user is None: | |||
| return | |||
|
|
|||
| if session_user == token_store.get_active_user(): | |||
| token_store.clear_active() | |||
There was a problem hiding this comment.
Extra credit 2: The docstring for TokenStore isn't totally right in that it maintains both a token store and an "active user" string. Which... makes its name kind of misleading. Is a refactor wanted?
There was a problem hiding this comment.
updated the docstring. No a refactor is not wanted, because there is only one auth.json file where this is stored, and so it makes sense that there is only one object to control it.
The authentication logic would drop expired tokens, and then ask for a password and, if that worked, add the fresh token back to the token store. However, the initial token drop had a side-effect of dropping the active user, which was not being restored. After this fix, the TokenStore doesn't try to be so smart. Instead, the logout-related CLI commands will explicitly clear the active user, but tokens dropped due to expiration are unaffected.
rb-determined-ai
force-pushed
the
rb/dropped-user
branch
from
60cd06b
to
641977d
Compare
The authentication logic would drop expired tokens, and then ask for a password and, if that worked, add the fresh token back to the token store.
However, the initial token drop had a side-effect of dropping the active user, which was not being restored.
After this fix, the TokenStore doesn't try to be so smart. Instead, the logout-related CLI commands will explicitly clear the active user, but tokens dropped due to expiration are unaffected.
Test Plan