feat: Add Sign-out functionality #46
Conversation
Codecov Report
@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 71.89% 72.20% +0.30%
==========================================
Files 64 71 +7
Lines 2711 2795 +84
Branches 30 36 +6
==========================================
+ Hits 1949 2018 +69
- Misses 592 605 +13
- Partials 170 172 +2
Continue to review full report at Codecov.
|
coderd/users.go
Outdated
| @@ -199,6 +199,20 @@ func (users *users) loginWithPassword(rw http.ResponseWriter, r *http.Request) { | |||
| }) | |||
| } | |||
|
|
|||
| // Clear the user's session cookie | |||
| func logout(rw http.ResponseWriter, r *http.Request) { | |||
There was a problem hiding this comment.
Minor nit: We should add the users strict to contain this operation. In an ideal world, the API key is invalidated too. Might be a good issue!
There was a problem hiding this comment.
A test for this would be good too!
There was a problem hiding this comment.
I fixed the receiver name in: af001cb
And had a go at adding a test in: 52d0136
One thing I wasn't sure about was the right level-of-abstraction for the test. I thought about adding codersdk functions for:
LogoutClearSessionTokens
but I realized when I started writing those that it actually doesn't test the functionality of sending-a-cookie-that-clears-a-session. But on the flip side, the test I wrote is different than the other tests (it constructs a request and validates the cookies returned are correct) - whereas the other test files call against codersdk. Curious to get your thoughts!
There was a problem hiding this comment.
Hmm. I'd add a call for Logout, even if it doesn't function as expected right now.
It seems odd to have no deauthenticate functionality inside of the codersdk.
I'm also fine with you adding a comment for that, since logout isn't a critical path for most anyways!
There was a problem hiding this comment.
Gave adding this a try here: 4dcd1fd
I added a Logout as the deauth version of LoginWithPassword, and ClearSessionTokens as the opposite of SetSessionToken (although ClearSessionTokens is currently clearing everything). Since the Client doesn't expose any internals about what cookies are around - the test is kinda awkward. It'd be better if it verified the state of cookies
While I was testing the Sign-out functionality in #46 , I found a bug on login. Sometimes, intermittently, the login wouldn't 'take' even though the login POST was successful and the subsequent GET to `/api/v2/user` was successful - the user would still be brought to the `/login` screen a second time:  ^ I log-in, and then the login screen just resets, and logging in a second time works 🤔 I tracked this down and found it is a new race condition that I inadvertently introduced when I migrated to SWR in #46. Because SWR caches responses based on the API path - we have to invalidate the cache for `/api/v2/user` when the user logins, otherwise we'll continue to serve the user-not-found error until SWR decides to retry. However, we weren't waiting for that cache-invalidation to go through - so there was the chance that an `/api/v2/user` request was still in-flight while we were redirecting after a successful login. If the request made it back prior to the redirect - login would work on the first try. If the request took longer - SWR would serve the stale, erroneous user from the cache. Luckily the fix is simple - `mutate` in the SWR API returns a promise, so we can just wait for that to go through before redirecting. Unfortunately, though, this is tricky to exercise in a unit test because with the SWR model, the login logic is spread between `_app.tsx`, `UserContext.tsx`, and `SignInForm.tsx`, and the race condition involves an integration of all of those places. I found that there is a lint rule that can help protect us from making this mistake - [`typescript-eslint/no-floating-promises`](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-floating-promises.md) (it's a lint rule we wanted to turn on in `coder/m`, too, actually - just a lot of work to clean up the code for it!), so I turned it on as part of this PR, and it caught a couple extra places to check.
bryphe-coder
force-pushed
the
bryphe/feat/user-profile-card
branch
from
9a779d0
to
af001cb
Compare
codersdk/client_test.go
Outdated
| err := client.SetSessionToken("test-session") | ||
| require.NoError(t, err, "Setting a session token should be successful") | ||
|
|
||
| err = client.ClearSessionToken() |
There was a problem hiding this comment.
This test is a little awkward because it's not really verifying anything, except that no errors occurr with SetSessionToken and ClearSessionToken.
The Client at the moment doesn't expose a way to look at the internal state of cookies, which is probably by design! But it would help to be able to poke inside and see if setting the session token actually did something, and likewise, if clearing the session token actually did stuff too.
codersdk/client.go
Outdated
| @@ -47,6 +47,24 @@ func (c *Client) SetSessionToken(token string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // ClearSessionToken clears tokens and authentication context in the current cline.t | |||
| func (c *Client) ClearSessionToken() error { | |||
There was a problem hiding this comment.
Could we just make SetSessionToken do this with an empty string? Then we don't need another func!
There was a problem hiding this comment.
Sure, this isn't used anywhere - I was thinking of having pairs of Login/Logout and SetSessionToken/ClearSessionToken but totally fine not to have that. Removed in aa84624
#37 implemented the Sign-in flow, but there wasn't a Sign-out flow as part of that PR (aside from letting the cookie expire... or manually deleting the cookie...), which is obviously not ideal.
This PR implements a basic sign-out flow, along with a very simple user dropdown:
Bringing in a few pruned down components for the
<UserDropdown />to integrate into the<NavBar />.In addition, this also implements a simple back-end API for
/logoutwhich just clears the session token.TODO: