Skip to content

Conversation

@4t8dd
Copy link
Contributor

@4t8dd 4t8dd commented Nov 23, 2025

Add clear, actionable error messages when OAuth providers do not support Dynamic Client Registration (DCR), helping users understand why authentication fails and how to configure client credentials manually.

Changes:

  • Detect missing registration_endpoint in OIDC discovery document
  • Recognize HTTP status codes indicating DCR is unavailable (404, 405, 501)
  • Provide actionable guidance directing users to --remote-auth-client-id and --remote-auth-client-secret flags
  • Add comprehensive test coverage for all DCR unsupported scenarios

Error message improvements:

  • Before: "dynamic client registration failed with status 404: {...}"
  • After: "this provider does not support Dynamic Client Registration (DCR) - HTTP 404. Please configure OAuth client credentials using --remote-auth-client-id and --remote-auth-client-secret flags, or register a client manually with the provider"

Fixes #2680

Add clear, actionable error messages when OAuth providers do not support
Dynamic Client Registration (DCR), helping users understand why authentication
fails and how to configure client credentials manually.

Changes:
- Detect missing registration_endpoint in OIDC discovery document
- Recognize HTTP status codes indicating DCR is unavailable (404, 405, 501)
- Provide actionable guidance directing users to --remote-auth-client-id and --remote-auth-client-secret flags
- Add comprehensive test coverage for all DCR unsupported scenarios

Error message improvements:
- Before: "dynamic client registration failed with status 404: {...}"
- After: "this provider does not support Dynamic Client Registration (DCR) - HTTP 404. Please configure OAuth client credentials using --remote-auth-client-id and --remote-auth-client-secret flags, or register a client manually with the provider"

Fixes stacklok#2680

Signed-off-by: 4t8dd <wanger.xyz@gmail.com>
ChrisJBurns
ChrisJBurns previously approved these changes Nov 23, 2025
@ChrisJBurns
Copy link
Collaborator

Thanks @4t8dd , approved, let's see what the CI gods say 👍

@codecov
Copy link

codecov bot commented Nov 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.61%. Comparing base (6800c99) to head (93d70dc).
⚠️ Report is 25 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2696      +/-   ##
==========================================
+ Coverage   55.59%   55.61%   +0.01%     
==========================================
  Files         314      314              
  Lines       30445    30456      +11     
==========================================
+ Hits        16925    16937      +12     
+ Misses      12033    12032       -1     
  Partials     1487     1487              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ChrisJBurns
Copy link
Collaborator

@4t8dd There's some linting issues it seems, to see these locally run task lint (if there are some formatting issues you can run task lint-fix. Don't worry about the PR Size Labeler, that's a new action we've added and we're still ironing out the kinks to it.

@4t8dd
Copy link
Contributor Author

4t8dd commented Nov 25, 2025

@4t8dd There's some linting issues it seems, to see these locally run task lint (if there are some formatting issues you can run task lint-fix. Don't worry about the PR Size Labeler, that's a new action we've added and we're still ironing out the kinks to it.

sure. Thanks for the info. I will fix this then.

Signed-off-by: 4t8dd <wanger.xyz@gmail.com>
@4t8dd
Copy link
Contributor Author

4t8dd commented Nov 25, 2025

@ChrisJBurns I just updated this branch with lint fix. I guess the CI should be manually activated again.
One more issue is: I think CI should run against every PR to filter any lint issues before merging. Then it would be OK to review and so on.

@github-actions github-actions bot added the size/XS Extra small PR: < 100 lines changed label Nov 25, 2025
@4t8dd
Copy link
Contributor Author

4t8dd commented Nov 26, 2025

I will check this again.

@4t8dd
Copy link
Contributor Author

4t8dd commented Nov 26, 2025

@ChrisJBurns I check this error

[FAILED] in [It] - /home/runner/work/toolhive/toolhive/test/e2e/thv-operator/virtualmcp/virtualmcp_inline_auth_test.go:208 @ 11/25/25 15:07:44.564
  STEP: Cleaning up test resources @ 11/25/25 15:07:44.564
• [FAILED] [30.062 seconds]
VirtualMCPServer Inline Auth with Local Incoming when using local incoming with inline outgoing auth [It] should proxy tool calls with inline auth configuration
/home/runner/work/toolhive/toolhive/test/e2e/thv-operator/virtualmcp/virtualmcp_inline_auth_test.go:162

  [FAILED] Tool call should succeed with inline auth
  Unexpected error:
      <*transport.Error | 0xc000471870>: 
      transport error: failed to send request: failed to send request: Post "http://localhost:30020/mcp": EOF
      {
          Err: <*fmt.wrapError | 0xc00012fa00>{
              msg: "failed to send request: failed to send request: Post \"http://localhost:30020/mcp\": EOF",
              err: <*fmt.wrapError | 0xc00012f9e0>{
                  msg: "failed to send request: Post \"http://localhost:30020/mcp\": EOF",
                  err: <*url.Error | 0xc000566330>{
                      Op: "Post",
                      URL: "http://localhost:30020/mcp",
                      Err: <*errors.errorString | 0x25b5530>{s: "EOF"},
                  },
              },
          },
      }

it looks have nothing to do with my patch.
I checked out the fix to main and run the test locally and it works good.
may I create another PR and close this one and check my changes?
let's just merge only my changes to see. Is it right for you?

@ChrisJBurns
Copy link
Collaborator

@4t8dd Yeah you're right, the e2e test are a bit flaky at the moment. We've raised #2741 to resolve this. I think you're changes should be good to do when the underlying flakiness has been resolved 🚀

@4t8dd
Copy link
Contributor Author

4t8dd commented Nov 27, 2025

I close this and open a new one based on the latest update.

@4t8dd 4t8dd closed this Nov 27, 2025
@4t8dd 4t8dd deleted the 2680 branch November 27, 2025 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Notify When DCR Is Unsupported for Remote MCP Server

2 participants