diff --git a/CLAUDE.md b/CLAUDE.md index a0da585..c8a0a5b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -32,6 +32,8 @@ Custom `SCAAccessService` follows SDK conventions: - Create client via `isp.FromISPAuth(ispAuth, "sca", ".", "", refreshCallback)` - Set `X-API-Version: 2.0` header on all requests - `httpClient` interface for DI/testing +- The service slug (`"sca"` / `"uar"`) is what `isp.FromISPAuth` resolves into the live host, and the retry/header tests overwrite `client.BaseURL` before issuing a request — so it is pinned separately, by asserting the constructed `BaseURL` against the fake-JWT tenant (`TestNewSCAAccessService_UsesSCAServiceSlug`, `TestNewAccessRequestService_UsesUARServiceSlug`) **before** any swap. Assert it before mutating `BaseURL`, never after +- Wire contracts are asserted on what is *sent*: the `mockHTTPClient` in both packages records `gotRoute`/`gotBody`/`gotParams` before dispatching. Assert the exact route and the full body contents — "non-nil body" lets a nil payload through, and a canned response tells you nothing about the request ## SCA Access API - **Base URL:** `https://{subdomain}.sca.{platform_domain}/api` diff --git a/docs/mutation-ledger.md b/docs/mutation-ledger.md index fd3e343..4332640 100644 --- a/docs/mutation-ledger.md +++ b/docs/mutation-ledger.md @@ -24,6 +24,20 @@ shifted by +13 for the non-interactive guard this branch inserts at are noted in the Mutation cell with `(was ...)`. **File:line is always the production site, never the test site.** +**Mutating a call whose only purpose is its side effect.** A bare deletion of +`sdkclient.DisableTransientRetry(...)` orphans the `internal/sdkclient` import, so +the build fails — that is a **compile kill, not a test kill**, and it proves +nothing about coverage. The honest mutation is the assignment form +`_ = sdkclient.DisableTransientRetry`, which keeps the import used so only +behaviour changes. This tripped an earlier verification pass (see SCA-18). The +same shape applies to any mutation that would leave an identifier or import +unused: use the semantic form, not a token deletion. + +**A sandbox that blocks loopback TCP cannot refute a row.** Several rows were once +recorded as "not reproducible" purely because the verifier could not bind an +`httptest` server. That is an environmental limitation, not a property of the code; +re-run such rows in an environment with loopback before recording a verdict. + **Verdicts** are the verifiers' conclusions, not the original reports': `CONFIRMED` (survivor reproduced), `OVERSTATED` (survivor real, stated consequence weaker than claimed), `REFUTED` (the claim is false — the mutant dies, or the @@ -87,44 +101,55 @@ premise does not hold). | OUT-27 | cmd/favorites | `cmd/favorites.go:248-250` | `if provider != "" { fav.Provider = provider }` → ignore the interactive `--provider`. **Mutant dies**: `TestFavoritesAddInteractiveMode/eligibility_fetch_fails` fails with `output missing "failed to fetch eligible targets"`. A genuine assertion kill — not a compile error, panic, or environment failure | REFUTED | refuted | n/a — already killed | — | todo | | OUT-28 | cmd/status docs | n/a | Claim: `computeRemainingTimeAt` is referenced but missing, and CLAUDE.md is stale. **False on both counts.** `rg computeRemainingTimeAt .` → no hits; the clock seam was deliberately removed in `2f34795`; current CLAUDE.md never claims it exists | REFUTED | refuted | n/a — no such symbol | — | todo | | OUT-29 | cmd test mocks | `cmd/test_mocks.go:26,41,54,198` | Claim: argument-ignoring mocks are the *general* root cause. Every mock already supports argument-aware callbacks (`loadFunc`, `listFunc`), and OUT-27 is killed by an argument-sensitive error-path test. The default return path is arg-blind, which explains individual weak fixtures — but not as a blanket root cause | REFUTED | refuted | n/a — superseded by PR4's capture convention | — | todo | -| SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | todo | -| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactBody` (add `gotBody` to `mockHTTPClient`) | PR8 | todo | -| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactBody` | PR8 | todo | -| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactBody` | PR8 | todo | -| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | `TestElevate_Route` (add `gotRoute`) | PR8 | todo | -| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestListSessions_Route` | PR8 | todo | -| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | `TestRevokeSessions_Route` | PR8 | todo | -| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestListGroupsEligibility_Route` | PR8 | todo | -| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | `TestElevateGroups_Route` | PR8 | todo | -| SCA-10 | internal/sca | `internal/sca/service.go:323` and `:356` | `"pageSize": -1` → `"pageSize": 10` at **both** on-demand call sites. The wire contract is genuinely untested; the *consequence* ("-1 means all, so 10 truncates the role picker") is **unevidenced** — neither the repo, the pinned SDK, nor official docs document this endpoint's `-1` semantics. Assert the sent value; do not assert a truncation story | OVERSTATED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | -| SCA-11 | internal/sca | `internal/sca/service.go:326` and `:360` | `"target_category": "cloud_console"` → `"WRONG"` at **both** on-demand call sites | CONFIRMED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | todo | -| SCA-12 | internal/sca | `internal/sca/service.go:258-262` | `ListSessions`'s `buildParams` closure returns `nil` instead of `map[string]string{"csp": string(*csp)}`. `TestListSessions_WithCSPFilter` is tautological — its canned response is already Azure and it never inspects params. `grant status --provider azure` does no local filtering, so all providers' sessions would display | CONFIRMED | test | Replace `TestListSessions_WithCSPFilter` with `TestListSessions_SendsCSPQueryParam` (add `gotParams`) | PR8 | todo | -| SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | todo | -| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestListEligibility_PropagatesDecodeError` | PR8 | todo | -| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lives inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Verifier could not execute it (loopback prohibited in that sandbox); coverage topology confirmed by grep | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | todo | -| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:28` | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | todo | -| SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | todo | -| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | Claim: deleting the call yields `inbound requests = 4, want 1`. **Not reproducible** in the verifier's sandbox (loopback prohibited); a literal deletion is a compile kill first (`"internal/sdkclient" imported and not used`). Same for the workflows twin. The guard tests exist and are correctly aimed; only their runtime assertion was unverifiable | OVERSTATED | refuted | n/a — `internal/sca/retry_policy_test.go` / `internal/workflows/retry_policy_test.go` already guard this | — | todo | -| WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | todo | -| WF-02 | internal/workflows | `internal/workflows/service.go:161` | Delete the `checkResponse(resp, "list requests")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-03 | internal/workflows | `internal/workflows/service.go:196` | Delete the `checkResponse(resp, "get request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-04 | internal/workflows | `internal/workflows/service.go:221` | Delete the `checkResponse(resp, "submit request")` guard. Verified consequence: a 500 carrying `{}` decodes to an empty request, so `grant request submit` prints a blank `Request ID:` / `State:` and exits 0 | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-05 | internal/workflows | `internal/workflows/service.go:245` | Delete the `checkResponse(resp, "cancel request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-06 | internal/workflows | `internal/workflows/service.go:272` | Delete the `checkResponse(resp, "finalize request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | todo | -| WF-07 | internal/workflows | `internal/workflows/logging_client.go:58` | Delete the `if redacted.Get("Authorization") != ""` redaction block. Real token-in-logs risk; the SCA twin is covered by `TestLoggingClient_DebugLogsHeaders` | CONFIRMED | test | new `internal/workflows/logging_client_test.go`, mirroring the sca one **including Authorization redaction** | PR8 | todo | -| WF-08 | internal/workflows | `internal/workflows/logging_client.go:24-26` | Route `Get` through `c.inner.Post(ctx, route, params)` | CONFIRMED | test | `TestLoggingClient_GetUsesGet` (workflows) | PR8 | todo | -| WF-09 | internal/workflows | `internal/workflows/logging_client.go:24-33` | Swallow the inner error and return a synthetic 200 response with `err = nil` | CONFIRMED | test | `TestLoggingClient_PropagatesInnerError` (workflows) | PR8 | todo | -| WF-10 | internal/workflows models | `internal/workflows/models/submit.go:6` | `RequestDetails map[string]interface{} \`json:"requestDetails"\`` → `json:"requestDetailsXX"` | CONFIRMED | test | `TestSubmitAccessRequest_JSONTags` | PR8 | todo | -| WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | todo | -| WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | todo | -| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_SendsFinalizationReason` | PR8 | todo | -| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`service_test.go:274`), which is the contrast that proves the gap | CONFIRMED | test | `TestFinalizeRequest_ExactRoute` | PR8 | todo | -| WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | todo | -| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_DefaultPageSize` | PR8 | todo | -| WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | todo | -| WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | todo | -| WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | -| WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | todo | +| SCA-01 | internal/sca models | `internal/sca/models/elevate.go:30` | `AccessCredentials *string \`json:"accessCredentials"\`` → `json:"accessCredentialsXX"`. Passes the **entire repo suite**. Only fixtures use `"accessCredentials": null`; service tests marshal Go structs whose field is nil. This is the one field `grant env` exists to deliver | CONFIRMED | test | `TestElevateResponse_DecodesPopulatedAccessCredentials` — decode a *populated* value off the wire through `ParseAWSCredentials` and assert all three values | PR8 | done | +| SCA-02 | internal/sca | `internal/sca/service.go:208` | `s.httpClient.Post(ctx, "/api/access/elevate", req)` → `..., nil)` | CONFIRMED | test | `TestElevate_SendsExactRouteAndBody` (added `gotRoute`/`gotBody` to `mockHTTPClient`) — one test, shared with SCA-05 | PR8 | done | +| SCA-03 | internal/sca | `internal/sca/service.go:236` | `s.httpClient.Post(ctx, "/api/access/sessions/revoke", req)` → `..., nil)` | CONFIRMED | test | `TestRevokeSessions_SendsExactRouteAndBody` — one test, shared with SCA-07 | PR8 | done | +| SCA-04 | internal/sca | `internal/sca/service.go:390` | `s.httpClient.Post(ctx, "/api/access/elevate/groups", req)` → `..., nil)` | CONFIRMED | test | `TestElevateGroups_SendsExactRouteAndBody` — one test, shared with SCA-09 | PR8 | done | +| SCA-05 | internal/sca | `internal/sca/service.go:208` | Route `"/api/access/elevate"` → `"/WRONG"` | CONFIRMED | test | Folded into `TestElevate_SendsExactRouteAndBody` (same test as SCA-02; it asserts route **and** body) | PR8 | done | +| SCA-06 | internal/sca | `internal/sca/service.go:258` | Route `"/api/access/sessions"` → `"/WRONG"` | CONFIRMED | test | `TestGetRoutes_Exact/list_sessions` | PR8 | done | +| SCA-07 | internal/sca | `internal/sca/service.go:236` | Route `"/api/access/sessions/revoke"` → `"/WRONG"` | CONFIRMED | test | Folded into `TestRevokeSessions_SendsExactRouteAndBody` (same test as SCA-03) | PR8 | done | +| SCA-08 | internal/sca | `internal/sca/service.go:285` | `route := fmt.Sprintf("/api/access/%s/eligibility/groups", csp)` → `fmt.Sprintf("/WRONG/%s", csp)` | CONFIRMED | test | `TestGetRoutes_Exact/list_groups_eligibility` | PR8 | done | +| SCA-09 | internal/sca | `internal/sca/service.go:390` | Route `"/api/access/elevate/groups"` → `"/WRONG"` | CONFIRMED | test | Folded into `TestElevateGroups_SendsExactRouteAndBody` (same test as SCA-04) | PR8 | done | +| SCA-10 | internal/sca | `internal/sca/service.go:323` and `:356` | `"pageSize": -1` → `"pageSize": 10` at **both** on-demand call sites. The wire contract is genuinely untested; the *consequence* ("-1 means all, so 10 truncates the role picker") is **unevidenced** — neither the repo, the pinned SDK, nor official docs document this endpoint's `-1` semantics. Assert the sent value; do not assert a truncation story | OVERSTATED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | done | +| SCA-11 | internal/sca | `internal/sca/service.go:326` and `:360` | `"target_category": "cloud_console"` → `"WRONG"` at **both** on-demand call sites | CONFIRMED | test | `TestListOnDemandResources_ExactQueryParams` | PR8 | done | +| SCA-12 | internal/sca | `internal/sca/service.go:258-262` | `ListSessions`'s `buildParams` closure returns `nil` instead of `map[string]string{"csp": string(*csp)}`. `TestListSessions_WithCSPFilter` is tautological — its canned response is already Azure and it never inspects params. `grant status --provider azure` does no local filtering, so all providers' sessions would display | CONFIRMED | test | Replace `TestListSessions_WithCSPFilter` with `TestListSessions_SendsCSPQueryParam` (add `gotParams`) | PR8 | done | +| SCA-13 | internal/sca | `internal/sca/service.go:144` | `s.httpClient.Get(ctx, route, p)` → `s.httpClient.Get(context.Background(), route, p)` in `paginate` | CONFIRMED | test | `TestPaginate_PropagatesContextCancellation` | PR8 | done | +| SCA-14 | internal/sca | `internal/sca/service.go:184` (and the sibling decoders at `:267`, `:291`) | In the `ListEligibility` decode closure, swallow the error: `if err := json.NewDecoder(r).Decode(&page); err != nil { return nil, nil, 0, nil }` | CONFIRMED | test | `TestPaginate_PropagatesDecodeError` | PR8 | done | +| SCA-15 | internal/sca | `internal/sca/service.go:74` | `client.SetHeader("X-API-Version", "2.0")` → delete the call, and separately → `"1.0"`. The **only** guard lived inside `TestNewSCAAccessServiceDisablesTransientRetry`, so a retry-motivated rename silently deletes the header assertion. Both mutants were subsequently executed and both fail `service_client_test.go:52` (deletion → `X-API-Version = ""`; `"1.0"` → `X-API-Version = "1.0"`) | OVERSTATED | test | Extract `TestNewSCAAccessService_SetsAPIVersionHeader` as its own named test | PR8 | done | +| SCA-16 | internal/sca models | `internal/sca/models/elevate.go:6` (was `:28`, which is `ElevateTargetResult` — the **response** model, killed by the pre-existing `TestElevateResponse_Success`) | `RoleID string \`json:"roleId"\`` → `json:"roleIdXX"` on the **request** model | CONFIRMED | test | `TestElevateRequest_JSONTags` | PR8 | done | +| SCA-17 | internal/sca models | `internal/sca/models/credentials.go:17` (`ParseAWSCredentials`) | Swap `SecretAccessKey` and `SessionToken` in the parser output. **Mutant dies repo-wide**: `env_test.go:77` and `root_elevate_test.go:426`. Nuance: swapping the *struct JSON tags* instead is also caught, by `TestAWSCredentials_JSONUnmarshal`. Recorded so this is not re-filed as a survivor | REFUTED | refuted | n/a — already killed | — | done | +| SCA-18 | internal/sca | `internal/sca/service.go:75-ish` (`sdkclient.DisableTransientRetry` call) | `_ = sdkclient.DisableTransientRetry` (the assignment form — a bare deletion orphans the import and is a compile kill, not a test kill). Reproduced on **both** services exactly as claimed: `inbound requests = 4, want 1` from `internal/sca/retry_policy_test.go:91` and `internal/workflows/retry_policy_test.go:90` | CONFIRMED | test | `TestNewSCAAccessServiceDisablesTransientRetry` and `TestNewAccessRequestServiceDisablesTransientRetry` (both pre-existing and correctly aimed; PR8 executed them against the mutant) | PR8 | done | +| WF-01 | internal/workflows | `internal/workflows/service.go:102` | Delete `if err := checkResponse(resp, "request forms"); err != nil { return nil, err }` | CONFIRMED | test | `TestWorkflows_Non200` (table over all six call sites) | PR8 | done | +| WF-02 | internal/workflows | `internal/workflows/service.go:161` | Delete the `checkResponse(resp, "list requests")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-03 | internal/workflows | `internal/workflows/service.go:196` | Delete the `checkResponse(resp, "get request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-04 | internal/workflows | `internal/workflows/service.go:221` | Delete the `checkResponse(resp, "submit request")` guard. Verified consequence: a 500 carrying `{}` decodes to an empty request, so `grant request submit` prints a blank `Request ID:` / `State:` and exits 0 | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-05 | internal/workflows | `internal/workflows/service.go:245` | Delete the `checkResponse(resp, "cancel request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-06 | internal/workflows | `internal/workflows/service.go:272` | Delete the `checkResponse(resp, "finalize request")` guard | CONFIRMED | test | `TestWorkflows_Non200` | PR8 | done | +| WF-07 | internal/workflows | `internal/workflows/logging_client.go:58` | Delete the `if redacted.Get("Authorization") != ""` redaction block. Real token-in-logs risk; the SCA twin is covered by `TestLoggingClient_DebugLogsHeaders` | CONFIRMED | test | new `internal/workflows/logging_client_test.go`, mirroring the sca one **including Authorization redaction** | PR8 | done | +| WF-08 | internal/workflows | `internal/workflows/logging_client.go:24-26` | Route `Get` through `c.inner.Post(ctx, route, params)` | CONFIRMED | test | `TestLoggingClient_GetUsesGet` (workflows) | PR8 | done | +| WF-09 | internal/workflows | `internal/workflows/logging_client.go:24-33` | Swallow the inner error and return a synthetic 200 response with `err = nil` | CONFIRMED | test | `TestLoggingClient_PropagatesInnerError` (workflows) | PR8 | done | +| WF-10 | internal/workflows models | `internal/workflows/models/submit.go:6` | `RequestDetails map[string]interface{} \`json:"requestDetails"\`` → `json:"requestDetailsXX"` | CONFIRMED | test | `TestSubmitAccessRequest_JSONTags` | PR8 | done | +| WF-11 | internal/workflows models | `internal/workflows/models/finalize.go:5` | `Result string \`json:"result"\`` → `json:"resultXX"` | CONFIRMED | test | `TestFinalizeAccessRequest_JSONTags` | PR8 | done | +| WF-12 | internal/workflows models | `internal/workflows/models/cancel.go:5` | `CancelReason *string \`json:"cancelReason"\`` → `json:"cancelReasonXX"` | CONFIRMED | test | `TestCancelAccessRequest_JSONTags` | PR8 | done | +| WF-13 | internal/workflows | `internal/workflows/service.go:263` | Delete `FinalizationReason: reason,` from the `FinalizeAccessRequest` literal | CONFIRMED | test | `TestFinalizeRequest_ExactRouteAndReason` — one test, shared with WF-14 | PR8 | done | +| WF-14 | internal/workflows | `internal/workflows/service.go:260` | `route := fmt.Sprintf("/api/workflows/requests/%s/finalize", requestID)` → `"/api/workflows/requests/finalize"`. The cancel twin *is* covered (`TestCancelRequest`), which is the contrast that proves the gap | CONFIRMED | test | Folded into `TestFinalizeRequest_ExactRouteAndReason` (same test as WF-13) | PR8 | done | +| WF-15 | internal/workflows | `internal/workflows/service.go:141` | Delete `qp["limit"] = strconv.Itoa(limit)` | CONFIRMED | test | `TestListRequests_SendsLimit` | PR8 | done | +| WF-16 | internal/workflows | `internal/workflows/service.go:124` | `const defaultPageSize = 50` → `= 1` | CONFIRMED | test | `TestListRequests_SendsLimit` (same test as WF-15; its `defaultPageSize` rows pin the 50) | PR8 | done | +| WF-17 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `Get(context.Background(), ...)` in the pagination loop | CONFIRMED | test | `TestListRequests_PropagatesContextCancellation` | PR8 | done | +| WF-18 | internal/workflows | `internal/workflows/service.go:201` | In `GetRequest`, swallow the decode error: `if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { return &result, nil }` | CONFIRMED | test | `TestGetRequest_PropagatesDecodeError` | PR8 | done | +| WF-19 | internal/workflows | `internal/workflows/service_config.go:9` | `ServiceName: "access-requests"` → `"WRONG"`. There is no workflows `service_config_test.go` at all | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | done | +| WF-20 | internal/workflows | `internal/workflows/service.go:45` | `base.Authenticator("isp")` → `base.Authenticator("WRONG")` | CONFIRMED | test | new `internal/workflows/service_config_test.go` | PR8 | done | +| SCA-19 | internal/sca | `internal/sca/service.go:60` | `isp.FromISPAuth(ispAuth, "sca", ...)` → `"WRONG"`. Not in the original audit; found while closing SCA-15. Both retry-policy tests overwrite `client.BaseURL` before issuing a request, so the slug — which is what resolves the live host — was unpinned. **Decision: pin, not won't-fix.** The assertion is one line against the already-constructed client, needs no network, and the failure mode (every request to the wrong host) is total | CONFIRMED | test | `TestNewSCAAccessService_UsesSCAServiceSlug` — asserts `BaseURL == "https://testtenant.sca.example.test"` from the existing fake JWT, **before** any BaseURL swap | PR8 | done | +| WF-21 | internal/workflows | `internal/workflows/service.go:56` | `isp.FromISPAuth(ispAuth, "uar", ...)` → `"WRONG"`. The UAR twin of SCA-19, same rationale and same decision | CONFIRMED | test | `TestNewAccessRequestService_UsesUARServiceSlug` | PR8 | done | +| WF-22 | internal/workflows | `internal/workflows/service.go:156` | `s.httpClient.Get(ctx, "/api/workflows/requests", qp)` → `"/apiZ/workflows/requests"`. The only unpinned route in either service: `TestListRequests_SendsLimit` already recorded `mock.gotRoute` and never asserted it, so `grant request list` could be pointed anywhere and stay green | CONFIRMED | test | `TestListRequests_SendsLimit` — route assertion added alongside the existing limit assertion | PR8 | done | +| WF-23 | internal/workflows | `internal/workflows/service.go:215` | In `SubmitRequest`, forward a stripped copy: `mangled := *req; mangled.RequestDetails = nil; s.httpClient.Post(ctx, "/api/workflows/requests", &mangled)`. `TestSubmitRequest` asserted only `TargetCategory`, so reason, role, target, dates and priority — the entire substance of `grant request submit` — were unasserted at the service boundary. The identical mutation on the two SCA pass-through POSTs dies, because those tests `DeepEqual` the whole body | CONFIRMED | test | `TestSubmitRequest_SendsExactRouteAndBody` — mirrors `TestElevate_SendsExactRouteAndBody`, `DeepEqual` against a populated, all-distinct `RequestDetails` | PR8 | done | +| SCA-20 | internal/sca models | `internal/sca/models/groups.go:20` | `GroupID string \`json:"groupId"\`` → `json:"ZgroupId"` on `GroupsElevateTarget` — the **request** model. Its single field is the whole per-target payload of `POST /api/access/elevate/groups`: mutated, every `grant --group` elevation sends `{"targets":[{"ZgroupId":"..."}]}` and group elevation is broken outright. `TestElevateRequest_JSONTags` pins the cloud twin; there was no groups twin | CONFIRMED | test | `TestGroupsElevateRequest_JSONTags` — asserts the **nested** `groupId` inside `targets`, plus `directoryId` and `csp` | PR8 | done | +| SCA-21 | internal/sca models | `internal/sca/models/groups.go:33` | `SessionID string \`json:"sessionId"\`` → `json:"ZsessionId"` on `GroupsElevateTargetResult`. Elevation still succeeds, but grant reports an empty session ID and the session can never be revoked by ID | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| SCA-22 | internal/sca models | `internal/sca/models/groups.go:32` | `GroupID string \`json:"groupId"\`` → `json:"ZgroupId"` on `GroupsElevateTargetResult` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| SCA-23 | internal/sca models | `internal/sca/models/groups.go:40` | `DirectoryID string \`json:"directoryId"\`` → `json:"ZdirectoryId"` on `GroupsElevateResponse` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| SCA-24 | internal/sca models | `internal/sca/models/groups.go:41` | `CSP CSP \`json:"csp"\`` → `json:"Zcsp"` on `GroupsElevateResponse` | CONFIRMED | test | `TestGroupsElevateResponse_DecodesPopulatedResult` | PR8 | done | +| WF-24 | internal/workflows models | `internal/workflows/models/request.go:32`, `:63` (and the rest of `AccessRequest`, `Entity`, `ApproverAction`, `ListRequestsResponse`) | Rename any response tag, e.g. `RequestDetails ... \`json:"requestDetails,omitempty"\`` → `json:"ZrequestDetails,omitempty"`, or `ApproverAction.Result \`json:"result"\`` → `json:"Zresult"`. `internal/workflows/models/wire_tags_test.go` pinned only the three request bodies while the SCA twin pinned both directions; every response test decoded a body marshaled from the same struct, so a rename round-tripped and `grant request get` / `list` would render blanks. Not claimed by the PR — recorded because it was found while closing WF-23 | CONFIRMED | test | `TestAccessRequest_DecodesPopulatedResponse` and `TestListRequestsResponse_DecodesPopulatedPage` — 18 tags verified killed (`requestId`, `targetCategory`, `requestState`, `requestResult`, `requestDetails`, `requestApprovers`, `requester`, `createdBy/At`, `updatedBy/At`, `entityId`, `entityName`, `approver`, `result`, `items`, `count`, `totalCount`) | PR8 | done | +| WF-25 | internal/workflows models | `internal/workflows/models/form.go` (`FormQuestion`, `Validator`) | Rename any form-metadata tag, e.g. `Validator.Regex \`json:"regex,omitempty"\`` → `json:"Zregex,omitempty"`. Deliberately **not** closed in PR8: this is validation metadata for the interactive `grant request submit` form, one step removed from the user-visible output that WF-24 covers, and pinning it well needs a populated-form fixture rather than another tag list. Recorded so the gap is stated rather than silent | CONFIRMED | test | Follow-up: a populated `RequestFormResponse` decode fixture pinning `requestForms`, `requestForm`, `questions`, `key`, `required`, `valueType`, `valueChoices`, `validators` and the `Validator` fields | — | todo | | ELV-01 | cmd/selection | `cmd/selection.go:78` | `return &items[i], nil` → `return &items[0], nil`. `TestFindItemByDisplay` only checks non-nil/error, so selecting one display value silently elevates the first sorted target and prints a success line naming the wrong one | CONFIRMED | test | `TestFindItemByDisplay_ReturnsMatchingItem` | PR4 | todo | | ELV-02 | cmd/root (unified elevate builder) | `cmd/root.go:786-793` | In `elevateCloud`, swap `WorkspaceID: selectedTarget.WorkspaceID` and `RoleID: selectedTarget.RoleInfo.ID` | CONFIRMED | test | `TestElevateCloud_RequestPayload` (`mockElevateService` history) | PR4 | todo | | ELV-03 | cmd/root (unified elevate builder) | `cmd/root.go:786-788` | In `elevateCloud`, blank both `CSP:` and `OrganizationID:` | CONFIRMED | test | `TestElevateCloud_RequestPayload` | PR4 | todo | @@ -216,28 +241,28 @@ premise does not hold). | PR5 — Output contracts | 32 | 30 | | PR6 — Cache and config semantics | 16 | 16 | | PR7 — UI behavior | 10 | 10 | -| PR8 — SCA / workflows / models wire contracts | 36 | 34 | -| *(no PR — settled, recorded only)* | 5 | 0 | -| **Total** | **165** | **152** | +| PR8 — SCA / workflows / models wire contracts | 39 | 37 | +| *(no PR — settled, recorded only)* | 4 | 0 | +| **Total** | **167** | **155** | ### By verdict | Verdict | Rows | |---|---| -| CONFIRMED | 152 | -| OVERSTATED | 9 | +| CONFIRMED | 155 | +| OVERSTATED | 8 | | REFUTED | 4 | ### By disposition | Disposition | Rows | |---|---| -| `test` | 149 | +| `test` | 152 | | `test + prod-fix` | 5 | | `prod-fix` | 1 | | `wont-fix` | 5 | -| `refuted` | 5 | -| **Total** | **165** | +| `refuted` | 4 | +| **Total** | **167** | The seven production changes, matching the plan's table: @@ -253,10 +278,11 @@ The seven production changes, matching the plan's table: ### Total CONFIRMED -**152 CONFIRMED**, plus **9 OVERSTATED** (real survivors whose stated consequence -was weaker than originally claimed) and **4 REFUTED**. **165 rows total.** -5 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10), so **160 rows require -work**, of which 6 carry a production change. +**155 CONFIRMED**, plus **8 OVERSTATED** (real survivors whose stated consequence +was weaker than originally claimed) and **4 REFUTED**. **167 rows total.** +5 rows are `wont-fix` (SFU-20, SFU-21, SFU-22, UI-09, UI-10) and 4 are closed by +review (`refuted`), so **158 rows require work**, of which 6 carry a production +change. ### Reconciliation against "145" @@ -268,14 +294,14 @@ to a named, independently reproduced finding in a verification report. |---|---|---|---| | 1 — cmd request/auth | 22 confirmed | 23 (22 CONFIRMED + 1 OVERSTATED) | **Yes.** The 22 CONFIRMED rows are mutation-level and match exactly. REQ-23 (integration suite absent from CI) is reported *outside* the 22. | | 2 — cmd status/favorites/list | 25 real of 29 claimed | 29 (23 CONFIRMED, 3 OVERSTATED, 3 REFUTED) | **Approximately.** All 29 claimed items are listed so the three refutations stay recorded. 23 + 3 OVERSTATED = 26 actionable, one above the verifier's "25" — its own prose is imprecise about whether OUT-21/OUT-25/OUT-26 count as "real". | -| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (34 CONFIRMED, 3 OVERSTATED, 1 REFUTED) | **Yes.** 34 CONFIRMED + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The other 3 rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on) and SCA-17/SCA-18, recorded so they are not re-filed. | +| 3 — internal/sca + workflows | 31 + 4 extra = 35 | 38 (35 CONFIRMED, 2 OVERSTATED, 1 REFUTED) | **Yes.** 34 of the CONFIRMED rows + SCA-10 (a real survivor, only its truncation consequence overstated) = exactly 35 survivors. The remaining rows are SCA-15 (X-API-Version coverage topology, which PR8 acts on), SCA-18 (reproduced by PR8, above the headline) and SCA-17, recorded so it is not re-filed. | | 4 — cmd root/elevate/env | 22 confirmed | 27 (26 CONFIRMED, 1 OVERSTATED) | **No — +5.** The report's headline groups its own sub-lettered mutations inconsistently: H2.1–2.3, H3.1–3.4, M4.1–2, M5.1–3, M6.1–6.4 and M9.1–3 are enumerated individually in the body, each with its own `go test ./cmd/ -count=1 → ok` transcript, but collapsed in the total. Listing them individually gives 25 production mutations plus 2 test-quality rows (ELV-26, ELV-27). | | 5 — cache/config/ui/selfupdate | 44 reproduced, 41 actionable | 48 (all CONFIRMED, 4 of them `wont-fix`) | **Partly — +4.** Same granularity problem: M7 and M9 are two mutations each; L1, L3, L4 and L5 are three each; L9 is three survivors. Enumerating every reproduced mutation gives 48; removing the 4 unreachable/defensive `wont-fix` rows (SFU-20, SFU-21, UI-09, UI-10) gives **44 actionable**, matching "44 reproduced" but not "41 actionable" — the report never itemises which 3 it dropped. | -**Net: 165 rows against a headline of 145.** Roughly 9 of the excess is finer +**Net: 167 rows against a headline of 145.** Roughly 9 of the excess is finer enumeration in batches 4 and 5 (mutations the reports reproduced individually but -totalled in groups); the rest is the 13 OVERSTATED/REFUTED rows the headline count -deliberately excluded but which belong here as settled questions. Nothing was +totalled in groups); most of the rest is the 12 OVERSTATED/REFUTED rows the headline +count deliberately excluded but which belong here as settled questions. Nothing was invented and nothing was dropped to hit a number. There are no `NEEDS-REVIEW` rows: every row's source report is unambiguous about diff --git a/internal/sca/models/credentials_test.go b/internal/sca/models/credentials_test.go index 9e1798c..4dc98dc 100644 --- a/internal/sca/models/credentials_test.go +++ b/internal/sca/models/credentials_test.go @@ -2,6 +2,7 @@ package models import ( "encoding/json" + "strings" "testing" ) @@ -54,16 +55,31 @@ func TestParseAWSCredentials(t *testing.T) { name string input string wantErr bool + // Asserted only when wantErr is true and non-empty. Without it the + // empty-string guard is inert: json.Unmarshal rejects "" anyway, so + // deleting the guard changes only the message. + wantErrContains string + // Asserted only when wantErr is false. Without these the parser could + // swap SecretAccessKey and SessionToken and this package would not + // notice — it discarded the parsed value entirely. + wantAccessKeyID string + wantSecretKey string + wantSessionToken string }{ { - name: "valid JSON string", - input: `{"aws_access_key":"AKIA","aws_secret_access_key":"secret","aws_session_token":"token"}`, - wantErr: false, + name: "valid JSON string", + // Distinguishable values: a swap of any two must change the result. + input: `{"aws_access_key":"AKIAVALUE","aws_secret_access_key":"SECRETVALUE","aws_session_token":"TOKENVALUE"}`, + wantErr: false, + wantAccessKeyID: "AKIAVALUE", + wantSecretKey: "SECRETVALUE", + wantSessionToken: "TOKENVALUE", }, { - name: "empty string", - input: "", - wantErr: true, + name: "empty string", + input: "", + wantErr: true, + wantErrContains: "empty credentials string", }, { name: "malformed JSON", @@ -85,9 +101,24 @@ func TestParseAWSCredentials(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - _, err := ParseAWSCredentials(tt.input) + creds, err := ParseAWSCredentials(tt.input) if (err != nil) != tt.wantErr { - t.Errorf("ParseAWSCredentials() error = %v, wantErr %v", err, tt.wantErr) + t.Fatalf("ParseAWSCredentials() error = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr { + if tt.wantErrContains != "" && !strings.Contains(err.Error(), tt.wantErrContains) { + t.Errorf("error = %v, want it to contain %q", err, tt.wantErrContains) + } + return + } + if creds.AccessKeyID != tt.wantAccessKeyID { + t.Errorf("AccessKeyID = %q, want %q", creds.AccessKeyID, tt.wantAccessKeyID) + } + if creds.SecretAccessKey != tt.wantSecretKey { + t.Errorf("SecretAccessKey = %q, want %q", creds.SecretAccessKey, tt.wantSecretKey) + } + if creds.SessionToken != tt.wantSessionToken { + t.Errorf("SessionToken = %q, want %q", creds.SessionToken, tt.wantSessionToken) } }) } diff --git a/internal/sca/models/wire_tags_test.go b/internal/sca/models/wire_tags_test.go new file mode 100644 index 0000000..8c61a7b --- /dev/null +++ b/internal/sca/models/wire_tags_test.go @@ -0,0 +1,238 @@ +package models + +import ( + "encoding/json" + "testing" +) + +// TestElevateRequest_JSONTags pins the request-body field names. The existing +// marshal tests decode into the same Go struct, so a renamed tag round-trips +// trivially; these assert the literal keys the API sees. +func TestElevateRequest_JSONTags(t *testing.T) { + t.Parallel() + + req := ElevateRequest{ + CSP: CSPAzure, + OrganizationID: "org-tag-1", + Targets: []ElevateTarget{ + {WorkspaceID: "ws-tag-2", RoleID: "role-tag-3", RoleName: "Role Tag Four"}, + }, + } + b, err := json.Marshal(req) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, key := range []string{"csp", "organizationId", "targets"} { + if _, ok := raw[key]; !ok { + t.Errorf("request body is missing key %q: %s", key, b) + } + } + + var targets []map[string]json.RawMessage + if err := json.Unmarshal(raw["targets"], &targets); err != nil { + t.Fatalf("unmarshal targets: %v", err) + } + if len(targets) != 1 { + t.Fatalf("targets len = %d, want 1", len(targets)) + } + for _, key := range []string{"workspaceId", "roleId", "roleName"} { + if _, ok := targets[0][key]; !ok { + t.Errorf("target is missing key %q: %s", key, b) + } + } +} + +// TestElevateResponse_DecodesPopulatedAccessCredentials is the guard for the +// single field `grant env` exists to deliver. Every prior test only ever saw +// "accessCredentials": null, or marshaled a Go struct whose field was nil, so +// renaming the tag passed the entire repo suite while every AWS elevation +// silently returned no credentials. +func TestElevateResponse_DecodesPopulatedAccessCredentials(t *testing.T) { + t.Parallel() + + // Distinguishable values so a swap in the parser cannot be masked. + const wire = `{ + "response": { + "csp": "AWS", + "organizationId": "org-creds-1", + "results": [ + { + "workspaceId": "111122223333", + "roleId": "arn:aws:iam::111122223333:role/Admin", + "sessionId": "sess-creds-2", + "accessCredentials": "{\"aws_access_key\":\"ACCESSKEYVALUE\",\"aws_secret_access_key\":\"SECRETKEYVALUE\",\"aws_session_token\":\"SESSIONTOKENVALUE\"}", + "errorInfo": null + } + ] + } + }` + + var resp ElevateResponse + if err := json.Unmarshal([]byte(wire), &resp); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(resp.Response.Results) != 1 { + t.Fatalf("results len = %d, want 1", len(resp.Response.Results)) + } + raw := resp.Response.Results[0].AccessCredentials + if raw == nil { + t.Fatal("accessCredentials did not decode: got nil, want the populated credentials string") + } + + creds, err := ParseAWSCredentials(*raw) + if err != nil { + t.Fatalf("ParseAWSCredentials: %v", err) + } + if creds.AccessKeyID != "ACCESSKEYVALUE" { + t.Errorf("AccessKeyID = %q, want %q", creds.AccessKeyID, "ACCESSKEYVALUE") + } + if creds.SecretAccessKey != "SECRETKEYVALUE" { + t.Errorf("SecretAccessKey = %q, want %q", creds.SecretAccessKey, "SECRETKEYVALUE") + } + if creds.SessionToken != "SESSIONTOKENVALUE" { + t.Errorf("SessionToken = %q, want %q", creds.SessionToken, "SESSIONTOKENVALUE") + } +} + +// TestElevateResponse_AccessCredentialsAbsentOrEmpty covers the non-populated +// shapes of the same field. +func TestElevateResponse_AccessCredentialsAbsentOrEmpty(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + result string + wantNil bool + wantParseErr bool + }{ + {name: "explicit null", result: `{"accessCredentials": null}`, wantNil: true}, + {name: "absent", result: `{"sessionId": "sess-1"}`, wantNil: true}, + {name: "empty string", result: `{"accessCredentials": ""}`, wantParseErr: true}, + {name: "malformed inner JSON", result: `{"accessCredentials": "{not json}"}`, wantParseErr: true}, + {name: "incomplete inner JSON", result: `{"accessCredentials": "{\"aws_access_key\":\"AK\"}"}`, wantParseErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + var res ElevateTargetResult + if err := json.Unmarshal([]byte(tt.result), &res); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if tt.wantNil { + if res.AccessCredentials != nil { + t.Fatalf("accessCredentials = %q, want nil", *res.AccessCredentials) + } + return + } + if res.AccessCredentials == nil { + t.Fatal("accessCredentials = nil, want a decoded string") + } + _, err := ParseAWSCredentials(*res.AccessCredentials) + if (err != nil) != tt.wantParseErr { + t.Errorf("ParseAWSCredentials error = %v, wantErr %v", err, tt.wantParseErr) + } + }) + } +} + +// TestGroupsElevateRequest_JSONTags is the twin of TestElevateRequest_JSONTags +// for the group-elevation request body. The nested `groupId` inside `targets` +// is the entire per-target payload of POST /api/access/elevate/groups: if it +// were renamed, every `grant --group` elevation would send +// {"targets":[{"ZgroupId":"..."}]} and group elevation would be broken outright. +func TestGroupsElevateRequest_JSONTags(t *testing.T) { + t.Parallel() + + // Distinguishable values so a swap between fields cannot be masked. + req := GroupsElevateRequest{ + DirectoryID: "dir-groups-1", + CSP: CSPAzure, + Targets: []GroupsElevateTarget{ + {GroupID: "group-groups-2"}, + }, + } + b, err := json.Marshal(req) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, key := range []string{"directoryId", "csp", "targets"} { + if _, ok := raw[key]; !ok { + t.Errorf("request body is missing key %q: %s", key, b) + } + } + if string(raw["directoryId"]) != `"dir-groups-1"` { + t.Errorf("directoryId = %s, want %q", raw["directoryId"], "dir-groups-1") + } + if string(raw["csp"]) != `"AZURE"` { + t.Errorf("csp = %s, want %q", raw["csp"], "AZURE") + } + + var targets []map[string]json.RawMessage + if err := json.Unmarshal(raw["targets"], &targets); err != nil { + t.Fatalf("unmarshal targets: %v", err) + } + if len(targets) != 1 { + t.Fatalf("targets len = %d, want 1", len(targets)) + } + got, ok := targets[0]["groupId"] + if !ok { + t.Fatalf("target is missing key %q: %s", "groupId", b) + } + if string(got) != `"group-groups-2"` { + t.Errorf("targets[0].groupId = %s, want %q", got, "group-groups-2") + } +} + +// TestGroupsElevateResponse_DecodesPopulatedResult pins the response side of +// group elevation. Every prior test marshaled a Go struct or decoded a body +// produced from one, so renaming a tag round-tripped trivially. sessionId is +// the ID `grant --group` reports and the only handle for revoking the session +// by ID: if it broke, elevation appears to succeed while grant prints an empty +// session ID and the session can never be revoked. +func TestGroupsElevateResponse_DecodesPopulatedResult(t *testing.T) { + t.Parallel() + + // Distinguishable values so a swap between fields cannot be masked. + const wire = `{ + "directoryId": "dir-groupsresp-1", + "csp": "AZURE", + "results": [ + { + "groupId": "group-groupsresp-2", + "sessionId": "sess-groupsresp-3", + "errorInfo": null + } + ] + }` + + var resp GroupsElevateResponse + if err := json.Unmarshal([]byte(wire), &resp); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if resp.DirectoryID != "dir-groupsresp-1" { + t.Errorf("directoryId = %q, want %q", resp.DirectoryID, "dir-groupsresp-1") + } + if resp.CSP != CSPAzure { + t.Errorf("csp = %q, want %q", resp.CSP, CSPAzure) + } + if len(resp.Results) != 1 { + t.Fatalf("results len = %d, want 1", len(resp.Results)) + } + if resp.Results[0].GroupID != "group-groupsresp-2" { + t.Errorf("results[0].groupId = %q, want %q", resp.Results[0].GroupID, "group-groupsresp-2") + } + if resp.Results[0].SessionID != "sess-groupsresp-3" { + t.Errorf("results[0].sessionId = %q, want %q", resp.Results[0].SessionID, "sess-groupsresp-3") + } +} diff --git a/internal/sca/service_client_test.go b/internal/sca/service_client_test.go new file mode 100644 index 0000000..622c622 --- /dev/null +++ b/internal/sca/service_client_test.go @@ -0,0 +1,73 @@ +package sca + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/cyberark/idsec-sdk-golang/pkg/common/isp" +) + +// ispClientFromService reaches through the logging decorator to the SDK client +// the constructor actually published. +func ispClientFromService(t *testing.T, svc *SCAAccessService) *isp.IdsecISPServiceClient { + t.Helper() + lc, ok := svc.httpClient.(*loggingClient) + if !ok { + t.Fatalf("httpClient is %T, want *loggingClient", svc.httpClient) + } + client, ok := lc.inner.(*isp.IdsecISPServiceClient) + if !ok { + t.Fatalf("inner client is %T, want *isp.IdsecISPServiceClient", lc.inner) + } + return client +} + +// TestNewSCAAccessService_SetsAPIVersionHeader is the standalone guard for the +// X-API-Version header. It supersedes — but does not remove — the two-line +// assertion inside TestNewSCAAccessServiceDisablesTransientRetry, which remains +// as deliberate redundancy. Without this standalone test, a retry-motivated +// rename or deletion of that test would have dropped the header guard silently. +func TestNewSCAAccessService_SetsAPIVersionHeader(t *testing.T) { + gotAPIVersion := make(chan string, 1) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAPIVersion <- r.Header.Get("X-API-Version") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + svc, err := NewSCAAccessService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewSCAAccessService: %v", err) + } + ispClientFromService(t, svc).BaseURL = srv.URL + + resp, err := svc.httpClient.Get(t.Context(), "/api/access/sessions", nil) + if err != nil { + t.Fatalf("Get: %v", err) + } + defer func() { _ = resp.Body.Close() }() + + if got := <-gotAPIVersion; got != "2.0" { + t.Errorf("X-API-Version = %q, want %q", got, "2.0") + } +} + +// TestNewSCAAccessService_UsesSCAServiceSlug pins the "sca" service slug passed +// to isp.FromISPAuth. The retry and header tests both overwrite BaseURL before +// issuing a request, so without this the slug could be changed to anything and +// no test in the repo would notice — while every live request would go to the +// wrong host. +func TestNewSCAAccessService_UsesSCAServiceSlug(t *testing.T) { + svc, err := NewSCAAccessService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewSCAAccessService: %v", err) + } + + // The fake JWT carries subdomain=testtenant, platform_domain=example.test. + const want = "https://testtenant.sca.example.test" + if got := ispClientFromService(t, svc).BaseURL; got != want { + t.Errorf("BaseURL = %q, want %q", got, want) + } +} diff --git a/internal/sca/service_config_test.go b/internal/sca/service_config_test.go index 63c6518..215b81a 100644 --- a/internal/sca/service_config_test.go +++ b/internal/sca/service_config_test.go @@ -2,8 +2,6 @@ package sca import ( "testing" - - "github.com/cyberark/idsec-sdk-golang/pkg/services" ) func TestServiceConfig_ServiceName(t *testing.T) { @@ -38,9 +36,3 @@ func TestServiceConfig_ActionsConfigurations(t *testing.T) { t.Errorf("expected nil ActionsConfigurations, got %v", config.ActionsConfigurations) } } - -func TestServiceConfig_ReturnsIdsecServiceConfig(t *testing.T) { - config := ServiceConfig() - // Verify it returns the correct SDK type - var _ services.IdsecServiceConfig = config -} diff --git a/internal/sca/service_test.go b/internal/sca/service_test.go index 78d3901..cb4c09d 100644 --- a/internal/sca/service_test.go +++ b/internal/sca/service_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "io" "net/http" + "reflect" "strings" "testing" @@ -21,9 +22,19 @@ type mockHTTPClient struct { getFunc func(ctx context.Context, route string, params interface{}) (*http.Response, error) // postFunc, when set, overrides postResponse/postError for dynamic responses. postFunc func(ctx context.Context, route string, body interface{}) (*http.Response, error) + + // Recorded arguments of the most recent call. They are populated before the + // call is dispatched, so they are available whichever response mechanism is + // in use. Wire-contract tests assert on them; without them, passing nil + // bodies or a typo'd route is invisible. + gotRoute string + gotBody interface{} + gotParams interface{} } func (m *mockHTTPClient) Get(ctx context.Context, route string, params interface{}) (*http.Response, error) { + m.gotRoute = route + m.gotParams = params if m.getFunc != nil { return m.getFunc(ctx, route, params) } @@ -34,6 +45,8 @@ func (m *mockHTTPClient) Get(ctx context.Context, route string, params interface } func (m *mockHTTPClient) Post(ctx context.Context, route string, body interface{}) (*http.Response, error) { + m.gotRoute = route + m.gotBody = body if m.postFunc != nil { return m.postFunc(ctx, route, body) } @@ -508,45 +521,60 @@ func TestListSessions_Empty(t *testing.T) { } } -func TestListSessions_WithCSPFilter(t *testing.T) { - csp := models.CSPAzure - resp := models.SessionsResponse{ - Response: []models.SessionInfo{ - { - SessionID: "session1", - UserID: "user1", - CSP: models.CSPAzure, - WorkspaceID: "sub1", - RoleID: "role1", - SessionDuration: 3600, - }, - }, - NextToken: nil, - Total: 1, - } +// TestListSessions_SendsCSPQueryParam replaces the former +// TestListSessions_WithCSPFilter, which asserted only that the canned response +// it fed the mock contained CSPAzure — returning nil query params survived it. +// The filter is server-side (grant does no local filtering), so if the param is +// dropped `grant status --provider azure` silently lists every provider. +func TestListSessions_SendsCSPQueryParam(t *testing.T) { + tests := []struct { + name string + csp *models.CSP + wantParams map[string]string // nil means: expect no params at all + }{ + {name: "filter sends csp param", csp: cspPtr(models.CSPAzure), wantParams: map[string]string{"csp": "AZURE"}}, + {name: "aws filter sends its own csp", csp: cspPtr(models.CSPAWS), wantParams: map[string]string{"csp": "AWS"}}, + {name: "no filter sends no params", csp: nil, wantParams: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body, _ := json.Marshal(models.SessionsResponse{ + Response: []models.SessionInfo{{SessionID: "session1", CSP: models.CSPAzure}}, + Total: 1, + }) + mock := &mockHTTPClient{ + getResponse: &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(string(body))), + }, + } - body, _ := json.Marshal(resp) - mock := &mockHTTPClient{ - getResponse: &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(strings.NewReader(string(body))), - }, - } + svc := &SCAAccessService{httpClient: mock} + if _, err := svc.ListSessions(t.Context(), tt.csp); err != nil { + t.Fatalf("expected no error, got %v", err) + } - svc := &SCAAccessService{httpClient: mock} - result, err := svc.ListSessions(t.Context(), &csp) + if tt.wantParams == nil { + if mock.gotParams != nil { + t.Fatalf("params = %#v, want nil", mock.gotParams) + } + return + } - if err != nil { - t.Fatalf("expected no error, got %v", err) - } - if len(result.Response) != 1 { - t.Errorf("expected 1 session, got %d", len(result.Response)) - } - if result.Response[0].CSP != models.CSPAzure { - t.Errorf("expected CSP AZURE, got %s", result.Response[0].CSP) + got, ok := mock.gotParams.(map[string]string) + if !ok { + t.Fatalf("params = %#v (%T), want map[string]string", mock.gotParams, mock.gotParams) + } + if !reflect.DeepEqual(got, tt.wantParams) { + t.Errorf("params = %#v, want %#v", got, tt.wantParams) + } + }) } } +func cspPtr(c models.CSP) *models.CSP { return &c } + func TestListGroupsEligibility_Success(t *testing.T) { resp := models.GroupsEligibilityResponse{ Response: []models.GroupsEligibleTarget{ diff --git a/internal/sca/wire_contract_test.go b/internal/sca/wire_contract_test.go new file mode 100644 index 0000000..f14286c --- /dev/null +++ b/internal/sca/wire_contract_test.go @@ -0,0 +1,326 @@ +package sca + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "reflect" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/sca/models" +) + +// Wire contracts: what grant actually *sends*. The mocks used to discard the +// route, the body and the query params, so a typo'd path or a nil body was +// invisible to every test in this package. + +func okBody(v interface{}) *http.Response { + b, _ := json.Marshal(v) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(string(b))), + } +} + +func TestElevate_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody(models.ElevateResponse{}), + } + svc := &SCAAccessService{httpClient: mock} + + // Distinguishable values: every field differs from every other so a swap + // or a dropped field cannot be masked. Do not "tidy" these to "test". + req := &models.ElevateRequest{ + CSP: models.CSPAzure, + OrganizationID: "org-elevate-1", + Targets: []models.ElevateTarget{ + {WorkspaceID: "ws-elevate-2", RoleID: "role-elevate-3", RoleName: "Role Elevate Four"}, + }, + } + if _, err := svc.Elevate(t.Context(), req); err != nil { + t.Fatalf("Elevate: %v", err) + } + + if mock.gotRoute != "/api/access/elevate" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/access/elevate") + } + got, ok := mock.gotBody.(*models.ElevateRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.ElevateRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got, req) { + t.Errorf("body = %#v, want %#v", got, req) + } +} + +func TestRevokeSessions_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody(models.RevokeResponse{}), + } + svc := &SCAAccessService{httpClient: mock} + + req := &models.RevokeRequest{SessionIDs: []string{"sess-revoke-1", "sess-revoke-2"}} + if _, err := svc.RevokeSessions(t.Context(), req); err != nil { + t.Fatalf("RevokeSessions: %v", err) + } + + if mock.gotRoute != "/api/access/sessions/revoke" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/access/sessions/revoke") + } + got, ok := mock.gotBody.(*models.RevokeRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.RevokeRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got.SessionIDs, req.SessionIDs) { + t.Errorf("sessionIds = %#v, want %#v", got.SessionIDs, req.SessionIDs) + } +} + +func TestElevateGroups_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody(map[string]interface{}{"response": models.GroupsElevateResponse{}}), + } + svc := &SCAAccessService{httpClient: mock} + + req := &models.GroupsElevateRequest{ + DirectoryID: "dir-groups-1", + CSP: models.CSPAzure, + Targets: []models.GroupsElevateTarget{ + {GroupID: "grp-groups-2"}, + }, + } + if _, err := svc.ElevateGroups(t.Context(), req); err != nil { + t.Fatalf("ElevateGroups: %v", err) + } + + if mock.gotRoute != "/api/access/elevate/groups" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/access/elevate/groups") + } + got, ok := mock.gotBody.(*models.GroupsElevateRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.GroupsElevateRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got, req) { + t.Errorf("body = %#v, want %#v", got, req) + } +} + +func TestGetRoutes_Exact(t *testing.T) { + tests := []struct { + name string + call func(svc *SCAAccessService) error + wantRoute string + }{ + { + name: "list sessions", + call: func(svc *SCAAccessService) error { + _, err := svc.ListSessions(t.Context(), nil) + return err + }, + wantRoute: "/api/access/sessions", + }, + { + name: "list eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListEligibility(t.Context(), models.CSPAzure) + return err + }, + wantRoute: "/api/access/AZURE/eligibility", + }, + { + name: "list groups eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListGroupsEligibility(t.Context(), models.CSPAzure) + return err + }, + wantRoute: "/api/access/AZURE/eligibility/groups", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getFunc: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(`{"response":[],"total":0}`)), + }, nil + }, + } + svc := &SCAAccessService{httpClient: mock} + if err := tt.call(svc); err != nil { + t.Fatalf("call: %v", err) + } + if mock.gotRoute != tt.wantRoute { + t.Errorf("route = %q, want %q", mock.gotRoute, tt.wantRoute) + } + }) + } +} + +// TestListOnDemandResources_ExactQueryParams pins the literal values grant puts +// on the wire for the on-demand role endpoints. It deliberately asserts only the +// values sent: the semantics of pageSize -1 are not documented in this repo, the +// pinned SDK, or CyberArk's published material, so no consequence is claimed. +func TestListOnDemandResources_ExactQueryParams(t *testing.T) { + t.Run("GET search object", func(t *testing.T) { + mock := &mockHTTPClient{ + getResponse: okBody([]models.OnDemandResource{}), + } + svc := &SCAAccessService{httpClient: mock} + + _, err := svc.ListOnDemandResources(t.Context(), models.OnDemandRequest{ + WorkspaceID: "ws-ondemand-1", + PlatformName: "azure_ad", + OrgID: "org-ondemand-2", + }) + if err != nil { + t.Fatalf("ListOnDemandResources: %v", err) + } + + if mock.gotRoute != "/api/cloud/resources/ondemand" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/cloud/resources/ondemand") + } + params, ok := mock.gotParams.(map[string]string) + if !ok { + t.Fatalf("params = %#v (%T), want map[string]string", mock.gotParams, mock.gotParams) + } + var search map[string]interface{} + if err := json.Unmarshal([]byte(params["search"]), &search); err != nil { + t.Fatalf("search param is not JSON: %v (%q)", err, params["search"]) + } + want := map[string]interface{}{ + "workspaceId": "ws-ondemand-1", + "pageNumber": float64(-1), + "pageSize": float64(-1), + "platformName": "azure_ad", + "org_id": "org-ondemand-2", + "target_category": "cloud_console", + } + if !reflect.DeepEqual(search, want) { + t.Errorf("search = %#v, want %#v", search, want) + } + }) + + t.Run("POST body", func(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: okBody([]models.OnDemandResource{}), + } + svc := &SCAAccessService{httpClient: mock} + + _, err := svc.ListOnDemandResources(t.Context(), models.OnDemandRequest{ + WorkspaceID: "ws-ondemand-3", + PlatformName: "azure_resource", + OrgID: "org-ondemand-4", + ResourceType: "management_group", + Ancestors: []string{"/org-ondemand-4", "/mg-ondemand-5"}, + }) + if err != nil { + t.Fatalf("ListOnDemandResources: %v", err) + } + + if mock.gotRoute != "/api/cloud/cloud-roles/ondemand" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/cloud/cloud-roles/ondemand") + } + body, ok := mock.gotBody.(map[string]interface{}) + if !ok { + t.Fatalf("body = %#v (%T), want map[string]interface{}", mock.gotBody, mock.gotBody) + } + want := map[string]interface{}{ + "workspaceId": "ws-ondemand-3", + "resourceType": "management_group", + "pageNumber": -1, + "pageSize": -1, + "platformName": "azure_resource", + "org_id": "org-ondemand-4", + "ancestors": []string{"/org-ondemand-4", "/mg-ondemand-5"}, + "target_category": "cloud_console", + } + if !reflect.DeepEqual(body, want) { + t.Errorf("body = %#v, want %#v", body, want) + } + }) +} + +// TestPaginate_PropagatesContextCancellation covers the ctx argument of the +// paginated GET. Every mock in this package ignores ctx, so replacing it with +// context.Background() otherwise survives untouched. +func TestPaginate_PropagatesContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + mock := &mockHTTPClient{ + getFunc: func(ctx context.Context, _ string, _ interface{}) (*http.Response, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + return okBody(models.SessionsResponse{}), nil + }, + } + svc := &SCAAccessService{httpClient: mock} + + _, err := svc.ListSessions(ctx, nil) + if err == nil { + t.Fatal("expected the canceled context to reach the HTTP client, got nil error") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("error = %v, want context.Canceled", err) + } +} + +// TestPaginate_PropagatesDecodeError covers all three paginated decode closures. +// Swallowing the decode error there would turn a malformed payload into an +// empty, successful result. +func TestPaginate_PropagatesDecodeError(t *testing.T) { + tests := []struct { + name string + call func(svc *SCAAccessService) error + }{ + { + name: "eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListEligibility(t.Context(), models.CSPAzure) + return err + }, + }, + { + name: "sessions", + call: func(svc *SCAAccessService) error { + _, err := svc.ListSessions(t.Context(), nil) + return err + }, + }, + { + name: "groups eligibility", + call: func(svc *SCAAccessService) error { + _, err := svc.ListGroupsEligibility(t.Context(), models.CSPAzure) + return err + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getFunc: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(`{"response": "not-an-array"}`)), + }, nil + }, + } + svc := &SCAAccessService{httpClient: mock} + + err := tt.call(svc) + if err == nil { + t.Fatal("expected a decode error, got nil") + } + if !strings.Contains(err.Error(), "failed to decode") { + t.Errorf("error = %v, want it to name the decode failure", err) + } + }) + } +} diff --git a/internal/workflows/logging_client_test.go b/internal/workflows/logging_client_test.go new file mode 100644 index 0000000..4cb4cdd --- /dev/null +++ b/internal/workflows/logging_client_test.go @@ -0,0 +1,317 @@ +package workflows + +import ( + "context" + "errors" + "fmt" + "net/http" + "strings" + "testing" +) + +// This file mirrors internal/sca/logging_client_test.go. The two logging +// clients are byte-identical, but this one had no test at all while sitting on +// the live path — NewAccessRequestService wraps every UAR request in it. + +type mockLogger struct { + calls []logCall +} + +type logCall struct { + level string + msg string +} + +func (m *mockLogger) Info(msg string, v ...interface{}) { + m.calls = append(m.calls, logCall{level: "info", msg: fmt.Sprintf(msg, v...)}) +} + +func (m *mockLogger) Error(msg string, v ...interface{}) { + m.calls = append(m.calls, logCall{level: "error", msg: fmt.Sprintf(msg, v...)}) +} + +func (m *mockLogger) Debug(msg string, v ...interface{}) { + m.calls = append(m.calls, logCall{level: "debug", msg: fmt.Sprintf(msg, v...)}) +} + +func (m *mockLogger) has(level, substr string) bool { + for _, c := range m.calls { + if c.level == level && strings.Contains(c.msg, substr) { + return true + } + } + return false +} + +func TestLoggingClient_Get(t *testing.T) { + tests := []struct { + name string + route string + resp *http.Response + err error + wantLevel string + wantSubstr string + }{ + { + name: "success logs method route and status", + route: "/api/workflows/requests", + resp: &http.Response{StatusCode: 200, Header: http.Header{}}, + wantLevel: "info", + wantSubstr: "GET /api/workflows/requests -> 200", + }, + { + name: "error logs method route and error", + route: "/api/workflows/requests", + err: errors.New("connection refused"), + wantLevel: "error", + wantSubstr: "GET /api/workflows/requests failed: connection refused", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ml := &mockLogger{} + inner := &mockHTTPClient{getResponses: []*http.Response{tt.resp}, getError: tt.err} + lc := newLoggingClient(inner, ml) + + resp, err := lc.Get(t.Context(), tt.route, nil) + + if tt.err != nil { + if err == nil { + t.Fatal("expected error, got nil") + } + } else { + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if resp.StatusCode != tt.resp.StatusCode { + t.Errorf("expected status %d, got %d", tt.resp.StatusCode, resp.StatusCode) + } + } + + if !ml.has(tt.wantLevel, tt.wantSubstr) { + t.Errorf("expected %s log containing %q, got calls: %v", tt.wantLevel, tt.wantSubstr, ml.calls) + } + }) + } +} + +func TestLoggingClient_Post(t *testing.T) { + tests := []struct { + name string + route string + resp *http.Response + err error + wantLevel string + wantSubstr string + }{ + { + name: "success logs method route and status", + route: "/api/workflows/requests", + resp: &http.Response{StatusCode: 200, Header: http.Header{}}, + wantLevel: "info", + wantSubstr: "POST /api/workflows/requests -> 200", + }, + { + name: "error logs method route and error", + route: "/api/workflows/requests", + err: errors.New("timeout"), + wantLevel: "error", + wantSubstr: "POST /api/workflows/requests failed: timeout", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ml := &mockLogger{} + inner := &mockHTTPClient{postResponse: tt.resp, postError: tt.err} + lc := newLoggingClient(inner, ml) + + resp, err := lc.Post(t.Context(), tt.route, nil) + + if tt.err != nil { + if err == nil { + t.Fatal("expected error, got nil") + } + } else { + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if resp.StatusCode != tt.resp.StatusCode { + t.Errorf("expected status %d, got %d", tt.resp.StatusCode, resp.StatusCode) + } + } + + if !ml.has(tt.wantLevel, tt.wantSubstr) { + t.Errorf("expected %s log containing %q, got calls: %v", tt.wantLevel, tt.wantSubstr, ml.calls) + } + }) + } +} + +// TestLoggingClient_GetUsesGet and its Post twin pin the verb dispatch: routing +// Get through inner.Post would otherwise be invisible. +func TestLoggingClient_GetUsesGet(t *testing.T) { + var gotGet, gotPost bool + inner := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotGet = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + postFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotPost = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + } + lc := newLoggingClient(inner, &mockLogger{}) + + if _, err := lc.Get(t.Context(), "/api/workflows/requests", nil); err != nil { + t.Fatalf("Get: %v", err) + } + if !gotGet { + t.Error("loggingClient.Get did not call inner.Get") + } + if gotPost { + t.Error("loggingClient.Get called inner.Post") + } +} + +func TestLoggingClient_PostUsesPost(t *testing.T) { + var gotGet, gotPost bool + inner := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotGet = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + postFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + gotPost = true + return &http.Response{StatusCode: 200, Header: http.Header{}}, nil + }, + } + lc := newLoggingClient(inner, &mockLogger{}) + + if _, err := lc.Post(t.Context(), "/api/workflows/requests", nil); err != nil { + t.Fatalf("Post: %v", err) + } + if !gotPost { + t.Error("loggingClient.Post did not call inner.Post") + } + if gotGet { + t.Error("loggingClient.Post called inner.Get") + } +} + +// TestLoggingClient_PropagatesInnerError guards against the decorator +// swallowing the inner error and manufacturing a success. +func TestLoggingClient_PropagatesInnerError(t *testing.T) { + sentinel := errors.New("inner transport failure") + + t.Run("get", func(t *testing.T) { + lc := newLoggingClient(&mockHTTPClient{getError: sentinel}, &mockLogger{}) + resp, err := lc.Get(t.Context(), "/api/workflows/requests", nil) + if !errors.Is(err, sentinel) { + t.Errorf("error = %v, want the inner error", err) + } + if resp != nil { + t.Errorf("response = %#v, want nil on error", resp) + } + }) + + t.Run("post", func(t *testing.T) { + lc := newLoggingClient(&mockHTTPClient{postError: sentinel}, &mockLogger{}) + resp, err := lc.Post(t.Context(), "/api/workflows/requests", nil) + if !errors.Is(err, sentinel) { + t.Errorf("error = %v, want the inner error", err) + } + if resp != nil { + t.Errorf("response = %#v, want nil on error", resp) + } + }) +} + +func TestLoggingClient_LogsDuration(t *testing.T) { + ml := &mockLogger{} + inner := &mockHTTPClient{getResponses: []*http.Response{{StatusCode: 200, Header: http.Header{}}}} + lc := newLoggingClient(inner, ml) + + _, _ = lc.Get(t.Context(), "/api/workflows/requests", nil) + + if !ml.has("info", "ms)") { + t.Errorf("expected info log containing duration in ms, got calls: %v", ml.calls) + } +} + +// TestLoggingClient_DebugLogsHeaders is the token-leak guard: response headers +// go to the debug log verbatim except Authorization, which must be redacted. +func TestLoggingClient_DebugLogsHeaders(t *testing.T) { + ml := &mockLogger{} + h := http.Header{} + h.Set("Content-Type", "application/json") + h.Set("Authorization", "Bearer eyJhbGci.secret.token") + inner := &mockHTTPClient{getResponses: []*http.Response{{StatusCode: 200, Header: h}}} + lc := newLoggingClient(inner, ml) + + _, _ = lc.Get(t.Context(), "/api/workflows/requests", nil) + + var debugCall *logCall + for i, c := range ml.calls { + if c.level == "debug" { + debugCall = &ml.calls[i] + break + } + } + if debugCall == nil { + t.Fatal("expected debug log for response headers") + } + if strings.Contains(debugCall.msg, "eyJhbGci") { + t.Error("expected Authorization header to be redacted, but found token value") + } + if !strings.Contains(debugCall.msg, "[REDACTED]") { + t.Error("expected [REDACTED] in debug log for Authorization header") + } + if !strings.Contains(debugCall.msg, "application/json") { + t.Error("expected Content-Type header in debug log") + } +} + +func TestRedactHeaders(t *testing.T) { + tests := []struct { + name string + headers http.Header + wantHas string + wantNot string + }{ + { + name: "redacts Authorization", + headers: func() http.Header { + h := http.Header{} + h.Set("Authorization", "Bearer eyJtoken123") + return h + }(), + wantHas: "Bearer [REDACTED]", + wantNot: "eyJtoken123", + }, + { + name: "preserves other headers", + headers: func() http.Header { + h := http.Header{} + h.Set("Content-Type", "application/json") + return h + }(), + wantHas: "application/json", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + str := fmt.Sprintf("%v", redactHeaders(tt.headers)) + + if tt.wantHas != "" && !strings.Contains(str, tt.wantHas) { + t.Errorf("expected %q in result %q", tt.wantHas, str) + } + if tt.wantNot != "" && strings.Contains(str, tt.wantNot) { + t.Errorf("did not expect %q in result %q", tt.wantNot, str) + } + }) + } +} diff --git a/internal/workflows/models/wire_tags_test.go b/internal/workflows/models/wire_tags_test.go new file mode 100644 index 0000000..1e61960 --- /dev/null +++ b/internal/workflows/models/wire_tags_test.go @@ -0,0 +1,203 @@ +package models + +import ( + "encoding/json" + "testing" +) + +// These pin the literal JSON keys of the three request bodies grant sends to +// the UAR API. Round-tripping through the same Go struct cannot catch a renamed +// tag, so each test inspects the marshaled keys directly. + +func TestSubmitAccessRequest_JSONTags(t *testing.T) { + t.Parallel() + + b, err := json.Marshal(SubmitAccessRequest{ + TargetCategory: "CLOUD_CONSOLE", + RequestDetails: map[string]interface{}{"reason": "need access to prod"}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + for _, key := range []string{"targetCategory", "requestDetails"} { + if _, ok := raw[key]; !ok { + t.Errorf("submit body is missing key %q: %s", key, b) + } + } + if string(raw["targetCategory"]) != `"CLOUD_CONSOLE"` { + t.Errorf("targetCategory = %s, want \"CLOUD_CONSOLE\"", raw["targetCategory"]) + } + if string(raw["requestDetails"]) != `{"reason":"need access to prod"}` { + t.Errorf("requestDetails = %s", raw["requestDetails"]) + } +} + +func TestFinalizeAccessRequest_JSONTags(t *testing.T) { + t.Parallel() + + reason := "approved after review" + b, err := json.Marshal(FinalizeAccessRequest{Result: "APPROVED", FinalizationReason: &reason}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if string(raw["result"]) != `"APPROVED"` { + t.Errorf("result = %s, want \"APPROVED\" under key \"result\": %s", raw["result"], b) + } + if string(raw["finalizationReason"]) != `"approved after review"` { + t.Errorf("finalizationReason = %s: %s", raw["finalizationReason"], b) + } + + // omitempty: a nil reason must not appear at all. + b, err = json.Marshal(FinalizeAccessRequest{Result: "REJECTED"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + raw = nil + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if _, ok := raw["finalizationReason"]; ok { + t.Errorf("finalizationReason present for a nil reason: %s", b) + } +} + +func TestCancelAccessRequest_JSONTags(t *testing.T) { + t.Parallel() + + reason := "no longer needed" + b, err := json.Marshal(CancelAccessRequest{CancelReason: &reason}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(b, &raw); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if string(raw["cancelReason"]) != `"no longer needed"` { + t.Errorf("cancelReason = %s: %s", raw["cancelReason"], b) + } +} + +// The response models below were previously unpinned in both directions: every +// test decoded a body produced by marshaling the same Go struct, so a renamed +// tag round-tripped trivially while `grant request get` / `grant request list` +// rendered blanks. These pin the keys that reach the user's screen. The +// validation metadata on form.go is still unpinned — see ledger row WF-25. + +// TestAccessRequest_DecodesPopulatedResponse pins the response keys that +// `grant request get` renders, decoding a literal wire body rather than a +// marshaled struct. +func TestAccessRequest_DecodesPopulatedResponse(t *testing.T) { + t.Parallel() + + // Distinguishable values: no two keys share a value, so a swap cannot be + // masked. Do not "tidy" these. + const wire = `{ + "requestId": "req-decode-1", + "targetCategory": "CLOUD_CONSOLE", + "requestState": "PENDING", + "requestResult": "UNKNOWN", + "requestDetails": {"reason": "reason-decode-2"}, + "requestApprovers": [ + {"approver": {"entityId": "ent-decode-3", "entityName": "approver-decode-4"}, "result": "APPROVED"} + ], + "requester": {"entityId": "ent-decode-5", "entityName": "requester-decode-6"}, + "createdBy": "createdby-decode-7", + "createdAt": "2025-08-12T09:41:00", + "updatedBy": "updatedby-decode-8", + "updatedAt": "2025-08-12T17:41:00" + }` + + var req AccessRequest + if err := json.Unmarshal([]byte(wire), &req); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if req.RequestID != "req-decode-1" { + t.Errorf("requestId = %q, want %q", req.RequestID, "req-decode-1") + } + if req.TargetCategory != "CLOUD_CONSOLE" { + t.Errorf("targetCategory = %q, want %q", req.TargetCategory, "CLOUD_CONSOLE") + } + if req.RequestState != RequestStatePending { + t.Errorf("requestState = %q, want %q", req.RequestState, RequestStatePending) + } + if req.RequestResult != RequestResultUnknown { + t.Errorf("requestResult = %q, want %q", req.RequestResult, RequestResultUnknown) + } + if got := req.DetailString("reason"); got != "reason-decode-2" { + t.Errorf("requestDetails[reason] = %q, want %q", got, "reason-decode-2") + } + if req.Requester == nil { + t.Fatal("requester = nil, want the decoded entity") + } + if req.Requester.EntityName != "requester-decode-6" { + t.Errorf("requester.entityName = %q, want %q", req.Requester.EntityName, "requester-decode-6") + } + if req.Requester.EntityID != "ent-decode-5" { + t.Errorf("requester.entityId = %q, want %q", req.Requester.EntityID, "ent-decode-5") + } + if len(req.RequestApprovers) != 1 { + t.Fatalf("requestApprovers len = %d, want 1", len(req.RequestApprovers)) + } + if req.RequestApprovers[0].Result != RequestResultApproved { + t.Errorf("requestApprovers[0].result = %q, want %q", req.RequestApprovers[0].Result, RequestResultApproved) + } + if req.RequestApprovers[0].Approver.EntityName != "approver-decode-4" { + t.Errorf("requestApprovers[0].approver.entityName = %q, want %q", + req.RequestApprovers[0].Approver.EntityName, "approver-decode-4") + } + if req.CreatedBy != "createdby-decode-7" { + t.Errorf("createdBy = %q, want %q", req.CreatedBy, "createdby-decode-7") + } + if req.CreatedAt != "2025-08-12T09:41:00" { + t.Errorf("createdAt = %q, want %q", req.CreatedAt, "2025-08-12T09:41:00") + } + if req.UpdatedBy != "updatedby-decode-8" { + t.Errorf("updatedBy = %q, want %q", req.UpdatedBy, "updatedby-decode-8") + } + if req.UpdatedAt != "2025-08-12T17:41:00" { + t.Errorf("updatedAt = %q, want %q", req.UpdatedAt, "2025-08-12T17:41:00") + } +} + +// TestListRequestsResponse_DecodesPopulatedPage pins the pagination envelope. +// count and totalCount drive both the pagination loop and what the user is told +// about how many requests exist. +func TestListRequestsResponse_DecodesPopulatedPage(t *testing.T) { + t.Parallel() + + const wire = `{ + "items": [{"requestId": "req-page-1"}, {"requestId": "req-page-2"}], + "count": 2, + "totalCount": 7 + }` + + var page ListRequestsResponse + if err := json.Unmarshal([]byte(wire), &page); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(page.Items) != 2 { + t.Fatalf("items len = %d, want 2", len(page.Items)) + } + if page.Items[0].RequestID != "req-page-1" { + t.Errorf("items[0].requestId = %q, want %q", page.Items[0].RequestID, "req-page-1") + } + if page.Count != 2 { + t.Errorf("count = %d, want 2", page.Count) + } + if page.TotalCount != 7 { + t.Errorf("totalCount = %d, want 7", page.TotalCount) + } +} diff --git a/internal/workflows/service_config_test.go b/internal/workflows/service_config_test.go new file mode 100644 index 0000000..69610bc --- /dev/null +++ b/internal/workflows/service_config_test.go @@ -0,0 +1,85 @@ +package workflows + +import ( + "testing" + + "github.com/cyberark/idsec-sdk-golang/pkg/common/isp" +) + +func TestServiceConfig_ServiceName(t *testing.T) { + config := ServiceConfig() + expected := "access-requests" + if config.ServiceName != expected { + t.Errorf("expected ServiceName %q, got %q", expected, config.ServiceName) + } +} + +func TestServiceConfig_RequiredAuthenticators(t *testing.T) { + config := ServiceConfig() + if len(config.RequiredAuthenticatorNames) != 1 { + t.Fatalf("expected 1 required authenticator, got %d", len(config.RequiredAuthenticatorNames)) + } + expected := "isp" + if config.RequiredAuthenticatorNames[0] != expected { + t.Errorf("expected required authenticator %q, got %q", expected, config.RequiredAuthenticatorNames[0]) + } +} + +func TestServiceConfig_OptionalAuthenticators(t *testing.T) { + config := ServiceConfig() + if len(config.OptionalAuthenticatorNames) != 0 { + t.Errorf("expected 0 optional authenticators, got %d", len(config.OptionalAuthenticatorNames)) + } +} + +func TestServiceConfig_ActionsConfigurations(t *testing.T) { + config := ServiceConfig() + if config.ActionsConfigurations != nil { + t.Errorf("expected nil ActionsConfigurations, got %v", config.ActionsConfigurations) + } +} + +// TestNewAccessRequestService_ResolvesISPAuthenticator pins the authenticator +// name the constructor looks up. Asking for anything other than "isp" makes the +// constructor fail outright. +func TestNewAccessRequestService_ResolvesISPAuthenticator(t *testing.T) { + svc, err := NewAccessRequestService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewAccessRequestService: %v", err) + } + if svc.ispAuth == nil { + t.Error("ispAuth = nil, want the resolved ISP authenticator") + } +} + +// TestNewAccessRequestService_UsesUARServiceSlug pins the "uar" service slug +// passed to isp.FromISPAuth. The retry test overwrites BaseURL before issuing a +// request, so without this the slug could be changed to anything and no test in +// the repo would notice — while every live request would go to the wrong host. +func TestNewAccessRequestService_UsesUARServiceSlug(t *testing.T) { + svc, err := NewAccessRequestService(ispAuthWithToken(t)) + if err != nil { + t.Fatalf("NewAccessRequestService: %v", err) + } + + // The fake JWT carries subdomain=testtenant, platform_domain=example.test. + const want = "https://testtenant.uar.example.test" + if got := ispClientFromService(t, svc).BaseURL; got != want { + t.Errorf("BaseURL = %q, want %q", got, want) + } +} + +// ispClientFromService reaches through the logging decorator to the SDK client +// the constructor actually published. +func ispClientFromService(t *testing.T, svc *AccessRequestService) *isp.IdsecISPServiceClient { + t.Helper() + lc, ok := svc.httpClient.(*loggingClient) + if !ok { + t.Fatalf("httpClient is %T, want *loggingClient", svc.httpClient) + } + client, ok := lc.inner.(*isp.IdsecISPServiceClient) + if !ok { + t.Fatalf("inner client is %T, want *isp.IdsecISPServiceClient", lc.inner) + } + return client +} diff --git a/internal/workflows/service_test.go b/internal/workflows/service_test.go index c696005..150976f 100644 --- a/internal/workflows/service_test.go +++ b/internal/workflows/service_test.go @@ -15,14 +15,51 @@ import ( type mockHTTPClient struct { getFn func(ctx context.Context, route string, params interface{}) (*http.Response, error) postFn func(ctx context.Context, route string, body interface{}) (*http.Response, error) + + // Fallbacks used when the corresponding Fn is nil. getResponse is consumed + // once per call: ListRequests paginates, and an *http.Response returned a + // second time has an already-drained Body, which surfaces as a confusing + // "failed to decode" instead of a clear assertion failure. Set getResponses + // to queue one response per page. + getResponses []*http.Response + getError error + postResponse *http.Response + postError error + + // Recorded arguments of the most recent call, populated before dispatch so + // wire-contract tests can assert what was actually sent. + gotRoute string + gotBody interface{} + gotParams interface{} } func (m *mockHTTPClient) Get(ctx context.Context, route string, params interface{}) (*http.Response, error) { - return m.getFn(ctx, route, params) + m.gotRoute = route + m.gotParams = params + if m.getFn != nil { + return m.getFn(ctx, route, params) + } + if m.getError != nil { + return nil, m.getError + } + if len(m.getResponses) == 0 { + return nil, errors.New("mockHTTPClient: Get called with no queued response left") + } + resp := m.getResponses[0] + m.getResponses = m.getResponses[1:] + return resp, nil } func (m *mockHTTPClient) Post(ctx context.Context, route string, body interface{}) (*http.Response, error) { - return m.postFn(ctx, route, body) + m.gotRoute = route + m.gotBody = body + if m.postFn != nil { + return m.postFn(ctx, route, body) + } + if m.postError != nil { + return nil, m.postError + } + return m.postResponse, nil } func jsonResponse(status int, body interface{}) *http.Response { diff --git a/internal/workflows/wire_contract_test.go b/internal/workflows/wire_contract_test.go new file mode 100644 index 0000000..4195396 --- /dev/null +++ b/internal/workflows/wire_contract_test.go @@ -0,0 +1,321 @@ +package workflows + +import ( + "context" + "errors" + "io" + "net/http" + "reflect" + "strings" + "testing" + + "github.com/aaearon/grant-cli/internal/workflows/models" +) + +// errorResponse builds a non-200 whose body still decodes cleanly into every +// response model. That is deliberate: if the checkResponse guard is removed, +// the operation succeeds and returns an empty value rather than failing, which +// is exactly the escape this table exists to catch. +func errorResponse(status int) *http.Response { + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(`{"message":"internal server error"}`)), + Header: make(http.Header), + } +} + +// TestWorkflows_Non200 drives a 500 through all six checkResponse call sites. +// TestCheckResponse_Error only calls the helper directly, which proves it +// formats an error but not that any operation invokes it — so deleting any of +// the six guards survived. The user-visible consequence at the submit site: a +// 500 decodes as an empty AccessRequest and `grant request submit` prints a +// blank request and exits 0. +func TestWorkflows_Non200(t *testing.T) { + tests := []struct { + name string + call func(svc *AccessRequestService) error + wantOperation string + }{ + { + name: "request forms", + call: func(svc *AccessRequestService) error { + _, err := svc.GetRequestForms(t.Context(), "CLOUD_CONSOLE", "ON_DEMAND") + return err + }, + wantOperation: "request forms", + }, + { + name: "list requests", + call: func(svc *AccessRequestService) error { + _, _, err := svc.ListRequests(t.Context(), ListRequestsParams{}) + return err + }, + wantOperation: "list requests", + }, + { + name: "get request", + call: func(svc *AccessRequestService) error { + _, err := svc.GetRequest(t.Context(), "req-500") + return err + }, + wantOperation: "get request", + }, + { + name: "submit request", + call: func(svc *AccessRequestService) error { + _, err := svc.SubmitRequest(t.Context(), &models.SubmitAccessRequest{TargetCategory: "CLOUD_CONSOLE"}) + return err + }, + wantOperation: "submit request", + }, + { + name: "cancel request", + call: func(svc *AccessRequestService) error { + _, err := svc.CancelRequest(t.Context(), "req-500", nil) + return err + }, + wantOperation: "cancel request", + }, + { + name: "finalize request", + call: func(svc *AccessRequestService) error { + _, err := svc.FinalizeRequest(t.Context(), "req-500", "APPROVED", nil) + return err + }, + wantOperation: "finalize request", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return errorResponse(http.StatusInternalServerError), nil + }, + postFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return errorResponse(http.StatusInternalServerError), nil + }, + } + svc := NewAccessRequestServiceWithClient(mock) + + err := tt.call(svc) + if err == nil { + t.Fatalf("%s returned nil error on HTTP 500", tt.wantOperation) + } + if !strings.Contains(err.Error(), tt.wantOperation) { + t.Errorf("error = %v, want it to name the operation %q", err, tt.wantOperation) + } + if !strings.Contains(err.Error(), "500") { + t.Errorf("error = %v, want it to carry the HTTP status", err) + } + }) + } +} + +// TestFinalizeRequest_ExactRouteAndReason supersedes the HasSuffix("/finalize") +// check in service_test.go, which let the request ID drop out of the route and +// let the approver's reason be dropped from the body. That looser check is still +// present there; the redundancy is harmless. The cancel twin already asserts both. +func TestFinalizeRequest_ExactRouteAndReason(t *testing.T) { + tests := []struct { + name string + result string + reason *string + }{ + {name: "approve with reason", result: "APPROVED", reason: strPtr("approved because it is justified")}, + {name: "reject with reason", result: "REJECTED", reason: strPtr("rejected because scope is too broad")}, + {name: "no reason", result: "APPROVED", reason: nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: jsonResponse(200, models.AccessRequest{RequestID: "req-finalize-1"}), + } + svc := NewAccessRequestServiceWithClient(mock) + + if _, err := svc.FinalizeRequest(t.Context(), "req-finalize-1", tt.result, tt.reason); err != nil { + t.Fatalf("FinalizeRequest: %v", err) + } + + const wantRoute = "/api/workflows/requests/req-finalize-1/finalize" + if mock.gotRoute != wantRoute { + t.Errorf("route = %q, want %q", mock.gotRoute, wantRoute) + } + body, ok := mock.gotBody.(*models.FinalizeAccessRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.FinalizeAccessRequest", mock.gotBody, mock.gotBody) + } + if body.Result != tt.result { + t.Errorf("result = %q, want %q", body.Result, tt.result) + } + switch { + case tt.reason == nil && body.FinalizationReason != nil: + t.Errorf("finalizationReason = %q, want nil", *body.FinalizationReason) + case tt.reason != nil && body.FinalizationReason == nil: + t.Errorf("finalizationReason = nil, want %q", *tt.reason) + case tt.reason != nil && *body.FinalizationReason != *tt.reason: + t.Errorf("finalizationReason = %q, want %q", *body.FinalizationReason, *tt.reason) + } + }) + } +} + +func strPtr(s string) *string { return &s } + +// TestListRequests_SendsLimit pins the route and the limit query parameter. +// Offset pagination is already well covered; limit was never asserted, so +// deleting it, or changing defaultPageSize, went unnoticed. The route was the +// only unpinned route in either service — the mock already recorded it and no +// test looked, so `grant request list` could have been pointed anywhere. +func TestListRequests_SendsLimit(t *testing.T) { + tests := []struct { + name string + params ListRequestsParams + wantLimit string + }{ + {name: "no limit uses defaultPageSize", params: ListRequestsParams{}, wantLimit: "50"}, + {name: "zero limit uses defaultPageSize", params: ListRequestsParams{Limit: 0}, wantLimit: "50"}, + {name: "negative limit uses defaultPageSize", params: ListRequestsParams{Limit: -3}, wantLimit: "50"}, + {name: "caller limit is sent verbatim", params: ListRequestsParams{Limit: 7}, wantLimit: "7"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockHTTPClient{ + getResponses: []*http.Response{jsonResponse(200, models.ListRequestsResponse{ + Items: []models.AccessRequest{{RequestID: "id-1"}}, Count: 1, TotalCount: 1, + })}, + } + svc := NewAccessRequestServiceWithClient(mock) + + if _, _, err := svc.ListRequests(t.Context(), tt.params); err != nil { + t.Fatalf("ListRequests: %v", err) + } + + if mock.gotRoute != "/api/workflows/requests" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/workflows/requests") + } + qp, ok := mock.gotParams.(map[string]string) + if !ok { + t.Fatalf("params = %#v (%T), want map[string]string", mock.gotParams, mock.gotParams) + } + if got := qp["limit"]; got != tt.wantLimit { + t.Errorf("limit = %q, want %q", got, tt.wantLimit) + } + }) + } +} + +// TestListRequests_PropagatesContextCancellation covers the ctx argument of the +// pagination loop's GET; every mock ignores ctx, so swapping it for +// context.Background() otherwise survives. +func TestListRequests_PropagatesContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + mock := &mockHTTPClient{ + getFn: func(ctx context.Context, _ string, _ interface{}) (*http.Response, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + return jsonResponse(200, models.ListRequestsResponse{}), nil + }, + } + svc := NewAccessRequestServiceWithClient(mock) + + _, _, err := svc.ListRequests(ctx, ListRequestsParams{}) + if err == nil { + t.Fatal("expected the canceled context to reach the HTTP client, got nil error") + } + if !errors.Is(err, context.Canceled) { + t.Errorf("error = %v, want context.Canceled", err) + } +} + +// TestGetRequest_PropagatesDecodeError guards the decode error return in +// GetRequest: swallowing it would return a blank request as a success. +func TestGetRequest_PropagatesDecodeError(t *testing.T) { + mock := &mockHTTPClient{ + getFn: func(_ context.Context, _ string, _ interface{}) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(`{"requestId": 12345}`)), + Header: make(http.Header), + }, nil + }, + } + svc := NewAccessRequestServiceWithClient(mock) + + result, err := svc.GetRequest(t.Context(), "req-decode-1") + if err == nil { + t.Fatalf("expected a decode error, got nil (result = %#v)", result) + } + if !strings.Contains(err.Error(), "failed to decode") { + t.Errorf("error = %v, want it to name the decode failure", err) + } +} + +// TestGetRequestForms_SendsExactParams pins the query params for the forms +// endpoint alongside its route. +func TestGetRequestForms_SendsExactParams(t *testing.T) { + mock := &mockHTTPClient{ + getResponses: []*http.Response{jsonResponse(200, models.RequestFormResponse{})}, + } + svc := NewAccessRequestServiceWithClient(mock) + + if _, err := svc.GetRequestForms(t.Context(), "CLOUD_CONSOLE", "ON_DEMAND"); err != nil { + t.Fatalf("GetRequestForms: %v", err) + } + if mock.gotRoute != "/api/workflows/request-forms" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/workflows/request-forms") + } + want := map[string]string{"targetCategory": "CLOUD_CONSOLE", "requestType": "ON_DEMAND"} + if !reflect.DeepEqual(mock.gotParams, want) { + t.Errorf("params = %#v, want %#v", mock.gotParams, want) + } +} + +// TestSubmitRequest_SendsExactRouteAndBody pins the submit wire contract. +// TestSubmitRequest asserts only TargetCategory, so RequestDetails — reason, +// role, target, dates, priority, i.e. the entire substance of +// `grant request submit` — could be dropped at the service boundary and every +// test still passed. Mirrors TestElevate_SendsExactRouteAndBody in internal/sca. +func TestSubmitRequest_SendsExactRouteAndBody(t *testing.T) { + mock := &mockHTTPClient{ + postResponse: jsonResponse(200, models.AccessRequest{RequestID: "req-submit-1"}), + } + svc := NewAccessRequestServiceWithClient(mock) + + // Distinguishable values: no two keys share a value, so a swap cannot be + // masked. Do not "tidy" these to "test". + req := &models.SubmitAccessRequest{ + TargetCategory: "CLOUD_CONSOLE", + RequestDetails: map[string]interface{}{ + "reason": "reason-submit-2", + "roleId": "role-id-submit-3", + "roleName": "Role Name Submit Four", + "targetId": "target-submit-5", + "priority": "priority-submit-6", + "startDate": "2025-08-12T09:41:00", + "endDate": "2025-08-12T17:41:00", + "timezone": "timezone-submit-7", + "provider": "provider-submit-8", + "workspaceId": "ws-submit-9", + }, + } + if _, err := svc.SubmitRequest(t.Context(), req); err != nil { + t.Fatalf("SubmitRequest: %v", err) + } + + if mock.gotRoute != "/api/workflows/requests" { + t.Errorf("route = %q, want %q", mock.gotRoute, "/api/workflows/requests") + } + got, ok := mock.gotBody.(*models.SubmitAccessRequest) + if !ok { + t.Fatalf("body = %#v (%T), want *models.SubmitAccessRequest", mock.gotBody, mock.gotBody) + } + if !reflect.DeepEqual(got, req) { + t.Errorf("body = %#v, want %#v", got, req) + } +}