refactor: policyId 紐づけ方式への移行対応 (GeonicDB #757)#82
Merged
Conversation
API キー・OAuth クライアントの認可モデルを policyId 紐づけ方式に移行。 - API キー: --scopes, --permissions, --entity-types を削除し --policy を追加 - OAuth クライアント: --scopes を削除し --policy を追加 - OAuth クライアント: clientName を name にリネーム - helpers.ts: SCOPES_HELP_NOTES, API_KEY_SCOPES_HELP_NOTES, VALID_PERMISSIONS, parsePermissions を削除 Closes #80, closes #81
- me api-keys create フラグ一覧: --scopes/--permissions/--entity-types を --policy に置換 - admin api-keys フラグ説明: 同上 - me oauth-clients create フラグ説明を追加 - Valid Scopes / Entity Type Restrictions セクションを Authorization Model セクションに統合 - CHANGELOG 更新
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughスコープ/権限ベースの認可モデルを廃止し、APIキーとOAuthクライアントの認可をXACMLポリシーID（ Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 分 Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/commands/admin/oauth-clients.ts (1)
55-83:⚠️ Potential issue | 🟠 Major
admin oauth-clients createの breaking change が CLI に出ていません。ここは JSON 例だけ
policyIdに更新されていますが、コマンド自体はまだ--policyを受け付けません。今回の変更説明ではadmin oauth-clientsも--policy <policyId>移行対象として案内されているので、このままだとドキュメント通りの実行が unknown option になります。実装を追加するか、案内を JSON payload の変更だけに絞ってください。差分案
const create = oauthClients .command("create [json]") .description( "Create a new OAuth client\n\n" + "JSON payload example:\n" + " {\n" + ' "name": "my-app",\n' + ' "policyId": "<policy-id>"\n' + " }", ) + .option("--name <name>", "Client name") + .option("--policy <policyId>", "Policy ID to attach") .action( withErrorHandler(async (json: unknown, _opts: unknown, cmd: Command) => { - const body = await parseJsonInput(json as string | undefined); + const opts = cmd.opts() as { name?: string; policy?: string }; + let body: unknown; + if (json) { + body = await parseJsonInput(json as string | undefined); + } else if (opts.name || opts.policy) { + const payload: Record<string, unknown> = {}; + if (opts.name) payload.name = opts.name; + if (opts.policy) payload.policyId = opts.policy; + body = payload; + } else { + body = await parseJsonInput(); + } const client = createClient(cmd); const format = getFormat(cmd); const response = await client.rawRequest("POST", "/admin/oauth-clients", { body, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/admin/oauth-clients.ts` around lines 55 - 83, The CLI example claims the create command accepts a --policy <policyId> option but the command defined by the create variable does not parse or forward that flag; add a commander option to the create command (e.g., .option("--policy <policyId>")) and update the action to accept opts (or read cmd.opts()) and, if provided, merge/override body.policyId with the flag value before calling client.rawRequest; reuse existing helpers (parseJsonInput, createClient, getFormat, outputResponse, printSuccess) and validate that you don't silently override an explicit policyId present in the JSON payload (either prefer the flag or error on conflict).
🧹 Nitpick comments (1)
tests/admin-api-keys.test.ts (1)
323-339: 削除した旧フラグの拒否も 1 本固定しておきたいです。
--policyの正系は確認できていますが、この PR の破壊的変更は--scopes/--permissions/--entity-typesを受け付けないことでもあります。今の suite だと古い option 定義が残っても見逃すので、unknown option を確認する回帰テストを 1 本だけでも残しておくと安全です。Also applies to: 431-449
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/admin-api-keys.test.ts` around lines 323 - 339, Add a regression test that verifies the CLI rejects the removed/deprecated flags (--scopes, --permissions, --entity-types) when creating API keys: in the tests/admin-api-keys.test.ts suite (alongside the existing "includes policyId when --policy flag is set" test and the related block around lines 431-449) call runCommand(makeProgram(), ["admin","api-keys","create","--scopes","foo"]) (and one for --permissions or --entity-types) and assert the command fails with an "unknown option" error (or that the process exits/non-zero and client.rawRequest is not called); use the same helpers (makeProgram, runCommand, client.rawRequest mocks) to locate and implement this single negative test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 11-14: The three Unreleased changelog entries and the Docs line
are missing the PR suffix; update each bullet (the lines starting with
"Breaking: API キー", "Breaking: OAuth クライアント", "Breaking: OAuth クライアントの
`clientName`", and "Docs: README") to append the PR number in the standard
format (e.g., " ( `#82` )" without extra spacing) so they match existing
CHANGELOG.md conventions and the other entries include the PR suffix.
In `@README.md`:
- Around line 198-199: README currently documents `me oauth-clients create`
flags but doesn't call out the breaking rename of JSON field `clientName` →
`name`; update the README to explicitly show the new JSON shape for both `me`
and `admin` examples (e.g. include a JSON snippet or inline note
`{"name":"..."}`), and mention the flag mapping (`--name` now corresponds to the
`name` field) so users know to migrate payloads and config entries from
`clientName` to `name`.
---
Outside diff comments:
In `@src/commands/admin/oauth-clients.ts`:
- Around line 55-83: The CLI example claims the create command accepts a
--policy <policyId> option but the command defined by the create variable does
not parse or forward that flag; add a commander option to the create command
(e.g., .option("--policy <policyId>")) and update the action to accept opts (or
read cmd.opts()) and, if provided, merge/override body.policyId with the flag
value before calling client.rawRequest; reuse existing helpers (parseJsonInput,
createClient, getFormat, outputResponse, printSuccess) and validate that you
don't silently override an explicit policyId present in the JSON payload (either
prefer the flag or error on conflict).
---
Nitpick comments:
In `@tests/admin-api-keys.test.ts`:
- Around line 323-339: Add a regression test that verifies the CLI rejects the
removed/deprecated flags (--scopes, --permissions, --entity-types) when creating
API keys: in the tests/admin-api-keys.test.ts suite (alongside the existing
"includes policyId when --policy flag is set" test and the related block around
lines 431-449) call runCommand(makeProgram(),
["admin","api-keys","create","--scopes","foo"]) (and one for --permissions or
--entity-types) and assert the command fails with an "unknown option" error (or
that the process exits/non-zero and client.rawRequest is not called); use the
same helpers (makeProgram, runCommand, client.rawRequest mocks) to locate and
implement this single negative test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18210052-5a7b-4978-a02d-f65eea67ae4e
📒 Files selected for processing (24)
CHANGELOG.mdREADME.mdsrc/commands/admin/api-keys.tssrc/commands/admin/oauth-clients.tssrc/commands/me-api-keys.tssrc/commands/me-oauth-clients.tssrc/helpers.tstests/admin-api-keys.test.tstests/admin-oauth-clients.test.tstests/admin-policies.test.tstests/admin-tenants.test.tstests/admin-users.test.tstests/auth.test.tstests/e2e/features/me-oauth-clients.featuretests/e2e/features/oauth-clients.featuretests/entities.test.tstests/health.test.tstests/helpers.test.tstests/me-api-keys.test.tstests/me-oauth-clients.test.tstests/setup-command-mocks.tstests/snapshots.test.tstests/temporal.test.tstests/types.test.ts
💤 Files with no reviewable changes (13)
- tests/admin-oauth-clients.test.ts
- tests/snapshots.test.ts
- tests/temporal.test.ts
- tests/admin-tenants.test.ts
- tests/auth.test.ts
- tests/admin-policies.test.ts
- tests/entities.test.ts
- tests/health.test.ts
- tests/types.test.ts
- tests/setup-command-mocks.ts
- src/helpers.ts
- tests/helpers.test.ts
- tests/admin-users.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/features/me-oauth-clients.feature (2)
23-29:--policyオプションのテストカバレッジが不足しています。以前は
--scopes read:entitiesが明示的にテストされていましたが、その代替となる--policy <policyId>オプションのテストシナリオがありません。新しい認可モデルの動作を保証するため、--policyオプションを使用したシナリオを追加することを検討してください。例:
Scenario: Create an OAuth client with --policy flag Given I am logged in When I run `geonic me oauth-clients create --name my-policy-bot --policy <testPolicyId>` Then the exit code should be 0 And the output should contain "OAuth client created." And stdout should be valid JSON And the JSON output should have key "clientId"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/me-oauth-clients.feature` around lines 23 - 29, Add a new Gherkin scenario mirroring the existing "Create an OAuth client with --name flag" but using the --policy flag: create a "Scenario: Create an OAuth client with --policy flag" that runs `geonic me oauth-clients create --name my-policy-bot --policy <testPolicyId>` (use a fixture or testPolicyId variable) and include the same assertions as the other scenario (exit code 0, output contains "OAuth client created.", stdout valid JSON, JSON has key "clientId") so the new policy-based flow is covered.
31-39:--saveフラグとの組み合わせテストも--policyなしのケースのみになっています。
--saveオプションと--policyオプションを組み合わせたシナリオ（認証情報の保存と同時にポリシーを指定するケース）のテストも追加すると、より堅牢なテストカバレッジになります。Also applies to: 57-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/features/me-oauth-clients.feature` around lines 31 - 39, Add an E2E scenario that tests creating an OAuth client with both --save and --policy flags together: mirror the existing "Create with --save stores credentials in config" scenario but run `geonic me oauth-clients create --name my-saved-bot --save --policy "<policy-name-or-json>"`, assert exit code 0, output contains "OAuth client created." and "Auto-reauth enabled", and verify the config contains "clientId", "clientSecret", "token", plus the specified policy is persisted (check output or config key representing the policy). Apply the same addition for the analogous scenario around the second instance noted in the comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/features/me-oauth-clients.feature`:
- Around line 23-29: Add a new Gherkin scenario mirroring the existing "Create
an OAuth client with --name flag" but using the --policy flag: create a
"Scenario: Create an OAuth client with --policy flag" that runs `geonic me
oauth-clients create --name my-policy-bot --policy <testPolicyId>` (use a
fixture or testPolicyId variable) and include the same assertions as the other
scenario (exit code 0, output contains "OAuth client created.", stdout valid
JSON, JSON has key "clientId") so the new policy-based flow is covered.
- Around line 31-39: Add an E2E scenario that tests creating an OAuth client
with both --save and --policy flags together: mirror the existing "Create with
--save stores credentials in config" scenario but run `geonic me oauth-clients
create --name my-saved-bot --save --policy "<policy-name-or-json>"`, assert exit
code 0, output contains "OAuth client created." and "Auto-reauth enabled", and
verify the config contains "clientId", "clientSecret", "token", plus the
specified policy is persisted (check output or config key representing the
policy). Apply the same addition for the analogous scenario around the second
instance noted in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21986dc7-342d-4897-9f52-5f19dfb57ba2
📒 Files selected for processing (3)
CHANGELOG.mdtests/e2e/features/me-oauth-clients.featuretests/e2e/features/oauth-clients.feature
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/features/oauth-clients.feature
geonicdb を #757 対応版に更新後、シナリオ間の DB クリーンアップで デフォルト XACML ポリシーとスーパー管理者ユーザーが削除され 'Access denied by policy' が全テストで発生していた。
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/support/hooks.ts`:
- Around line 48-52: The current cleanup in the hooks uses preserveCollections
(Set of "users","policies","tenant") and skips dropping them, which leaves their
documents between scenarios; change the logic in the hook that iterates
collections (using mongoClient, db.listCollections(), collections) so that
instead of skipping preserveCollections you call
db.collection(<name>).deleteMany({}) to clear documents for those names (or
restore from a known initial snapshot if available), and continue dropping
non-preserved collections as before; update the loop that checks
c.name.startsWith("system.") and preserveCollections.has(c.name) to perform
deleteMany on preserved names and drop() for others.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54cb2e81-4c6f-4125-b767-706e862b4818
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
tests/e2e/support/hooks.ts
super_admin はデータ API アクセス不可のため、E2E セットアップで テナント + tenant_admin ユーザーを作成し、データ操作テストは tenant_admin で実行するよう変更。 - hooks.ts: BeforeAll でテナント・tenant_admin を API 経由で作成 - world.ts: performLogin を tenant_admin に、performSuperAdminLogin を追加 - admin 系 feature: Given I am logged in as super admin に変更 - auth.feature: whoami の期待メールアドレスを tenant_admin に変更
- client.ts: サーバーが不正JWTに403を返すケースでもトークンリフレッシュを発動 - helpers.ts: テナント未割り当て403を認証エラーとして処理 - common.steps.ts: extractId で ruleId を tenantId より優先 - snapshots.feature: clone テストをサーバー仕様に合わせて簡素化 ローカル E2E: 130/130 pass
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GeonicDB #757 (認証情報とポリシーの分離 — policyId 紐づけ方式への移行) に対応する CLI 側の変更。
admin api-keys,me api-keys):--scopes,--permissions,--entity-typesオプションを削除し、--policy <policyId>オプションを追加admin oauth-clients,me oauth-clients):--scopesオプションとallowedScopesフィールドを削除し、--policy <policyId>オプションを追加。clientName→nameにリネームSCOPES_HELP_NOTES,API_KEY_SCOPES_HELP_NOTES,VALID_PERMISSIONS,parsePermissionsを削除Breaking Changes
--scopes,--permissions,--entity-types--policy <policyId>--scopes,allowedScopes--policy <policyId>clientNameフィールドnameフィールドTest plan
npm run lint— passnpm run typecheck— passnpm test— 628/628 passCloses #80, closes #81
Summary by CodeRabbit
破壊的変更
--scopes/--permissions/--entity-typesを廃止し、代わりに--policy <policyId>を導入clientNameをnameに変更ドキュメント