Conversation
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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3315 +/- ##
==========================================
+ Coverage 65.98% 66.02% +0.03%
==========================================
Files 413 420 +7
Lines 41075 41374 +299
==========================================
+ Hits 27104 27316 +212
- Misses 11872 11951 +79
- Partials 2099 2107 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
There was a problem hiding this comment.
Do you mind if we generalize this a little bit more? I am really keen on this authorizer because it's so general purpose. Having an HTTP-based authorizer is something that's applicable quite generally and could potentially be used with other PDPs that would respect the same API signature.
So... Tell me what you think about this:
- Let's rename this authorizer to something more general like:
httpv1or something of the sort. - We shall keep the PORC mappings and... basically keep this same implementation.
- Let's remove the manetu MPE policy domain examples from this particular PR to keep it constrained and smaller
There are some particular pieces that are tied to MPE (e.g. probe mode), but that's fine, we can keep those in this PR and generalize later.
What do you think?
Regarding the MPE policy domain samples: I want to find a good place for folks to view an e2e sample of this, and there we could add the MPE policy samples. What do you think?
|
@JAORMX I pushed an update with your suggested changes |
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. |
There was a problem hiding this comment.
Pull request overview
This PR adds a general-purpose HTTP-based Policy Decision Point (PDP) authorizer that enables ToolHive to delegate authorization decisions to external PDP servers using the PORC (Principal-Operation-Resource-Context) model. The implementation supports configurable context inclusion and JWT claim mapping.
Changes:
- Implements HTTP PDP authorizer with PORC-based decision API
- Adds configurable context options for controlling what MCP information is included in authorization requests
- Provides comprehensive test coverage for all major components (config, HTTP client, PORC builder, core authorizer)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/authz/authorizers/http/config.go | Configuration types and validation for HTTP PDP connection and context options |
| pkg/authz/authorizers/http/config_test.go | Tests for configuration parsing and validation |
| pkg/authz/authorizers/http/core.go | Core authorizer implementation with factory registration |
| pkg/authz/authorizers/http/core_test.go | Integration tests for the authorizer with mock PDP server |
| pkg/authz/authorizers/http/http_client.go | HTTP client for communicating with PDP decision endpoint |
| pkg/authz/authorizers/http/http_client_test.go | HTTP client tests including error cases and validation |
| pkg/authz/authorizers/http/porc.go | PORC builder for mapping MCP requests to PDP format |
| pkg/authz/authorizers/http/porc_test.go | Comprehensive tests for PORC building logic |
| pkg/authz/authorizers.go | Registration import for the HTTP authorizer |
| examples/authz-httpv1-config.yaml | Example configuration file for HTTP PDP setup |
| docs/authz.md | Documentation for HTTP PDP authorizer with API contract and examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @ghaskins how are you doing? I triggered some review for you some days ago, and i also see that the PR is failing in lint. I am approaching you to see the status of the work, if you are able to continue the collaboration. We can also take the issues on your behalf and put this PR on shape to be merged. Please keep us informed, thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
IIntroduce a general-purpose authorization backend using HTTP-based Policy
Decision Points (PDPs). This authorizer can work with any PDP server that
implements the PORC (Principal-Operation-Resource-Context) decision endpoint.
Key features:
The authorizer uses a simple API contract:
Compatible with Manetu PolicyEngine (MPE) and any custom PDP implementing
the same API.
Large PR Justification
Multiple related changes that would break if separated