Conversation
dlokesh
left a comment
There was a problem hiding this comment.
Approving: the client side matches the contract (header only when set, profile default under the root override, validation against the list) and the server owns all enforcement. Two P2s inline that I would fix before release, plus:
- Must land after #15266 deploys (as noted). If #15266 picks up pagination (
showing/truncated) in review,tenant listshould surface those. - The vendored snapshot declares
SummationMachineAuth: [], but the live spec injectsagent:readfor GETs; harmless, the nightly reconciliation will rewrite it. - Nit:
SUMCLI_ORGis profile-agnostic, so switching--profileacross environments with it exported yields an unknown-org 401 that is hard to diagnose. Worth a line in the help text.
| envvar="SUMCLI_ORG", | ||
| help="Act in this organization instead of your home org. Internal multi-tenant " | ||
| "operators only; sum-api refuses it for anyone else. List the orgs you can target " | ||
| "with `sumcli tenants list`, or set a default with `sumcli tenants use <org_id>`.", |
There was a problem hiding this comment.
P2 The group is registered as tenant (app.add_typer(tenant.app, name="tenant")), so sumcli tenants list fails with "No such command". Same wording in the commands.py docstring, the config.py comment, and the PR description. The tenant resource's own help already says sumcli tenant use <org_id>.
There was a problem hiding this comment.
Fixed in 2421f17. The --org help now says sumcli tenant list / sumcli tenant use; same wording fixed in the config.py comment and the resolved_org docstring. Also added a line noting SUMCLI_ORG is profile-agnostic.
|
|
||
|
|
||
| def _list_targetable_orgs(ctx: typer.Context, profile: str | None) -> list[dict]: | ||
| with api_client(ctx, profile) as c: |
There was a problem hiding this comment.
P2 api_client attaches the current resolved org (profile default or --org) to this discovery call. Example: after tenant use org-y, if org-y is deactivated or the caller loses the role, the override check fails first, so tenant list and tenant use <other> both error and the user cannot recover unless they know about --clear. Discovery is about the caller's home identity; build the client for these two commands without resolved_org.
There was a problem hiding this comment.
Fixed in 2421f17. tenant list and tenant use now build the client with include_resolved_org=False, so discovery runs as the caller's home identity. A stale or now-invalid override no longer locks the user out; added a test asserting the discovery call carries no override header.
| "/v1/tenant/orgs": { | ||
| "get": { | ||
| "description": "List the organizations the caller may act in.\n\nAn internal multi-tenant operator sees every tenant; every other caller sees exactly their own organization. Pair a returned ``org_id`` with the ``x-summation-resolved-org`` request header to target that tenant on a later call. Minimal fields only.", | ||
| "operationId": "list_tenant_orgs", |
There was a problem hiding this comment.
Noting for the release ordering: this entry is ahead of prod on purpose and the commands 404 until #15266 deploys. Fine as agreed, but if #15266's response shape changes in review (pagination fields), this snapshot and tenant list need a follow-up.
There was a problem hiding this comment.
Acknowledged, ordering unchanged. tenant list now surfaces total/showing/truncated to match #15266's pagination, and requests limit=500 so tenant use validation stays whole. The snapshot stays ahead of prod by design; the commands 404 until #15266 deploys.
| return Config(**base) # type: ignore[arg-type] | ||
|
|
||
|
|
||
| def test_resolved_org_header_sent_when_set(monkeypatch) -> None: |
There was a problem hiding this comment.
P2 Header tests cover the transport, but nothing covers tenant use (validation against the list, --clear removing the key), the --org over profile-default precedence in commands.resolved_org, or resolved_org loading from the profile file. A couple of CliRunner tests with the API stubbed would cover the user-facing paths.
There was a problem hiding this comment.
Fixed in 2421f17. Added tests/test_tenant.py: tenant use validation and --clear, --org over profile-default precedence in commands.resolved_org, and resolved_org loading from the profile file.
- fetch org discovery without the resolved-org override so a stale override cannot lock a user out of tenant list/use - tenant list surfaces total/showing/truncated from the paginated endpoint - fix tenant(s) wording in --org help, config comment, resolved_org docstring; note SUMCLI_ORG is profile-agnostic - CliRunner tests for tenant use validation/--clear, resolved_org precedence, profile-file loading Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@doppelwerk review |
PR B (client) of the cross-tenant-sumcli proposal. Lets an internal multi-tenant operator act in another org from sumcli without a per-tenant login.
What
--org/SUMCLI_ORGsends thex-summation-resolved-orgheader on every request. sum-api honors it only for amulti_tenant_user(re-checked server-side); for anyone else it is a no-op or a 403, so sending it is always safe.sumcli tenant list— the orgs the caller may target (operator: the fleet; everyone else: their own org), fromGET /v1/tenant/orgs.sumcli tenant use <org_id>/--clear— persist (or clear) a default target org on the active profile, validated againsttenant list. Equivalent to passing--orgon every call.Depends on sum-api PR A (#15266)
This references
GET /v1/tenant/orgsand the resolved-org session override, which ship in #15266. Until that deploys, the commands 404 and the endpoint is absent from the live spec — so the vendored OpenAPI snapshot entry added here is ahead of prod on purpose (same gating as the durable-queue client PR). Release must land #15266 first; the nightly snapshot reconciliation will then match.Tests: header sent only when set; existing client/contract suites green (566 passed).