OCPBUGS-99757: Include extra scopes in OIDC token cache key - #2323
OCPBUGS-99757: Include extra scopes in OIDC token cache key#2323michaelryanmcneill wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@michaelryanmcneill: This pull request references Jira Issue OCPBUGS-99757, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughToken cache keys now include ChangesToken cache scope identity
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@michaelryanmcneill: This pull request references Jira Issue OCPBUGS-99757, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@michaelryanmcneill: This pull request references Jira Issue OCPBUGS-99757, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/cli/gettoken/tokencache/tokencache_test.go (2)
11-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd descriptive comments to the new test functions.
Each added unexported test function needs a concise comment describing the cache behavior it verifies.
As per coding guidelines, “Add descriptive comments to all exported and unexported Go types, functions, and methods.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gettoken/tokencache/tokencache_test.go` around lines 11 - 154, Add concise descriptive comments immediately above each newly added test function, including TestComputeFilename_DifferentScopes, TestComputeFilename_SortedScopesDeterministic, TestComputeFilename_UnsortedScopesDiffer, TestComputeFilename_SameKeysSameFilename, TestSaveAndFindByKey, TestFindByKey_DifferentScopesMiss, and TestSave_FilePermissions, stating the cache behavior each test verifies.Source: Coding guidelines
11-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a table-driven test for filename scenarios.
These four tests exercise variants of the same key-to-filename behavior; combine them into named
t.Runcases with expected equal/different results.As per coding guidelines, “Use table-driven tests with the standard
testing.Tframework.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gettoken/tokencache/tokencache_test.go` around lines 11 - 83, Combine TestComputeFilename_DifferentScopes, TestComputeFilename_SortedScopesDeterministic, TestComputeFilename_UnsortedScopesDiffer, and TestComputeFilename_SameKeysSameFilename into one table-driven test with named t.Run cases. Define each case’s key pair and whether filenames should match, then reuse shared computeFilename and assertion logic while preserving the existing order-sensitive scope behavior.Source: Coding guidelines
pkg/cli/gettoken/tokencache/tokencache.go (1)
24-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the scope-ordering invariant.
ExtraScopesis filename-significant and must be sorted by callers. Add aKeydoc comment stating this contract so directRepository.Save/FindByKeycallers do not create order-dependent cache entries.Proposed documentation
+// Key identifies a token cache entry. ExtraScopes must be in canonical sorted order. type Key struct {As per coding guidelines, “Add descriptive comments to all exported and unexported Go types, functions, and methods.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gettoken/tokencache/tokencache.go` around lines 24 - 26, The exported ExtraScopes field in the token cache key lacks documentation for its ordering contract. Add a Go doc comment directly above ExtraScopes stating that callers must keep the scopes sorted, preserving consistent keys for direct Repository.Save and FindByKey usage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cli/gettoken/tokencache/tokencache_test.go`:
- Line 146: Update the test flow around computeFilename to handle its returned
error instead of discarding it. Check the error immediately after the call and
fail or return through the test’s established error-handling mechanism, only
using the computed filename when computation succeeds.
---
Nitpick comments:
In `@pkg/cli/gettoken/tokencache/tokencache_test.go`:
- Around line 11-154: Add concise descriptive comments immediately above each
newly added test function, including TestComputeFilename_DifferentScopes,
TestComputeFilename_SortedScopesDeterministic,
TestComputeFilename_UnsortedScopesDiffer,
TestComputeFilename_SameKeysSameFilename, TestSaveAndFindByKey,
TestFindByKey_DifferentScopesMiss, and TestSave_FilePermissions, stating the
cache behavior each test verifies.
- Around line 11-83: Combine TestComputeFilename_DifferentScopes,
TestComputeFilename_SortedScopesDeterministic,
TestComputeFilename_UnsortedScopesDiffer, and
TestComputeFilename_SameKeysSameFilename into one table-driven test with named
t.Run cases. Define each case’s key pair and whether filenames should match,
then reuse shared computeFilename and assertion logic while preserving the
existing order-sensitive scope behavior.
In `@pkg/cli/gettoken/tokencache/tokencache.go`:
- Around line 24-26: The exported ExtraScopes field in the token cache key lacks
documentation for its ordering contract. Add a Go doc comment directly above
ExtraScopes stating that callers must keep the scopes sorted, preserving
consistent keys for direct Repository.Save and FindByKey usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cccbd085-baa1-420f-8e5e-9d9850f8c2d5
📒 Files selected for processing (3)
pkg/cli/gettoken/gettoken.gopkg/cli/gettoken/tokencache/tokencache.gopkg/cli/gettoken/tokencache/tokencache_test.go
6899b92 to
14695d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cli/gettoken/tokencache/tokencache_test.go (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd comments to the test functions.
These four test functions lack descriptive Go comments. Add concise comments explaining the behavior each test protects.
As per coding guidelines, “Add descriptive comments to all exported and unexported Go types, functions, and methods.”
Also applies to: 65-65, 93-93, 117-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/cli/gettoken/tokencache/tokencache_test.go` at line 11, Add concise descriptive Go comments to the four test functions, including TestComputeFilename and the tests at the other referenced locations. Each comment should explain the behavior that test protects and follow Go documentation comment conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/cli/gettoken/tokencache/tokencache_test.go`:
- Around line 111-114: Update the FindByKey assertion in the differentKey test
case to verify os.IsNotExist(err) rather than only checking that err is non-nil,
while retaining the failure message for unexpected successful lookups.
---
Nitpick comments:
In `@pkg/cli/gettoken/tokencache/tokencache_test.go`:
- Line 11: Add concise descriptive Go comments to the four test functions,
including TestComputeFilename and the tests at the other referenced locations.
Each comment should explain the behavior that test protects and follow Go
documentation comment conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 11c7cd59-8499-4c2c-871a-d0bcd17f1bd5
📒 Files selected for processing (3)
pkg/cli/gettoken/gettoken.gopkg/cli/gettoken/tokencache/tokencache.gopkg/cli/gettoken/tokencache/tokencache_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/cli/gettoken/tokencache/tokencache.go
- pkg/cli/gettoken/gettoken.go
14695d6 to
9ce4e72
Compare
9ce4e72 to
72d0c55
Compare
The token cache key only included issuer URL and client ID, causing cached tokens to be returned even when different --extra-scopes were requested. This resulted in ID tokens missing scope-dependent claims (e.g., email, profile) after the initial authentication. Add ExtraScopes to the tokencache.Key struct so that different scope sets produce distinct cache entries. Scopes are sorted before key construction to ensure deterministic cache filenames regardless of flag ordering. Signed-off-by: michaelryanmcneill <michael@michaelryanmcneill.com>
72d0c55 to
51c1921
Compare
|
@ardaguclu ready for final review! Thanks! |
|
Thank you |
|
Scheduling required tests: |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, michaelryanmcneill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@michaelryanmcneill: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Bug: https://issues.redhat.com/browse/OCPBUGS-99757
The OIDC exec plugin token cache key is computed from only
IssuerURLandClientID. When a user changes--extra-scopes, the stale cached token (missing the new scope claims) is returned instead of triggering a fresh auth code flow.Changes:
ExtraScopesfield totokencache.Keyso different scope sets produce distinct cache entriescomputeFilenameto ensure deterministic cache filenames regardless of flag ordering, with explicit nil handlingWorkaround (prior to this fix):
rm ~/.kube/cache/oc/*and re-authenticate.Note: existing cached tokens will require a one-time re-authentication after this change, as the cache key format has changed.
Summary by CodeRabbit