[gmann14]

Fixes #210

The refresh_token grant reused the WhoIs result saved at /authorize and never looked the user up again. This PR makes it check the user before issuing tokens. It also makes deleting a client in the web UI revoke its tokens, the way DELETE /clients/{id} already does. Two commits.

Re-check the user on refresh

handleRefreshTokenGrant now calls a new recheckRefreshUser after client authentication. It runs WhoIs for the user's node and refuses the refresh with invalid_grant if:

• the lookup fails (tailscaled no longer knows the node, or can't be reached),
• the address now belongs to a different node,
• the node is tagged,
• the node belongs to a different user.

Otherwise the new tokens are built from the fresh WhoIs result, so CapMap, extraClaims, profile fields and the resource checks use current data. sub can't change, because the user ID has to match.

Which identifier to look up: the refresh request comes from the relying party, so its remote address is the app, not the user. The AuthRequest already holds the user's node, so recheckRefreshUser calls WhoIs on the node's first Tailscale IP. That's the same call /authorize makes, and it needs no new field. Tailscale IPs don't change when a node's key rotates, so a routine device re-auth doesn't end the user's app sessions. If an address is ever given to a different node, the node ID check refuses it.

The alternative is WhoIsNodeKey with the stored node key. That's stricter: a refresh would also fail after the device re-authenticates and gets a new key. Happy to switch if you'd prefer that.

Behaviour changes:

• One LocalAPI WhoIs call per refresh.
• If the lookup fails for any reason, including tailscaled being briefly unavailable, the user has to log in again. Refresh tokens are single-use and are already deleted by that point, which is how other refresh errors behave today.
• With no LocalClient, or no node address stored with the grant, the refresh is refused rather than skipped.

Revoke tokens on UI delete

The token cleanup in serveDeleteClient moves into deleteClientTokensLocked. handleEditClient's delete action calls it once the client list has been saved, since that path puts the client back if saving fails.

Not in this PR

A cap on the total lifetime of a refresh chain. Each refresh still starts a new 30-day window. With the re-check, a chain now ends when the user loses access, but a cap from the original login would need a new field on AuthRequest and a limit you'd want to choose.

#209 (also mine) added a test that stubbed WhoIs with a node that had no address. I've pushed a commit to #209 that gives that node an address, so both PRs pass whichever merges first (checked by merging the two locally).

Testing

• TestRefreshTokenRechecksUser (new) logs in through serveAuthorize, exchanges the code, changes what WhoIs returns, then refreshes twice. Same user and node: both refreshes work. Grant changed or removed: both work and carry the new extraClaims value. WhoIs fails, node now belongs to another user, node now tagged, address now belongs to another node: refused with invalid_grant. It also checks that the lookup goes to the stored node address, not the caller's address.
• TestRecheckRefreshUserRefusesWithoutLookup (new): no LocalClient, no stored user and no stored node address are all refused.
• TestEditClientDeleteRevokesTokens (new): after a UI delete, the client's code, access token and refresh token are gone, another client's are kept, and the deleted client's refresh token gets invalid_grant.
• Four existing refresh tests stored a node with no address and ran without a LocalClient. They now set an address and use newTestWhoIsClient.
• On main, all seven TestRefreshTokenRechecksUser cases fail: no lookup happens, the refused cases get 200, and changed grants still return role=admin. Without the second commit, TestEditClientDeleteRevokesTokens fails: the tokens are still there and the refresh returns 200. With the change all pass. gofmt -l ., go vet ./..., go test ./... and go test -race ./server are clean.
• Not run against a real tailnet. WhoIs is stubbed the same way as in server/helpers_test.go.

I used Claude Code to help write this change and the tests, and it also ran them.

[jentfoo]

LGTM, thank you for the report and PR!

[jentfoo]

One case missed after looking again

[jentfoo]

Can you add the check:

  case who.Node.Expired:
      return nil, fmt.Errorf("node %v key has expired", who.Node.ID)