Repository navigation
UN-4399 [FEAT] Organisation capability permissions API - #2344
Conversation
|
ritwik-g
left a comment
ritwik-g
left a comment
There was a problem hiding this comment.
Self-review round 1 — unstract:review-deep (INITIAL) at tip 5b1da17de
Review: review-deep. The PR adds a migration, a permission class and a new endpoint. It is stacked on #2342, so the base is feat/UN-4398-platform-audit (merge base b0fa57faa). Scope is 15 files, 909 lines; the file count matches GitHub's.
Signals: lenses 4, 5, 7, 13 and 16 raised, from account_v2/0008, RequiresCapability and the destroy permission lists, the new endpoint and serializers, the new tests, and CAPABILITIES.md and the README.
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 1 · Medium: 6 · Low: 5 · Lenses run: 19/19 (lenses 12, 14 and 19 are N/A).
Security deep-dive: done. No findings at confidence ≥ 8. The one candidate (filtered at 3/10) is filed as #3.
boundary-sweep: 2 sinks traced, clean. Those are the audit event payloads and the refusal messages; neither carries secrets.
Lens checklist
- Spec & intent — Clean (code-reviewer)
- Architectural fit & precedent — Clean (code-reviewer)
- Correctness & edge cases — See findings #3, #4
- Security — See findings #3
- Data integrity & migrations — See findings #2
- Concurrency — Clean (infra agent:
update_fieldsscoped per key) - API & contract compatibility — See findings #8, #9 (orchestrator consumer search:
organization/settingsresponse unchanged; no consumers of the new endpoint) - Reliability & resilience — Clean (infra agent)
- Performance & cost — Clean (code-reviewer: one org query per delete; admin lookup memoized)
- Observability — Clean (code-reviewer)
- Operational safety — See findings #2
- LLM/agent-specific — N/A — no LLM code
- Testing — See findings #1, #7, #11
- Dependencies & build — N/A — none
- Code quality — See findings #12 (orchestrator: pre-commit passed)
- Doc & comment accuracy — See findings #5, #6, #10
- Cross-document rule consistency — See findings #5 (orchestrator: README vs CAPABILITIES.md vs code)
- On-prem deployment experience — See findings #2 (an on-prem image downgrade hits the same NOT NULL insert)
- Scaling & workload lifecycle — N/A — none
Open questions for the author
- Does the deploy pipeline ever roll back the backend image without reversing migrations? This affects #2, which is fixed either way.
Assumptions made
- Platform-key sessions can reach
organization/capabilities/.
cost: agents=8 wall=12:00 lenses-examined=16 of 19 files=15 lines=909
ritwik-g
left a comment
ritwik-g
left a comment
There was a problem hiding this comment.
Self-review round 2 — unstract:review-deep (FOLLOWUP) at tip 10969981d
Review: review-deep. The PR carries a migration and an authz change.
Delta: 5b1da17de..10969981d (162 lines). There is no cold read this round because the delta is under 200 lines.
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 1 · Medium: 3 · Low: 2 · Lenses run: 19/19 (lenses 12, 14 and 19 are N/A).
Prior context fetched — followup-fetch.sh exited 0. "Since" boundary: the previous review's SHA, 5b1da17de.
Scope change — NO
Security deep-dive: done. No findings at confidence ≥ 8; not degraded.
Boundary sweep: 2 sinks traced, clean.
Prior findings status
#1[High] destroy ownership untested →RESOLVED. The non-owner case and anIsOwnerpresence check are added; mutants killed.#2[Medium] no database defaults on the new columns →RESOLVED. RunSQLSET DEFAULT, verified on Postgres 15. The test gap is #16.#3[Medium] adapter type change on update →RESOLVED. The new type is enforced; the allowed side is untested (#15).#4[Medium] service accounts on unenforced capabilities →NOT RESOLVED. The fix overshot; see #13.#5,#6,#7,#9,#10,#11→RESOLVED.#8[Low] enum names →NOT RESOLVED. The override has no effect; see #14.#12[Low] "organisation" spelling →WAIVED. It is the spec's literal wording (dossier decision).
Lens checklist
- Spec & intent — Clean (code-reviewer)
- Architectural fit & precedent — Clean (code-reviewer)
- Correctness & edge cases — See findings #13, #18
- Security — Clean (orchestrator; security deep-dive done)
- Data integrity & migrations — See findings #16
- Concurrency — Clean (infra agent)
- API & contract compatibility — See findings #14 (orchestrator: no consumers yet)
- Reliability & resilience — Clean (infra agent)
- Performance & cost — Clean (code-reviewer)
- Observability — Clean (code-reviewer)
- Operational safety — Clean (infra agent: the RunSQL defaults match the model; reverse is clean)
- LLM/agent-specific — N/A — no LLM code
- Testing — See findings #15, #16, #17
- Dependencies & build — N/A — none
- Code quality — Clean (orchestrator: pre-commit passed)
- Doc & comment accuracy — See findings #13, #17
- Cross-document rule consistency — See findings #13
- On-prem deployment experience — Clean (infra agent: on-prem runs the same migrate; the defaults keep an image downgrade safe)
- Scaling & workload lifecycle — N/A — none
Open questions for the author
- Should repointing an existing LLM adapter to another provider count as "creating" one? For now this is documented as not governed (#18).
Assumptions made
- None.
cost: agents=7 wall=09:00 lenses-examined=16 of 19 files=16 lines=1019
ritwik-g
left a comment
ritwik-g
left a comment
There was a problem hiding this comment.
Self-review round 3 — unstract:review-deep (FOLLOWUP) at tip 9ba8ed2dd
Review: review-deep. The PR carries a migration and an authz change, so review-shallow's gate refuses it.
Pinned to b4dfa18fb: pre-commit.ci's ruff-format autofix, pushed during the round. It differs from the reviewed tip 9ba8ed2dd only by that formatting, which fixes #21.
Delta: 10969981d..9ba8ed2dd (136 lines, 5 files). There is no cold read this round because the delta is under 200 lines.
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 0 · Medium: 1 · Low: 2 · Lenses run: 19/19 (N/A: 12, 14).
Prior context fetched — followup-fetch.sh fetched 6 of 6. "Since" boundary: 10969981d.
Scope change — NO
Security deep-dive: done. The find pass returned no candidates at confidence ≥ 6, so no filtering was needed; not degraded.
Boundary sweep: 0 new sinks in the delta. Clean.
Base fetch: ok. The count is verified against GitHub (16 files).
Prior findings status
#4,#8,#13–#18→RESOLVEDin9ba8ed2dd. Each thread has a reply and is resolved. Mutants were killed for #4, #8, #15 and #16.
Lens checklist
- Spec & intent — Clean (code-reviewer)
- Architectural fit & precedent — Clean (code-reviewer)
- Correctness & edge cases — Clean (code-reviewer)
- Security — Clean (orchestrator, security deep-dive done, 0 findings)
- Data integrity & migrations — Clean (dispatched infra agent: the static test pins both columns and values)
- Concurrency — Clean (orchestrator: the delta adds no shared state)
- API & contract compatibility — See findings #19, #20 (no consumers in frontend, unstract-cloud or clients)
- Reliability & resilience — Clean (orchestrator: no I/O changed)
- Performance & cost — Clean (code-reviewer)
- Observability — Clean (code-reviewer)
- Operational safety — Clean (orchestrator: settings-only enum-name change)
- LLM/agent-specific — N/A — no LLM code
- Testing — Clean (pr-test-analyzer, 5 mutants killed in a private worktree, no test weakened)
- Dependencies & build — N/A — none changed
- Code quality — See findings #21
- Doc & comment accuracy — See findings #19
- Cross-document rule consistency — See findings #19 (merge sweep: allowed_for_me statements agree except the schema help_text)
- On-prem deployment experience — Clean (orchestrator: no chart, schema or migration change in the delta)
- Scaling & workload lifecycle — Clean (orchestrator: no workload config)
cost: agents=7 wall=05:22 lenses-examined=17 of 19 files=5 lines=136
ritwik-g
left a comment
ritwik-g
left a comment
There was a problem hiding this comment.
Self-review round 4 — unstract:review-deep (FOLLOWUP) at tip 4ec12071e
Review: review-deep. The PR carries a migration and an authz change, so review-shallow's gate refuses it.
Delta: 9ba8ed2dd..4ec12071e (36 lines, 4 files). That covers pre-commit.ci's b4dfa18fb and the round-3 fixes. There is no cold read this round because the delta is under 200 lines.
Verdict — APPROVE
Summary — Critical: 0 · High: 0 · Medium: 0 · Low: 0 · Lenses run: 19/19 (N/A: 12, 14).
Prior context fetched — followup-fetch.sh fetched 6 of 6. "Since" boundary: 9ba8ed2dd.
Scope change — NO
Security deep-dive: done. No candidates; filtering was not needed; not degraded.
Boundary sweep: 0 new sinks in the delta. Clean.
Base fetch: ok. The count is verified (16 files).
Prior findings status
#19[Medium] allowed_for_me help_text →RESOLVEDin4ec12071e.#20[Low] KeyEnum →RESOLVEDin4ec12071e. A CapabilityKey override was added and a schema test asserts it; the mutant was killed.#21[Low] ruff-format →RESOLVEDinb4dfa18fb(pre-commit.ci). The pinned pre-commit run is clean at4ec12071e.
New findings
None.
Lens checklist
- Spec & intent — Clean (code-reviewer)
- Architectural fit & precedent — Clean (code-reviewer)
- Correctness & edge cases — Clean (code-reviewer)
- Security — Clean (orchestrator, security deep-dive done, 0 findings)
- Data integrity & migrations — Clean (orchestrator, no migration or model in the delta)
- Concurrency — Clean (orchestrator, no shared state)
- API & contract compatibility — Clean (type-design-analyzer, full-backend schema has no collision, no consumers)
- Reliability & resilience — Clean (orchestrator, no I/O)
- Performance & cost — Clean (code-reviewer)
- Observability — Clean (code-reviewer)
- Operational safety — Clean (orchestrator, schema-name-only settings change)
- LLM/agent-specific — N/A — no LLM code
- Testing — Clean (pr-test-analyzer, 3 mutants killed, nothing weakened)
- Dependencies & build — N/A — none changed
- Code quality — Clean (orchestrator ran the pinned pre-commit, all hooks pass)
- Doc & comment accuracy — Clean (comment-analyzer)
- Cross-document rule consistency — Clean (orchestrator, help_text, docstrings and CAPABILITIES.md now agree)
- On-prem deployment experience — Clean (orchestrator, no chart, schema or migration in the delta)
- Scaling & workload lifecycle — Clean (orchestrator, no workload config)
cost: agents=5 wall=03:30 lenses-examined=17 of 19 files=4 lines=36
 [Medium impact] The PR appears safe to merge, with no actionable issues found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…n succeeds - Both settings endpoints read 'before' and save under select_for_update, so of two concurrent changes the second audits against the first's result. - The capability matrix creates a real connector and asserts 201 instead of treating any non-403 as allowed. Addresses Greptile review on #2344. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n succeeds - Both settings endpoints read 'before' and save under select_for_update, so of two concurrent changes the second audits against the first's result. - The capability matrix creates a real connector and asserts 201 instead of treating any non-403 as allowed. Addresses Greptile review on #2344. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a8bdbd4 to
6958f22
Compare
…n succeeds - Both settings endpoints read 'before' and save under select_for_update, so of two concurrent changes the second audits against the first's result. - The capability matrix creates a real connector and asserts 201 instead of treating any non-403 as allowed. Addresses Greptile review on #2344. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6958f22 to
cb54e73
Compare
…n succeeds - Both settings endpoints read 'before' and save under select_for_update, so of two concurrent changes the second audits against the first's result. - The capability matrix creates a real connector and asserts 201 instead of treating any non-403 as allowed. Addresses Greptile review on #2344. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cb54e73 to
e32449f
Compare
|
@greptileai Re-review (head rebased onto the current stack; no code change since your last review) |
…n succeeds - Both settings endpoints read 'before' and save under select_for_update, so of two concurrent changes the second audits against the first's result. - The capability matrix creates a real connector and asserts 201 instead of treating any non-403 as allowed. Addresses Greptile review on #2344. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e32449f to
b938b9a
Compare
|
@greptileai Re-review (rebased onto #2342's latest head; no change to this PR's own diff) |
Deepak-Kesavan
left a comment
Deepak-Kesavan
left a comment
There was a problem hiding this comment.
Automated review pass (Claude, run for @Deepak-Kesavan) on this PR's own layer (feat/UN-4398-platform-audit...fac6e43d1; its own diff is identical to b938b9af2). Deduplicated against the self-review rounds and Greptile. These are starting points: closing any of them is a legitimate outcome, and nothing here blocks the merge.
Legacy sync: clean. The two legacy flags are the storage for create_llm_adapters and create_connectors, and both endpoints write through _save_restrictions under the row lock, so the two can't drift. A v0 client sees the same gate, the same 400s and the same body. The migration's defaults plus SET DEFAULT keep existing orgs and old images on today's behaviour, and 0008 is still free on main. PATCH is admin-only with no path to roles, and the three part-2 capabilities can't be opened, which matches the "Any member" rulings.
Findings 1 and 3 are inline. One more doesn't map to a line in this diff:
2. [Medium] Cloud's Agentic Studio creates adapters without the capability check. In unstract-cloud (main @ 83a9cb6f), pluggable_apps/agentic_studio_v1/views/connectors.py:80 (connectors_list, gated only by IsOrganizationMember) maps type to LLM, X2TEXT, EMBEDDING or VECTOR_DB and calls AdapterInstance.objects.create at :153. connector_detail also accepts a type change (:232). So a non-admin can create an embedding adapter under create_other_adapters=admins_only. The LLM half already bypasses the legacy flag; this PR is where the guarantee gets published. Suggest calling enforce_capability there, perhaps in the cloud companion, or noting it as a known gap in CAPABILITIES.md.
One more item for the Frontend v1 Permissions tab (P3-T12): three server labels (create_other_adapters, manage_api_keys, manage_members) differ from Figma frame P12. The label also appears in the 403 text and in the audit target_name, so aligning SPECS with the P12 copy would keep the toasts and the audit log consistent with the UI.
Deepak-Kesavan
left a comment
Deepak-Kesavan
left a comment
There was a problem hiding this comment.
🤖 Automated re-review (head a61aaff81)
Automated pass (Claude, run for @Deepak-Kesavan). These are starting points: closing any of them is a legitimate outcome, and nothing here blocks the merge.
Since the last review, this PR's own diff has one new commit, 244091526; the rest came in through #2342 merges. No new defects in it.
| Earlier finding | Status |
|---|---|
1. Adapter capability trusted the client's adapter_type |
Resolved in 244091526, verified (reply on the thread) |
| 2. Cloud Agentic Studio creates adapters without the capability check | Still open. unstract-cloud main @ d0e478bf: pluggable_apps/agentic_studio_v1/views/connectors.py:79 (IsOrganizationMember only) still calls AdapterInstance.objects.create at :153, and connector_detail still accepts a type change. #1868 doesn't touch it. Calling enforce_capability there, or naming it as a known gap in CAPABILITIES.md, would close it. |
3. No configurable flag |
Resolved in 244091526, verified (reply on the thread) |
SonarCloud: quality gate passed, no security hotspots, and one open issue on this PR's own lines (inline below).
Deepak-Kesavan
left a comment
Deepak-Kesavan
left a comment
There was a problem hiding this comment.
Approving at becc640cd. Both inline findings are fixed and verified (244091526: registry type check, configurable), and Sonar S7519 is fixed in 34c4740e9 (0 open issues). The cloud Agentic Studio adapter path noted in the review body lives in unstract-cloud and does not block this PR.
becc640 to
30555ff
Compare
|
@greptileai review |
30555ff to
37d2d2f
Compare
|
@greptileai review |
37d2d2f to
52cad45
Compare
|
Unstract test resultsPer-group results
Critical paths
|
|
@greptileai review |
What
PR-1 of UN-4399 (Frontend v1 PB-T04): organisation capability permissions. An org admin chooses, per capability, whether any member or only admins may use it.
restrict_*boolean onOrganization.restrict_llm_adapter_creationandrestrict_connector_creation, are the storage for the first two capabilities.account_v2/0008), with defaults that match today's behaviour.GET/PATCH /api/v1/unstract/{org}/organization/capabilities/.allowed_for_me. Any member may call it.organization/settingsis unchanged and stays in step by construction, because both endpoints read and write the same fields.capability.changedevent through the PB-T01 recorder, whichever endpoint made the change.IsOwner, so withany_membernothing changes, and withadmins_onlyowners who are not admins are refused.backend/tenant_account_v2/CAPABILITIES.md(endpoint, payloads, errors, TypeScript interfaces, whatallowed_for_memeans) plus@extend_schema, following the PB convention.Stacked on #2342 (PB-T01). This PR's base is that branch, so the diff shows only PB-T04.
Why
How
tenant_account_v2/capabilities.pyholds the registry:Capability,CapabilitySetting, a spec per capability,is_allowed,enforce_capabilityand theRequiresCapabilityDRF permission.OrganizationMemberService.is_user_organization_admin.Decide-and-flag choices (tracker
decisions/D-PB-T04-*; UN-4399 comment 47083)any_member. Until PR-2 lands, PATCH refusesany_memberformanage_api_keys,manage_membersandchange_platform_defaults. Their endpoints are still admin-only, so storing that value would makeallowed_for_memisleading.capabilities.pyand is a one-place swap.capability.changed, the key PB-T01 already registered. The legacyorganization/settingsPATCH now recordscapability.changedinstead ofplatform_setting.changed. PB-T01's README and its test are updated in this branch to match.adapter/default_triadstays open to members, as in the dossier's default.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
delete_workflows_and_deploymentstoadmins_only.organization/settingsPATCH now recordscapability.changedin the audit log, notplatform_setting.changed. That only matters if UN-4398 [FEAT] Platform audit log: event store and admin read API #2342 is merged first.Database Migrations
account_v2/0008_organization_capability_settingsadds five boolean columns toorganization, each with a default.Env Config
Relevant Docs
backend/tenant_account_v2/CAPABILITIES.mdRelated Issues or PRs
Dependencies Versions
Notes on Testing
backend/tenant_account_v2/test_capabilities.pycovers:allowed_for_mefor admin, member and service account;makemigrations --checkis clean.unit-backendandintegration-backendrig tiers were run locally, because CI skips draft PRs. Results are in a comment below.Screenshots
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code