Repository navigation
Return 403 for unauthorized CLA manager removal - #5243
Conversation
DELETE /v4/company/{companyID}/project/{projectSFID}/cla-manager/{userLFID}
answered an authorization denial with HTTP 400. Return the 403 the spec
already documents. The create handler's denial message named
DeleteCLAManager; name the right operation, and in both messages print
the company SFID the scope check used instead of the internal company ID.
Found by the M3 role-matrix run against dev
(linuxfoundation/lfx-self-serve#1968).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Michal Lehotsky <mlehotsky@linuxfoundation.org>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughThe CLA Manager create and delete handlers now report the company external ID in authorization-denial messages. Delete authorization failures return Forbidden. Tests cover unauthorized requests for both operations. ChangesCLA Manager authorization denials
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The CLA manager denial changes appear ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟢 Approval recommended
The narrow changes preserve authorization logic and have focused regression coverage, with no unresolved findings.
0 open findings
What changed in this PR
Aligns CLA manager authorization-denial responses with the API contract without changing permissions.
Changes:
- Returns 403 instead of 400 for unauthorized removal.
- Corrects denial messages to identify the operation and company SFID.
- Adds regression tests for unauthorized create and delete requests.
| File | Description |
|---|---|
| cla-backend-go/v2/cla_manager/handlers.go | Corrects denial status and messages. |
| cla-backend-go/v2/cla_manager/handlers_test.go | Verifies 403 responses, messages, and no service calls. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
lukaszgryglicki
left a comment
There was a problem hiding this comment.
/lgtm - provide the same PR for the main branch @mlehotskylf as we now also have M3 features in prod.
Summary
DELETE /v4/company/{companyID}/project/{projectSFID}/cla-manager/{userLFID}answered an authorization denial with 400. It now returns the 403 the spec already documents (forbidden-or-company-sanctioned).POST .../cla-managerdenial message saidDeleteCLAManager. It now saysCreateCLAManager.Who is allowed does not change. Found by the M3 API role-matrix run against dev (linuxfoundation/lfx-self-serve#1968).
Clients
classifyOrgClaManagerRefusal(packages/shared/src/utils/org-cla-manager.utils.tsin linuxfoundation/lfx-self-serve) checks the body text before the status, so it classifies the refusal the same way.remove-cla-manager.tspasses the status through as the result code and does not branch on it.Test plan
go test ./v2/cla_manager/: newTestClaManagerCreateDeleteHandlersRejectUnauthorizedUserscovers an LF admin and an unrelated-company scope on create and delete. It expects 403, no service call, and a message that names the operation and the SFID. It failed before the fix.go build ./...cla-manager.deletedeny cells show 403.🤖 Generated with Claude Code