Skip to content

feat: oidc background token refresh #23727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

feat: oidc background token refresh #23727

wants to merge 9 commits into from
+827 −160

Conversation

@devopsjedi
Copy link
Contributor

@devopsjedi devopsjedi commented Jul 10, 2025 •
edited
Loading

Closes #12189
Adds support for background OIDC token refresh

  • Adds refreshTokenThreshold field to oidc.config spec. When authentication middleware verifies the current token, the remaining lifetime of the token is compared to the refresh token threshold. The token is refreshed by the server when the lifetime is lower than the threshold and a new token is returned to the client.
    - Adds OpenTelemetry tracing for authentication flow

Tested against keycloak.

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).
  • The title of the PR conforms to the Toolchain Guide
  • 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.
  • 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).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@bunnyshell
Copy link

bunnyshell bot commented Jul 10, 2025 •
edited
Loading

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@devopsjedi devopsjedi force-pushed the issue12189 branch 5 times, most recently from 8d3a313 to 85d98b8 Compare July 10, 2025 14:20
@codecov
Copy link

codecov bot commented Jul 10, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 73.59307% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.91%. Comparing base (05ccc01) to head (45bcb96).

Files with missing lines Patch % Lines
util/oidc/oidc.go 73.09% 32 Missing and 14 partials ⚠️
server/server.go 56.25% 5 Missing and 2 partials ⚠️
util/http/http.go 83.33% 2 Missing and 1 partial ⚠️
util/settings/settings.go 76.92% 2 Missing and 1 partial ⚠️
server/application/websocket.go 0.00% 1 Missing ⚠️
util/oidc/provider.go 80.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #23727      +/-   ##
==========================================
+ Coverage   60.89%   60.91%   +0.02%     
==========================================
  Files         351      351              
  Lines       60489    60625     +136     
==========================================
+ Hits        36832    36931      +99     
- Misses      20736    20766      +30     
- Partials     2921     2928       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@devopsjedi devopsjedi force-pushed the issue12189 branch 23 times, most recently from 6452b72 to d0d9cc7 Compare July 14, 2025 02:08
@todaywasawesome
Copy link
Contributor

todaywasawesome commented Oct 2, 2025

@agaudreault The PR has been updated to remove the OTEL feature for now and is ready for review.

@todaywasawesome todaywasawesome added the ready-for-review An approver should give a final review and merge the PR label Oct 2, 2025
@github-project-automation github-project-automation bot moved this to Ready for final review in Argo CD Review Oct 2, 2025
@agaudreault
Copy link
Member

agaudreault commented Oct 6, 2025

@devopsjedi can you resolve conflicts and ping me on here/slack when it is done. 🙇

@devopsjedi devopsjedi requested a review from a team as a code owner October 6, 2025 22:16
@devopsjedi
Copy link
Contributor Author

devopsjedi commented Oct 6, 2025

@devopsjedi can you resolve conflicts and ping me on here/slack when it is done. 🙇

Rebased and resolved 🫡 @agaudreault

agaudreault
agaudreault reviewed Oct 7, 2025
View reviewed changes
agaudreault
agaudreault requested changes Oct 7, 2025
View reviewed changes
Copy link
Member

@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Few nitpicks and maybe a refactor on how the cache is managed would help clarify the ccode. Please don't force push follow up commit.

@github-project-automation github-project-automation bot moved this from Ready for final review to Changes Requested in Argo CD Review Oct 7, 2025
@devopsjedi
Copy link
Contributor Author

devopsjedi commented Oct 7, 2025

Overall LGTM. Few nitpicks and maybe a refactor on how the cache is managed would help clarify the ccode. Please don't force push follow up commit.

Thanks- updated based on feedback

@devopsjedi devopsjedi requested a review from agaudreault October 8, 2025 16:49
agaudreault
agaudreault reviewed Oct 8, 2025
View reviewed changes
@sidewinder12s
Copy link

sidewinder12s commented Oct 20, 2025

Might want to update the PR description that OTEL was pulled out. Thanks for working through this though!

@johnpekcan
Copy link

johnpekcan commented Oct 20, 2025

Yes! thank you so much for chasing this down! We are very appreciative 👏

@devopsjedi devopsjedi requested a review from a team as a code owner October 21, 2025 01:07
@devopsjedi
feat: oidc background token refresh
8d8951a 
Adds background refresh functionality for OIDC tokens to prevent
session timeouts and improve user experience. The refresh happens
automatically when tokens are near expiration based on configurable
threshold settings.

Key changes:
- Add OIDCRefreshTokenThreshold configuration
- Implement CheckAndRefreshToken in OIDC client
- Add token refresh logic to session verification
- Update session manager to pass context for token operations

Signed-off-by: Mike Cutsail <[email protected]>
@devopsjedi
Improved error handling; Updated cache set functions to clarify usage
ae10081 
Signed-off-by: Mike Cutsail <[email protected]>
@devopsjedi
fix args to oidc-refresh-feature
88c4795 
Signed-off-by: Mike Cutsail <[email protected]>
@devopsjedi
rename groupClaims to finalMapClaims
7af54a7 
Signed-off-by: Mike Cutsail <[email protected]>
@devopsjedi
feat: oidc background token refresh
6984a2d 
Adds background refresh functionality for OIDC tokens to prevent
session timeouts and improve user experience. The refresh happens
automatically when tokens are near expiration based on configurable
threshold settings.

Key changes:
- Add OIDCRefreshTokenThreshold configuration
- Implement CheckAndRefreshToken in OIDC client
- Add token refresh logic to session verification
- Update session manager to pass context for token operations

Signed-off-by: Mike Cutsail <[email protected]>

# Conflicts:
#	server/server.go
#	util/oidc/oidc.go
#	util/oidc/oidc_test.go
@devopsjedi
rename groupClaims to finalMapClaims
5a91eea 
Signed-off-by: Mike Cutsail <[email protected]>
@devopsjedi
Address clarity and deduplication comments
6710f9f 
Signed-off-by: Mike Cutsail <[email protected]>
@devopsjedi
Copy link
Contributor Author

devopsjedi commented Oct 21, 2025

@agaudreault Updated based on your latest feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@agaudreault agaudreault Awaiting requested review from agaudreault

1 more reviewer

@guru-disco guru-disco guru-disco approved these changes

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

ready-for-review An approver should give a final review and merge the PR

Projects

Argo CD Review
Status: Changes Requested

Milestone

v3.3

Development

Successfully merging this pull request may close these issues.

Argo CD UI getting logged out in 5 mins while integrated with keycloak

7 participants

@devopsjedi @todaywasawesome @agaudreault @sidewinder12s @johnpekcan @guru-disco @crenshaw-dev