feat(plugin): persist tenant-scoped installation authority [superseded by #169] - #156
feat(plugin): persist tenant-scoped installation authority [superseded by #169]#156seonghobae wants to merge 31 commits into
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough플러그인 설치 기록을 PostgreSQL에 저장하는 스키마와 저장소를 추가했습니다. 입력 및 저장 행 검증, 충돌 replay, workspace·설치자 사용자 범위 조회, 활성 설치 철회를 구현했습니다. 애플리케이션 계약, 테스트, 연구 문서와 변경 로그를 갱신했습니다. Changes플러그인 설치 영속성
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
apps/integration-service/migrations/0001_plugin_installation_record.sql (1)
10-10: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
granted_capabilities원소 수준 제약을 추가하면 좋습니다.현재 제약은 배열 길이만 제한합니다. 배열 원소는 NULL, 빈 문자열, 과도한 길이를 가질 수 있습니다. 저장소는 읽기 시점에 이런 행을 거부하므로, 데이터는 저장되지만 조회가 실패합니다. 쓰기 시점에 실패하도록 제약을 추가하십시오.
♻️ 제안 변경
CHECK (cardinality(granted_capabilities) BETWEEN 0 AND 32), + CHECK (array_position(granted_capabilities, NULL) IS NULL), + CHECK ( + NOT EXISTS ( + SELECT 1 + FROM unnest(granted_capabilities) AS capability_name + WHERE char_length(capability_name) NOT BETWEEN 1 AND 256 + ) + ),주의: PostgreSQL의 테이블 CHECK 제약은 서브쿼리를 허용하지 않습니다. 서브쿼리 대신
IMMUTABLE함수 또는 도메인 타입을 사용하십시오.Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/integration-service/migrations/0001_plugin_installation_record.sql` at line 10, Update the granted_capabilities constraint in the migration to validate every array element at write time: disallow NULL or empty strings and enforce the maximum element length expected by the reader. Implement this without a CHECK subquery, using an IMMUTABLE helper function or domain type, while preserving the existing array-level constraints.apps/integration-service/src/plugin-installation-repository.ts (1)
71-172: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win검증 헬퍼 6쌍이 오류 팩토리만 다릅니다. 매개변수화하십시오.
inputUuid/storedUuid,boundedInputText/boundedStoredText,inputDigest/storedDigest,inputCapabilities/storedCapabilities는 로직이 동일합니다. 차이는invalidInput과invalidEvidence뿐입니다.parseInstant가 이미 사용하는 방식대로invalid: () => never를 인자로 받으면 중복이 사라집니다. 한쪽만 수정되는 위험도 줄어듭니다.♻️ 제안 변경 예시
-function inputUuid(value: unknown): string { - if (typeof value !== 'string' || !UUID_V4_PATTERN.test(value)) { - return invalidInput(); - } - return value.toLowerCase(); -} - -function storedUuid(value: unknown): string { - if (typeof value !== 'string' || !UUID_V4_PATTERN.test(value)) { - return invalidEvidence(); - } - return value.toLowerCase(); -} +/** Validates one UUIDv4 identifier and normalizes it to lowercase. */ +function parseUuid(value: unknown, invalid: () => never): string { + if (typeof value !== 'string' || !UUID_V4_PATTERN.test(value)) { + return invalid(); + } + return value.toLowerCase(); +}
inputUuid/storedUuid는 얇은 래퍼로 유지할 수 있습니다. 동일한 방식을 나머지 3쌍에도 적용하십시오.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/integration-service/src/plugin-installation-repository.ts` around lines 71 - 172, Parameterize the duplicated validation helpers using an invalid: () => never callback, following parseInstant: consolidate inputUuid/storedUuid, boundedInputText/boundedStoredText, inputDigest/storedDigest, and inputCapabilities/storedCapabilities into shared helpers, while keeping thin wrappers that pass invalidInput or invalidEvidence as appropriate.apps/integration-service/src/plugin-installation-tenant-lookup.test.ts (1)
22-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
workspaceId를 필수 매개변수로 선언하십시오.
PluginInstallationStore.findById는workspaceId: string을 요구합니다. 테스트 더블은workspaceId?: string으로 선언합니다. TypeScript는 이 구현을 허용하지만, 계약이 느슨해집니다. 필수로 선언하면 애플리케이션이 인자를 빠뜨리는 회귀가 컴파일 시점에 드러납니다.♻️ 제안 변경
async findById( installationId: string, - workspaceId?: string, + workspaceId: string, ): Promise<PluginInstallationRecord | undefined> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/integration-service/src/plugin-installation-tenant-lookup.test.ts` around lines 22 - 28, Update the test double’s findById method to declare workspaceId as a required string parameter, matching the PluginInstallationStore contract, while preserving its existing lookupArguments recording and undefined return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/integration-service/src/plugin-installation-migration.test.ts`:
- Around line 5-9: Update the MIGRATION_PATH definition to resolve the
migrations directory from the test module’s location instead of process.cwd(),
preserving the existing 0001_plugin_installation_record.sql target so the test
works regardless of the invocation directory.
In `@apps/integration-service/src/plugin-installation-repository.test.ts`:
- Around line 151-179: Replace mock-interaction and SQL-string assertions with
realistic domain-result and failure-behavior tests. In
apps/integration-service/src/plugin-installation-repository.test.ts:151-179, add
coverage for cross-workspace conflicts, workspace-mismatched findById returning
undefined, and revoked or missing revokeActive paths. In
apps/integration-service/src/plugin-installation-migration.test.ts:12-40, use
PGlite or temporary PostgreSQL to verify rejection of invalid status/revoked_at
combinations, invalid digest lengths, and 33 capabilities. In
apps/integration-service/src/plugin-installation-tenant-lookup.test.ts:37-51,
assert getInstallation returns no record for another workspace and the expected
record for a matching workspace.
- Around line 62-64: Replace the repositoryModule dynamic import helper with a
static import of the repository class from ./plugin-installation-repository. In
the affected tests, remove repositoryModule() calls, typeof Store assertions,
and constructor type casts, then instantiate and use the imported class directly
so its signatures are checked at compile time.
In `@apps/integration-service/src/plugin-installation-repository.ts`:
- Around line 50-61: Add explanatory module-level docstrings to
PluginInstallationRow, invalidInput, invalidEvidence, oneOrUndefined,
validateCreate, validateRevocation, parseRow, and exactCandidate. Document each
declaration’s contract, including the fail-closed behavior of parseRow and
exactCandidate, and explicitly explain that exactCandidate intentionally does
not compare installedAt so the original durable timestamp is preserved.
- Around line 300-314: Update the insert/reconciliation flow around
oneOrUndefined(inserted.rows) to retry the conflicting-row SELECT when
concurrent insertion causes no row to be visible in the current snapshot. Bound
retries with MAXIMUM_REPLAY_ATTEMPTS, add a short delay between attempts, and
only return invalidEvidence() after the retry limit is exhausted.
In `@docs/research/2026-08-10-plugin-installation-authority-standards.md`:
- Around line 29-35: Update the four bibliography entries in the references
section to include explicit publication-status labels while preserving their APA
7 formatting: identify RFC 9562 as a published Standards Track RFC, NIST SP
800-53 Rev. 5 as a final publication, and both PostgreSQL 18 documentation
entries as published release documentation; do not label any of them as drafts
or preprints.
- Line 25: Update the PR `#156` evidence statement in the documentation to match
the tests actually provided: either add an integration test using PostgreSQL or
PGlite that persists data across a process restart and verifies replay, or
narrow the wording to the existing durable-row lookup behavior during conflicts.
Ensure the accepted-evidence document reflects only tests that have actually
passed.
---
Nitpick comments:
In `@apps/integration-service/migrations/0001_plugin_installation_record.sql`:
- Line 10: Update the granted_capabilities constraint in the migration to
validate every array element at write time: disallow NULL or empty strings and
enforce the maximum element length expected by the reader. Implement this
without a CHECK subquery, using an IMMUTABLE helper function or domain type,
while preserving the existing array-level constraints.
In `@apps/integration-service/src/plugin-installation-repository.ts`:
- Around line 71-172: Parameterize the duplicated validation helpers using an
invalid: () => never callback, following parseInstant: consolidate
inputUuid/storedUuid, boundedInputText/boundedStoredText,
inputDigest/storedDigest, and inputCapabilities/storedCapabilities into shared
helpers, while keeping thin wrappers that pass invalidInput or invalidEvidence
as appropriate.
In `@apps/integration-service/src/plugin-installation-tenant-lookup.test.ts`:
- Around line 22-28: Update the test double’s findById method to declare
workspaceId as a required string parameter, matching the PluginInstallationStore
contract, while preserving its existing lookupArguments recording and undefined
return behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 491aa431-77d6-4df8-9f12-0e6c89199b21
📒 Files selected for processing (7)
apps/integration-service/migrations/0001_plugin_installation_record.sqlapps/integration-service/src/plugin-installation-migration.test.tsapps/integration-service/src/plugin-installation-repository.test.tsapps/integration-service/src/plugin-installation-repository.tsapps/integration-service/src/plugin-installation-tenant-lookup.test.tsapps/integration-service/src/plugin-installation.tsdocs/research/2026-08-10-plugin-installation-authority-standards.md
|
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/integration-service/migrations/0001_plugin_installation_record.sql (1)
17-34: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win영속 테이블에서 UUIDv4 및 정렬된 고유 capability 계약을 강제하십시오.
Line 18-20은 모든 PostgreSQL UUID 버전을 허용합니다. Line 24와 Line 34는 capability 항목 길이만 검사합니다. 따라서 직접 SQL 쓰기 또는 향후 backfill이 UUIDv4가 아닌 식별자, 중복 capability, 정렬되지 않은 capability를 저장할 수 있습니다.
UUIDv4 버전과 variant를 검사하는
CHECK제약을 추가하십시오.capability_array_is_valid도inputCapabilities와 같은 정렬 기준으로 고유성과 정렬 순서를 검사하도록 확장하십시오. 이 제약을 실제 PostgreSQL 삽입 테스트로 검증하십시오.As per coding guidelines: "Internal identifiers must be opaque UUIDv4 strings; numeric provider identifiers must never become internal primary keys."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/integration-service/migrations/0001_plugin_installation_record.sql` around lines 17 - 34, Update the plugin_installation_record UUID constraints to require UUIDv4 version and RFC 4122 variant for installation_id, workspace_id, and installed_by_user_id. Extend capability_array_is_valid to enforce the same inputCapabilities ordering rule while rejecting duplicate or unsorted entries, preserving existing length and null checks. Add PostgreSQL insertion tests covering invalid UUIDs, duplicate capabilities, and unsorted capabilities, plus valid UUIDv4 and sorted unique capability cases.Source: Coding guidelines
🧹 Nitpick comments (2)
apps/integration-service/src/plugin-installation.ts (1)
41-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win공개 revocation 계약을 문서화하십시오.
installedByUserId는 설치 권한 범위를 결정합니다.RevokePluginInstallation에 이 값이 인증된 context에서 와야 하며, 조회와 철회에서 설치 소유자를 제한한다는 JSDoc을 추가하십시오.As per coding guidelines: "Production declarations must include explanatory docstrings sufficient for a new contributor to understand the contract without reconstructing the implementation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/integration-service/src/plugin-installation.ts` around lines 41 - 45, Add JSDoc to the public RevokePluginInstallation interface documenting that installedByUserId must come from the authenticated context and must constrain both installation lookup and revocation to the installation owner.Source: Coding guidelines
apps/integration-service/src/plugin-installation-migration.test.ts (1)
117-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win동적으로 조합한 SQL 값을 바인딩하거나 SQL 내부 상수로 바꾸십시오.
tooManyCapabilities,oversizedCapability,installationId,capabilities를 SQL 템플릿에 직접 보간합니다. 현재 fixture는 통제되지만 SQL 구조가 동적으로 조합됩니다.psql변수 바인딩을 추가하거나repeat및generate_series같은 고정 SQL 표현식으로 fixture를 구성하십시오.As per coding guidelines,
Keep SQL structure static and parameterize dynamic values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/integration-service/src/plugin-installation-migration.test.ts` around lines 117 - 177, 정적으로 유지해야 하는 SQL 템플릿에 tooManyCapabilities, oversizedCapability, installationId, capabilities를 직접 보간하지 마십시오. 해당 값은 psql 변수 바인딩으로 전달하거나 repeat 및 generate_series 같은 고정 SQL 표현식으로 생성하도록 expectSqlFailure 테스트를 수정하고, SQL 구조는 항상 상수로 유지하십시오.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/integration-service/src/plugin-installation-migration.test.ts`:
- Around line 73-77: Update expectSqlFailure to accept the expected SQLSTATE or
constraint name for each fixture, then assert that executeSql’s stderr contains
that identifier in addition to requiring a nonzero status. Pass the appropriate
expected value from every call site so syntax errors and PostgreSQL connection
failures cannot be treated as constraint violations.
- Around line 13-15: Update the DATABASE_URL initialization in
plugin-installation-migration.test.ts to use only INTEGRATION_DATABASE_URL,
removing the PLANNING_DATABASE_URL fallback. Keep describeWithPostgres
conditional on the dedicated integration test URL so the destructive migration
test runs only against the disposable Integration database.
---
Outside diff comments:
In `@apps/integration-service/migrations/0001_plugin_installation_record.sql`:
- Around line 17-34: Update the plugin_installation_record UUID constraints to
require UUIDv4 version and RFC 4122 variant for installation_id, workspace_id,
and installed_by_user_id. Extend capability_array_is_valid to enforce the same
inputCapabilities ordering rule while rejecting duplicate or unsorted entries,
preserving existing length and null checks. Add PostgreSQL insertion tests
covering invalid UUIDs, duplicate capabilities, and unsorted capabilities, plus
valid UUIDv4 and sorted unique capability cases.
---
Nitpick comments:
In `@apps/integration-service/src/plugin-installation-migration.test.ts`:
- Around line 117-177: 정적으로 유지해야 하는 SQL 템플릿에 tooManyCapabilities,
oversizedCapability, installationId, capabilities를 직접 보간하지 마십시오. 해당 값은 psql 변수
바인딩으로 전달하거나 repeat 및 generate_series 같은 고정 SQL 표현식으로 생성하도록 expectSqlFailure 테스트를
수정하고, SQL 구조는 항상 상수로 유지하십시오.
In `@apps/integration-service/src/plugin-installation.ts`:
- Around line 41-45: Add JSDoc to the public RevokePluginInstallation interface
documenting that installedByUserId must come from the authenticated context and
must constrain both installation lookup and revocation to the installation
owner.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e319b75-27ec-4968-be63-f78e2e3c7852
📒 Files selected for processing (9)
CHANGELOG.mdapps/integration-service/migrations/0001_plugin_installation_record.sqlapps/integration-service/src/plugin-installation-migration.test.tsapps/integration-service/src/plugin-installation-repository.test.tsapps/integration-service/src/plugin-installation-repository.tsapps/integration-service/src/plugin-installation-tenant-lookup.test.tsapps/integration-service/src/plugin-installation-user-authority.test.tsapps/integration-service/src/plugin-installation.tsdocs/research/2026-08-10-plugin-installation-authority-standards.md
seonghobae
changed the title
1 participant
Buyer/security outcome
Advance #130 from protected host-owned installation/grant authority to durable restart-safe installation evidence without introducing plugin secret storage or outbound network authority.
Test-first sequence
The original RED contracts required descriptive service-owned persistence, opaque UUIDv4 installation/workspace/actor identity, exact manifest SHA-256, normalized explicit grants, lifecycle timestamps, no secret/token/credential plaintext columns, fixed parameterized SQL, bounded replay and atomic revocation.
Manual security review then found two widening defects:
installedByUserId, so another authenticated member of the same workspace could potentially observe or revoke an installation created by a different user.New RED regressions require both trusted workspace and requesting-user authority to reach the persistence boundary. The application now passes
(installationId, workspaceId, installedByUserId), verifies the returned record against both authorities, and includes the user in revocation input. PostgreSQLSELECT, conflict-replay lookup, active->revokedUPDATE, and revoked replay queries all include workspace and installing-user predicates.Implementation
PostgresPluginInstallationStoreuses only fixed parameterized SQL.createIfAbsentusesINSERT ... ON CONFLICT DO NOTHINGand reads a replay winner only inside the exact workspace+user scope.findByIdvalidates all three UUIDs before SQL and refuses mismatched durable evidence.revokeActiveperforms one conditional installation+workspace+user+active transition and replays only already-revoked evidence inside the same authority scope.Scope
This remains only the durable installation-authority persistence foundation. Plugin secret/KMS lifecycle, outbound HTTPS delivery/SSRF controls, retry/dead-letter evidence, operator/public APIs and runtime composition remain subsequent #130 slices. No plaintext provider/plugin credential is stored or returned here.
Acceptance
The current exact head must pass package tests/typecheck/build, configured exact coverage/docstring gates, CI, AppGuardrail, SAST Semgrep, Security Scan, Commercial Readiness, current review findings and live-base mergeability before merge. Active work is not protected-main truth until integration.
Refs #130; follows protected-main #151.
Summary by CodeRabbit
새 기능
보안
문서