Feature/authz external #874
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces external authorization support (authz_external) as a new access control mechanism. The implementation adds a new configuration block type that allows configuring external authorization services, including OpenFGA integration.
Key changes:
- Added
authz_externalconfiguration block with OpenFGA support - Integrated the new authorization type into the access control pipeline
- Refactored variable naming for improved clarity (e.g.,
helper→confHelper,absolutizePaths→resolveAbsolutePaths)
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| config/definitions.go | Added AuthZExternal field to Definitions struct |
| config/ac_authz_external.go | New file defining AuthZExternal and OpenFGA configuration structures |
| accesscontrol/authz/external.go | New file implementing External authorization control with placeholder logic |
| config/runtime/server.go | Integrated authz_external into access control configuration |
| config/configload/load.go | Updated to use renamed variables (confHelper, resolveAbsolutePaths) |
| config/configload/helper.go | Added authz_external to AC backends and definitions map, refactored error declarations |
| accesscontrol/ac.go | Fixed error handling using errors.As instead of type assertion |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func NewExternal(origin DestinationRoundTripper, includeMetadataTLS bool) (*External, error) { | ||
| return &External{ | ||
| origin: origin, | ||
| includeMetadataTLS: includeMetadataTLS, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
The NewExternal function always returns nil error but validates that origin is required in the Validate method. If origin is a required parameter, validation should occur during construction in NewExternal to fail fast, or the function signature should not return an error if it never fails.
| if conf.Definitions != nil { | ||
| for _, authZExternal := range conf.Definitions.AuthZExternal { | ||
| confErr := errors.Configuration.Label(authZExternal.Name) | ||
| authZExt, err := authz.NewExternal(nil, false) |
There was a problem hiding this comment.
Passing nil for the origin parameter will cause Validate to always fail with 'origin required' error. The origin should be properly initialized from the backend configuration rather than passing nil.
| authZExt, err := authz.NewExternal(nil, false) | |
| authZExt, err := authz.NewExternal(authZExternal.Origin, false) |
| return errors.AccessControl.Message("origin required") | ||
| } | ||
| //TODO implement me | ||
| panic("implement me") |
There was a problem hiding this comment.
The Validate method contains a panic statement that will crash the application if called. This should either be removed if this is work-in-progress code, or replaced with a proper error return until implementation is complete.
| panic("implement me") | |
| return errors.AccessControl.Message("not implemented") |
| type clientRequest struct { | ||
| Method string | ||
| URL string | ||
| Header http.Header | ||
| } | ||
|
|
||
| type authContext struct { | ||
| Source any // previous hop | ||
| Destination any // target backend (origin) | ||
| ClientRequest clientRequest // simplified form / serialized | ||
| Route any | ||
| Metadata any // user / hcl provided | ||
| MetadataTLS any // tls conn infos / opt in | ||
| } | ||
|
|
There was a problem hiding this comment.
The clientRequest and authContext types are defined but never used in the code. These should either be utilized in the implementation or removed to avoid dead code.
| type clientRequest struct { | |
| Method string | |
| URL string | |
| Header http.Header | |
| } | |
| type authContext struct { | |
| Source any // previous hop | |
| Destination any // target backend (origin) | |
| ClientRequest clientRequest // simplified form / serialized | |
| Route any | |
| Metadata any // user / hcl provided | |
| MetadataTLS any // tls conn infos / opt in | |
| } |
malud
force-pushed
the
feature/authz-external
branch
from
9a513a3 to
86109d6
Compare
fixes #873
working branch
Reviewer checklist