feat(authorization): hierarchical PDP, start-login helper, and PATs - #103
feat(authorization): hierarchical PDP, start-login helper, and PATs#103seonghobae wants to merge 9 commits into
Conversation
Issue software-unit ACL, menu ABAC/RBAC, SSO combination scopes, and org-path inheritance from Orgmetra assignment snapshots without copying employment truth. Add a Keyverse-owned start-login helper and hashed programmable application tokens. ADR-0008 remains the RP PEP boundary; ADR-0009 stays reserved for the unmerged LineageWeave claim profile. Tracks #2. Implements #102. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Warning Review limit reached
Next review available in: 11 minutes Limit details: You’ve used the included review currently available. 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?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (31)
📝 WalkthroughWalkthrough계층형 권한 평면, 로컬 IdP 기반 로그인 시작 헬퍼, 범위 제한 애플리케이션 토큰의 실행 로직과 테스트가 추가되었습니다. 관련 아키텍처, 온보딩, 운영, ADR 문서도 갱신되었습니다. Changes계층형 권한 평면
연합 로그인 시작 헬퍼
프로그래밍 가능한 애플리케이션 토큰
문서 및 설계 기록
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🔴 Critical · up to The PR adds new authorization, login, and token-management endpoints, but the current head still permits unauthenticated token administration in direct router deployments and can redirect users to an arbitrary issuer; it also ignores software-unit access constraints, lacks tenant isolation in authorization keys and matching, and can leave token state inconsistent after failures. These are concrete security and authorization-integrity risks, so merge should be blocked until fixed. Sequence Diagram(s)권한 결정 흐름sequenceDiagram
participant Orgmetra
participant Keyverse
participant RelyingParty
Orgmetra->>Keyverse: assignment snapshot 전달
Keyverse->>Keyverse: grant 및 조직 경로 결정
Keyverse-->>RelyingParty: authorization decision 반환
RelyingParty->>RelyingParty: 로컬 enforcement 수행
로그인 시작 흐름sequenceDiagram
participant Application
participant Keyverse
participant Keycloak
Application->>Keyverse: start-login 요청
Keyverse->>Keyverse: 로컬 IdP registry 조회
Keyverse-->>Application: authorization URL 반환
Application->>Keycloak: PKCE authorization URL로 리디렉션
애플리케이션 토큰 흐름sequenceDiagram
participant Operator
participant Keyverse
participant Application
Operator->>Keyverse: 토큰 발급 요청
Keyverse-->>Operator: plaintext token 1회 반환
Application->>Keyverse: 토큰 검증 요청
Keyverse-->>Application: allow 또는 deny 반환
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Cover omitted assignment snapshots, failed menu grant PUTs, and policy errors on token verify so statement and branch coverage stay at 100% on the authorization slice. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Hosted account-unification-tests failed at uv sync --locked because uv.lock still recorded coverage 7.15.2 and setuptools 83.0.0 after pyproject.toml moved to 7.15.4 and 84.0.0. Refresh the lock metadata only so CI can install the already-reviewed pins. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
@coderabbitai review |
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head44fb43428eab0075b9e5ee114a5ade56bb18eec2. -
Head SHA:
44fb43428eab0075b9e5ee114a5ade56bb18eec2 -
Workflow run: 32095234736
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (12 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (12 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (21 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (21 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (4 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (4 files)"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (12 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (12 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (21 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (21 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (4 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (4 files)"]
R3 --> V3["targeted test run"]
|
|
Please independently re-review PR 103 at exact current head 44fb434 against main. The current hosted checks are terminal-success except the known Strix failure; verify whether any current-head actionable changes remain, and do not self-approve or transfer predecessor coverage evidence. |
|
Security evidence note for exact head 44fb434: the authorization grant and combination routes are mounted with the shared operator_auth_dependency in services/account_unification/app/main.py, and auth.py documents this as a coarse operator-admin boundary. actor_identity_id is persisted grant metadata, not an untrusted end-user identity extracted from the bearer request. The Strix PoC models User A/User B changing grants with different body identities but does not establish two distinct authenticated principals at this operator-only boundary. Please independently validate this trust-boundary interpretation and re-run or review the exact head; keep the failure blocking until validated. |
|
Exact-head evidence update for 44fb434: local full pytest passed; Ruff, compileall, Interrogate 100%, branch coverage 3649 statements / 938 branches at 100%, uv build, and diff check passed. The remaining hosted failure is Strix run 32092025335/job 95576032571. Its report requests actor_identity_id ownership checks but also states the issue was already fixed, while the current routes are operator-admin gated by the shared operator_auth_dependency and actor_identity_id is grant/audit metadata. Keep the Strix failure blocking; independently validate this trust-boundary interpretation and do not transfer predecessor evidence. |
|
@opencode-agent please re-review exact current head |
|
@opencode-agent Please re-review exact current head 157b768. Addressed the current Strix authorization finding at the trust boundary: the authorization router now carries operator bearer authentication and privileged-path validation itself, so direct embedding cannot mount grant administration open; the application factory no longer supplies that router's only gate. Added a RED→GREEN direct-embedding regression (unauthenticated 401, configured operator bearer 200). Preserved the documented coarse operator-admin model: actor_identity_id remains grant metadata, not an end-user principal. Local exact-head verification passed: complete pytest, Ruff, Interrogate 100%, compileall, 100% statement/branch coverage (3,651/938), uv build, and diff check. Re-run Strix and validate the trust-boundary interpretation on this exact SHA. |
|
@opencode-agent Please independently review exact current head |
|
@opencode-agent @cwl-noema-review Please independently review exact current head |
|
@opencode-agent @cwl-noema-review Please independently review exact current head |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@opencode-agent @cwl-noema-review Please independently review exact current head |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head review request: please re-review commit |
|
Correction to the current-head request above: the exact full SHA is |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Follow-up review request for exact current head Validated and pushed a focused fix for the current-head Devin finding: application-token rotation now fails closed with HTTP 409 when the predecessor is revoked, already rotated, or expired, so retired credentials cannot be revived. Added a parametrized regression covering all three states and updated CHANGELOG.md. Local exact-head evidence before push:
Please re-review exact head |
|
Correction: the exact current head is |
|
Exact current head update: Added the required architecture, operations, ADR, and doctoring wording for the fixed rotation contract: only active, unexpired predecessors may rotate; revoked, rotated, and expired predecessors fail closed. This commit is documentation-only; |
|
@opencode-agent Please submit a fresh independent review bound only to exact current head |
2 participants
Summary
Buyer-facing Keyverse authorization slice on
main. It makes these six capabilities real as issuer-side contracts plus tests and docs, without restacking on open PRs #83, #100, or #101:kc_idp_hint, no metadata fetch)Tracks #2. Implements #102. Does not close #2 (cross-system SSO epic still includes live federation acceptance and remaining P5 work).
Boundary decisions
keyverse_subject,org_path, optionalassignment_record_id) and does not copy the Orgmetra tree.role/org/workspaceprofile on PR Add account-derived LineageWeave RP profile #100. Hierarchical names aregroup_company,legal_entity,business_unit,team,person, andorg_path.ADRs
Head
Current head:
44fb43428eab0075b9e5ee114a5ade56bb18eec2Hosted CI
Exact-head product CI on this SHA is green (run 32092028160):
account-unification-testsjob 95575987666 — successrealm-config-validatesjob 95575987605 — successcompose-config-validatesjob 95575987627 — successPredecessor
363b26cfailed inaccount-unification-testsjob 95575377973 (run 32091826557) at Install locked dependencies.uv sync --lockedrefuseduv.lockbecause it still recorded coverage 7.15.2 and setuptools 83.0.0 afterpyproject.tomlmoved to 7.15.4 and 84.0.0. This head only resynchronizes the lock to those already-reviewed pins.Operator / app next action
See
docs/authorization-onboarding.md:POST /federation/identity-providers:start-login, then adds PKCE locally and redirects tostart_login_url.POST /application-tokens, stored in the application secret manager, and presented only toPOST /application-tokens:verify.Evidence still required
This PR does not claim production federation, login, or downstream PEP acceptance. Those remain controlled runtime evidence after IdP apply, confidential secret placement, and RP token validation.
Safety
No password authenticator, no unverified-email merge, no portable-realm application clients, no RP-environment secrets, no restack of #83 / #100 / #101. Draft remains draft.
Summary by CodeRabbit