[management] Validate account ID in URL matches authenticated account

[John-Dixon-IV]

Summary

• The updateAccount and deleteAccount handlers extracted accountId from the URL parameter without validating it against the authenticated user's account from the auth context. Every other handler in the codebase (users, groups, routes, peers, dns) uses userAuth.AccountId from the auth context — these two were the only ones trusting the URL parameter.
• Added explicit validation that the URL accountId matches userAuth.AccountId, returning 403 if they differ.
• The backend ValidateUserPermissions check mitigates single-account exploitation, but handlers should enforce this as defense-in-depth.

Test plan

• PutAccount with mismatched accountId returns forbidden — URL has different accountId than auth context → 403
• TestDeleteAccount_CrossAccountForbidden — DELETE with mismatched accountId → 403
• All 9 existing account handler tests continue to pass

Summary by CodeRabbit

• Bug Fixes

  • Strengthened account access control to ensure users can only update or delete their own accounts. Attempts to modify accounts belonging to other users now return permission denied responses.

• Tests

  • Added comprehensive test coverage for cross-account access restriction scenarios during account updates and deletions.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds account ID validation in the accounts handler to enforce that authenticated users can only access and modify their own accounts. When updating or deleting an account, the handler now compares the path parameter account ID against the authenticated account ID, returning a 403 Forbidden error on mismatch.

Changes

Cohort / File(s)	Summary
Account Access Control management/server/http/handlers/accounts/accounts_handler.go	Added validation in updateAccount and deleteAccount handlers to ensure the account ID from the URL path matches the authenticated account ID; returns PermissionDenied (403) on mismatch.
Handler Tests management/server/http/handlers/accounts/accounts_handler_test.go	Added test cases for cross-account access scenarios: mismatched accountId in PUT request and new TestDeleteAccount_CrossAccountForbidden function validating 403 responses.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• [management] fixed ischild check #5279 — Modifies middleware validation of AccountId for impersonation scenarios, directly related to the auth-driven AccountId comparisons introduced in this PR's handler checks.

Suggested reviewers

• pascal-fischer
• crn4

Poem

🐰 A rabbit hops through the account door,
Checks the ID—must match to be sure!
No crossing the fence to thy neighbor's store,
Just your own burrow, nothing more! 🔐

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: adding validation that account ID in the URL matches the authenticated account ID.
Description check	✅ Passed	The PR description provides a comprehensive summary of changes, clear test plan with confirmed tests, and rationale for the change.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[coderabbitai]

🧹 Nitpick comments (1)

management/server/http/handlers/accounts/accounts_handler_test.go (1)

343-370: Consider asserting the delete manager call is never reached on cross-account requests.

Status-code assertion is good; adding a “not called” assertion makes the handler-level short-circuit guarantee explicit.

Suggested test hardening

 func TestDeleteAccount_CrossAccountForbidden(t *testing.T) {
 	accountID := "test_account"
 	adminUser := types.NewAdminUser("test_user")

 	handler := initAccountsTestData(t, &types.Account{
 		Id:      accountID,
 		Domain:  "hotmail.com",
 		Network: types.NewNetwork(),
 		Users: map[string]*types.User{
 			adminUser.Id: adminUser,
 		},
 		Settings: &types.Settings{},
 	})
+	deleteCalled := false
+	mockAM, ok := handler.accountManager.(*mock_server.MockAccountManager)
+	if !ok {
+		t.Fatal("expected MockAccountManager")
+	}
+	mockAM.DeleteAccountFunc = func(ctx context.Context, accountID, userID string) error {
+		deleteCalled = true
+		return nil
+	}

 	recorder := httptest.NewRecorder()
 	req := httptest.NewRequest(http.MethodDelete, "/api/accounts/different_account_id", nil)
 	req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{
 		UserId:    adminUser.Id,
 		AccountId: accountID,
 		Domain:    "hotmail.com",
 	})

 	router := mux.NewRouter()
 	router.HandleFunc("/api/accounts/{accountId}", handler.deleteAccount).Methods("DELETE")
 	router.ServeHTTP(recorder, req)

 	assert.Equal(t, http.StatusForbidden, recorder.Code, "cross-account delete should be forbidden")
+	assert.False(t, deleteCalled, "DeleteAccount should not be called for cross-account requests")
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@management/server/http/handlers/accounts/accounts_handler_test.go` around
lines 343 - 370, The test TestDeleteAccount_CrossAccountForbidden should also
assert that the account-deletion manager method is never invoked when requesting
a different account; modify the setup returned by initAccountsTestData to inject
a mock or spy manager and add an assertion that its DeleteAccount (or equivalent
method on the handler's manager) was not called after invoking
handler.deleteAccount, keeping the existing HTTP 403 assertion intact.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@management/server/http/handlers/accounts/accounts_handler_test.go`:
- Around line 343-370: The test TestDeleteAccount_CrossAccountForbidden should
also assert that the account-deletion manager method is never invoked when
requesting a different account; modify the setup returned by
initAccountsTestData to inject a mock or spy manager and add an assertion that
its DeleteAccount (or equivalent method on the handler's manager) was not called
after invoking handler.deleteAccount, keeping the existing HTTP 403 assertion
intact.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0ca5953 and bd8e206.

📒 Files selected for processing (2)

• management/server/http/handlers/accounts/accounts_handler.go
• management/server/http/handlers/accounts/accounts_handler_test.go

[mlsmaycon]

Hello @John-Dixon-IV, thanks for your PR. The userAuth.AccountId derives from the URL implicitly. So the check implemented here is done in the managers. We are currently moving the permissions validation in the PR #5482 and it will be better to focus the fix if needed after that is merged.