fix: Don't record the user claims of invalid tokens

[jMarkP]

Fixes #5687.

The Authenticate() function correctly returns an error when an invalid token is presented. However, as a side effect it always stores tokens (valid or not) on the context. Normally this would be fine as the failed Authenticate call would terminate the request, but there are some endpoints which perform this auth, ignore errors (because they don't require an authenticated user), and then examine the claims left in the context.

GetUserInfo() is one such endpoint. The result of all this is that users who were logged in but whose token have expired (or who have a genuinely invalid token) will be reported by the UI as logged in, but will get auth errors when attempting any protected action.

This PR switches round the Authenticate() function so that it only adds valid token claims to the context.

A change to the auth code path feels scary so would appreciate some pointers on what more I can do to validate this update other than adding a unit test case for it!

Signed-off-by: Mark Pim j.mark.pim@gmail.com

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[codecov]

Codecov Report

Merging #5897 (2fb803e) into master (2f785cd) will increase coverage by 0.02%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master    #5897      +/-   ##
==========================================
+ Coverage   40.91%   40.93%   +0.02%     
==========================================
  Files         147      147              
  Lines       19620    19640      +20     
==========================================
+ Hits         8027     8040      +13     
- Misses      10489    10496       +7     
  Partials     1104     1104              

Impacted Files	Coverage Δ
server/server.go	55.59% <78.57%> (+0.76%)	⬆️
util/helm/helm.go	61.11% <0.00%> (-4.41%)	⬇️
controller/state.go	69.15% <0.00%> (-0.18%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f785cd...2fb803e. Read the comment docs.

[jMarkP]

Thanks for the feedback @sbose78! Do you have any way to retrigger the CI checks to run? The error on the most recent commit looks like a transient Docker error (the previous commit was successful)

[alexmt]

LGTM. Thank you for the improvement and details PR description!

[maxb]

I think this is a good change, but for additional defence-in-depth, it would be good to also change SessionManager.VerifyToken to always return a nil jwt.Claims when returning a non-nil error value.

[maxb]

Simplification opportunity: This function does not need to return an "ok" bool and an error - ok is always true when error is nil, and false when error is not-nil.