fix(registry): derive bearer service from realm - #1895
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
📝 WalkthroughWalkthroughRegistry authentication now derives ChangesRegistry auth service derivation
Sequence Diagram(s)sequenceDiagram
participant GetToken
participant ProcessChallenge
participant GetAuthURL
participant Registry
GetToken->>Registry: GET /v2/
Registry-->>GetToken: Bearer challenge with realm only
GetToken->>ProcessChallenge: parse challenge values
ProcessChallenge->>ProcessChallenge: derive service from realm host
ProcessChallenge-->>GetToken: challenge values
GetToken->>GetAuthURL: build auth URL
GetAuthURL-->>GetToken: auth URL with derived service
GetToken->>Registry: GET /token with service and scope
Registry-->>GetToken: bearer token
GetToken-->>GetToken: return token
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 25 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/registry/auth/auth.go (1)
528-538: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAvoid re-parsing the challenge
ProcessChallenge’s derivedservicenever reachesGetBearerHeader;GetAuthURLparses the raw challenge again, so this path keeps two service derivations in sync. Reuse the parsed values instead of recomputing them.🤖 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/registry/auth/auth.go` around lines 528 - 538, ProcessChallenge currently derives service from realm, but GetAuthURL re-parses the raw challenge and recomputes the same values, so the derived result never gets reused. Update the auth flow around ProcessChallenge and GetAuthURL so the parsed challenge values (realm/service/scope) are passed through and reused instead of parsing the challenge twice, keeping the service derivation in one place.
🤖 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.
Nitpick comments:
In `@pkg/registry/auth/auth.go`:
- Around line 528-538: ProcessChallenge currently derives service from realm,
but GetAuthURL re-parses the raw challenge and recomputes the same values, so
the derived result never gets reused. Update the auth flow around
ProcessChallenge and GetAuthURL so the parsed challenge values
(realm/service/scope) are passed through and reused instead of parsing the
challenge twice, keeping the service derivation in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 13f09301-8f71-40cd-965a-86e616ea3f01
📒 Files selected for processing (2)
pkg/registry/auth/auth.gopkg/registry/auth/auth_test.go
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/registry/auth/auth.go (1)
276-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading error classification for auth-URL construction failure.
A failure from
GetAuthURLis a challenge/URL construction problem, but it is wrapped witherrFailedDecodeResponse. ConsidererrInvalidChallengeHeader(Line 58) or a dedicated construction error so operators get an accurate cause.🤖 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/registry/auth/auth.go` around lines 276 - 280, The failure path in GetAuthURL is using the wrong error classification: when bearer auth URL construction fails, it should not be wrapped with errFailedDecodeResponse. Update the return in the GetAuthURL error branch to use errInvalidChallengeHeader or a dedicated auth-URL construction error so the wrapped cause matches the actual failure, and keep the existing debug log with logrus.WithError and fields.
🤖 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.
Nitpick comments:
In `@pkg/registry/auth/auth.go`:
- Around line 276-280: The failure path in GetAuthURL is using the wrong error
classification: when bearer auth URL construction fails, it should not be
wrapped with errFailedDecodeResponse. Update the return in the GetAuthURL error
branch to use errInvalidChallengeHeader or a dedicated auth-URL construction
error so the wrapped cause matches the actual failure, and keep the existing
debug log with logrus.WithError and fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5865093e-7014-4c3b-8ea3-ad2b6df50efa
📒 Files selected for processing (1)
pkg/registry/auth/auth.go
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1895 +/- ##
==========================================
+ Coverage 74.80% 75.95% +1.14%
==========================================
Files 61 90 +29
Lines 10415 11722 +1307
==========================================
+ Hits 7791 8903 +1112
- Misses 2339 2479 +140
- Partials 285 340 +55
... and 53 files with indirect coverage changes 🚀 New features to boost your workflow:
|
- Group errFailedConstructBearerAuthURL with related bearer token errors - Fix formatting in ProcessChallenge, addBasicAuth, and auth_test.go
- Test deriving service from realm when service field is empty - Test deriving service from realm host with port and trailing slash - Test error handling for realm without scheme and missing service
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/registry/auth/auth_test.go (1)
1030-1031: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider restoring the original value of
WATCHTOWER_REGISTRY_TLS_SKIP.The current implementation sets the config value to
falseand then usesdeferto redundantly set it tofalseagain, rather than restoring its original value. To ensure no side effects leak into other test suites, consider capturing and restoring the initial value.♻️ Proposed refactor
- viper.Set("WATCHTOWER_REGISTRY_TLS_SKIP", false) - defer viper.Set("WATCHTOWER_REGISTRY_TLS_SKIP", false) + originalTLSSkip := viper.GetBool("WATCHTOWER_REGISTRY_TLS_SKIP") + viper.Set("WATCHTOWER_REGISTRY_TLS_SKIP", false) + defer viper.Set("WATCHTOWER_REGISTRY_TLS_SKIP", originalTLSSkip)🤖 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/registry/auth/auth_test.go` around lines 1030 - 1031, Update the test setup around WATCHTOWER_REGISTRY_TLS_SKIP to capture its original value before setting it to false, then defer restoring that captured value instead of assigning false again. Keep the test’s false configuration while preserving the surrounding suite’s prior state.
🤖 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.
Nitpick comments:
In `@pkg/registry/auth/auth_test.go`:
- Around line 1030-1031: Update the test setup around
WATCHTOWER_REGISTRY_TLS_SKIP to capture its original value before setting it to
false, then defer restoring that captured value instead of assigning false
again. Keep the test’s false configuration while preserving the surrounding
suite’s prior state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d6aa334d-c0e0-403d-bccd-5e6d9286f2e7
📒 Files selected for processing (1)
pkg/registry/auth/auth_test.go
|
@llc1123 Thank you for submitting this PR. Sorry for holding off on getting to this while working through the API refactor. I added a few more tests and a few minor stylistic changes. |
2 participants
Summary
servicevalues from the challengerealmhostscope/v2/challengeFixes #1894
Testing
go test ./pkg/registry/... -count=1go test ./... -count=1go test -race ./pkg/registry/... -count=1Summary by CodeRabbit
Bug Fixes
servicefrom therealm, ensuring correct/tokenexchanges and more reliable auth URL generation.WWW-Authenticatechallenges, rejecting malformed or incomplete headers more consistently (including cases with missing scheme or unusablerealm/service).Tests
service, asserting correct query parameter derivation and successful token retrieval behavior.