feat: add operation authorization forward middleware#3021
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/controlplane/internal/usercontext/operation_authorization_middleware.go">
<violation number="1" location="app/controlplane/internal/usercontext/operation_authorization_middleware.go:96">
P1: Cache key omits `orgID`, but the authorization request includes it. If the external endpoint's decision depends on the organization, a cached "allowed" result for one org will incorrectly apply to another org for the same user and operation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
| return nil, fmt.Errorf("marshaling request: %w", err) | ||
| } | ||
|
|
||
| httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) |
There was a problem hiding this comment.
we might want to forward the API token as a header
There was a problem hiding this comment.
That should be the way. @Piskoo check how we do with policy providers. We use plain Bearer header so that providers can authenticate it automatically using standard middlewares.
| return nil, fmt.Errorf("marshaling request: %w", err) | ||
| } | ||
|
|
||
| httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body)) |
There was a problem hiding this comment.
That should be the way. @Piskoo check how we do with policy providers. We use plain Bearer header so that providers can authenticate it automatically using standard middlewares.
| return nil, func() {}, nil | ||
| } | ||
|
|
||
| conn, err := grpcconn.New(cfg.GetGrpc().GetAddr(), "", grpcconn.WithInsecure(cfg.GetInsecure())) |
There was a problem hiding this comment.
this might require also take into account custom certs? btw, why did we chose to use grpc here?
migmartri
left a comment
migmartri
left a comment
There was a problem hiding this comment.
My main question is why did we decide to use gRPC for this forwarder, when the policyProvider one uses http
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
I've misread the comment in the other PR, reverted it back to HTTP |
Signed-off-by: Sylwester Piskozub <sylwesterpiskozub@gmail.com>
| // ServerOperationsMap is a map of server operations to their authorization requirements. | ||
| // Each entry specifies the Casbin policies required (all must pass) and whether the | ||
| // operation also requires external authorization. | ||
| var ServerOperationsMap = map[string]*OperationPolicy{ |
There was a problem hiding this comment.
Note that this is a breaking change. Please create an issue to update consumers (platform) to this new API.
There was a problem hiding this comment.
@jiparis Seems like platform has it's own type defined, so no changes needed there
3 participants
Summary
Added a middleware that forwards operations to an external authorization endpoint before allowing them to proceed. Disabled by default.