chore: sign user out if token is expired #109
Conversation
ethanndickson
changed the title
| @@ -94,6 +96,9 @@ class AppState: ObservableObject { | |||
| ) | |||
| if hasSession { | |||
| _sessionToken = Published(initialValue: keychainGet(for: Keys.sessionToken)) | |||
| if sessionToken == nil || sessionToken!.isEmpty == true { | |||
There was a problem hiding this comment.
Can end up in this state if the user deleted it from the keychain themselves.
ethanndickson
force-pushed
the
ethan/sign-out-on-expiry
branch
from
15066a3 to
0617964
Compare
| // Sign out if token is expired | ||
| Task { @MainActor in | ||
| await state.handleTokenExpiry() | ||
| } |
There was a problem hiding this comment.
Will this run initially or periodically?
There was a problem hiding this comment.
Just once on launch. The token gets refreshed on use, so if the user toggles the VPN another week (by default) gets added to the expiry date. That means we're not covering the case where the user keeps the app open for a while without using it.
There was a problem hiding this comment.
Got it. Is this a case we should consider or is it unlikely for GA?
There was a problem hiding this comment.
I'd definitely like to be able to handle that case but, I don't think running it on a fixed interval is the right solution.
I realised it would be ideal if we could check the expiry if:
- The user is signed in
- The VPN is off
- The menu bar icon is clicked
If the VPN is on, then the token has almost certainly been refreshed recently.
So, I did this with a quick commit to our FluidMenuBarExtra fork.
I think having these onAppear and onDisappear handlers is useful, in general, in case we ever want to refresh some state when the menu is opened.
For reference, the existing View.onAppear only runs when the view first appears for our menu bar window, as the view technically always exists, it's just not visible.
From pr desc:
We could have checked this when the VPN is enabled, but the UX seemed worse, and the implementation would have been messy. We would have needed to sign the user out and show an error.
This approach also future-proofs for when functionality becomes usable without the VPN
There was a problem hiding this comment.
Got it, thanks. I was thinking of the Tailscale experience, which shows me how long the device keys remain valid and when my login expires.
Can we potentially fetch information on when the token should expire (in the regular scenario) and display it in the UI? For example, if the token will expire in 24 hours, we conditionally show a red text that says "token expiring soon, please log in soon again" (or something comparable), but otherwise, it won't be shown.
There was a problem hiding this comment.
I'm not sure a warning is useful, the session tokens have a pretty long default duration (7 days, and we only made it configurable late last year) and they refresh on use. If someone sees the warning by opening the menu, they were probably going to use the app anyway, which would refresh it.
As an aside, there's unfortunately no way to list cli-auth tokens, as there's no way to view information of tokens of type loginTypePassword via our API. Manually created tokens (coder tokens create) are of type loginTypetoken, which is all we support listing.
I think even if we did add a way to fetch those session durations, we have a HTTP middleware that refreshes on every request that requires a token, making the duration immediately either outdated or equal to the post-refresh duration.
ethanndickson
force-pushed
the
ethan/sign-out-on-expiry
branch
from
dbd3eaf to
cc50b5d
Compare
ethanndickson
changed the title
ethanndickson
force-pushed
the
ethan/sign-out-on-expiry
branch
2 times, most recently
from
687f946 to
bf95ba9
Compare
ethanndickson
force-pushed
the
ethan/sign-out-on-expiry
branch
from
bf95ba9 to
c244963
Compare
Closes #107.
When the menu bar icon is clicked, and the user is signed in, and the VPN is disabled, the app will check if the token is expired. If it is, the user will be signed out.
We could have checked this when the VPN is enabled, but the UX seemed worse, and the implementation would have been messy. We would have needed to sign the user out and show an error. Instead, we'll check for expiry in a scenario where the next user step would likely be an interaction that requires a session.
This approach also future-proofs for when functionality becomes usable without the VPN.