Skip to content

Conversation

@skoeva
Copy link
Contributor

@skoeva skoeva commented Dec 1, 2025

This change extracts the RefreshAndSetToken function from headlamp.go into the auth package.

Part of:

Updates

  • Add params struct to help move future functions
  • Add comprehensive tests in auth_test.go

Testing

cd backend
go test -v ./pkg/auth -run RefreshAndSetToken

@skoeva skoeva self-assigned this Dec 1, 2025
@skoeva skoeva added backend Issues related to the backend auth Authentication or authorization related quality Related to improving the quality of the app oidc Issue related to OIDC labels Dec 1, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: skoeva
Once this PR has been reviewed and has the lgtm label, please assign illume for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 1, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2025
@skoeva skoeva force-pushed the auth9 branch 2 times, most recently from 7e34534 to c928a61 Compare December 1, 2025 16:21
Copy link
Contributor

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Is there a chance (honest question) we could have the tests being done for the same logic, before the extraction commit. This way we could verify that everything keeps working whereas right now we are testing for the new changes already.

@illume illume added this to the v0.40.0 milestone Dec 15, 2025
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 2, 2026
@illume illume requested a review from Copilot February 1, 2026 16:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extracts the OIDC token refresh-and-cookie-setting logic from HeadlampConfig into the shared auth package and adds focused tests around it. It supports the ongoing effort in #3482 to progressively move and test OIDC-related auth functionality.

Changes:

  • Introduces auth.RefreshAndSetTokenParams and auth.RefreshAndSetToken, encapsulating token refresh, cookie update, and telemetry recording in the auth package.
  • Replaces the old (*HeadlampConfig).refreshAndSetToken method with a call to the new auth.RefreshAndSetToken within OIDCTokenRefreshMiddleware.
  • Adds comprehensive tests in auth_test.go for RefreshAndSetToken, including default ID token behavior, access token mode, and error handling (no cookie set on failure).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
backend/pkg/auth/auth.go Adds RefreshAndSetTokenParams and RefreshAndSetToken, wiring together token refresh, cookie setting, and telemetry with configurable token type and issuer URL.
backend/pkg/auth/auth_test.go Adds helpers and three tests validating that RefreshAndSetToken picks the correct token (ID vs access), sets the expected cookie, and avoids setting cookies on refresh errors.
backend/cmd/headlamp.go Removes the HeadlampConfig.refreshAndSetToken method and switches OIDCTokenRefreshMiddleware to use the new auth.RefreshAndSetToken API while passing the existing config and telemetry fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@skoeva
Copy link
Contributor Author

skoeva commented Feb 3, 2026

There aren't any changes in the function logic so coupling the tests with the extracted code should be fine

@illume illume modified the milestones: v0.40.0, v0.41.0 Feb 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auth Authentication or authorization related backend Issues related to the backend cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. oidc Issue related to OIDC quality Related to improving the quality of the app size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants