Repository navigation
Rename pkg/oauth to pkg/oauthproto and move DCR primitives - #5036
Conversation
bb969b5 to
3cb8de5
Compare
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Looks good. One follow-up worth tracking under #4977 before we close it out: the authserver still defines its own DCRRequest/DCRResponse in pkg/authserver/server/registration/dcr.go alongside the new types in pkg/oauthproto. Reconciling Scope string vs ScopeList is the blocking work, so it doesn't belong in this PR, but it's worth a line in the #4977 plan so the two don't drift.
Minor aside, not blocking: the doc.go surface-area bullet says "RFC 7591 Dynamic Client Registration: request/response types" which overclaims slightly while the authserver still has its own. Tightening the wording to make it clear these are the client-side types would avoid future confusion.
3cb8de5 to
5e365c9
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Implements Phase 1 of issue #4977: - Rename pkg/oauth/ -> pkg/oauthproto/; update all 17 import sites (8 with alias can now drop the alias since pkg name matches) - Move DCR types, ScopeList, RegisterClientDynamically, and helpers from pkg/auth/oauth/dynamic_registration.go into pkg/oauthproto/dcr.go - RegisterClientDynamically now takes *http.Client directly (nil builds a default); drop registerClientDynamicallyWithClient and HTTPClient iface - Relocate IsLocalhost logic into pkg/oauthproto/IsLoopbackHost; pkg/networking keeps its IsLocalhost as a thin wrapper to avoid touching all callers - Move UserAgent constant from pkg/auth/oauth/oidc.go to pkg/oauthproto/constants.go - Generalize handleHTTPResponse error string (remove CLI-flag hints) - Host NewDynamicClientRegistrationRequest in pkg/auth/discovery/dcr_request.go (sole caller; loopback redirect-URI assumption stays out of protocol pkg) - Add tests in pkg/oauthproto/dcr_test.go covering the moved behaviour - Update pkg/oauthproto/doc.go to document the expanded surface area Address code review feedback Fixed issues from code review: - CRITICAL: Delete orphaned pkg/oauth/ directory (7 files); no importers remain - HIGH: Copy request in RegisterClientDynamically before validateAndSetDefaults to avoid mutating the caller's struct; nil guard moved up before the deref - HIGH: Drop redundant oauthproto alias from 4 test files (token_test.go, oidc_test.go, discovery_test.go, upstream/oidc_test.go) - MEDIUM: Remove isLoopbackHost private wrapper; dcr.go calls IsLoopbackHost directly - MEDIUM: Replace 13-line Apache header in pkg/oauthproto/doc.go with 2-line SPDX - MEDIUM: Add comment in registerDynamicClient clarifying CLI-flag hint is intentional; reference #4978 for future structured error handling Address second round of code review feedback Fixed issues from code review: - HIGH: Replace 13-line Apache header and redundant package doc comment in pkg/oauthproto/constants.go with the 2-line SPDX form; the file was substantially rewritten (UserAgent added, original URN constants removed) so it qualifies as "rewritten in full" per the project header rule - MEDIUM: Drain response body with io.Copy(io.Discard) before returning the content-type error in handleHTTPResponse, allowing net/http to reuse the underlying TCP connection per the project drain-before-close style rule Add missing io.LimitReader body-cap test for handleHTTPResponse The Testing Strategy in issue #4977 requires that handleHTTPResponse is tested for its 1 MB io.LimitReader cap. The non-JSON content-type and missing client_id cases were already covered; this commit adds the cap test. The test sends a body padded well past 1 MB and verifies the call returns promptly without hanging or panicking — a future refactor that removes or raises the cap would break the JSON decode and surface the regression immediately.
5e365c9 to
16e2153
Compare
github-actions
Bot
added
size/XL
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5036 +/- ##
==========================================
- Coverage 69.81% 69.75% -0.06%
==========================================
Files 562 564 +2
Lines 56647 56649 +2
==========================================
- Hits 39548 39518 -30
- Misses 14063 14101 +38
+ Partials 3036 3030 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
pkg/auth/oauthto reuse the Dynamic Client Registration (DCR) client because doing so would create an import cycle. This blocked Phase 2 of the DCR story (Authserver DCR integration (Phase 2, Steps 2a-2g) #4978), which wires the authserver to upstream OAuth providers.pkg/oauthtopkg/oauthproto, moves the RFC 7591 DCR primitives frompkg/auth/oauth/dynamic_registration.gointopkg/oauthproto/dcr.go, and relocates the loopback-host helper intopkg/oauthprotoso the package remains a true leaf with nopkg/networkingdependency.Implements (most of) #4977 (the authserver still defines its own DCRRequest/DCRResponse in pkg/authserver/server/registration/dcr.go alongside the new types in pkg/oauthproto. Reconciling Scope string vs ScopeList is the blocking work, so it doesn't belong in this PR).
Type of change
Test plan
task test)task lint-fix)Unit tests for all moved DCR logic now live in
pkg/oauthproto/dcr_test.go(packageoauthproto_test), coveringRegisterClientDynamicallyhappy paths, nil-client default, endpoint validation,validateAndSetDefaults,handleHTTPResponseerror mapping (including regression guard that the CLI-flag hint is absent),ScopeListmarshal/unmarshal,isLoopbackHost, and the 1 MB body-cap viaio.LimitReader.Changes
pkg/oauth/(all files)pkg/oauthproto/;package oauth→package oauthprotopkg/oauthproto/dcr.goRegisterClientDynamically, and helpers moved frompkg/auth/oauth/dynamic_registration.go;getHTTPClient/registerClientDynamicallyWithClientremoved;*http.Clientparameter replaces thenetworking.HTTPClientinterface indirectionpkg/oauthproto/dcr_test.gopkg/auth/oauth/dynamic_registration_test.go; mock-client tests rewritten usinghttptest.NewServerpkg/oauthproto/locality.goIsLoopbackHost(exported) hosts the canonical loopback check; keepspkg/oauthprotofree of anypkg/networkingimportpkg/oauthproto/constants.goUserAgent = "ToolHive/1.0"moved here frompkg/auth/oauth/oidc.gopkg/oauthproto/doc.gopkg/networking/utilities.goIsLocalhostmade a thin wrapper overoauthproto.IsLoopbackHost— all existing callers unchangedpkg/auth/discovery/dcr_request.goNewDynamicClientRegistrationRequestfactory (CLI-specific loopback redirect URI assumption) extracted here frompkg/auth/oauth/dynamic_registration.gopkg/auth/discovery/discovery.goregisterDynamicClientupdated to call the local factory andoauthproto.RegisterClientDynamicallypkg/auth/oauth/dynamic_registration.gopkg/auth/oauth/dynamic_registration_test.gopkg/oauthproto/dcr_test.go)pkg/oauth→pkg/oauthproto; the 8 sites that already aliased asoauthprotodrop the aliasImplementation Details
pkg/oauthprotois a leaf package: it depends only on the standard library andfosite(already pulled in byredirect.go).pkg/networkingnow importspkg/oauthprotofor the loopback check, not the other way around. This is the structural change that unblockspkg/authserverfrom importing the DCR client in Phase 2.RegisterClientDynamicallynow takes*http.Clientdirectly (nil means "use the 30 s / 10 s / 10 s default"). The formernetworking.HTTPClientinterface indirection and the privateregisterClientDynamicallyWithCliententry point are removed, eliminating a layer of unnecessary abstraction.--remote-auth-client-id…") inhandleHTTPResponseis replaced with a protocol-neutral message before the function moves into the protocol package.NewDynamicClientRegistrationRequestremains inpkg/auth/discovery/because it hardcodeshttp://localhost:<port>/callback— a CLI assumption that must not leak into a protocol-level package.Does this introduce a user-facing change?
No.
Special notes for reviewers
go list -deps ./pkg/oauthproto/...should show nopkg/networkingpath. Agrep -r "pkg/networking" pkg/oauthprotoreturns no hits.pkg/auth/oauth/dynamic_registration_test.go→pkg/oauthproto/dcr_test.gorename is a large hunk in the diff but is mostly a structural move; the test logic is equivalent with mock-based tests converted tohttptest.NewServer.pkg/oauth/doc.gois deleted;pkg/oauthproto/doc.goreplaces it with updated content reflecting the expanded surface area.Generated with Claude Code
Large PR Justification