feat: MVP1 RBAC 권한 모델 적용 - #65
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38bd6cb3e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| auth_state = row[0] if isinstance(row, tuple) else row | ||
| result = stronger_resource_auth_state(result, auth_state) |
There was a problem hiding this comment.
The project depends on SQLAlchemy >=2.0, where single-column ORM queries like db.query(TeamWorkflowPermission.auth_state).all() return Row objects rather than plain tuples. Because this only indexes tuples, each row is passed wholesale into stronger_resource_auth_state, normalizes to none, and every team/direct grant is ignored; non-organization-manager users remain denied for workflows and LLM credentials even after being granted access.
Useful? React with 👍 / 👎.
| .filter( | ||
| UserWorkflowPermission.user_id == user_uuid, | ||
| UserWorkflowPermission.workflow_id == workflow.id, | ||
| UserWorkflowPermission.grantee_organization_id == organization_uuid, | ||
| ) |
There was a problem hiding this comment.
Direct workflow grants are evaluated only by user/resource/org, unlike team grants which require an active team membership. If an org manager grants a user direct access and later removes that user's last team membership, TeamService.remove_membership deletes only the membership row, so this stale UserWorkflowPermission row still qualifies and the removed user keeps access; the UserLLMPermission path below has the same issue. Join/check current active membership here or delete direct grants on removal.
Useful? React with 👍 / 👎.
| workflow = db.query(Workflow).filter(Workflow.id == workflow_id).first() | ||
| if not workflow: | ||
| raise HTTPException(status_code=404, detail="Workflow not found") | ||
| ensure_workflow_permission(db, current_user, workflow_id, "read") |
There was a problem hiding this comment.
When a caller lacks read access to this workflow, ensure_workflow_permission raises an HTTPException(403), but it is inside the broad try and the except Exception below converts it into a 500 response. This makes authorization failures on GET /workflows/{workflow_id}/stats look like server errors while the other workflow read endpoints correctly return 403/404; add an except HTTPException: raise before the catch-all or move the permission check outside the try.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f9b64a221
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| target_workflow_id = workflow_id | ||
| if app_id and not target_workflow_id: | ||
| app = db.query(App).filter(App.id == app_id).first() |
There was a problem hiding this comment.
When both app_id and workflow_id are supplied, this check authorizes only the supplied workflow_id because the app lookup is skipped, while DeploymentService.list_deployments still returns deployments for app_id when it is present. A caller with read access to any workflow can therefore pass that workflow_id together with another app's id and read that app's deployment graph snapshots; resolve the app's workflow and reject mismatches or ignore app_id in this case.
Useful? React with 👍 / 👎.
| if not has_organization_manager_permission(db, user_id, organization_id): | ||
| raise PermissionError("Credential creation requires organization manager") |
There was a problem hiding this comment.
For a user without an existing default membership, the fallback just above creates a default org via ensure_user_default_organization, but gateway sessions are configured with autoflush=False, so the new org/team/membership are still pending when this permission query runs. The manager lookup will not see the newly-created org and first credential registration through the fallback incorrectly returns 403; flush in the helper before returning or before this check.
Useful? React with 👍 / 👎.
HyungminYoon1
left a comment
HyungminYoon1
left a comment
There was a problem hiding this comment.
검토 결과, RBAC foundation 방향은 전반적으로 맞습니다. dev 기준 merge result도 conflict 없이 생성되고, team/user direct permission, workflow/deployment/LLM credential 권한 적용은 #56 요구와 대체로 일치합니다.
다만 병합 전 수정이 필요한 항목이 있습니다.
- High: LLM pricing 관리 endpoint가 인증/권한 없이 열려 있습니다.
apps/gateway/api/v1/endpoints/llm.py의 POST /api/v1/llm/models/sync-pricing, PUT /api/v1/llm/models/{model_id}/pricing가 get_current_user나 system admin 권한 check 없이 DB state를 변경합니다. 그런데 docs/api/llm-credentials.md는 두 endpoint를 system admin 권한으로 명시하고 있어 문서와 구현도 충돌합니다.
병합 전 admin/system permission dependency와 deny test를 추가해 주세요.
- Medium:
/api/v1/llm/providers문서와 구현이 불일치합니다.
docs/api/llm-credentials.md는 GET /api/v1/llm/providers를 authenticated로 명시하지만, route 구현에는 current_user dependency가 없습니다. provider/model catalog를 공개로 둘 정책이면 문서를 바꾸고, authenticated가 맞으면 route에 dependency를 추가해야 합니다.
- Low/follow-up: permission denied audit metadata가 최소 수준입니다.
apps/gateway/auth/permissions.py의 record_permission_denied는 permission_action, effective_auth_state만 남깁니다. #56 acceptance에는 충분할 수 있지만, 이후 #59 trace/audit 요구와 합칠 때는 request path/method/request_id/policy reason 같은 정보를 공통 helper에 포함하는 편이 좋겠습니다.
참고: PR branch tree만 보면 audit enum/filter가 빠진 것처럼 보일 수 있지만, origin/dev merge result 기준으로는 audit_log.py의 values_callable=enum_values와 audit log server-side filter가 보존됩니다. 이 둘은 blocker로 보지 않았습니다.
검토 방식: gh pr view, git fetch, git merge-tree origin/dev origin/pr/65, merge result tree의 주요 파일 diff/line inspection. 전체 테스트는 실행하지 않았습니다.
HyungminYoon1
left a comment
HyungminYoon1
left a comment
There was a problem hiding this comment.
재검토 결과, 이전에 요청한 blocking 항목은 해결된 것으로 확인했습니다.
- LLM pricing 관리 endpoint에 인증과
system_admincheck가 추가되었습니다. /api/v1/llm/providers도 문서와 맞게 authenticated route가 되었습니다.- 관련 deny test와 deployment list 권한 보강 test가 추가되었습니다.
- merge result 기준 audit enum mapping과 audit log server-side filter도 보존됩니다.
남은 사항은 TraceAccessService.is_system_admin() provider wiring이 기본 deny-all이라는 운영상 follow-up입니다. 보안상 열린 상태는 아니므로 이 PR의 merge blocker로 보지 않습니다.
2 participants
변경 사항
PermissionService를 추가했습니다.Row반환을 권한 grant 강도 비교 전에 unpack하도록 수정했습니다.Row객체 때문에none으로 정규화되어 무시되는 문제를 막았습니다.GET /workflows/{workflow_id}/stats에서 read 권한이 없을 때HTTPException(403)이 catch-all에 잡혀 500으로 바뀌지 않게 했습니다.문서 변경 사항:
docs/api/organization-rbac.mddocs/api/llm-credentials.mddocs/api/deployments.mddocs/implementation-plan/mvp-1-development-issue-plan.md관련 이슈
Closes #56
변경 유형
테스트
검증한 명령:
apps/gateway/.venv/bin/python -m compileall apps/shared apps/gateway apps/workflow_engineapps/gateway/.venv/bin/python -m compileall apps/gateway/api/v1/endpoints/workflow.py apps/gateway/tests/api/test_workflow_stats_permissions.pyapps/gateway/.venv/bin/python -m pytest apps/shared/tests/services/test_permissions.py apps/gateway/tests/services/test_llm_service_permissions.py apps/gateway/tests/services/test_team_service_permissions.py apps/gateway/tests/api/test_permission_helpers.py apps/gateway/tests/api/test_permission_denied_audit.py -qapps/gateway/.venv/bin/python -m pytest apps/gateway/tests/api/test_workflow_stats_permissions.py -qapps/workflow_engine/.venv/bin/python -m pytest apps/workflow_engine/tests/nodes/test_llm_node_runtime.py apps/shared/tests/services/test_tracing_access.py -q스크린샷 (UI 변경 시)
UI 변경 없음