Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated a dependency and hardened impersonation handling in auth middleware: when an account impersonation parameter is present, middleware calls Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Middleware as AuthMiddleware
participant Integrations as IntegrationsService
Client->>Middleware: HTTP request (token + optional account param)
Middleware->>Middleware: parse token -> userAuth (userId, AccountId)
alt account param present
Middleware->>Integrations: IsValidChildAccount(ctx, userId, parentAccount, targetAccount)
Integrations-->>Middleware: true / false
alt true
Middleware->>Middleware: set AccountId = targetAccount\nset IsChild = true
else false
Middleware->>Middleware: keep AccountId = parentAccount\nIsChild = false
end
end
Middleware->>Client: continue request with resolved auth context
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/http/middleware/auth_middleware.go (1)
133-136:⚠️ Potential issue | 🔴 CriticalJWT impersonation path lacks the same
IsValidChildAccountvalidation.The PAT path (lines 210–214) now properly validates the impersonated account via
integrations.IsValidChildAccount, but the JWT path here still unconditionally trusts theaccountquery parameter. This means any JWT-authenticated user can impersonate any account by passing?account=<target>, bypassing the child-account check entirely.If the fix was intentional only for PAT, please confirm. Otherwise this is a security gap that should be addressed.
🔒 Proposed fix
if impersonate, ok := r.URL.Query()["account"]; ok && len(impersonate) == 1 { - userAuth.AccountId = impersonate[0] - userAuth.IsChild = ok + if integrations.IsValidChildAccount(ctx, userAuth.AccountId, impersonate[0]) { + userAuth.AccountId = impersonate[0] + userAuth.IsChild = true + } }
🧹 Nitpick comments (1)
management/server/http/middleware/auth_middleware.go (1)
210-215: Silent no-op whenIsValidChildAccountreturns false — consider returning an error or logging.If a caller passes
?account=<invalid>, the request proceeds silently with the original account context. This could be confusing to API consumers who believe they are operating on a child account but are actually operating on their own. Returning an authorization error (or at minimum logging a warning) would make failures observable.Proposed approach
if impersonate, ok := r.URL.Query()["account"]; ok && len(impersonate) == 1 { if integrations.IsValidChildAccount(r.Context(), userAuth.AccountId, impersonate[0]) { userAuth.AccountId = impersonate[0] userAuth.IsChild = true + } else { + return r, status.Errorf(status.Forbidden, "account %s is not a valid child account", impersonate[0]) } }
There was a problem hiding this comment.
Pull request overview
Fixes child-account impersonation handling in the management HTTP auth middleware by validating the requested child account instead of blindly marking requests as “child” when the account query param is present.
Changes:
- Add a child-account validation check (
integrations.IsValidChildAccount) before allowing PAT-basedaccountimpersonation. - Update
management-integrations/integrationsdependency version to pick up the new validation helper. - Update module sums accordingly.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| management/server/http/middleware/auth_middleware.go | Gate PAT account impersonation on a child-account validity check. |
| go.mod | Bump github.com/netbirdio/management-integrations/integrations to a newer pseudo-version. |
| go.sum | Add checksums for the updated integrations dependency and related transitive updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if integrations.IsValidChildAccount(r.Context(), userAuth.AccountId, impersonate[0]) { | ||
| userAuth.AccountId = impersonate[0] | ||
| userAuth.IsChild = true | ||
| } |
There was a problem hiding this comment.
When the account query param is present but not a valid child account, the request silently proceeds with the parent account. This can lead clients to unintentionally operate on the wrong account; consider returning a PermissionDenied/InvalidArgument error when impersonation is requested but validation fails. Also note checkJWTFromRequest still sets IsChild based only on query presence; it should likely use the same validation to avoid inconsistent/unsafe behavior.
| if integrations.IsValidChildAccount(r.Context(), userAuth.AccountId, impersonate[0]) { | |
| userAuth.AccountId = impersonate[0] | |
| userAuth.IsChild = true | |
| } | |
| if !integrations.IsValidChildAccount(r.Context(), userAuth.AccountId, impersonate[0]) { | |
| return r, status.Errorf(status.PermissionDenied, "impersonation requested for non-child account") | |
| } | |
| userAuth.AccountId = impersonate[0] | |
| userAuth.IsChild = true |
| if impersonate, ok := r.URL.Query()["account"]; ok && len(impersonate) == 1 { | ||
| userAuth.AccountId = impersonate[0] | ||
| userAuth.IsChild = ok | ||
| if integrations.IsValidChildAccount(r.Context(), userAuth.AccountId, impersonate[0]) { | ||
| userAuth.AccountId = impersonate[0] | ||
| userAuth.IsChild = true | ||
| } |
There was a problem hiding this comment.
This change introduces a hard dependency on integrations.IsValidChildAccount, which makes existing middleware unit tests non-deterministic (and will break TestAuthMiddleware_Handler_Child’s PAT child case unless the validator can be controlled). Consider injecting a child-account validation function into AuthMiddleware (defaulting to integrations.IsValidChildAccount) so tests can mock it, and add a negative test for invalid child impersonation.
Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>
Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>
Signed-off-by: bcmmbaga <bethuelmbaga12@gmail.com>
|
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
bug fix
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Chores
Bug Fixes
Tests