-
Notifications
You must be signed in to change notification settings - Fork 0
feat: geoserver crud functionality, basic authorization #50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general this looks good, some small changes requested. I think we can do some refactoring after we merged this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds geoserver CRUD functionality with basic authorization, focusing on supporting WFS transactions.
- Introduces new WFS transaction types and XML mappings (internal/wfs/transaction.go).
- Adds a utility for extracting transaction metadata (internal/utils/utils.go).
- Updates configuration settings and improves authorization logic for transaction requests (config.yaml, cmd/filter-proxy/main.go).
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
internal/wfs/transaction.go | Added new WFS transaction types and XML mapping |
internal/utils/utils.go | Added GetTransactionMetadata utility function |
config.yaml | Updated backend URL and CORS configuration |
cmd/filter-proxy/main.go | Enhanced request authorization and transaction handling |
Comments suppressed due to low confidence (2)
cmd/filter-proxy/main.go:207
- The 'bytes' package is used here but not imported; please add 'import "bytes"' to avoid build errors.
requestBody = bytes.NewReader(marshaledBody)
cmd/filter-proxy/main.go:431
- [nitpick] Consider correcting 'laagnaam' to a more standardized term like 'layer name' to improve clarity in log messages.
log.Printf("%+v", "laagnaam"+layerName)
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 1181 🏁 Script executed:
Length of output: 26314 @justiandevs Hi there! I see you've mentioned me on PR #50 "feat: geoserver crud functionality, basic authorization". I'd be happy to help you with this PR that adds GeoServer CRUD functionality and basic authorization support. I can:
What would you like me to help with specifically? |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 15489 @justiandevs I see you're working on PR #50 which adds GeoServer CRUD functionality and basic authorization. Looking at the changes, I notice you've implemented:
I can help you with:
Let me know what specific aspect of the PR you'd like me to focus on! |
@coderabbitai I want a full PR review |
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed:
Length of output: 35935 🏁 Script executed:
Length of output: 3152 🏁 Script executed:
Length of output: 2279 🏁 Script executed:
Length of output: 6108 @justiandevs Here's my full review of PR #50 "feat: geoserver crud functionality, basic authorization": 👍 Code Additions
🛠️ Suggestions for Improvement
|
WalkthroughThe changes introduce WFS (Web Feature Service) transaction detection and handling into the filter-proxy service. This includes parsing and validating XML transaction bodies, extracting metadata, updating the authorization flow, and modifying request forwarding logic. Supporting utilities and new XML struct definitions for WFS transactions are also added. Configuration updates enable CORS and adjust backend URLs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Proxy
participant AuthService
participant Backend
Client->>Proxy: HTTP Request (may include WFS Transaction XML)
Proxy->>Proxy: Parse and preserve body
Proxy->>Proxy: If not passthrough, check for WFS Transaction
alt WFS Transaction detected
Proxy->>Proxy: Unmarshal XML to Transaction struct
Proxy->>Proxy: Extract metadata (layer name, count)
Proxy->>AuthService: Authorize with transaction info
AuthService-->>Proxy: Authorization response
Proxy->>Backend: Forward request with validated XML body
else Not a WFS Transaction
Proxy->>AuthService: Authorize with standard info
AuthService-->>Proxy: Authorization response
Proxy->>Backend: Forward request as-is
end
Backend-->>Proxy: Backend response
Proxy-->>Client: Return response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)cmd/filter-proxy/main.go (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (7)
config.yaml (4)
7-7
: Avoid hard-coding a trailing slash inauthorizationServiceUrl
Several places in the code join the base URL with additional path fragments; having a hard-coded trailing slash can lead to duplicated “//” or broken URLs when
path.Join
is used elsewhere. Consider removing the slash and let the client code normalise the path.-authorizationServiceUrl: http://localhost:8000/atlas/api/v1/authorize/ +authorizationServiceUrl: http://localhost:8000/atlas/api/v1/authorize
16-25
: YAML indentation does not follow the 2-space rule
yamllint
is already warning that the list items are indented with 4 spaces instead of 2.
While it does not break runtime behaviour, it pollutes CI pipelines that lint the repository.- allowedOrigins: - - http://localhost:8000 + allowedOrigins: + - http://localhost:8000Apply the same fix to
allowedHeaders
,allowedMethods
, etc.🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 19-19: wrong indentation: expected 2 but found 4
(indentation)
32-34
: Restrict POST to the subset of operations you actually supportThe proxy currently allows any POST on
/api/ows
. The back-end only handles POST for<Transaction>
requests; everything else is a GET.
Attackers could combine this with XML/JSON payloads you do not expect.If feasible, add a validator in the request handler (or here, add separate path entries) to explicitly reject non-transaction POSTs.
🧰 Tools
🪛 YAMLlint (1.35.1)
[warning] 33-33: wrong indentation: expected 4 but found 6
(indentation)
169-169
: Missing newline at end of fileA final newline avoids false-diffs in subsequent commits and is a POSIX convention.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 169-169: no new line character at the end of file
(new-line-at-end-of-file)
internal/wfs/transaction.go (1)
26-33
:Filter
only supports one<FeatureId>
– consider slice for completenessReal-world transactions often specify multiple feature IDs. Modelling this as a slice keeps the parser future-proof:
-type Filter struct { - FeatureID FeatureID `xml:"FeatureId"` -} +type Filter struct { + FeatureID []FeatureID `xml:"FeatureId"` +}cmd/filter-proxy/main.go (2)
431-434
: Remove temporary debug logging
log.Printf("%+v", "laagnaam"+layerName)
leaks internal details and clutters logs.
Either switch to structured logging or delete.- log.Printf("%+v", "laagnaam"+layerName)
398-402
: Clarify error for simultaneous body & query parameters
Invalid: both XML and query param are set
gives little context.
Include the offending parameter names to aid client debugging and return a RFC-consistent 400 message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/filter-proxy/main.go
(9 hunks)config.yaml
(3 hunks)internal/utils/utils.go
(2 hunks)internal/wfs/transaction.go
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/utils/utils.go (1)
internal/wfs/transaction.go (1)
Transaction
(5-14)
🪛 YAMLlint (1.35.1)
config.yaml
[warning] 19-19: wrong indentation: expected 2 but found 4
(indentation)
[warning] 28-28: wrong indentation: expected 0 but found 2
(indentation)
[warning] 33-33: wrong indentation: expected 4 but found 6
(indentation)
[error] 169-169: no new line character at the end of file
(new-line-at-end-of-file)
🪛 golangci-lint (1.64.8)
internal/wfs/transaction.go
9-9: SA5008: invalid XML tag: unknown option "xmlns:xsi"
(staticcheck)
10-10: SA5008: invalid XML tag: unknown option "xsi:schemaLocation"
(staticcheck)
🔇 Additional comments (1)
config.yaml (1)
146-149
: Check that both GeoServer back-end URLs are kept in sync
geoserver
now points athttp://localhost:8080/geoserver
, butgeoserver-wmts
still points tohttp://localhost/geoserver
.
If WMTS also moved to port 8080 this will 404 silently.Please verify the intended port.
XSI string `xml:",xmlns:xsi,attr"` | ||
SchemaLocation string `xml:",xsi:schemaLocation,attr"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalid encoding/xml
struct tags – will break (un)marshalling
xml:",xmlns:xsi,attr"
and xml:",xsi:schemaLocation,attr"
are not valid tag syntaxes and are flagged by staticcheck
.
Use the attribute name directly before ,attr
.
- XSI string `xml:",xmlns:xsi,attr"`
- SchemaLocation string `xml:",xsi:schemaLocation,attr"`
+ XSI string `xml:"xmlns:xsi,attr"`
+ SchemaLocation string `xml:"xsi:schemaLocation,attr"`
Without this fix the proxy will silently drop those attributes on marshal and error on unmarshal if validation is strict.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
XSI string `xml:",xmlns:xsi,attr"` | |
SchemaLocation string `xml:",xsi:schemaLocation,attr"` | |
XSI string `xml:"xmlns:xsi,attr"` | |
SchemaLocation string `xml:"xsi:schemaLocation,attr"` |
🧰 Tools
🪛 golangci-lint (1.64.8)
9-9: SA5008: invalid XML tag: unknown option "xmlns:xsi"
(staticcheck)
10-10: SA5008: invalid XML tag: unknown option "xsi:schemaLocation"
(staticcheck)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@justiandevs ik twijfel hier nog over. Is dit niet nodig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Als ik hier een , voor weghaal dan breekt alle functionaliteit en gaat het niet goed met het vormgeven van de request vanuit FilterProxy naar GeoServer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
cmd/filter-proxy/main.go (1)
372-532
: 🛠️ Refactor suggestionFunction is too large and complex - needs refactoring
This function has grown to 160+ lines and handles multiple responsibilities. As noted in previous reviews, it would benefit from being split into smaller, focused functions.
Consider breaking it down like this:
func authorizeRequestWithService(config *config.Config, backend config.Backend, path config.Path, r *http.Request, filterParams map[string]interface{}, body []byte) (int, *AuthorizationResponse, bool) { // Validate request structure if err := validateRequest(r, body); err != nil { return http.StatusBadRequest, nil, false } // Build authorization payload based on service type authBody, isTransaction, err := buildAuthorizationBody(backend, path, r, body) if err != nil { return http.StatusBadRequest, nil, false } // Perform authorization request return performAuthorization(config, r, authBody, isTransaction) } func validateRequest(r *http.Request, body []byte) error { /* ... */ } func buildAuthorizationBody(backend config.Backend, path config.Path, r *http.Request, body []byte) (map[string]interface{}, bool, error) { /* ... */ } func performAuthorization(config *config.Config, r *http.Request, authBody map[string]interface{}, isTransaction bool) (int, *AuthorizationResponse, bool) { /* ... */ }
♻️ Duplicate comments (1)
cmd/filter-proxy/main.go (1)
98-99
:⚠️ Potential issueRequest body consumption still not addressed
This issue was previously identified but remains unfixed. Reading the request body without restoring it causes downstream handlers to see EOF when they try to read the body.
The previous suggestion to restore the body should be implemented:
body, _ := io.ReadAll(r.Body) +r.Body.Close() +r.Body = io.NopCloser(bytes.NewReader(body))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/filter-proxy/main.go
(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/filter-proxy/main.go (3)
internal/wfs/transaction.go (1)
Transaction
(5-14)internal/config/config.go (3)
Config
(48-59)Backend
(9-25)Path
(27-37)internal/utils/utils.go (3)
QueryParamsContainMultipleKeys
(25-38)QueryParamsToLower
(14-23)GetTransactionMetadata
(108-125)
🔇 Additional comments (1)
cmd/filter-proxy/main.go (1)
134-134
: Function signature enhancement looks goodThe addition of the
isTransaction
boolean return value is well-integrated and necessary for the new WFS transaction handling logic.
Summary by CodeRabbit