diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..89e6c3a --- /dev/null +++ b/.dockerignore @@ -0,0 +1,11 @@ +**/bin/ +**/obj/ +.git/ +.github/ +tests/ +deploy/ +*.md +src/z-logs/ +src/Simulators/ +src/docker-compose.yml +src/Dockerfile diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 0000000..ba5496e --- /dev/null +++ b/.editorconfig @@ -0,0 +1,60 @@ +root = true + +[*] +charset = utf-8 +end_of_line = lf +insert_final_newline = true +trim_trailing_whitespace = true +indent_style = space +indent_size = 4 + +[*.{json,yml,yaml,props,csproj,slnx,xml}] +indent_size = 2 + +[*.md] +trim_trailing_whitespace = false + +[*.cs] +# usings: System first, then alphabetical, outside the file-scoped namespace +dotnet_sort_system_directives_first = true +dotnet_separate_import_directive_groups = false +csharp_using_directive_placement = outside_namespace:error +dotnet_diagnostic.IDE0005.severity = error +dotnet_diagnostic.IDE0065.severity = error + +csharp_style_namespace_declarations = file_scoped:error +dotnet_diagnostic.IDE0161.severity = error + +# house style: "this." on every field access, and only on fields +dotnet_style_qualification_for_field = true:error +dotnet_style_qualification_for_property = false +dotnet_style_qualification_for_method = false +dotnet_style_qualification_for_event = false +dotnet_diagnostic.IDE0009.severity = error + +dotnet_style_readonly_field = true:error +dotnet_diagnostic.IDE0044.severity = error + +csharp_prefer_braces = true:error +dotnet_diagnostic.IDE0011.severity = error + +# private static readonly fields are PascalCase, like constants +dotnet_naming_rule.static_readonly_fields_pascal_case.severity = error +dotnet_naming_rule.static_readonly_fields_pascal_case.symbols = static_readonly_fields +dotnet_naming_rule.static_readonly_fields_pascal_case.style = pascal_case +dotnet_naming_symbols.static_readonly_fields.applicable_kinds = field +dotnet_naming_symbols.static_readonly_fields.required_modifiers = static, readonly +dotnet_naming_style.pascal_case.capitalization = pascal_case + +# private instance fields are camelCase without an underscore prefix +dotnet_naming_rule.private_fields_camel_case.severity = error +dotnet_naming_rule.private_fields_camel_case.symbols = private_fields +dotnet_naming_rule.private_fields_camel_case.style = camel_case +dotnet_naming_symbols.private_fields.applicable_kinds = field +dotnet_naming_symbols.private_fields.applicable_accessibilities = private +dotnet_naming_symbols.private_fields.required_modifiers = readonly +dotnet_naming_style.camel_case.capitalization = camel_case + +[tests/**.cs] +# test names read as Method_Scenario_Expectation +dotnet_diagnostic.CA1707.severity = none diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..d0d6371 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,2 @@ +* text=auto eol=lf +*.png binary diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..47fcf6a --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,25 @@ +version: 2 +updates: + # versions live in Directory.Packages.props; one grouped PR a week keeps the noise down + - package-ecosystem: nuget + directory: / + schedule: + interval: weekly + groups: + nuget: + patterns: ["*"] + + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + + - package-ecosystem: docker + directory: /src + schedule: + interval: weekly + + - package-ecosystem: docker-compose + directory: /src + schedule: + interval: weekly diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..e3803f6 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,98 @@ +name: CI + +on: + push: + branches: [main] + pull_request: + workflow_dispatch: + +permissions: + contents: read + +concurrency: + group: ci-${{ github.ref }} + cancel-in-progress: true + +env: + DOTNET_NOLOGO: true + DOTNET_CLI_TELEMETRY_OPTOUT: true + +jobs: + build-and-test: + name: Build, format, unit + integration tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-dotnet@v6 + with: + global-json-file: global.json + + - name: Restore + run: dotnet restore PaymentGatewayApi.slnx + + # analyzers and warnings-as-errors come from Directory.Build.props + - name: Build + run: dotnet build PaymentGatewayApi.slnx --no-restore --configuration Release + + - name: Check formatting + run: dotnet format PaymentGatewayApi.slnx --no-restore --verify-no-changes + + - name: Check for vulnerable packages + run: | + dotnet list PaymentGatewayApi.slnx package --vulnerable --include-transitive | tee vulnerable.txt + if grep -q "has the following vulnerable packages" vulnerable.txt; then exit 1; fi + + - name: Unit tests + run: > + dotnet test --project tests/PaymentGatewayApiUnitTests --no-build --configuration Release + --coverage --coverage-output-format cobertura --coverage-output unit.cobertura.xml + --coverage-settings coverage.settings.xml + + # DynamoDB Local runs in a Testcontainers container, the runner's Docker is enough + - name: Integration tests + run: > + dotnet test --project tests/PaymentGatewayApiIntegrationTests --no-build --configuration Release + --coverage --coverage-output-format cobertura --coverage-output integration.cobertura.xml + --coverage-settings coverage.settings.xml + + - name: Coverage report + if: ${{ !cancelled() }} + uses: danielpalme/ReportGenerator-GitHub-Action@5.5.11 + with: + reports: tests/**/TestResults/*.cobertura.xml + targetdir: coverage + reporttypes: MarkdownSummaryGithub;Cobertura;HtmlInline + + - name: Publish coverage summary + if: ${{ !cancelled() }} + run: cat coverage/SummaryGithub.md >> "$GITHUB_STEP_SUMMARY" + + - uses: actions/upload-artifact@v7 + if: ${{ !cancelled() }} + with: + name: coverage + path: coverage + + end-to-end: + name: End-to-end tests against the compose stack + needs: build-and-test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-dotnet@v6 + with: + global-json-file: global.json + + - name: Start DynamoDB Local, the bank simulator and the api container + working-directory: src + run: docker compose --profile api up --detach --build --wait + + - name: End-to-end tests + run: dotnet test --project tests/PaymentGatewayApiEndToEndTests + + - name: Stack logs + if: ${{ failure() }} + working-directory: src + run: docker compose --profile api logs diff --git a/.gitignore b/.gitignore index 197aae9..db8d000 100644 --- a/.gitignore +++ b/.gitignore @@ -7,5 +7,4 @@ obj/ src/z-logs/*.log -deploy/aws/* -!deploy/aws/.gitkeep +TestResults/ diff --git a/AGENTS.md b/AGENTS.md index 7bbc7a2..ea355c1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,6 +1,7 @@ -# CLAUDE.md +# AGENTS.md -This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +Guidance for coding agents (and people) working in this repository. `README.md` is the +overview for humans: what the gateway does, its API and its design decisions. ## Commands @@ -11,18 +12,26 @@ The API cannot run without its two local dependencies. Start them first, **from cd src && docker compose up -d ``` -That brings up DynamoDB Local on `:8000`, creates the `Payments` table, and starts the -WireMock acquiring-bank simulator on `:8080`. Add `--profile api` to also build and run the -API container on `:5067` (don't combine it with `dotnet run`, they share the port). +That brings up DynamoDB Local on `:8000`, creates the `Payments` and `PaymentIdempotencyKeys` +tables, and starts the WireMock acquiring-bank simulator on `:8080`. Add `--profile api` to +also build and run the API container on `:5067` (don't combine it with `dotnet run`, they +share the port). `dotnet run` needs some AWS credentials in the environment +(`AWS_ACCESS_KEY_ID` / `AWS_SECRET_ACCESS_KEY`, any value): DynamoDB Local ignores them, but +the SDK refuses to sign without them. The compose file sets dummy ones for the container. ```bash dotnet build PaymentGatewayApi.slnx # from repo root -dotnet run --project src/PaymentGatewayApi.csproj # http://localhost:5067, Swagger at /swagger -docker build -t payment-gateway-api src/ # container image +dotnet run --project src/PaymentGatewayApi.csproj # http://localhost:5067, Swagger UI at /swagger (Development only) +docker build -f src/Dockerfile -t payment-gateway-api . # container image, from repo root ``` -`dotnet run` sets the working directory to `src/`, which is what makes `logconfig.json` and -the `z-logs/` output path resolve. +`dotnet run` sets the working directory to `src/`, which is what makes the Development +`z-logs/` output path resolve. `/health/live` answers when the process is up, `/health/ready` +also checks that both tables are reachable. The OpenAPI document (`/openapi/v1.json`, +from `Microsoft.AspNetCore.OpenApi`) and Swagger UI are only mapped in Development. + +The container image is `aspnet:10.0-noble-chiseled` running as its non-root user: it has no +shell or curl, so there is no `HEALTHCHECK`; probe the health endpoints over HTTP. ### Tests @@ -35,71 +44,156 @@ dotnet test --project tests/PaymentGatewayApiUnitTests dotnet run --project tests/PaymentGatewayApiUnitTests # equivalent, xUnit v3 projects are executables ``` -Target the test project explicitly: a bare `dotnet test` (or `--solution`) fails because the -web project's `Microsoft.NET.Test.Sdk` reference makes it look like a VSTest project. +A bare `dotnet test` from the repository root runs all three suites, including the E2E one, +which fails unless the compose stack is up; target a project to run one suite. + +`BankClient` is unit-tested through a `StubHttpMessageHandler`. `PaymentsRepository` is only +mocked in unit tests; the integration tests exercise it against real DynamoDB Local. -`BankHttpClient` and `PaymentsRepository` are deliberately not unit-tested; they are only -mocked through their interfaces. Tests commented `// NOTE: defect` pin current, known-wrong -behaviour — update them when the defect is fixed. +Integration tests live in `tests/PaymentGatewayApiIntegrationTests`: the whole API in process +through `WebApplicationFactory` (environment `Testing`), DynamoDB Local started by +Testcontainers (so Docker must be running, nothing else), and the production bank client and +resilience pipeline in front of a `StubBank` handler that answers like the simulator unless a +test sets up its own answer for a card. Tests run in parallel, so each one uses its own card +numbers (`Cards.Unique`) and never shares stub state. They cover the money paths: no retry, +bank timeouts, the outcome write failing, a client disconnecting mid-payment, concurrent +idempotency keys, rate limiting, startup validation and no card number in any log line. + +```bash +dotnet test --project tests/PaymentGatewayApiIntegrationTests # ~10s, Docker required +``` + +Don't pin known-wrong behaviour in a passing test: fix the defect, or skip the test with a +reason that links an issue (`[Fact(Skip = "... #N")]`). End-to-end tests live in `tests/PaymentGatewayApiEndToEndTests`. They are black-box: they call -the running API over HTTP (the `src` project is referenced only for its models), so they need +the running API over HTTP and do not reference the `src` project, so they need the whole stack up, including the API container, which sits behind the compose `api` profile: ```bash -cd src && docker compose --profile api up -d --build # api on :5067, same port as dotnet run +cd src && docker compose --profile api up -d --build --wait # api on :5067, same port as dotnet run dotnet test --project tests/PaymentGatewayApiEndToEndTests ``` -An assembly fixture (`ApiFixture`) polls the healthcheck for 30s before failing. The base URL +An assembly fixture (`ApiFixture`) polls `/health/live` for 30s before failing. The base URL defaults to `http://localhost:5067`; override it with `E2E_BASE_URL` (e.g. to run against -`dotnet run` instead of the container). The suite takes ~10s because the bank's `503` goes -through the standard resilience handler's retries. String results (`"ok"`, the payment id, -status names) come back as `text/plain`, not JSON strings. - -For manual checks, use the Bruno collection at `tests/bruno-collection.yml` (covers health, -get-by-id, authorized/declined/500 posts, and a direct simulator post), or the `curl` lines -written above each action in `Controllers/PaymentController.cs`. +`dotnet run` instead of the container). The bank client's resilience handler never retries +the `POST` (it is not idempotent), so the bank's `503` fails fast. The suite owns its request +record and reads responses as raw JSON (`JsonNode`), so it catches contract drift. -## Architecture +### CI -A `POST /api/payment` flows through one chain, each link behind an interface: +`.github/workflows/ci.yml` runs on pushes to `main` and on pull requests: restore, a Release +build (warnings are errors), `dotnet format --verify-no-changes`, a vulnerable-package check, +then the unit and integration tests with coverage (`coverage.settings.xml` limits it to the +api assembly and skips generated code), merged by ReportGenerator into the job summary and a +`coverage` artifact. A second job starts the compose stack with `--wait` and runs the E2E +suite. To collect coverage locally: -`PaymentController` assigns a new `PaymentId` GUID, runs all five -`IPaymentRequestValidator` checks (`Services/PaymentRequestValidator.cs`), then hands off to -`IBankService` (`Services/BankService.cs`). `BankService` calls `IBankHttpClient` -(`Services/BankHttpClient.cs`, POSTs to the `SimulatorBankEndpoint`), passes a successful -response to `IBankResponseProcessor` (`Services/BankResponseProcessor.cs`) to map it onto a -`PostPaymentResponse`, and persists that via `IPostPaymentRepository` -(`Services/PaymentsRepository.cs`) before returning the status. +```bash +dotnet test --project tests/PaymentGatewayApiUnitTests --coverage --coverage-output-format cobertura \ + --coverage-output unit.cobertura.xml --coverage-settings coverage.settings.xml +``` -`PaymentStatus` (`Models/PaymentStatus.cs`) is the contract between layers: +Dependabot (`.github/dependabot.yml`) proposes NuGet, Actions and image updates weekly. -- `Rejected` — local validation failed; the bank was never called. -- `Declined` — the bank answered `authorized: false`. -- `InternalError` — the bank returned non-2xx, or its body could not be parsed. +For manual checks, use the Bruno collection at `tests/bruno-collection.yml` (covers health, +get-by-id, authorized/declined/502 posts, and a direct simulator post), or the `curl` lines +written above each action in `Controllers/PaymentsController.cs`. -### Models come in two pairs, deliberately +## Architecture -`PaymentRequest` / `PostPaymentResponse` are this gateway's own API shape. -`BankRequest` / `BankResponse` model the acquiring bank's contract and use `snake_case` -through `[JsonPropertyName]` (`card_number`, `expiry_date`, `authorization_code`). Keep -them separate — collapsing them couples our public API to the bank's. +A `POST /api/payments` flows through one chain, each link behind an interface: + +`PaymentsController` asks `IPaymentRequestValidator.Validate` (`Services/PaymentRequestValidator.cs`) +for every broken rule and answers `400` `ValidationProblemDetails` if there are any; otherwise +it hands the request to `IPaymentService` (`Services/PaymentService.cs`), which: + +1. stores a `Payment` with status `Pending` through `IPaymentsRepository` + (`Services/PaymentsRepository.cs`), so a card is never charged without a record; if this + write fails, the request fails and the bank is never called; +2. calls `IBankClient.Authorize` (`Services/BankClient.cs`), a typed `HttpClient` that owns + the bank contract and turns every answer, or its absence, into a `BankOutcome`; +3. records the outcome and the bank's authorization code with `Complete`, which only + overwrites a payment that is still `Pending`. If that write fails, the merchant still gets + the real outcome, and a `Critical` log line carries what reconciliation needs. + +`PaymentStatus` (`Models/PaymentStatus.cs`) is the payment's lifecycle, stored by name: + +- `Pending` — recorded, the bank has not answered. +- `Authorized` / `Declined` — the bank's answer. +- `Failed` — the bank answered 4xx or 503, or the circuit breaker is open: nothing was charged. +- `Unknown` — a timeout, a dropped connection, another 5xx or an unreadable body: the card + may have been charged, so the payment needs reconciliation. + +A request that fails validation never becomes a payment. + +### HTTP contract + +Every endpoint needs an `X-Api-Key` header (the authorization fallback policy), except +`/health/*` and the Development OpenAPI document. `ApiKeyAuthenticationHandler` +(`Security/`) maps the key to a merchant through `ApiKeys:Merchants`, which holds only +SHA-256 hashes of the keys. The local keys are `dev-key-merchant-a` and `dev-key-merchant-b` +(`appsettings.Development.json`, and the compose file for the container). A payment belongs +to the merchant that created it; another merchant's `GET` is a `404`, like a missing id. +`POST` is rate limited per merchant (`RateLimiting`, `429` with `Retry-After`). + +| Outcome | Response | +| --- | --- | +| `Authorized` / `Declined` | `201` + `Location: /api/payments/{id}` + the payment | +| invalid request (validator or model binding) | `400` `ValidationProblemDetails`, errors keyed by JSON field | +| `Failed` / `Unknown` | `502` `ProblemDetails` with `paymentId` and `paymentStatus` extensions | +| `GET` of an unknown id, or another merchant's | `404` `ProblemDetails` | +| missing or unknown API key | `401` `ProblemDetails`, `WWW-Authenticate: ApiKey` | +| rate limit exceeded | `429` `ProblemDetails` with `Retry-After` | +| repeated `Idempotency-Key`, same request | the first request's response, plus `Idempotent-Replayed: true` | +| repeated `Idempotency-Key`, first still `Pending` | `409` `ProblemDetails` with `paymentId` | +| repeated `Idempotency-Key`, different request | `422` `ProblemDetails` | +| unhandled exception | `500` `ProblemDetails` (`GlobalExceptionHandler`), never the exception message | + +Amounts are integers in the currency's minor unit (`1050` is £10.50), currencies come from +`Payments:SupportedCurrencies`, and statuses serialize as names. `PaymentRequest` is a +record whose `ToString` masks the card number and CVV. + +### Models come in pairs, deliberately + +`PaymentRequest` / `PaymentResponse` are this gateway's own API shape, `Payment` is its +domain record, and `BankRequest` / `BankResponse` model the acquiring bank's contract with +`snake_case` names through `[JsonPropertyName]` (`card_number`, `expiry_date`, +`authorization_code`). Keep them separate: collapsing them couples our public API to the +bank's, or our storage to either. ### Persistence -DynamoDB table `Payments`, hash key `PaymentId`. Each item is just `PaymentId` (S) plus -`PaymentRaw` (S) holding the JSON-serialized `PostPaymentResponse`. Only the last four card -digits are ever stored. +DynamoDB table `Payments`, hash key `PaymentId`. Each item has one attribute per `Payment` +field (`MerchantId`, `Status` as its name, timestamps as ISO 8601, `AuthorizationCode` only +once authorized); only the last four card digits are ever stored. The mapping lives in +`PaymentsRepository.ToItem` / `FromItem`. + +Table `PaymentIdempotencyKeys`, hash key `IdempotencyKey` = `"{MerchantId}#{Key}"`, holds +`PaymentId`, a `RequestFingerprint` and `ExpiresAt` (epoch seconds, the TTL attribute; +`Payments:IdempotencyKeyRetention`, 24h). With an `Idempotency-Key`, the pending payment and +the key are written in one `TransactWriteItems`, so of two concurrent requests with the same +key exactly one reaches the bank. The fingerprint hashes the last four digits, expiry, +currency and amount: never the CVV or the full card number, which PCI DSS forbids keeping +under an unkeyed hash. The compose init script creates both tables; after pulling a schema +change, recreate local data with `docker compose down` then `up`. ### Wiring -All DI registration lives in `Program.cs`. Services are scoped; the repository is a -singleton, as is the `IAmazonDynamoDB` client registered through -`AddAWSService()`. New services follow the existing pattern — interface -and implementation in the same file under `Services/` — and get registered there. +All DI registration lives in `Program.cs`. The repository and the validator are stateless +singletons, as is the `IAmazonDynamoDB` client registered through +`AddAWSService()`. `BankClient` is a typed `HttpClient` (transient) whose +resilience handler has retries disabled, and `PaymentService` is scoped so it never captures +it. New services follow the existing pattern (interface and implementation in the same file +under `Services/`) and get registered there. Authentication, the merchant claim and the rate +limit policy live in `Security/`, options classes in `Configuration/`, middleware in +`Middleware/`, the readiness check in `Health/`. -Middleware order is `LogMiddleware` then `ErrorMiddleware`. +Middleware order: `CorrelationIdMiddleware`, `UseSerilogRequestLogging`, `UseExceptionHandler` +(with `GlobalExceptionHandler` and `AddProblemDetails`), `UseStatusCodePages` so bodiless errors +get a `ProblemDetails` too, Swagger UI (Development) or HSTS + HTTPS redirection (elsewhere), +then authentication, authorization, the rate limiter and a `MerchantId` log-context push. ## Bank simulator behaviour @@ -108,25 +202,39 @@ The WireMock mappings in `src/Simulators/wiremock/mappings/` select a response f | Card ends in | Simulator response | Gateway result | | --- | --- | --- | -| odd digit | `200` `authorized: true` | `Authorized` | -| even digit (not 0) | `200` `authorized: false` | `Declined` | -| `0` | `503` | `InternalError` | -| any missing field | `400` | `InternalError` | +| odd digit | `200` `authorized: true` | `201` `Authorized` | +| even digit (not 0) | `200` `authorized: false` | `201` `Declined` | +| `0` | `503` | `502` `Failed` | +| any missing field | `400` | `502` `Failed` | +The three `200`/`503` stubs also require `expiry_date` in the bank's `MM/yyyy` format, so a +malformed expiry falls through like any other unmatched request. Anything else hits the catch-all `default.json` (`400`). It carries `"priority": 10` so it never shadows the specific stubs; WireMock breaks equal-priority ties by load order. ## Conventions +- **The build enforces the style.** `Directory.Build.props` turns on `latest-recommended` + analyzers, code-style enforcement and warnings-as-errors; `.editorconfig` holds the rules. + Run `dotnet format PaymentGatewayApi.slnx` before committing. Package versions live only in + `Directory.Packages.props` (central package management). - **Explicit `using` directives** — `ImplicitUsings` is disabled, so every file declares - its own. `Nullable` is enabled. -- **`this.` prefix** on all field access, consistently throughout. -- **Structured logging** follows one template shape: + its own, System first; unused ones fail the build (IDE0005). `Nullable` is enabled. +- **`this.` prefix** on all field access and only there (IDE0009). +- **Logging goes through `[LoggerMessage]`** partial methods at the bottom of each class + (`CA1848` rejects `logger.LogX(...)` calls). Templates follow one shape: `"MethodName, PaymentId={PaymentId}, Status={Status}"`, with `Status` values like - `"Started"` / `"Completed"` / `"Failed"`. `CallerId` and `CorrelationId` are enriched - automatically by `LogMiddleware` from the request headers (a missing `CorrelationId` - header gets a generated GUID), so never pass them by hand. -- **Serilog is configured from `src/logconfig.json`, not `appsettings.json`.** - `appsettings.json` holds `SimulatorBankEndpoint` and the `AWS` block (pointed at - `http://localhost:8000` for DynamoDB Local). Verbose file logs go to `src/z-logs/` - (gitignored); the console sink is deliberately terse. + `"Started"` / `"Completed"` / `"Failed"`. `CorrelationIdMiddleware` pushes `CorrelationId` + (the caller's `X-Correlation-ID` if it matches `^[A-Za-z0-9-]{1,64}$`, a new GUID otherwise, + echoed on the response and in every `ProblemDetails`), and Serilog adds `TraceId`/`SpanId`, + so never pass them by hand. Never log a `PaymentRequest`'s card number or CVV. +- **Serilog is configured from the `Serilog` section of `appsettings*.json`.** The base file + writes compact JSON to the console (what containers collect); `appsettings.Development.json` + swaps in a readable console format and adds files under `src/z-logs/` (gitignored). + `UseSerilogRequestLogging` writes one summary line per request. +- **Settings bind to validated options classes** in `Configuration/` (`Bank`, `Payments`, + `Dynamo`, `ApiKeys`, `RateLimiting`), checked at startup (`ApiKeys` through the + source-generated `[OptionsValidator]`, which also validates each merchant). Local-only + values (the simulator URL, `AWS:ServiceURL` pointed at DynamoDB Local, the dev API key + hashes) live in `appsettings.Development.json`; the compose file sets them for the + container through environment variables. diff --git a/Directory.Build.props b/Directory.Build.props new file mode 100644 index 0000000..34c49af --- /dev/null +++ b/Directory.Build.props @@ -0,0 +1,17 @@ + + + + net10.0 + enable + disable + + true + latest-recommended + true + + + true + $(NoWarn);CS1591 + + + diff --git a/Directory.Packages.props b/Directory.Packages.props new file mode 100644 index 0000000..76eb63a --- /dev/null +++ b/Directory.Packages.props @@ -0,0 +1,25 @@ + + + + true + + + + + + + + + + + + + + + + + + + + + diff --git a/PaymentGatewayApi.slnx b/PaymentGatewayApi.slnx index fabdbf6..138bf97 100644 --- a/PaymentGatewayApi.slnx +++ b/PaymentGatewayApi.slnx @@ -4,6 +4,7 @@ + diff --git a/README.md b/README.md index dba2873..843c0f1 100644 --- a/README.md +++ b/README.md @@ -1,11 +1,167 @@ -Building a basic payment gateway +# Payment Gateway API -https://en.wikipedia.org/wiki/Payment_gateway#Typical_transaction_processes -https://stripe.com/gb/resources/more/payment-gateways-101#how-does-a-payment-gateway-work +[![CI](https://github.com/cusnirati/payment-gateway-api/actions/workflows/ci.yml/badge.svg)](https://github.com/cusnirati/payment-gateway-api/actions/workflows/ci.yml) -Intentional kept simple so that can demo software building principles like SOLID. -Certain things are omitted (verbosity in logs, authorization, https, http retries, configuration file validation, etc). +A payment gateway in ASP.NET Core (.NET 10): merchants submit card payments, the gateway +validates them, asks an acquiring bank to authorize them, records the outcome in DynamoDB and +lets the merchant read it back. The bank is simulated with WireMock. -More verbose than console ones logs are in directory `z-logs`. +The interesting part is what happens when things go wrong. The gateway never charges a card +without a record of it, never retries a charge on its own, lets merchants retry safely with an +`Idempotency-Key`, and tells merchants whether a failure was theirs or the bank's. -add later how to run / debug \ No newline at end of file +## Quick start + +Requirements: the .NET 10 SDK and Docker. + +```bash +cd src && docker compose --profile api up -d --build --wait # DynamoDB Local, the bank simulator, the api on :5067 +``` + +```bash +curl -i -X POST http://localhost:5067/api/payments \ + -H "X-Api-Key: dev-key-merchant-a" \ + -H "Content-Type: application/json" \ + -H "Idempotency-Key: order-1001" \ + -d '{"cardNumber":"2222405343248873","expiryMonth":4,"expiryYear":2030,"currency":"GBP","amount":1050,"cvv":"123"}' +``` + +To debug the api instead of running its container, start only the dependencies and run it +from the SDK (Swagger UI at http://localhost:5067/swagger): + +```bash +cd src && docker compose up -d +export AWS_ACCESS_KEY_ID=local AWS_SECRET_ACCESS_KEY=local # DynamoDB Local ignores them, the AWS SDK insists on some +dotnet run --project src/PaymentGatewayApi.csproj +``` + +`tests/bruno-collection.yml` is a [Bruno](https://www.usebruno.com/) collection with the same requests. + +## Tests + +| Suite | What it proves | Needs | Command | +| --- | --- | --- | --- | +| Unit (`tests/PaymentGatewayApiUnitTests`) | each class in isolation: validation rules, the bank contract and its failure mapping, the payment lifecycle, the HTTP mapping | nothing | `dotnet test --project tests/PaymentGatewayApiUnitTests` | +| Integration (`tests/PaymentGatewayApiIntegrationTests`) | the whole api in process against real DynamoDB Local and the real resilience pipeline: no retry on POST, timeouts, a failed outcome write, client disconnects, concurrent idempotency keys, rate limits, startup validation, no card number in any log | Docker | `dotnet test --project tests/PaymentGatewayApiIntegrationTests` | +| End-to-end (`tests/PaymentGatewayApiEndToEndTests`) | the container, over HTTP, as a black box with its own request model | the compose stack | `dotnet test --project tests/PaymentGatewayApiEndToEndTests` | + +CI runs all three on every pull request, with coverage in the job summary (see `.github/workflows/ci.yml`). + +## API + +Every endpoint needs an `X-Api-Key` header except the health checks. Locally the keys are +`dev-key-merchant-a` and `dev-key-merchant-b`. Amounts are integers in the currency's minor +unit (`1050` is £10.50); the currencies are `GBP`, `EUR` and `USD`. Errors are +[RFC 9457](https://www.rfc-editor.org/rfc/rfc9457) `ProblemDetails` with a `traceId` and a +`correlationId`. Send an `X-Correlation-ID` header to choose the correlation id yourself; the +response echoes it. + +| Request | Response | +| --- | --- | +| `POST /api/payments`, bank authorizes | `201 Created`, `Location: /api/payments/{id}`, the payment with `"status": "Authorized"` | +| `POST /api/payments`, bank declines | `201 Created`, `"status": "Declined"`: the request was valid, the issuer said no | +| `POST /api/payments`, invalid request | `400` `ValidationProblemDetails` listing every invalid field; no payment is created | +| `POST /api/payments`, bank failed or unreachable | `502` `ProblemDetails` with `paymentId` and `paymentStatus` (`Failed` or `Unknown`) | +| `POST` with an `Idempotency-Key` used before | the first request's response, plus `Idempotent-Replayed: true`; `409` while the first is still in flight; `422` if the body differs | +| `POST` over the merchant's rate limit | `429` with `Retry-After` | +| `GET /api/payments/{id}` | `200` with the payment, or `404` if it does not exist or belongs to another merchant | +| `GET /health/live`, `GET /health/ready` | `200 Healthy` when the process is up / when it can also reach DynamoDB | + +```json +{ + "id": "0b7c6f6e-7f0e-4a4b-9a1d-3f1d2f0c9a11", + "status": "Authorized", + "cardNumberLastFour": "8873", + "expiryMonth": 4, + "expiryYear": 2030, + "currency": "GBP", + "amount": 1050 +} +``` + +### Steering the bank simulator + +The WireMock stubs in `src/Simulators/wiremock/mappings/` answer by the card number's last digit: + +| Card ends in | Bank answers | Gateway answers | +| --- | --- | --- | +| odd digit | `200` authorized | `201` `Authorized` | +| even digit (not 0) | `200` declined | `201` `Declined` | +| `0` | `503` | `502` `Failed` | + +A request without a well-formed field (for example an expiry that is not `MM/yyyy`) gets a +`400` from the simulator, and the gateway answers `502` `Failed`. + +## How a payment flows + +```mermaid +sequenceDiagram + participant M as Merchant + participant G as PaymentsController / PaymentService + participant D as DynamoDB + participant B as Acquiring bank + M->>G: POST /api/payments (X-Api-Key, Idempotency-Key) + G->>G: validate every field (400 on failure, nothing stored) + G->>D: store payment as Pending (+ idempotency key, one transaction) + G->>B: authorize (timeouts and circuit breaker, never retried) + B-->>G: authorized / declined / error / nothing + G->>D: record Authorized / Declined / Failed / Unknown + authorization code + G-->>M: 201 with the payment, or 502 with its id +``` + +- `PaymentsController` validates the request and maps the result to HTTP. +- `PaymentService` owns the payment's lifecycle. +- `BankClient` owns the bank's contract. It is a typed `HttpClient`, and it turns every + answer, or the absence of one, into a `BankOutcome`. +- `PaymentsRepository` owns the DynamoDB items. +- Each layer sits behind an interface. `AGENTS.md` has the details for contributors. + +## Design decisions and trade-offs + +- **The bank POST is never retried.** Authorizing a card is not idempotent, and the simulated + bank takes no idempotency key. A retry after a lost response could charge the card twice. + The standard resilience handler therefore keeps its timeouts and circuit breaker but has + retries disabled for unsafe methods. The merchant retries instead, safely, with an + `Idempotency-Key`. +- **The payment is recorded before the bank is called.** If that write fails, the bank is + never called. If the write of the outcome fails afterwards, the merchant still gets the + real outcome, the record stays `Pending`, and a `Critical` log line carries what + reconciliation needs. +- **`Failed` and `Unknown` are different statuses.** A 4xx, a 503 or an open circuit means + the card was not charged. A timeout, a dropped connection, another 5xx or an unreadable + body means it may have been. Both are `502` to the merchant, because the bank, not the + merchant, is at fault, and a 4xx would tell the merchant not to retry. +- **Cancellation stops at the bank's door.** A client that disconnects cancels the payment + only until the bank is called. After that, cancelling would leave the bank's outcome + unknown, so the call and the write that records it run to completion. +- **Three model families.** The public API (`PaymentRequest` / `PaymentResponse`), the domain + record (`Payment`) and the bank's contract (`BankRequest` / `BankResponse`, `snake_case`) + are kept apart. Changing one never forces a migration of another. +- **Card data.** Only the last four digits are stored. `PaymentRequest.ToString` masks the + card number and CVV, and an integration test checks that no log event carries a full card + number. The idempotency fingerprint hashes neither the CVV nor the full number, because + PCI DSS forbids keeping them under an unkeyed hash. +- **DynamoDB.** A payment is a single item read and written by key, which is the access + pattern DynamoDB is built for. It is serverless, and it gives the idempotency key and the + payment an atomic write through `TransactWriteItems`. Merchant-wide queries such as + "list my payments" would need a GSI on `MerchantId`. +- **API keys, hashed.** The settings hold only the SHA-256 of each key. A merchant can read + only its own payments, and another merchant's id looks exactly like a missing one. +- **No Luhn check.** The simulator steers its answer by the last digit, and a Luhn check digit + would fix that digit. The issuer validates the number anyway. + +## Known limitations and next steps + +- **Reconciliation.** Payments left `Pending` (the outcome write failed) or `Unknown` (no + usable answer from the bank) need a job that asks the bank for their status. The data it + needs is stored and logged, but the job is not built. +- **Secrets.** Local API key hashes live in `appsettings.Development.json` and the compose + file. A deployment would load them from a secret store and rotate them, and would use a + keyed hash (HMAC) if it ever needed to recognise full card numbers. +- **TLS.** HSTS and HTTPS redirection are on outside Development. Behind a TLS-terminating + load balancer, the app also needs `ForwardedHeaders` configured for that proxy. +- **Infrastructure as code.** The tables are created by the compose init script and by the + integration tests. A deployment would define them, with TTL on `ExpiresAt`, in CDK or + Terraform. +- **Multiple instances.** The rate limiter is per instance. A shared limit needs a + distributed store. diff --git a/audit.md b/audit.md new file mode 100644 index 0000000..fc04fd2 --- /dev/null +++ b/audit.md @@ -0,0 +1,452 @@ +# Payment Gateway API: audit for a senior .NET reviewer + +**Scope:** the whole repository as published on branch `claude` at `ccaacc4`: source, tests, Docker/compose, config, docs and git history. +**Question:** what has to change so that a senior .NET reviewer concludes the author builds high-quality software? +**Method:** I read every tracked file and ran the build, the unit tests, the vulnerable-package scan and the outdated-package scan. I did not change any existing code. The only new file is this report. + +| Check | Result | +| --- | --- | +| `dotnet build PaymentGatewayApi.slnx` (SDK 10.0.112) | ✅ 0 warnings, 0 errors (but no analyzers or warnings-as-errors are configured) | +| `dotnet test --project tests/PaymentGatewayApiUnitTests` | ✅ 61/61 passed in 1.3 s | +| `dotnet list package --vulnerable --include-transitive` | ✅ no known vulnerabilities | +| `dotnet list package --outdated` | ⚠️ `AWSSDK.DynamoDBv2` 4.0.101.2 → 4.0.104, `Moq` 4.20.69 → 4.20.72 | +| E2E suite | Not run. It needs the Docker stack; I only reviewed its code. | + +--- + +## 1. Verdict + +The repo already shows good habits, and a reviewer will notice them: + +- The layers sit behind interfaces. +- `Nullable` is enabled. +- The public API models are kept apart from the bank's contract models. +- Card data is truncated to the last four digits before it is stored. +- Logging is structured. +- There is a reproducible local stack (DynamoDB Local plus a WireMock bank). +- There is a unit suite that is fast and readable. +- There is a black-box E2E suite. +- Known defects are recorded honestly in tests. + +A senior reviewer will judge a **payment** gateway on something else first: **does money move correctly when things go wrong?** On that question the code fails in several concrete ways. It can charge a card twice. It can charge a card and lose the record. It sends the bank a badly formatted expiry date. It discards the authorization code. It reports a bank outage as the client's fault. Next to those problems, a reviewer also sees: + +- ASP.NET Core features rebuilt by hand, or skipped: Options, typed `HttpClient`, `ProblemDetails`, health checks, `TimeProvider`, `CancellationToken`. +- An API contract that is inconsistent between endpoints. +- Repo hygiene that reads as unfinished: TODOs, commented-out code, "." commit messages, a placeholder README, no CI. + +**The fastest route to a convincing repo:** +1. Fix the money-correctness issues (§2). +2. Make the HTTP contract honest (§3). +3. Replace the hand-rolled plumbing with the framework's own features (§4). +4. Add CI plus in-process integration tests, so that quality is *demonstrated*, not claimed (§6, §7). + +Severity legend: 🔴 critical (a reviewer stops reading) · 🟠 high · 🟡 medium · ⚪ polish. + +--- + +## 2. Payment correctness and resilience + +### 🔴 2.1 POST to the bank is retried automatically, so one payment can be charged twice +`src/Program.cs:23-27` + +```csharp +builder.Services.ConfigureHttpClientDefaults(http => { http.AddStandardResilienceHandler(); }); +``` + +The standard resilience handler retries **every** HTTP method on 5xx, 408, 429 and timeouts. `POST /payments` to an acquiring bank is not idempotent. If the bank authorizes the payment but the response is lost or slow, the gateway sends it again. The E2E suite shows this happening: `PostPayment_CardEndingZero_InternalError` is slow *because* the POST is retried. The README even says "http retries" are omitted, which contradicts the code. + +**Change:** +- Scope the resilience handler to a single typed bank client (see 4.2), not to `ConfigureHttpClientDefaults`. +- Turn retries off for unsafe methods: `options.Retry.DisableForUnsafeHttpMethods()`. +- Alternatively, retry only if the bank supports an idempotency key, and send one derived from `PaymentId`. +- Keep the attempt and total timeouts plus the circuit breaker. + +### 🔴 2.2 The payment is persisted only *after* the bank call, so a failed write loses a charged payment +`src/Services/BankService.cs:35-41` + +The order today is: call the bank, get `Authorized`, then call `paymentRepo.Add(...)`. If DynamoDB throws at that point: +- the card has been charged, +- no record exists, +- `ErrorMiddleware` returns a 500 to the merchant, +- the merchant will probably retry and charge the card again. + +When the bank call fails (non-2xx, timeout or exception), nothing is stored at all, so there is no audit trail. + +**Change:** write the payment first with status `Pending`. Call the bank. Then update it to `Authorized`, `Declined` or `Failed`/`Unknown`. For a timeout, record the uncertain outcome explicitly; a reconciliation step would resolve it later (you can document this without building it). This one change shows more payments maturity than anything else in the repo. + +### 🔴 2.3 No idempotency for merchant requests +`src/Controllers/PaymentController.cs:57-84` + +Every POST creates a new `PaymentId`. When a merchant retries after a network blip, a second charge is created. Payment APIs (Stripe, Checkout.com, Adyen) accept an `Idempotency-Key` header. + +**Change:** accept `Idempotency-Key`. Store the key along with the result, for example as a DynamoDB item with a conditional put (`attribute_not_exists`). Replay the stored response on a duplicate. Return `409`/`422` when the same key arrives with a different body. + +### 🔴 2.4 The expiry date sent to the bank has the wrong format +`src/Services/BankHttpClient.cs:34-35` + +```csharp +// "04/2025" +string expiry = $"{payment.ExpiryMonth}/{payment.ExpiryYear}"; // produces "4/2025" +``` + +The comment on the line above states the contract (`MM/yyyy`), but the code does not follow it. Nothing catches the bug, because the WireMock stubs only check that `expiry_date` *exists*. + +**Change:** +- Use `$"{payment.ExpiryMonth:D2}/{payment.ExpiryYear}"`. +- Add a unit test for the mapping. +- Tighten the stubs so `expiry_date` must match `^(0[1-9]|1[0-2])/\d{4}$`. Then the simulator enforces the contract and does not just mirror the implementation. + +### 🟠 2.5 The bank's authorization code is thrown away +`src/Services/BankResponseProcessor.cs:50-57`, `src/Models/Responses/PostPaymentResponse.cs` + +`BankResponse.AuthorizationCode` is parsed but never mapped or stored. Capture, refund, dispute and reconciliation all need that code. + +**Change:** store it on the persisted payment record. It does not have to be exposed in the public response. + +### 🟠 2.6 An unparseable bank response is stored with a null hash key +`src/Services/BankResponseProcessor.cs:28-48`, `src/Services/BankService.cs:38-39` + +This defect is already pinned in `BankServiceTests.ExecutePayment_BankBodyUnparseable_StillPersistsResponseWithoutPaymentId`. The error path returns a `PostPaymentResponse` with only `Status` set, and `BankService` persists it anyway. `PaymentId = null` in the key makes DynamoDB reject the write. The exception reaches `ErrorMiddleware`, which returns a 500 and leaks DynamoDB's message. + +**Change:** +- The processor should return a result type such as `BankOutcome { Authorized | Declined | Failed(reason) }`, not a half-filled DTO. +- `BankService` decides what gets persisted. +- Then remove the `// NOTE: defect` test. + +### 🟠 2.7 Transport exceptions are not handled (`// todo try catch`) +`src/Services/BankHttpClient.cs:48`, `src/Services/PaymentsRepository.cs:20` + +The following all propagate as a 500 whose body contains the exception message: +- `HttpRequestException`, +- `TaskCanceledException`/`TimeoutRejectedException`, +- `BrokenCircuitException`, +- `AmazonDynamoDBException`. + +**Change:** catch them at the bank-client boundary, log them, and convert them into a domain outcome (`BankUnavailable`). Map that outcome to `502`/`503`. Do not leave TODOs about error handling in a payments codebase: either do the work or remove the comment and open an issue. + +### 🟠 2.8 An invalid expiry year crashes instead of rejecting +`src/Services/PaymentRequestValidator.cs:51` + +This defect is already pinned in unit and E2E tests. `new DateTime(0, m, 1)` throws, so the response is a 500 instead of `Rejected`. + +**Change:** check the range (for example `now.Year <= year <= now.Year + 20`) before building the date, or compare `(year, month)` tuples without creating a `DateTime`. Delete both "defect" tests and replace them with `Rejected` assertions. + +> **About the `// NOTE: defect` tests.** Recording bugs as tests is a good engineering instinct. In a portfolio repo, though, a reviewer reads *three* documented bugs that are easy to fix and asks why they were not fixed. Fix them. If one must stay, use `[Fact(Skip = "…issue #N")]` linked to a GitHub issue, so the suite does not stay green on known-wrong behaviour. + +### 🟡 2.9 Money and domain rules are underspecified +`src/Services/PaymentRequestValidator.cs:64-91`, `src/Models/Requests/PaymentRequest.cs:22` + +- `Amount` is a `decimal` in major units with a rule of `>= 1`. That rejects £0.50, accepts `1.2345`, has no upper bound, and ignores zero-decimal currencies such as JPY. Gateways normally take an **integer amount in minor units** (`long amount = 1050` → £10.50). Pick one approach and document it. +- The currency check only tests `Length == 3`: `"123"` passes, and a test even pins that behaviour. Use an ISO-4217 allow-list from configuration (`GBP`, `EUR`, `USD` is enough). Normalise the case too. +- There is no Luhn check on the card number. A senior reviewer expects one, and it is about ten lines. +- `\d` in `^\d{14,19}$` and `^\d{3,4}$` matches any Unicode digit (Arabic-Indic, full-width and so on). Use `[0-9]`, and use `[GeneratedRegex]` (source-generated, allocation-free, analyzer-friendly) instead of `Regex.IsMatch` with a string pattern. + +--- + +## 3. API contract (what the merchant sees) + +### 🔴 3.1 Status codes misreport who is at fault +`src/Controllers/PaymentController.cs:70-83` + +| Outcome | Returned today | What a reviewer expects | +| --- | --- | --- | +| Authorized | `200` + plain-text id | `201 Created` + `Location: /api/payments/{id}` + payment JSON | +| Declined | `400 "Declined"` | `201`/`200` + payment JSON with `status: "Declined"`. The request was valid; the issuer said no. | +| Rejected (validation) | `400 "Rejected"` (plain text) | `400` `ValidationProblemDetails` listing *which* fields failed | +| Bank down / unparseable | `400 "InternalError"` | `502 Bad Gateway` / `503` `ProblemDetails`. **Never** 4xx: the client did nothing wrong, and 4xx tells it not to retry. | +| Unhandled exception | `500` plain text with `exception.Message` | `500` `ProblemDetails` with `traceId`, and no internal message | + +### 🟠 3.2 The POST does not return the payment it created +`src/Controllers/PaymentController.cs:77-83` + +`PostPaymentResponse` exists and the repository builds and stores it, but POST returns only the id (when authorized) or only a status string. A declined payment is persisted, yet the merchant never learns its id, so `GET /{id}` cannot be used for it. + +**Change:** return the `PostPaymentResponse` for both Authorized and Declined. + +### 🟠 3.3 The endpoints disagree on types and formats +- `PostPayment` declares `ActionResult` (`:58`) but returns strings, so the OpenAPI document is wrong. `GetHealthcheck` declares `ActionResult`. +- The POST returns statuses as text (`"Declined"`). The GET returns `PostPaymentResponse.Status` as a **number** (`0`), because `JsonStringEnumConverter` is not registered. The persisted `PaymentRaw` JSON also stores the enum as a number, so reordering `PaymentStatus` would silently corrupt stored data. +- Model-binding failures, such as a missing `required` property, already return `ProblemDetails` JSON through `[ApiController]`, while validator failures return the plain string `"Rejected"`. The same endpoint has two different shapes for 400. +- `GET /api/payment/{paymentId}` accepts any string. It should be `{paymentId:guid}` with a `Guid` parameter, so malformed ids return 400/404 without a database call. +- `NotFoundObjectResult("payment not found")` returns a plain string. Use `NotFound()`, which returns ProblemDetails. +- The code uses `new OkObjectResult(...)`/`new BadRequestObjectResult(...)` instead of the `ControllerBase` helpers (`Ok`, `CreatedAtAction`, `ValidationProblem`, `Problem`). The helpers are idiomatic and shorter. + +**Change:** +- Add `[Produces("application/json")]` and `[ProducesResponseType]` on every action. +- Add `builder.Services.AddProblemDetails()`. +- Configure `JsonStringEnumConverter` globally, for MVC and for the repository serializer. +- Use a plural resource name: `api/payments`. + +### 🟠 3.4 The request model accepts a server-owned field +`src/Models/Requests/PaymentRequest.cs:23` + +`PaymentId` sits on the inbound DTO. Swagger therefore advertises it as an input, and the controller mutates the request object to carry it (`:60`). + +**Change:** remove it from the request. Pass the id as a parameter, or create a domain `Payment` from the request. Make DTOs immutable (`record` with `init`/`required`). + +### 🟡 3.5 Storage is coupled to the public API +`src/Services/PaymentsRepository.cs:37` + +The author (correctly) keeps the bank models apart from the API models, then persists the **public response DTO** as the storage record. Any change to the public API becomes a data migration. + +**Change:** introduce a persistence record (a `PaymentEntity` with its own attributes) and map to and from it. The stored record also needs `AuthorizationCode`, `CreatedAt`, `MerchantId`/`CallerId` and `IdempotencyKey`, which have no place in the public response. + +--- + +## 4. ASP.NET Core idioms (showing framework fluency) + +### 🟠 4.1 Configuration: use the Options pattern and validate at startup +`src/Services/BankHttpClient.cs:45`, `src/Services/PaymentsRepository.cs:39,49` + +```csharp +string uri = this.config["SimulatorBankEndpoint"] ?? string.Empty; +``` + +Injecting `IConfiguration` into services, reading magic strings, and **falling back silently to an empty URI** means a missing setting only surfaces at request time. The table name `"Payments"` is hard-coded twice. + +**Change:** +```csharp +builder.Services.AddOptions().BindConfiguration("Bank") + .ValidateDataAnnotations().ValidateOnStart(); +``` +Apply the same pattern to `DynamoOptions { TableName }`. The README lists "configuration file validation" as omitted; it takes about five lines, so add it. + +### 🟠 4.2 Use a typed `HttpClient` and keep `HttpResponseMessage` out of the abstractions +`src/Program.cs:23-27,39`, `src/Services/BankHttpClient.cs:14,46`, `src/Services/BankResponseProcessor.cs:13` + +- `IHttpClientFactory.CreateClient()` with no name, plus global defaults, means *every* HttpClient in the process gets the retry policy. +- `IBankHttpClient` returns `HttpResponseMessage`, and `IBankResponseProcessor` receives it. The transport type leaks through two abstractions, and nobody disposes the response (`BankService.cs:35`). + +**Change:** +```csharp +builder.Services.AddHttpClient((sp, c) => + c.BaseAddress = sp.GetRequiredService>().Value.BaseUrl) + .AddStandardResilienceHandler(o => o.Retry.DisableForUnsafeHttpMethods()); +``` +`IBankClient.AuthorizeAsync(BankRequest, CancellationToken)` then returns a domain `BankOutcome`. The client owns serialization, status-code mapping and disposal. `BankResponseProcessor` either becomes a private mapping inside the client or a pure static mapper. Both are easier to test than a class that needs an `HttpResponseMessage`. + +### 🟠 4.3 Pass `CancellationToken` through the whole chain +There is none anywhere: the controller, `IBankService`, `IBankHttpClient`, `IPaymentsRepository`, `PutItemAsync` and `GetItemAsync` all lack it. A disconnected client keeps consuming bank and database capacity. + +**Change:** add `CancellationToken cancellationToken` to every async interface method and flow it from the action parameter, which MVC binds from `HttpContext.RequestAborted`. Reviewers look for this first. + +### 🟠 4.4 Use `TimeProvider` instead of `DateTime.Now` +`src/Controllers/PaymentController.cs:65`, `src/Services/PaymentRequestValidator.cs:11` + +The controller reads the local server clock (`DateTime.Now`, which depends on the time zone) and passes it into the validator, only so that the validator can be tested. + +**Change:** inject `TimeProvider` into the validator (registered as `TimeProvider.System`) and use `FakeTimeProvider` from `Microsoft.Extensions.TimeProvider.Testing` in tests. `IsExpiryValid(payment)` then loses its extra parameter. + +### 🟡 4.5 Validation design +`src/Services/PaymentRequestValidator.cs`, `src/Controllers/PaymentController.cs:63-68` + +Five boolean methods, chained with `&&` in the controller: +- stop at the first failure, +- return no error detail to the caller, +- force the controller to know the list of rules, +- make the controller tests use a string `switch` to steer mocks (`PaymentControllerTests.cs:106-123`). + +**Change:** one `ValidationResult Validate(PaymentRequest)` that collects every error keyed by field and returns `ValidationProblemDetails`. Either write it by hand or use FluentValidation. An `IEndpointFilter`/action filter can run it so that the action body only handles the happy path. + +### 🟡 4.6 Error handling: use the built-in pipeline +`src/Middleware/ErrorMiddleware.cs:25-37` + +- `exception.Message` goes straight to the client. That is information disclosure: DynamoDB and HTTP messages include hosts, table names and so on. `ErrorMiddlewareTests.InvokeAsync_NextThrows_Returns500WithMessage` asserts that the leak happens. +- The code sets `StatusCode` without checking `context.Response.HasStarted`, so it throws if streaming has begun. +- A client abort (`OperationCanceledException`) is logged as an error and answered with a 500. +- The comment `// see if can replace message with correlationId` is an open design question left in the code. + +**Change:** `builder.Services.AddExceptionHandler()` + `AddProblemDetails()` + `app.UseExceptionHandler()`. The handler logs the exception and returns `ProblemDetails` with `traceId`/`correlationId` and a generic message. Fix the test so it asserts that the message is *not* leaked. + +### 🟡 4.7 Correlation and caller context +`src/Middleware/LogMiddleware.cs:21,32` + +- `CallerId` is pushed as a raw `StringValues` from an unauthenticated header. It has no length limit and is not sanitised, so it enables log injection and log bloat. It is also not a scalar; the unit test works around that with `value.ToString()` + `Contains`. +- The client-supplied `CorrelationId` is trusted without validation and never echoed back in a response header, so a merchant cannot quote it to support. +- .NET already has W3C trace context (`Activity.Current.TraceId`, the `traceparent` header). The standard header name is `X-Correlation-ID` or `traceparent`, not `CorrelationId`. + +**Change:** +- Validate the header (GUID or `^[A-Za-z0-9-]{1,64}$`) and echo it on the response. +- Push `.ToString()`. +- Enrich logs with `TraceId`. +- Take the caller identity from authentication (§5.1), not from a free-text header. +- Rename the middleware to what it does, for example `CorrelationIdMiddleware`. + +### 🟡 4.8 Health checks +`src/Controllers/PaymentController.cs:33-40` + +`GET /api/payment` returning `"ok"` is a business route used as a health check. It proves the process is up, but not that DynamoDB or the bank can be reached. + +**Change:** `builder.Services.AddHealthChecks().AddCheck("dynamodb")` and `app.MapHealthChecks("/health/live")` plus `/health/ready`. Use them in `ApiFixture`, in a compose `healthcheck:` and in the Dockerfile. + +### ⚪ 4.9 Smaller idiom issues +- **OpenAPI:** .NET 9+ ships `Microsoft.AspNetCore.OpenApi` (`AddOpenApi()`/`MapOpenApi()`), and the .NET 10 templates no longer include Swashbuckle. Moving shows the author tracks the platform. `AddEndpointsApiExplorer()` is redundant with controllers. +- **Swagger is served in every environment** (`Program.cs:47-48`). Wrap it in `if (app.Environment.IsDevelopment())`. +- Add `public partial class Program;` so that `WebApplicationFactory` works (§6.1). +- **DI:** a scoped validator, response processor and bank service hold no request state. Singleton or a static pure function is fine, and saying *why* each lifetime was chosen shows intent. +- **Naming:** `BankService` coordinates validation → bank → persistence, so it is really a `PaymentService`/`PaymentProcessor`. The log template calls it `ExecuteBankPayment` while the method is `ExecutePayment`. The controller also talks to the repository directly for GET, which bypasses the service layer that POST uses. +- `Substring(request.CardNumber.Length - 4)` → `request.CardNumber[^4..]`. +- `[HttpGet()]` → `[HttpGet]`. +- `PaymentStatus.InternalError` is commented "Should never happen" (`PaymentStatus.cs:8`), yet it is returned on every bank outage. Rename it to `Failed`/`BankUnavailable` or fix the comment. +- Consider the `[LoggerMessage]` source generator for the fixed templates. It is allocation-free, and it enforces the "one template shape" convention at compile time instead of by discipline. Today the shapes already drift: `Validator.IsExpiryMonthValid` / `IsValid=` versus the documented `MethodName, PaymentId, Status`. + +--- + +## 5. Security and compliance posture + +The README says auth and HTTPS are "intentionally omitted". A reviewer accepts omissions that are *scoped* but not ones that are *unacknowledged in the code*. Show the seams. + +### 🟠 5.1 No authentication, and GET exposes any payment to anyone (IDOR) +Any caller who knows or guesses a payment id can read another merchant's payment. + +**Minimum change:** +- An API-key or JWT authentication handler that maps a key to a `MerchantId`. +- `[Authorize]` on the controller. +- Store `MerchantId` on the payment and filter `GET` by it, returning 404 for other merchants' payments. + +This is small, and it is the difference between "demo" and "gateway". + +### 🟡 5.2 Transport and abuse protection +- `UseHttpsRedirection`/HSTS for non-development environments. +- `AddRateLimiter` (built in since .NET 7) per merchant on `POST`. +- `AllowedHosts: "*"` in the base `appsettings.json`. + +### 🟡 5.3 Card-data handling (PCI DSS mindset) +Truncating to the last four digits is good. To make it visible and enforced: +- Mark `CardNumber`/`Cvv` with a redaction attribute (`Microsoft.Extensions.Compliance.Redaction` + `[DataClassification]`), or at least override `ToString()` on `PaymentRequest`, so a future `{@payment}` log call cannot leak a PAN or CVV. +- Add a test that sends a full PAN through the pipeline and checks that no log event contains it. The E2E test already asserts this for the GET response, which is the right instinct. + +### ⚪ 5.4 Container +`src/Dockerfile` +- Add `USER $APP_UID`: the .NET 8+ images ship a non-root `app` user, and not using it is a common review flag. +- Consider the `-chiseled`/`noble-chiseled` runtime image. +- Use `dotnet publish --no-restore`. +- Add a `HEALTHCHECK`. + +--- + +## 6. Tests + +The unit suite is clean: arrange/act/assert, xUnit v3, `NullLogger`, and folders that mirror `src/`. Where it falls short is **coverage of the risky parts** and **evidence**. + +### 🟠 6.1 In-process integration tests are missing +The two layers that handle money and I/O, `BankHttpClient` and `PaymentsRepository`, are "deliberately not unit-tested". The only thing that exercises them is an E2E suite that needs `docker compose` and is not run anywhere automatically. + +**Change:** add `PaymentGatewayApiIntegrationTests` built on `WebApplicationFactory`: +- Replace the bank with a stub `HttpMessageHandler`, or `WireMock.Net` in-process (the repo already has the mapping files). +- Run DynamoDB through `Testcontainers.DynamoDb`, or replace `IPaymentsRepository` for the pure HTTP-contract tests. +- Cover: status codes and body shapes per §3.1, the `MM/yyyy` expiry mapping (§2.4), **no retry on POST** (§2.1: the handler counts calls), the bank timing out, DynamoDB throwing after authorization (§2.2), idempotent replay (§2.3), and `CancellationToken` flow. + +### 🟡 6.2 The E2E tests reuse the server's own models +`tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj:16-19` + +A "black-box" test that serializes with the server's own `PaymentRequest`/`PostPaymentResponse` cannot detect contract drift. Rename a property and both sides change together, and the test still passes. + +**Change:** give the E2E project its own small request/response records, or assert on raw JSON (`JsonNode`) with explicit property names. + +### 🟡 6.3 Specific test smells +- `PaymentControllerTests.PostPayment_CardNumberInvalid_SkipsRemainingChecks` (`:133-144`) has a commented-out line (`// this.SetupValidator(isValid: false);`). It relies on Moq's loose default returning `false`. Set the card check to `false` explicitly. +- `PostPayment_ValidationFails_…` uses a `switch` on method names to configure mocks. That is a sign the validator interface is shaped wrong (§4.5). +- `ErrorMiddlewareTests` asserts the leak (`Assert.Contains("boom", body)`); see §4.6. +- `IsCurrencyCodeValid_ThreeCharacters_ReturnsTrue("123")` pins a weak rule as correct. +- The E2E `Expiry` case uses `DateTime.Now.Year - 1` and `TestData` uses `DateTime.Now.Year + 1`. These are fine, but a fixed clock (`FakeTimeProvider`, §4.4) removes the time-of-year dependence from the unit layer entirely. +- No coverage is collected. Add `Microsoft.Testing.Extensions.CodeCoverage` and publish the percentage in CI. A reviewer trusts a badge more than a claim. +- Moq 4.20.69 is outdated (4.20.72). Given the 2023 SponsorLink episode, some reviewers prefer NSubstitute. That is optional, but know why you chose Moq. + +--- + +## 7. Engineering hygiene (first impressions) + +A reviewer sees these items **before** reading any C#. + +### 🟠 7.1 No CI +There is no `.github/workflows`. + +**Change:** add one workflow that runs `dotnet restore`, `dotnet build -warnaserror`, `dotnet format --verify-no-changes`, the unit and integration tests with coverage, and the vulnerable-package scan. Optionally add a second job that runs `docker compose --profile api up` plus the E2E suite. Put a status badge in the README. + +### 🟠 7.2 README +The current README is five lines. It contains `add later how to run / debug`, typos ("Intentional kept simple so that can demo"), and a claim that retries are omitted when they are not. Meanwhile `AGENTS.md`, written for an AI agent, is thorough. A reviewer notices the imbalance: the machine got better documentation than the human. + +**Change:** a README with: +- a one-paragraph purpose and an architecture diagram (the request flow in `AGENTS.md` is ready to reuse), +- a quick start (`docker compose up`, `dotnet run`, test commands), +- an API table with example requests and responses, +- the card-number → simulator behaviour table, +- **design decisions and trade-offs**: why DynamoDB, why the separate bank models, the retry and idempotency stance, what is out of scope and what would come next. + +A short "Known limitations / next steps" section turns the omissions into deliberate choices. + +### 🟡 7.3 Leftovers that read as unfinished +- `Program.cs:20-21`: a stale comment ("using simple httpClient for dev purpose") directly above code that does something else; commented-out `AddHttpClient()`; three blank lines (`:28-30`); commented-out links and code at the end (`:56-59`). +- `// todo try catch`, `// todo add try catch for all possible scenarios`, `// or return bad request if missing correlationId`, `// see if can replace message with correlationId`, `// most probably not needed, date should be already valid`: resolve each one or move it to a GitHub issue. +- Commented-out XML in `PaymentGatewayApiUnitTests.csproj:10,13-15`. +- `deploy/aws/.gitkeep` with `deploy/aws/*` ignored: an empty promise. Remove it or add real IaC (CDK/Terraform for the table). +- `launchSettings.json` still has the IIS Express profile and an unused `sslPort` from the template, plus a profile named `PaymentGateway.Api` that does not match the project name. +- `PutItemResponse response = …` is assigned and never used (`PaymentsRepository.cs:39`). +- Unused `using` directives: `System.Collections.Generic`, `System.Net`, `System.Net.Http`, `System.Text.Json` and `Microsoft.AspNetCore.Http` in `PaymentController.cs`; `Serilog` in `BankHttpClient.cs` and `BankService.cs`; `PaymentGatewayApi.Services` inside its own namespace in `BankService.cs`; `System`/`System.Net` in `BankResponseProcessor.cs`; `System.Net` in `PaymentsRepository.cs`. `ImplicitUsings` is disabled on purpose, and that choice only pays off if the explicit lists stay exact. Enforce it with IDE0005 as an error. +- Inconsistent `using` order: `System.*`, then `PaymentGatewayApi.*`, then `Microsoft.*` in some files; alphabetical in the tests. + +### 🟡 7.4 Build-quality guard rails +There is no `Directory.Build.props`, `.editorconfig`, `Directory.Packages.props` or `.gitattributes`. + +**Change:** +- A `Directory.Build.props` with `TargetFramework`, `Nullable`, `ImplicitUsings`, `TreatWarningsAsErrors`, `AnalysisLevel=latest-recommended`, `EnforceCodeStyleInBuild=true`, and optionally `GenerateDocumentationFile` for the API project. That removes the duplication across three csproj files. +- Central Package Management (`Directory.Packages.props`), which gives one place to bump versions. +- An `.editorconfig` that encodes the house style, including `this.` qualification (`dotnet_style_qualification_for_field = true:error`), so the convention is enforced by tooling and not only described in `AGENTS.md`. +- A `.gitattributes` (`* text=auto eol=lf`). Today `PaymentGatewayApiUnitTests.csproj` mixes CRLF and LF, `PaymentsRepository.cs` alone has a UTF-8 BOM, and seven files have no final newline (`README.md`, `ErrorMiddleware.cs`, `BankResponseProcessor.cs`, `BankService.cs`, `PaymentsRepository.cs`, `logconfig.json`, the unit-test csproj). +- Bump `AWSSDK.DynamoDBv2` to 4.0.104. Consider Dependabot. + +### 🟡 7.5 Logging configuration +`src/Program.cs:14-16`, `src/logconfig.json` + +- A second `ConfigurationBuilder` reads `logconfig.json` relative to the **working directory**. It gets no environment overrides (`logconfig.Development.json`), no environment variables and no reload. +- There is no bootstrap logger and no `try/catch/finally Log.CloseAndFlush()` around `app.Run()`. Startup failures go unlogged, and the file sink may lose buffered events on shutdown. +- In the container, logs are written to files inside an ephemeral filesystem. The console sink, the one that containers and cloud platforms collect, is "deliberately terse" and **omits CorrelationId**, which is the property that matters most in production. +- There is no `UseSerilogRequestLogging()`, so there is no single summary line per request with status and elapsed time. + +**Change:** move the Serilog section into `appsettings.json` / `appsettings.Development.json` and use `builder.Services.AddSerilog((sp, lc) => lc.ReadFrom.Configuration(builder.Configuration))`. Development gets pretty console plus file output; other environments get JSON to the console (`CompactJsonFormatter`). Add request logging and the flush. + +### 🟡 7.6 Environment configuration +`appsettings.json` has `AWS:ServiceURL = http://localhost:8000`. That is a development-only value in the **base** file, so any environment that forgets to override it points at localhost. + +**Change:** move it to `appsettings.Development.json`. Also document that `dotnet run` against DynamoDB Local still needs *some* AWS credentials to resolve (compose sets dummy ones for the container only). + +### ⚪ 7.7 Git history +The history has several commits named `.` (`95e9870`, `555f4f2`, `7a4ed04`, `0ffd78e`) and many named `minor …`. Reviewers do read `git log`. + +**Change:** +- From now on, use imperative, descriptive messages (Conventional Commits is optional) and squash-merge PRs with a meaningful title. +- Rewriting already-published history is your call. If the repo is mainly a portfolio piece, a single squash onto a clean `main` is defensible. +- Fix the heading `# CLAUDE.md` in `AGENTS.md`. +- Add a `LICENSE`. + +### ⚪ 7.8 Documentation drift in `AGENTS.md` +- Line 73 names `IPostPaymentRepository`, but the code has `IPaymentsRepository`. +- Line 39 says a bare `dotnet test` fails because of "the web project's `Microsoft.NET.Test.Sdk` reference". `src/PaymentGatewayApi.csproj` has no such reference, directly or transitively. + +Inaccurate docs cost a reviewer more trust than missing ones. + +--- + +## 8. Prioritised plan + +These are ordered by how much each item moves a senior reviewer's opinion per hour of work. + +| # | Change | Refs | Effort | +| --- | --- | --- | --- | +| 1 | No automatic retry on bank POST; typed bank client scoped to its own resilience pipeline | 2.1, 4.2 | S | +| 2 | Fix `MM/yyyy` expiry formatting, and make WireMock enforce the format | 2.4 | XS | +| 3 | Fix the three pinned defects (year 0, null-key persist, unhandled transport errors) and delete the `NOTE: defect` tests | 2.6-2.8 | S | +| 4 | Honest HTTP contract: 201 + `Location` + body, Declined as a result not a 400, 502/503 for bank failures, `ProblemDetails`/`ValidationProblemDetails` everywhere, string enums, `{id:guid}` | 3.1-3.4 | M | +| 5 | `AddExceptionHandler` + `AddProblemDetails`; stop leaking exception messages | 4.6 | S | +| 6 | Persist `Pending` → call the bank → update; store `AuthorizationCode`; separate persistence entity | 2.2, 2.5, 3.5 | M | +| 7 | `CancellationToken` everywhere; `TimeProvider`; Options + `ValidateOnStart` | 4.1, 4.3, 4.4 | S | +| 8 | CI workflow (build with warnings as errors, format check, tests + coverage, vulnerable-package scan) + `Directory.Build.props` + `.editorconfig` + CPM | 7.1, 7.4 | S | +| 9 | `WebApplicationFactory` integration tests covering the scenarios in 6.1 | 6.1 | M | +| 10 | README rewrite with design decisions and trade-offs; remove TODOs and commented code; fix `AGENTS.md` drift | 7.2, 7.3, 7.8 | S | +| 11 | `Idempotency-Key` support | 2.3 | M | +| 12 | API-key authentication + merchant scoping on GET; rate limiting; HTTPS/HSTS outside development | 5.1, 5.2 | M | +| 13 | Health checks (live and ready), non-root container, Serilog config consolidation, console output in JSON | 4.8, 5.4, 7.5 | S | +| 14 | Validator redesign (collect all errors, Luhn, ISO-4217 allow-list, minor units, `[GeneratedRegex]` with `[0-9]`) | 2.9, 4.5 | S | + +Items 1-10 are roughly two to three focused days. Once they are done, the repo shows a reviewer that the author knows where payment systems fail and knows the framework's own tools. That is the signal "builds high-quality software" depends on. diff --git a/coverage.settings.xml b/coverage.settings.xml new file mode 100644 index 0000000..1fe3bf4 --- /dev/null +++ b/coverage.settings.xml @@ -0,0 +1,24 @@ + + + + + + + .*PaymentGatewayApi\.dll$ + + + + + + ^System\.CodeDom\.Compiler\.GeneratedCodeAttribute$ + ^System\.Diagnostics\.CodeAnalysis\.ExcludeFromCodeCoverageAttribute$ + + + + + .*\.g\.cs$ + .*[\\/]obj[\\/].* + + + + diff --git a/deploy/aws/.gitkeep b/deploy/aws/.gitkeep deleted file mode 100644 index e69de29..0000000 diff --git a/global.json b/global.json index 3140116..1d364c6 100644 --- a/global.json +++ b/global.json @@ -1,4 +1,8 @@ { + "sdk": { + "version": "10.0.100", + "rollForward": "latestFeature" + }, "test": { "runner": "Microsoft.Testing.Platform" } diff --git a/src/.dockerignore b/src/.dockerignore deleted file mode 100644 index 1249f94..0000000 --- a/src/.dockerignore +++ /dev/null @@ -1,7 +0,0 @@ -bin/ -obj/ -z-logs/ -Simulators/ -docker-compose.yml -Dockerfile -.dockerignore diff --git a/src/Configuration/ApiKeyOptions.cs b/src/Configuration/ApiKeyOptions.cs new file mode 100644 index 0000000..3745576 --- /dev/null +++ b/src/Configuration/ApiKeyOptions.cs @@ -0,0 +1,41 @@ +using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using System.Linq; +using Microsoft.Extensions.Options; + +namespace PaymentGatewayApi.Configuration; + +public sealed class ApiKeyOptions : IValidatableObject +{ + public const string SectionName = "ApiKeys"; + + [Required] + [MinLength(1)] + [ValidateEnumeratedItems] + public MerchantApiKey[] Merchants { get; set; } = []; + + public IEnumerable Validate(ValidationContext validationContext) + { + // one key, one merchant: a shared hash would make the key authenticate as either + if (this.Merchants.Select(merchant => merchant.KeySha256).Distinct().Count() != this.Merchants.Length) + { + yield return new ValidationResult("Each KeySha256 must be unique.", [nameof(this.Merchants)]); + } + } +} + +// only the SHA-256 of a key is configured, so the settings (and anyone who can read them) never hold a usable key +public sealed class MerchantApiKey +{ + [Required] + public string MerchantId { get; set; } = null!; + + // lower-case hex, e.g. printf %s "$KEY" | sha256sum + [Required] + [RegularExpression("^[0-9a-f]{64}$")] + public string KeySha256 { get; set; } = null!; +} + +// source-generated, and unlike ValidateDataAnnotations it validates each configured merchant too +[OptionsValidator] +public sealed partial class ApiKeyOptionsValidator : IValidateOptions; diff --git a/src/Configuration/BankOptions.cs b/src/Configuration/BankOptions.cs new file mode 100644 index 0000000..a372645 --- /dev/null +++ b/src/Configuration/BankOptions.cs @@ -0,0 +1,13 @@ +using System; +using System.ComponentModel.DataAnnotations; + +namespace PaymentGatewayApi.Configuration; + +public sealed class BankOptions +{ + public const string SectionName = "Bank"; + + // the acquiring bank's base address, payments are POSTed to "payments" under it + [Required] + public Uri BaseUrl { get; set; } = null!; +} diff --git a/src/Configuration/DynamoOptions.cs b/src/Configuration/DynamoOptions.cs new file mode 100644 index 0000000..ef2f094 --- /dev/null +++ b/src/Configuration/DynamoOptions.cs @@ -0,0 +1,14 @@ +using System.ComponentModel.DataAnnotations; + +namespace PaymentGatewayApi.Configuration; + +public sealed class DynamoOptions +{ + public const string SectionName = "Dynamo"; + + [Required] + public string PaymentsTable { get; set; } = null!; + + [Required] + public string IdempotencyKeysTable { get; set; } = null!; +} diff --git a/src/Configuration/PaymentOptions.cs b/src/Configuration/PaymentOptions.cs new file mode 100644 index 0000000..08553b2 --- /dev/null +++ b/src/Configuration/PaymentOptions.cs @@ -0,0 +1,18 @@ +using System; +using System.ComponentModel.DataAnnotations; + +namespace PaymentGatewayApi.Configuration; + +public sealed class PaymentOptions +{ + public const string SectionName = "Payments"; + + // ISO 4217 codes the gateway accepts + [Required] + [MinLength(1)] + public string[] SupportedCurrencies { get; set; } = []; + + // how long an Idempotency-Key replays its payment before it can be used again + [Range(typeof(TimeSpan), "00:01:00", "30.00:00:00")] + public TimeSpan IdempotencyKeyRetention { get; set; } +} diff --git a/src/Configuration/RateLimitOptions.cs b/src/Configuration/RateLimitOptions.cs new file mode 100644 index 0000000..18aebfc --- /dev/null +++ b/src/Configuration/RateLimitOptions.cs @@ -0,0 +1,16 @@ +using System; +using System.ComponentModel.DataAnnotations; + +namespace PaymentGatewayApi.Configuration; + +// payment POSTs each merchant may send per window, before answering 429 +public sealed class RateLimitOptions +{ + public const string SectionName = "RateLimiting"; + + [Range(1, int.MaxValue)] + public int PermitLimit { get; set; } + + [Range(typeof(TimeSpan), "00:00:01", "01:00:00")] + public TimeSpan Window { get; set; } +} diff --git a/src/Controllers/PaymentController.cs b/src/Controllers/PaymentController.cs deleted file mode 100644 index 8e05c74..0000000 --- a/src/Controllers/PaymentController.cs +++ /dev/null @@ -1,85 +0,0 @@ -using System; -using System.Collections.Generic; -using System.Net; -using System.Net.Http; -using System.Text.Json; -using System.Threading.Tasks; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Microsoft.AspNetCore.Http; -using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Logging; - -namespace PaymentGatewayApi.Controllers; - -[Route("api/[controller]")] -[ApiController] -public class PaymentController : ControllerBase -{ - private readonly ILogger logger; - private readonly IPaymentsRepository paymentRepo; - private readonly IPaymentRequestValidator validator; - private readonly IBankService bankService; - - public PaymentController( - ILogger logger, IPaymentsRepository paymentRepo, IPaymentRequestValidator validator, IBankService bankService) - { - this.logger = logger; - this.paymentRepo = paymentRepo; - this.validator = validator; - this.bankService = bankService; - } - - // curl -k http://localhost:5067/api/payment - [HttpGet()] - public ActionResult GetHealthcheck() - { - this.logger.LogInformation("GetHealthcheck, Status={Status}", "Started"); - - return new OkObjectResult("ok"); - } - - // curl -k http://localhost:5067/api/payment/16bd4749-d150-48d1-a3ce-257457d74d1f - [HttpGet("{paymentId}")] - public async Task> GetPayment(string paymentId) - { - this.logger.LogInformation("GetPayment, PaymentId={PaymentId}, Status={Status}", paymentId, "Started"); - - var payment = await this.paymentRepo.Get(paymentId); - if (payment is null) - { - return new NotFoundObjectResult("payment not found"); - } - - return new OkObjectResult(payment); - } - - [HttpPost()] - public async Task> PostPayment([FromBody] PaymentRequest payment) - { - payment.PaymentId = Guid.NewGuid().ToString(); - this.logger.LogInformation("PostPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Started"); - - bool isValid = - this.validator.IsCardNumberValid(payment) && - this.validator.IsExpiryValid(payment, DateTime.Now) && - this.validator.IsAmountValid(payment) && - this.validator.IsCurrencyCodeValid(payment) && - this.validator.IsCvvValid(payment); - - if (!isValid) - { - return new BadRequestObjectResult(PaymentStatus.Rejected.ToString()); - } - - // good place to split into a separate service if validation would include fraud detection, sanction screening, etc - - var status = await this.bankService.ExecutePayment(payment); - if (status == PaymentStatus.Authorized) - { - return new OkObjectResult(payment.PaymentId); - } - - return new BadRequestObjectResult(status.ToString()); - } -} diff --git a/src/Controllers/PaymentsController.cs b/src/Controllers/PaymentsController.cs new file mode 100644 index 0000000..4b9bcfc --- /dev/null +++ b/src/Controllers/PaymentsController.cs @@ -0,0 +1,157 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.AspNetCore.RateLimiting; +using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Security; +using PaymentGatewayApi.Services; + +namespace PaymentGatewayApi.Controllers; + +[ApiController] +[Authorize] +[Route("api/payments")] +public partial class PaymentsController : ControllerBase +{ + private readonly ILogger logger; + private readonly IPaymentRequestValidator validator; + private readonly IPaymentService paymentService; + + public PaymentsController(ILogger logger, IPaymentRequestValidator validator, IPaymentService paymentService) + { + this.logger = logger; + this.validator = validator; + this.paymentService = paymentService; + } + + // curl http://localhost:5067/api/payments/16bd4749-d150-48d1-a3ce-257457d74d1f -H "X-Api-Key: dev-key-merchant-a" + [HttpGet("{paymentId:guid}")] + [ProducesResponseType(StatusCodes.Status200OK, "application/json")] + [ProducesResponseType(StatusCodes.Status404NotFound, "application/problem+json")] + public async Task> GetPayment(Guid paymentId, CancellationToken cancellationToken) + { + this.LogGetPayment(paymentId, "Started"); + + Payment? payment = await this.paymentService.FindPayment(this.User.GetMerchantId(), paymentId, cancellationToken); + if (payment is null) + { + this.LogGetPayment(paymentId, "NotFound"); + return this.NotFound(); + } + + return PaymentResponse.From(payment); + } + + // curl -X POST http://localhost:5067/api/payments -H "X-Api-Key: dev-key-merchant-a" -H "Content-Type: application/json" \ + // -d '{"cardNumber":"2222405343248873","expiryMonth":4,"expiryYear":2030,"currency":"GBP","amount":1050,"cvv":"123"}' + [HttpPost] + [EnableRateLimiting(RateLimitPolicies.PaymentsPerMerchant)] + [Consumes("application/json")] + [ProducesResponseType(StatusCodes.Status201Created, "application/json")] + [ProducesResponseType(StatusCodes.Status400BadRequest, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status409Conflict, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status422UnprocessableEntity, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status429TooManyRequests, "application/problem+json")] + [ProducesResponseType(StatusCodes.Status502BadGateway, "application/problem+json")] + public async Task> PostPayment( + PaymentRequest request, + [FromHeader(Name = IdempotencyRecord.HeaderName)] string? idempotencyKey, + CancellationToken cancellationToken) + { + Dictionary errors = new(this.validator.Validate(request)); + if (idempotencyKey is not null && !IdempotencyRecord.IsValidKey(idempotencyKey)) + { + errors[IdempotencyRecord.HeaderName] = ["Must be 1 to 255 visible ASCII characters."]; + } + + if (errors.Count > 0) + { + // no payment is created for an invalid request, so the bank is never called + return this.ValidationProblem(ToModelState(errors)); + } + + PaymentAttempt attempt = await this.paymentService.ProcessPayment( + this.User.GetMerchantId(), request, idempotencyKey, cancellationToken); + + switch (attempt) + { + case PaymentAttempt.Processed processed: + return this.PaymentResult(processed.Payment); + + case PaymentAttempt.Replayed replayed: + // the same answer as the first request with this key, marked so the merchant can tell + this.Response.Headers["Idempotent-Replayed"] = "true"; + return this.PaymentResult(replayed.Payment); + + case PaymentAttempt.InProgress inProgress: + ObjectResult conflict = this.ProblemResult( + StatusCodes.Status409Conflict, + "A payment with this Idempotency-Key is still being processed.", + "Retry the same request later to get its outcome."); + ((ProblemDetails)conflict.Value!).Extensions["paymentId"] = inProgress.PaymentId; + return conflict; + + default: + return this.ProblemResult( + StatusCodes.Status422UnprocessableEntity, + "This Idempotency-Key was already used for a different payment.", + "Use a new key for a new payment."); + } + } + + private ActionResult PaymentResult(Payment payment) + { + // a declined payment is a valid outcome, the issuer said no: it is created like an authorized one + if (payment.Status is PaymentStatus.Authorized or PaymentStatus.Declined) + { + return this.CreatedAtAction(nameof(this.GetPayment), new { paymentId = payment.Id }, PaymentResponse.From(payment)); + } + + return this.BankProblem(payment); + } + + // the bank, not the merchant, is at fault, so never a 4xx (which tells a client not to retry) + private ObjectResult BankProblem(Payment payment) + { + string detail = payment.Status == PaymentStatus.Failed + ? "The acquiring bank did not process the payment and the card was not charged. It is safe to retry as a new payment, with a new Idempotency-Key if one was sent." + : "The acquiring bank's answer was not received, so the card may have been charged. Check the payment before retrying."; + + ObjectResult result = this.ProblemResult(StatusCodes.Status502BadGateway, "The acquiring bank could not complete the payment.", detail); + ProblemDetails problem = (ProblemDetails)result.Value!; + problem.Extensions["paymentId"] = payment.Id; + problem.Extensions["paymentStatus"] = payment.Status.ToString(); + + return result; + } + + // like ControllerBase.Problem, but the caller can still add extensions to the ProblemDetails + private ObjectResult ProblemResult(int statusCode, string title, string detail) + { + ProblemDetails problem = this.ProblemDetailsFactory.CreateProblemDetails(this.HttpContext, statusCode, title, detail: detail); + return new ObjectResult(problem) { StatusCode = statusCode }; + } + + private static ModelStateDictionary ToModelState(IReadOnlyDictionary errors) + { + ModelStateDictionary modelState = new(); + foreach ((string field, string[] messages) in errors) + { + foreach (string message in messages) + { + modelState.AddModelError(field, message); + } + } + + return modelState; + } + + [LoggerMessage(Level = LogLevel.Information, Message = "GetPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogGetPayment(Guid paymentId, string status); +} diff --git a/src/Dockerfile b/src/Dockerfile index d6fd2a8..54916e4 100644 --- a/src/Dockerfile +++ b/src/Dockerfile @@ -1,17 +1,26 @@ +# built from the repository root, it needs the shared Directory.Build.props / Directory.Packages.props: +# docker build -f src/Dockerfile -t payment-gateway-api . FROM mcr.microsoft.com/dotnet/sdk:10.0 AS build -WORKDIR /src +WORKDIR /repo -COPY PaymentGatewayApi.csproj . -RUN dotnet restore PaymentGatewayApi.csproj +COPY global.json Directory.Build.props Directory.Packages.props .editorconfig ./ +COPY src/PaymentGatewayApi.csproj src/ +RUN dotnet restore src/PaymentGatewayApi.csproj -COPY . . -RUN dotnet publish PaymentGatewayApi.csproj \ +COPY src/ src/ +RUN dotnet publish src/PaymentGatewayApi.csproj \ -c Release \ -o /app/publish \ + --no-restore \ /p:UseAppHost=false -FROM mcr.microsoft.com/dotnet/aspnet:10.0 AS final +# chiseled: no shell, no package manager, only what the runtime needs. There is no curl for a +# HEALTHCHECK either; the orchestrator probes /health/live and /health/ready over HTTP instead. +FROM mcr.microsoft.com/dotnet/aspnet:10.0-noble-chiseled AS final WORKDIR /app COPY --from=build /app/publish . +# the image's built-in non-root user +USER $APP_UID + ENTRYPOINT ["dotnet", "PaymentGatewayApi.dll"] diff --git a/src/Health/DynamoHealthCheck.cs b/src/Health/DynamoHealthCheck.cs new file mode 100644 index 0000000..241a19e --- /dev/null +++ b/src/Health/DynamoHealthCheck.cs @@ -0,0 +1,38 @@ +using System.Threading; +using System.Threading.Tasks; +using Amazon.DynamoDBv2; +using Amazon.DynamoDBv2.Model; +using Microsoft.Extensions.Diagnostics.HealthChecks; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; + +namespace PaymentGatewayApi.Health; + +// ready means both tables can be reached and are active (an exception, e.g. DynamoDB being +// unreachable, is reported as unhealthy by the health check service); the bank has no +// side-effect-free endpoint to probe, its outages are handled per request by the circuit breaker +public class DynamoHealthCheck : IHealthCheck +{ + private readonly IAmazonDynamoDB dynamo; + private readonly string[] tableNames; + + public DynamoHealthCheck(IAmazonDynamoDB dynamo, IOptions options) + { + this.dynamo = dynamo; + this.tableNames = [options.Value.PaymentsTable, options.Value.IdempotencyKeysTable]; + } + + public async Task CheckHealthAsync(HealthCheckContext context, CancellationToken cancellationToken = default) + { + foreach (string tableName in this.tableNames) + { + DescribeTableResponse response = await this.dynamo.DescribeTableAsync(tableName, cancellationToken); + if (response.Table.TableStatus != TableStatus.ACTIVE) + { + return HealthCheckResult.Unhealthy($"Table {tableName} is {response.Table.TableStatus}."); + } + } + + return HealthCheckResult.Healthy(); + } +} diff --git a/src/Middleware/CorrelationIdMiddleware.cs b/src/Middleware/CorrelationIdMiddleware.cs new file mode 100644 index 0000000..08c8df5 --- /dev/null +++ b/src/Middleware/CorrelationIdMiddleware.cs @@ -0,0 +1,54 @@ +using System; +using System.Text.RegularExpressions; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Serilog.Context; + +namespace PaymentGatewayApi.Middleware; + +// gives every request a correlation id: the caller's X-Correlation-ID when it is well formed, a new +// GUID otherwise. It is pushed onto every log line of the request and echoed on the response, so a +// merchant can quote it to support. The W3C trace id (traceparent) flows alongside it on its own. +public partial class CorrelationIdMiddleware +{ + public const string HeaderName = "X-Correlation-ID"; + + private static readonly object ItemKey = new(); + + private readonly RequestDelegate next; + + public CorrelationIdMiddleware(RequestDelegate next) + { + this.next = next; + } + + public static string? Get(HttpContext context) + { + return context.Items.TryGetValue(ItemKey, out object? value) ? value as string : null; + } + + public async Task InvokeAsync(HttpContext context) + { + // an unauthenticated header lands in every log line: anything that is not a short token + // (newlines, markup, kilobytes of padding) is replaced rather than trusted + string header = context.Request.Headers[HeaderName].ToString(); + string correlationId = ValidCorrelationId().IsMatch(header) ? header : Guid.NewGuid().ToString(); + + context.Items[ItemKey] = correlationId; + + // set when the response starts, so it survives the exception handler clearing the headers + context.Response.OnStarting(() => + { + context.Response.Headers[HeaderName] = correlationId; + return Task.CompletedTask; + }); + + using (LogContext.PushProperty("CorrelationId", correlationId)) + { + await this.next(context); + } + } + + [GeneratedRegex("^[A-Za-z0-9-]{1,64}$")] + private static partial Regex ValidCorrelationId(); +} diff --git a/src/Middleware/ErrorMiddleware.cs b/src/Middleware/ErrorMiddleware.cs deleted file mode 100644 index 13922cd..0000000 --- a/src/Middleware/ErrorMiddleware.cs +++ /dev/null @@ -1,40 +0,0 @@ -using System; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Http; -using Microsoft.Extensions.Logging; - -namespace PaymentGatewayApi.Middleware; - -public class ErrorMiddleware -{ - private readonly RequestDelegate next; - private readonly ILogger logger; - - public ErrorMiddleware(RequestDelegate next, ILogger logger) - { - this.next = next; - this.logger = logger; - } - - public async Task InvokeAsync(HttpContext context) - { - try - { - await next(context); - } - catch (Exception exception) - { - this.logger.LogError( - exception, - "Unhandled exception, Status={Status}", - "Failed"); - - context.Response.StatusCode = StatusCodes.Status500InternalServerError; - - // see if can replace message with correlationId - await context.Response.WriteAsync($"Unhandled exception\n{exception.Message}\n"); - - return; - } - } -} \ No newline at end of file diff --git a/src/Middleware/GlobalExceptionHandler.cs b/src/Middleware/GlobalExceptionHandler.cs new file mode 100644 index 0000000..1e238f1 --- /dev/null +++ b/src/Middleware/GlobalExceptionHandler.cs @@ -0,0 +1,57 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Diagnostics; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.Extensions.Logging; + +namespace PaymentGatewayApi.Middleware; + +// last resort for anything the pipeline did not handle: log it, answer a generic ProblemDetails +// with the trace id, and never echo the exception message (it can name hosts, tables, keys) +public partial class GlobalExceptionHandler : IExceptionHandler +{ + // nginx's convention for "client closed request", the client is gone and never reads it + public const int ClientClosedRequest = 499; + + private readonly ILogger logger; + private readonly IProblemDetailsService problemDetailsService; + + public GlobalExceptionHandler(ILogger logger, IProblemDetailsService problemDetailsService) + { + this.logger = logger; + this.problemDetailsService = problemDetailsService; + } + + public async ValueTask TryHandleAsync(HttpContext httpContext, Exception exception, CancellationToken cancellationToken) + { + if (exception is OperationCanceledException && httpContext.RequestAborted.IsCancellationRequested) + { + this.LogClientAborted(httpContext.Request.Path, "Cancelled"); + httpContext.Response.StatusCode = ClientClosedRequest; + return true; + } + + this.LogUnhandled(exception, httpContext.Request.Path, "Failed"); + + httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError; + return await this.problemDetailsService.TryWriteAsync(new ProblemDetailsContext + { + HttpContext = httpContext, + Exception = exception, + ProblemDetails = new ProblemDetails + { + Status = StatusCodes.Status500InternalServerError, + Title = "An unexpected error occurred.", + Detail = "Quote the traceId to support." + } + }); + } + + [LoggerMessage(Level = LogLevel.Error, Message = "Unhandled exception, Path={Path}, Status={Status}")] + private partial void LogUnhandled(Exception exception, PathString path, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "Request aborted by the client, Path={Path}, Status={Status}")] + private partial void LogClientAborted(PathString path, string status); +} diff --git a/src/Middleware/LogMiddleware.cs b/src/Middleware/LogMiddleware.cs deleted file mode 100644 index f98d992..0000000 --- a/src/Middleware/LogMiddleware.cs +++ /dev/null @@ -1,38 +0,0 @@ -using System; -using System.Linq; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Http; -using Serilog.Context; - -namespace PaymentGatewayApi.Middleware; - -public class LogMiddleware -{ - private readonly RequestDelegate next; - - public LogMiddleware(RequestDelegate next) - { - this.next = next; - } - - public async Task InvokeAsync(HttpContext context) - { - string? correlationId; - string? correlationHeader = context.Request.Headers["CorrelationId"].FirstOrDefault(); - if (!string.IsNullOrWhiteSpace(correlationHeader)) - { - correlationId = correlationHeader; - } - else - { - // or return bad request if missing correlationId - correlationId = Guid.NewGuid().ToString(); - } - - using (LogContext.PushProperty("CallerId", context.Request.Headers["CallerId"])) - using (LogContext.PushProperty("CorrelationId", correlationId)) - { - await next(context); - } - } -} diff --git a/src/Models/BankOutcome.cs b/src/Models/BankOutcome.cs new file mode 100644 index 0000000..46ecdae --- /dev/null +++ b/src/Models/BankOutcome.cs @@ -0,0 +1,25 @@ +namespace PaymentGatewayApi.Models; + +// what the acquiring bank said about one payment, in the gateway's own terms +public sealed record BankOutcome +{ + public static readonly BankOutcome Declined = new(PaymentStatus.Declined, null); + public static readonly BankOutcome Failed = new(PaymentStatus.Failed, null); + public static readonly BankOutcome Unknown = new(PaymentStatus.Unknown, null); + + private BankOutcome(PaymentStatus status, string? authorizationCode) + { + this.Status = status; + this.AuthorizationCode = authorizationCode; + } + + public PaymentStatus Status { get; } + + // needed later to capture, refund or dispute the payment + public string? AuthorizationCode { get; } + + public static BankOutcome Authorized(string? authorizationCode) + { + return new BankOutcome(PaymentStatus.Authorized, authorizationCode); + } +} diff --git a/src/Models/IdempotencyRecord.cs b/src/Models/IdempotencyRecord.cs new file mode 100644 index 0000000..56fc3ba --- /dev/null +++ b/src/Models/IdempotencyRecord.cs @@ -0,0 +1,50 @@ +using System; +using System.Globalization; +using System.Security.Cryptography; +using System.Text; +using System.Text.RegularExpressions; + +namespace PaymentGatewayApi.Models; + +// ties a merchant's Idempotency-Key to the payment it created, so a retried request replays that +// payment instead of charging the card again +public sealed partial record IdempotencyRecord +{ + public const string HeaderName = "Idempotency-Key"; + + public required string MerchantId { get; init; } + public required string Key { get; init; } + public required Guid PaymentId { get; init; } + + // tells a genuine retry from the same key reused for a different payment + public required string RequestFingerprint { get; init; } + + public required DateTimeOffset CreatedAt { get; init; } + + // after this the key may be used again; DynamoDB's TTL deletes the record some time later + public required DateTimeOffset ExpiresAt { get; init; } + + // visible ASCII, e.g. a GUID or the merchant's own order reference + public static bool IsValidKey(string key) + { + return ValidKey().IsMatch(key); + } + + // hashes what identifies the payment, except the full card number and the CVV: PCI DSS forbids + // keeping the CVV in any form and allows the full number only under a keyed hash + public static string Fingerprint(PaymentRequest request) + { + string identity = string.Join( + '|', + request.CardNumber[^4..], + request.ExpiryMonth.ToString(CultureInfo.InvariantCulture), + request.ExpiryYear.ToString(CultureInfo.InvariantCulture), + request.Currency.ToUpperInvariant(), + request.Amount.ToString(CultureInfo.InvariantCulture)); + + return Convert.ToHexStringLower(SHA256.HashData(Encoding.UTF8.GetBytes(identity))); + } + + [GeneratedRegex("^[\\x21-\\x7E]{1,255}$")] + private static partial Regex ValidKey(); +} diff --git a/src/Models/Payment.cs b/src/Models/Payment.cs new file mode 100644 index 0000000..85dd35c --- /dev/null +++ b/src/Models/Payment.cs @@ -0,0 +1,21 @@ +using System; + +namespace PaymentGatewayApi.Models; + +// the gateway's record of a payment; only the last four card digits are ever kept +public sealed record Payment +{ + public required Guid Id { get; init; } + + // the authenticated merchant that created it, the only one allowed to read it + public required string MerchantId { get; init; } + public required PaymentStatus Status { get; init; } + public required string CardNumberLastFour { get; init; } + public required int ExpiryMonth { get; init; } + public required int ExpiryYear { get; init; } + public required string Currency { get; init; } + public required int Amount { get; init; } + public string? AuthorizationCode { get; init; } + public required DateTimeOffset CreatedAt { get; init; } + public required DateTimeOffset UpdatedAt { get; init; } +} diff --git a/src/Models/PaymentAttempt.cs b/src/Models/PaymentAttempt.cs new file mode 100644 index 0000000..14e0438 --- /dev/null +++ b/src/Models/PaymentAttempt.cs @@ -0,0 +1,23 @@ +using System; + +namespace PaymentGatewayApi.Models; + +// what became of one POST: a payment taken through the bank, or, for a repeated Idempotency-Key, +// what the first request with that key produced +public abstract record PaymentAttempt +{ + private PaymentAttempt() + { + } + + public sealed record Processed(Payment Payment) : PaymentAttempt; + + // the key was used before with the same request: this is that request's payment, unchanged + public sealed record Replayed(Payment Payment) : PaymentAttempt; + + // the key's first request has not finished yet, the caller should retry later + public sealed record InProgress(Guid PaymentId) : PaymentAttempt; + + // the key was used before for a different request + public sealed record KeyReused : PaymentAttempt; +} diff --git a/src/Models/PaymentStatus.cs b/src/Models/PaymentStatus.cs index 77cdc8a..c3336d9 100644 --- a/src/Models/PaymentStatus.cs +++ b/src/Models/PaymentStatus.cs @@ -1,9 +1,11 @@ namespace PaymentGatewayApi.Models; +// persisted by name, so members can be reordered without corrupting stored payments public enum PaymentStatus { - Authorized, // the payment was authorized by the call to the acquiring bank - Declined, // the payment was declined by the call to the acquiring bank - Rejected, // No payment could be created as invalid information was supplied to the payment gateway - InternalError // Payment could not be completed due to an internal error. Should never happen + Pending, // recorded, the bank has not answered yet + Authorized, // the acquiring bank authorized the payment + Declined, // the acquiring bank declined the payment + Failed, // the bank refused the request or was unavailable, the card was not charged + Unknown // no usable answer from the bank (timeout, unreadable body), the card may have been charged and needs reconciliation } diff --git a/src/Models/Requests/BankRequest.cs b/src/Models/Requests/BankRequest.cs index c670f42..4f50e47 100644 --- a/src/Models/Requests/BankRequest.cs +++ b/src/Models/Requests/BankRequest.cs @@ -7,7 +7,7 @@ namespace PaymentGatewayApi.Models; // "card_number": "2222405343248873", // "expiry_date": "04/2025", // "currency": "GBP", -// "amount": 100, +// "amount": 1050, // "cvv": "123" // } @@ -20,6 +20,9 @@ public class BankRequest public required string ExpiryDate { get; set; } public required string Currency { get; set; } - public decimal Amount { get; set; } + + // minor currency unit + public int Amount { get; set; } + public required string Cvv { get; set; } } diff --git a/src/Models/Requests/PaymentRequest.cs b/src/Models/Requests/PaymentRequest.cs index fce54e7..ffccbd3 100644 --- a/src/Models/Requests/PaymentRequest.cs +++ b/src/Models/Requests/PaymentRequest.cs @@ -1,24 +1,39 @@ +using System.Globalization; +using System.Text; + namespace PaymentGatewayApi.Models; // example json payload // { -// "cardnumber": "2222405343248873", -// "ExpiryMonth": "11", -// "ExpiryYear": "2027", +// "cardNumber": "2222405343248873", +// "expiryMonth": 4, +// "expiryYear": 2027, // "currency": "GBP", -// "amount": 100, +// "amount": 1050, // "cvv": "123" // } -public class PaymentRequest +public sealed record PaymentRequest { - public required string CardNumber { get; set; } - public required string Cvv { get; set; } - public required string Currency { get; set; } + public required string CardNumber { get; init; } + public required int ExpiryMonth { get; init; } + public required int ExpiryYear { get; init; } + + // ISO 4217 code, one of Payments:SupportedCurrencies + public required string Currency { get; init; } + + // in the currency's minor unit, 1050 is £10.50 + public required int Amount { get; init; } - public int ExpiryMonth { get; set; } - public int ExpiryYear { get; set; } + public required string Cvv { get; init; } - public decimal Amount { get; set; } - public string? PaymentId { get; set; } + // a record prints every property by default; keep the card number and CVV out of any + // log line or exception message that formats the request + private bool PrintMembers(StringBuilder builder) + { + builder.Append("CardNumber = ****, "); + builder.Append(CultureInfo.InvariantCulture, $"ExpiryMonth = {this.ExpiryMonth}, ExpiryYear = {this.ExpiryYear}, "); + builder.Append(CultureInfo.InvariantCulture, $"Currency = {this.Currency}, Amount = {this.Amount}, Cvv = ***"); + return true; + } } diff --git a/src/Models/Responses/PaymentResponse.cs b/src/Models/Responses/PaymentResponse.cs new file mode 100644 index 0000000..9e7b42b --- /dev/null +++ b/src/Models/Responses/PaymentResponse.cs @@ -0,0 +1,29 @@ +using System; + +namespace PaymentGatewayApi.Models; + +public sealed record PaymentResponse +{ + public required Guid Id { get; init; } + public required PaymentStatus Status { get; init; } + public required string CardNumberLastFour { get; init; } + public required int ExpiryMonth { get; init; } + public required int ExpiryYear { get; init; } + public required string Currency { get; init; } + public required int Amount { get; init; } + + // the authorization code and timestamps stay internal to the gateway + public static PaymentResponse From(Payment payment) + { + return new PaymentResponse + { + Id = payment.Id, + Status = payment.Status, + CardNumberLastFour = payment.CardNumberLastFour, + ExpiryMonth = payment.ExpiryMonth, + ExpiryYear = payment.ExpiryYear, + Currency = payment.Currency, + Amount = payment.Amount + }; + } +} diff --git a/src/Models/Responses/PostPaymentResponse.cs b/src/Models/Responses/PostPaymentResponse.cs deleted file mode 100644 index d84de15..0000000 --- a/src/Models/Responses/PostPaymentResponse.cs +++ /dev/null @@ -1,14 +0,0 @@ -using System; - -namespace PaymentGatewayApi.Models; - -public class PostPaymentResponse -{ - public string? PaymentId { get; set; } - public PaymentStatus Status { get; set; } - public string? CardNumberLastFour { get; set; } - public int ExpiryMonth { get; set; } - public int ExpiryYear { get; set; } - public string? Currency { get; set; } - public decimal Amount { get; set; } -} diff --git a/src/PaymentGatewayApi.csproj b/src/PaymentGatewayApi.csproj index 9a3d37a..e522374 100644 --- a/src/PaymentGatewayApi.csproj +++ b/src/PaymentGatewayApi.csproj @@ -1,17 +1,12 @@ - - net10.0 - enable - disable - - - - - - - + + + + + + diff --git a/src/Program.cs b/src/Program.cs index 25ef009..6ddd5a8 100644 --- a/src/Program.cs +++ b/src/Program.cs @@ -1,59 +1,171 @@ using System; +using System.Text.Json.Serialization; using Amazon.DynamoDBv2; -using Amazon.Extensions.NETCore.Setup; -using PaymentGatewayApi.Middleware; -using PaymentGatewayApi.Services; +using Microsoft.AspNetCore.Authentication; +using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Diagnostics.HealthChecks; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Http.Resilience; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; +using PaymentGatewayApi.Health; +using PaymentGatewayApi.Middleware; +using PaymentGatewayApi.Security; +using PaymentGatewayApi.Services; using Serilog; +using Serilog.Context; +using Serilog.Formatting.Compact; -var builder = WebApplication.CreateBuilder(args); +// catches what goes wrong before the configured logger exists, e.g. an invalid setting +Log.Logger = new LoggerConfiguration().WriteTo.Console(new RenderedCompactJsonFormatter()).CreateLogger(); -var config = new ConfigurationBuilder().AddJsonFile("logconfig.json").Build(); -var log = new LoggerConfiguration().ReadFrom.Configuration(config).CreateLogger(); -builder.Host.UseSerilog(log); +try +{ + var builder = WebApplication.CreateBuilder(args); -builder.Services.AddControllers(); + // sinks, levels and formats come from the "Serilog" section: JSON to the console by default, + // readable console plus files under z-logs/ in Development; ReadFrom.Services lets tests add a sink. + // The host owns (and flushes) this logger; the static Log.Logger stays the startup logger above. + builder.Services.AddSerilog( + (services, logger) => logger.ReadFrom.Configuration(builder.Configuration).ReadFrom.Services(services), + preserveStaticLogger: true); -// using simple httpClient for dev purpose, ideally should be a separate, fine tuned library for retries, circuit breaker -// builder.Services.AddHttpClient(); + builder.Services.AddControllers() + .AddJsonOptions(options => + { + // statuses read as names, and a null where the model says non-nullable is a 400, not a crash later + options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter()); + options.JsonSerializerOptions.RespectNullableAnnotations = true; + }); -builder.Services.ConfigureHttpClientDefaults(http => -{ - http.AddStandardResilienceHandler(); -}); -builder.Services.AddHttpClient(); + // every error body is a ProblemDetails with the request's traceId and correlationId, never an exception message + builder.Services.AddProblemDetails(options => options.CustomizeProblemDetails = context => + { + if (CorrelationIdMiddleware.Get(context.HttpContext) is { } correlationId) + { + context.ProblemDetails.Extensions["correlationId"] = correlationId; + } + }); + builder.Services.AddExceptionHandler(); + + // live: the process answers; ready: it can also reach its table (tagged checks only) + builder.Services.AddHealthChecks() + .AddCheck("dynamodb", tags: ["ready"], timeout: TimeSpan.FromSeconds(3)); + + // a missing or malformed setting fails the startup, not the first request that needs it + builder.Services.AddOptions() + .BindConfiguration(BankOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + builder.Services.AddOptions() + .BindConfiguration(PaymentOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + builder.Services.AddOptions() + .BindConfiguration(DynamoOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + builder.Services.AddOptions() + .BindConfiguration(ApiKeyOptions.SectionName) + .ValidateOnStart(); + builder.Services.AddSingleton, ApiKeyOptionsValidator>(); + builder.Services.AddOptions() + .BindConfiguration(RateLimitOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + builder.Services.AddSingleton(TimeProvider.System); -var awsOptions = builder.Configuration.GetAWSOptions(); -awsOptions.DefaultClientConfig.Timeout = TimeSpan.FromSeconds(10); -builder.Services.AddDefaultAWSOptions(awsOptions); -builder.Services.AddAWSService(); + // merchants authenticate with an API key; every endpoint requires it unless it opts out + // (health checks, the development OpenAPI document), so a new endpoint is never public by accident + builder.Services.AddSingleton(); + builder.Services.AddAuthentication(ApiKeyAuthenticationHandler.SchemeName) + .AddScheme(ApiKeyAuthenticationHandler.SchemeName, null); + builder.Services.AddAuthorizationBuilder() + .SetFallbackPolicy(new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build()); + builder.Services.AddPaymentRateLimiting(); -builder.Services.AddSingleton(); + // a payment POST is not idempotent, a retry after a lost response could charge the card twice; + // keep the timeouts and circuit breaker (tunable under Bank:Resilience), but never retry it + builder.Services.AddHttpClient((services, client) => + client.BaseAddress = services.GetRequiredService>().Value.BaseUrl) + .AddStandardResilienceHandler(builder.Configuration.GetSection("Bank:Resilience")) + .Configure(options => options.Retry.DisableForUnsafeHttpMethods()); -builder.Services.AddScoped(); -builder.Services.AddScoped(); -builder.Services.AddScoped(); -builder.Services.AddScoped(); + var awsOptions = builder.Configuration.GetAWSOptions(); + awsOptions.DefaultClientConfig.Timeout = TimeSpan.FromSeconds(10); + builder.Services.AddDefaultAWSOptions(awsOptions); + builder.Services.AddAWSService(); -builder.Services.AddEndpointsApiExplorer(); -builder.Services.AddSwaggerGen(); + // stateless, so shared; the payment service is scoped because it holds the typed bank client, + // which must not be captured by a singleton (its handler is rotated by IHttpClientFactory) + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + builder.Services.AddScoped(); -var app = builder.Build(); -app.UseSwagger(); -app.UseSwaggerUI(); + // the platform's own OpenAPI document generator, served at /openapi/v1.json + builder.Services.AddOpenApi(); -app.UseMiddleware(); -app.UseMiddleware(); + var app = builder.Build(); -app.MapControllers(); -app.Run(); + // the correlation id wraps everything else, so every log line of the request carries it, + // including the request summary and the exception handler's + app.UseMiddleware(); + app.UseSerilogRequestLogging(options => options.EnrichDiagnosticContext = (diagnostics, context) => + diagnostics.Set("MerchantId", context.User.FindMerchantId())); + app.UseExceptionHandler(); + app.UseStatusCodePages(); -// https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-10.0#header-propagation-middleware -// https://github.com/dotnet/aspnetcore/issues/48581 -// .AddPollyRetries(log); -// app.UseHeaderPropagation(); + if (app.Environment.IsDevelopment()) + { + // the API description and its UI are a development aid, not part of the deployed surface + app.UseSwaggerUI(options => options.SwaggerEndpoint("/openapi/v1.json", "Payment Gateway API")); + } + else + { + // redirects only once an HTTPS port is configured; behind a TLS-terminating load balancer + // that also needs forwarded headers, see the README + app.UseHsts(); + app.UseHttpsRedirection(); + } + + app.UseAuthentication(); + app.UseAuthorization(); + app.UseRateLimiter(); + + // every log line after authentication names the merchant + app.Use(async (context, next) => + { + using (LogContext.PushProperty("MerchantId", context.User.FindMerchantId())) + { + await next(context); + } + }); + + app.MapHealthChecks("/health/live", new HealthCheckOptions { Predicate = _ => false }).AllowAnonymous(); + app.MapHealthChecks("/health/ready", new HealthCheckOptions { Predicate = check => check.Tags.Contains("ready") }).AllowAnonymous(); + app.MapControllers(); + + if (app.Environment.IsDevelopment()) + { + app.MapOpenApi().AllowAnonymous(); + } + + app.Run(); + return 0; +} +catch (Exception exception) when (exception is not HostAbortedException) +{ + // HostAbortedException is how tooling such as WebApplicationFactory stops the host on purpose + Log.Fatal(exception, "Startup, Status={Status}", "Failed"); + return 1; +} +finally +{ + // the file sink buffers, flush it before the process exits + await Log.CloseAndFlushAsync(); +} diff --git a/src/Properties/launchSettings.json b/src/Properties/launchSettings.json index ef6f386..9496cf5 100644 --- a/src/Properties/launchSettings.json +++ b/src/Properties/launchSettings.json @@ -1,15 +1,7 @@ { "$schema": "https://json.schemastore.org/launchsettings.json", - "iisSettings": { - "windowsAuthentication": false, - "anonymousAuthentication": true, - "iisExpress": { - "applicationUrl": "http://localhost:62785", - "sslPort": 44332 - } - }, "profiles": { - "PaymentGateway.Api": { + "PaymentGatewayApi": { "commandName": "Project", "dotnetRunMessages": true, "launchBrowser": true, @@ -18,14 +10,6 @@ "environmentVariables": { "ASPNETCORE_ENVIRONMENT": "Development" } - }, - "IIS Express": { - "commandName": "IISExpress", - "launchBrowser": true, - "launchUrl": "swagger", - "environmentVariables": { - "ASPNETCORE_ENVIRONMENT": "Development" - } } } } diff --git a/src/Security/ApiKeyAuthenticationHandler.cs b/src/Security/ApiKeyAuthenticationHandler.cs new file mode 100644 index 0000000..a3e4dbb --- /dev/null +++ b/src/Security/ApiKeyAuthenticationHandler.cs @@ -0,0 +1,49 @@ +using System.Security.Claims; +using System.Text.Encodings.Web; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authentication; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; + +namespace PaymentGatewayApi.Security; + +// authenticates a merchant from its X-Api-Key header; the merchant id becomes a claim that scopes +// every payment the request creates or reads +public class ApiKeyAuthenticationHandler : AuthenticationHandler +{ + public const string SchemeName = "ApiKey"; + public const string HeaderName = "X-Api-Key"; + + private readonly IMerchantApiKeys merchantApiKeys; + + public ApiKeyAuthenticationHandler( + IOptionsMonitor options, ILoggerFactory logger, UrlEncoder encoder, IMerchantApiKeys merchantApiKeys) + : base(options, logger, encoder) + { + this.merchantApiKeys = merchantApiKeys; + } + + protected override Task HandleAuthenticateAsync() + { + string apiKey = this.Request.Headers[HeaderName].ToString(); + if (apiKey.Length == 0) + { + return Task.FromResult(AuthenticateResult.NoResult()); + } + + string? merchantId = this.merchantApiKeys.FindMerchant(apiKey); + if (merchantId is null) + { + return Task.FromResult(AuthenticateResult.Fail("Unknown API key.")); + } + + ClaimsIdentity identity = new([new Claim(MerchantClaims.MerchantId, merchantId)], SchemeName); + return Task.FromResult(AuthenticateResult.Success(new AuthenticationTicket(new ClaimsPrincipal(identity), SchemeName))); + } + + protected override Task HandleChallengeAsync(AuthenticationProperties properties) + { + this.Response.Headers.WWWAuthenticate = SchemeName; + return base.HandleChallengeAsync(properties); + } +} diff --git a/src/Security/MerchantApiKeys.cs b/src/Security/MerchantApiKeys.cs new file mode 100644 index 0000000..2848439 --- /dev/null +++ b/src/Security/MerchantApiKeys.cs @@ -0,0 +1,37 @@ +using System; +using System.Collections.Frozen; +using System.Collections.Generic; +using System.Security.Cryptography; +using System.Text; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; + +namespace PaymentGatewayApi.Security; + +public interface IMerchantApiKeys +{ + // the merchant the key belongs to, or null for an unknown key + string? FindMerchant(string apiKey); +} + +public class MerchantApiKeys : IMerchantApiKeys +{ + private readonly FrozenDictionary merchantsByKeyHash; + + public MerchantApiKeys(IOptions options) + { + this.merchantsByKeyHash = options.Value.Merchants.ToFrozenDictionary( + merchant => merchant.KeySha256, merchant => merchant.MerchantId, StringComparer.Ordinal); + } + + public static string Hash(string apiKey) + { + return Convert.ToHexStringLower(SHA256.HashData(Encoding.UTF8.GetBytes(apiKey))); + } + + public string? FindMerchant(string apiKey) + { + // the lookup compares hashes, so its timing says nothing about how much of a key was right + return this.merchantsByKeyHash.GetValueOrDefault(Hash(apiKey)); + } +} diff --git a/src/Security/MerchantClaims.cs b/src/Security/MerchantClaims.cs new file mode 100644 index 0000000..d2f3a49 --- /dev/null +++ b/src/Security/MerchantClaims.cs @@ -0,0 +1,20 @@ +using System; +using System.Security.Claims; + +namespace PaymentGatewayApi.Security; + +public static class MerchantClaims +{ + public const string MerchantId = "merchant_id"; + + public static string? FindMerchantId(this ClaimsPrincipal user) + { + return user.FindFirstValue(MerchantId); + } + + // for endpoints behind [Authorize], where a missing claim is a wiring bug, not a client error + public static string GetMerchantId(this ClaimsPrincipal user) + { + return user.FindMerchantId() ?? throw new InvalidOperationException("The request has no authenticated merchant."); + } +} diff --git a/src/Security/RateLimitPolicies.cs b/src/Security/RateLimitPolicies.cs new file mode 100644 index 0000000..6f436d0 --- /dev/null +++ b/src/Security/RateLimitPolicies.cs @@ -0,0 +1,62 @@ +using System; +using System.Globalization; +using System.Threading; +using System.Threading.RateLimiting; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.RateLimiting; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; + +namespace PaymentGatewayApi.Security; + +public static class RateLimitPolicies +{ + public const string PaymentsPerMerchant = "payments-per-merchant"; + + // a fixed window per authenticated merchant, so one merchant's burst cannot starve the others + public static IServiceCollection AddPaymentRateLimiting(this IServiceCollection services) + { + return services.AddRateLimiter(options => + { + options.AddPolicy(PaymentsPerMerchant, context => + { + RateLimitOptions limits = context.RequestServices.GetRequiredService>().Value; + string partition = context.User.FindMerchantId() ?? "anonymous"; + + return RateLimitPartition.GetFixedWindowLimiter(partition, _ => new FixedWindowRateLimiterOptions + { + PermitLimit = limits.PermitLimit, + Window = limits.Window, + QueueLimit = 0 + }); + }); + + options.OnRejected = RejectWithProblem; + }); + } + + private static async ValueTask RejectWithProblem(OnRejectedContext context, CancellationToken cancellationToken) + { + HttpContext httpContext = context.HttpContext; + httpContext.Response.StatusCode = StatusCodes.Status429TooManyRequests; + if (context.Lease.TryGetMetadata(MetadataName.RetryAfter, out TimeSpan retryAfter)) + { + httpContext.Response.Headers.RetryAfter = ((int)Math.Ceiling(retryAfter.TotalSeconds)).ToString(CultureInfo.InvariantCulture); + } + + await httpContext.RequestServices.GetRequiredService().WriteAsync(new ProblemDetailsContext + { + HttpContext = httpContext, + ProblemDetails = new ProblemDetails + { + Status = StatusCodes.Status429TooManyRequests, + Title = "Too many payment requests.", + Detail = "Retry after the time given in the Retry-After header." + } + }); + } +} diff --git a/src/Services/BankClient.cs b/src/Services/BankClient.cs new file mode 100644 index 0000000..fee2d45 --- /dev/null +++ b/src/Services/BankClient.cs @@ -0,0 +1,119 @@ +using System; +using System.Net; +using System.Net.Http; +using System.Net.Http.Json; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using PaymentGatewayApi.Models; +using Polly.CircuitBreaker; + +namespace PaymentGatewayApi.Services; + +public interface IBankClient +{ + Task Authorize(Guid paymentId, PaymentRequest payment, CancellationToken cancellationToken); +} + +// owns the acquiring bank's contract: the request shape, and turning every answer (or its absence) into a BankOutcome +public partial class BankClient : IBankClient +{ + private static readonly Uri PaymentsUri = new("payments", UriKind.Relative); + + private readonly ILogger logger; + private readonly HttpClient client; + + // the base address comes from BankOptions, set where the typed client is registered + public BankClient(ILogger logger, HttpClient client) + { + this.logger = logger; + this.client = client; + } + + public static BankRequest ToBankRequest(PaymentRequest payment) + { + return new BankRequest + { + // the bank expects "MM/yyyy", e.g. "04/2025" + CardNumber = payment.CardNumber, + ExpiryDate = $"{payment.ExpiryMonth:D2}/{payment.ExpiryYear}", + Currency = payment.Currency.ToUpperInvariant(), + Amount = payment.Amount, + Cvv = payment.Cvv + }; + } + + public async Task Authorize(Guid paymentId, PaymentRequest payment, CancellationToken cancellationToken) + { + HttpResponseMessage response; + try + { + response = await this.client.PostAsJsonAsync(PaymentsUri, ToBankRequest(payment), cancellationToken); + } + catch (BrokenCircuitException exception) + { + // the circuit breaker short-circuited the call, the request never left the gateway + this.LogBankUnavailable(exception, paymentId, "Failed"); + return BankOutcome.Failed; + } + catch (Exception exception) when (exception is not OperationCanceledException || !cancellationToken.IsCancellationRequested) + { + // timed out or the connection dropped: the bank may have received and authorized the payment + this.LogBankUnavailable(exception, paymentId, "Unknown"); + return BankOutcome.Unknown; + } + + using (response) + { + if (!response.IsSuccessStatusCode) + { + return this.FromErrorStatus(paymentId, response.StatusCode); + } + + BankResponse? bankResponse; + try + { + bankResponse = await response.Content.ReadFromJsonAsync(cancellationToken); + } + catch (Exception exception) when (exception is JsonException or NotSupportedException) + { + this.LogBankAnswerUnreadable(exception, paymentId, "Unknown"); + return BankOutcome.Unknown; + } + + if (bankResponse is null) + { + this.LogBankAnswerUnreadable(null, paymentId, "Unknown"); + return BankOutcome.Unknown; + } + + this.LogBankAnswered(paymentId, "Completed", bankResponse.Authorized); + + return bankResponse.Authorized ? BankOutcome.Authorized(bankResponse.AuthorizationCode) : BankOutcome.Declined; + } + } + + private BankOutcome FromErrorStatus(Guid paymentId, HttpStatusCode statusCode) + { + // a 4xx means the bank rejected the request and a 503 that it did not process it, so nothing was charged; + // any other 5xx says nothing reliable about whether the payment went through + bool notCharged = (int)statusCode < 500 || statusCode == HttpStatusCode.ServiceUnavailable; + BankOutcome outcome = notCharged ? BankOutcome.Failed : BankOutcome.Unknown; + + this.LogBankError(paymentId, outcome.Status.ToString(), (int)statusCode); + return outcome; + } + + [LoggerMessage(Level = LogLevel.Error, Message = "Authorize, PaymentId={PaymentId}, Status={Status}")] + private partial void LogBankUnavailable(Exception exception, Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Error, Message = "Authorize, PaymentId={PaymentId}, Status={Status}, StatusCode={StatusCode}")] + private partial void LogBankError(Guid paymentId, string status, int statusCode); + + [LoggerMessage(Level = LogLevel.Error, Message = "Authorize, PaymentId={PaymentId}, Status={Status}, the bank's answer could not be read")] + private partial void LogBankAnswerUnreadable(Exception? exception, Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "Authorize, PaymentId={PaymentId}, Status={Status}, Authorized={Authorized}")] + private partial void LogBankAnswered(Guid paymentId, string status, bool authorized); +} diff --git a/src/Services/BankHttpClient.cs b/src/Services/BankHttpClient.cs deleted file mode 100644 index ec64bb8..0000000 --- a/src/Services/BankHttpClient.cs +++ /dev/null @@ -1,69 +0,0 @@ -using System; -using System.Net; -using System.Net.Http; -using System.Net.Http.Json; -using System.Threading.Tasks; -using PaymentGatewayApi.Models; -using Microsoft.Extensions.Configuration; -using Microsoft.Extensions.Logging; -using Serilog; - -namespace PaymentGatewayApi.Services; - -public interface IBankHttpClient -{ - Task PostBankPayment(PaymentRequest payment); -} - - - -public class BankHttpClient : IBankHttpClient -{ - private readonly ILogger logger; - private readonly IConfiguration config; - private readonly IHttpClientFactory httpFactory; - - public BankHttpClient(ILogger logger, IConfiguration config, IHttpClientFactory httpFactory) - { - this.logger = logger; - this.config = config; - this.httpFactory = httpFactory; - } - - public async Task PostBankPayment(PaymentRequest payment) - { - // "04/2025" - string expiry = $"{payment.ExpiryMonth}/{payment.ExpiryYear}"; - BankRequest request = new() - { - CardNumber = payment.CardNumber, - ExpiryDate = expiry, - Currency = payment.Currency, - Amount = payment.Amount, - Cvv = payment.Cvv - }; - - string uri = this.config["SimulatorBankEndpoint"] ?? string.Empty; - HttpClient client = this.httpFactory.CreateClient(); - - HttpResponseMessage response; - try - { - response = await client.PostAsJsonAsync(uri, request); - } - catch (Exception exception) - { - // bank unreachable, timed out, or circuit open once the resilience handler gave up; - // answer as the bank would when unavailable so BankService maps it to InternalError - this.logger.LogError(exception, "PostBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Failed"); - - return new HttpResponseMessage(HttpStatusCode.ServiceUnavailable); - } - - this.logger.LogInformation( - "PostBankPayment, PaymentId={PaymentId}, StatusCode={StatusCode}", - payment.PaymentId, (int)response.StatusCode); - - return response; - } -} diff --git a/src/Services/BankResponseProcessor.cs b/src/Services/BankResponseProcessor.cs deleted file mode 100644 index 6b24e7b..0000000 --- a/src/Services/BankResponseProcessor.cs +++ /dev/null @@ -1,65 +0,0 @@ -using System; -using System.Net.Http; -using System.Net.Http.Json; -using System.Threading.Tasks; -using PaymentGatewayApi.Models; -using Microsoft.Extensions.Logging; -using System.Net; - -namespace PaymentGatewayApi.Services; - -public interface IBankResponseProcessor -{ - Task ParseBankResponse(PaymentRequest payment, HttpResponseMessage response); -} - -public class BankResponseProcessor : IBankResponseProcessor -{ - private readonly ILogger logger; - - public BankResponseProcessor(ILogger logger) - { - this.logger = logger; - } - - public async Task ParseBankResponse(PaymentRequest request, HttpResponseMessage response) - { - BankResponse? bankResponse; - PostPaymentResponse payResponse = new(); - - try - { - bankResponse = await response.Content.ReadFromJsonAsync(); - } - catch (Exception exception) - { - this.logger.LogError(exception, "ParseBankResponse exception, PaymentId={PaymentId}, Status={Status}", request.PaymentId, "Failed"); - - payResponse.Status = PaymentStatus.InternalError; - return payResponse; - } - - if (bankResponse is null) - { - this.logger.LogError("ParseBankResponse null response, PaymentId={PaymentId}, Status={Status}", request.PaymentId, "Failed"); - - payResponse.Status = PaymentStatus.InternalError; - return payResponse; - } - - payResponse.Status = bankResponse.Authorized ? PaymentStatus.Authorized : PaymentStatus.Declined; - payResponse.CardNumberLastFour = request.CardNumber.Substring(request.CardNumber.Length - 4); - - payResponse.PaymentId = request.PaymentId; - payResponse.ExpiryMonth = request.ExpiryMonth; - payResponse.ExpiryYear = request.ExpiryYear; - payResponse.Currency = request.Currency; - payResponse.Amount = request.Amount; - - this.logger.LogInformation( - "ParseBankResponse, PaymentId={PaymentId}, Status={Status}, Authorized={Authorized}", - request.PaymentId, "Completed", bankResponse.Authorized); - - return payResponse; - } -} \ No newline at end of file diff --git a/src/Services/BankService.cs b/src/Services/BankService.cs deleted file mode 100644 index ddb83bb..0000000 --- a/src/Services/BankService.cs +++ /dev/null @@ -1,49 +0,0 @@ -using System.Net.Http; -using System.Threading.Tasks; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Microsoft.Extensions.Logging; -using Serilog; - -namespace PaymentGatewayApi.Services; - -public interface IBankService -{ - Task ExecutePayment(PaymentRequest payment); -} - -public class BankService : IBankService -{ - private readonly ILogger logger; - private readonly IBankHttpClient bankHttpClient; - private readonly IBankResponseProcessor responseProcessor; - private readonly IPaymentsRepository paymentRepo; - - public BankService( - ILogger logger, IBankHttpClient bankHttpClient, IBankResponseProcessor responseProcessor, IPaymentsRepository paymentRepo) - { - this.logger = logger; - this.bankHttpClient = bankHttpClient; - this.responseProcessor = responseProcessor; - this.paymentRepo = paymentRepo; - } - - public async Task ExecutePayment(PaymentRequest payment) - { - this.logger.LogInformation("ExecuteBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Started"); - - HttpResponseMessage bankResponse = await this.bankHttpClient.PostBankPayment(payment); - if (bankResponse.IsSuccessStatusCode) - { - PostPaymentResponse paymentResponse = await this.responseProcessor.ParseBankResponse(payment, bankResponse); - await this.paymentRepo.Add(paymentResponse); - - return paymentResponse.Status; - } - else - { - this.logger.LogError("ExecuteBankPayment, PaymentId={PaymentId}, Status={Status}", payment.PaymentId, "Failed"); - return PaymentStatus.InternalError; - } - } -} \ No newline at end of file diff --git a/src/Services/PaymentRequestValidator.cs b/src/Services/PaymentRequestValidator.cs index 86c7c94..600bc4b 100644 --- a/src/Services/PaymentRequestValidator.cs +++ b/src/Services/PaymentRequestValidator.cs @@ -1,107 +1,99 @@ using System; +using System.Collections.Generic; using System.Text.RegularExpressions; -using PaymentGatewayApi.Models; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; +using PaymentGatewayApi.Models; namespace PaymentGatewayApi.Services; public interface IPaymentRequestValidator { - bool IsCardNumberValid(PaymentRequest payment); - bool IsExpiryValid(PaymentRequest payment, DateTime now); - bool IsAmountValid(PaymentRequest payment); - bool IsCurrencyCodeValid(PaymentRequest payment); - bool IsCvvValid(PaymentRequest payment); + // every rule the request breaks, keyed by its JSON field name; empty when the request is valid + IReadOnlyDictionary Validate(PaymentRequest request); } -public class PaymentRequestValidator : IPaymentRequestValidator +public partial class PaymentRequestValidator : IPaymentRequestValidator { + // cards are issued with an expiry a few years out, anything further is a typo or garbage + private const int MaxExpiryYearsAhead = 20; + private readonly ILogger logger; + private readonly TimeProvider timeProvider; + private readonly HashSet supportedCurrencies; - public PaymentRequestValidator(ILogger logger) + public PaymentRequestValidator( + ILogger logger, TimeProvider timeProvider, IOptions options) { this.logger = logger; + this.timeProvider = timeProvider; + this.supportedCurrencies = new HashSet(options.Value.SupportedCurrencies, StringComparer.OrdinalIgnoreCase); } - public bool IsCardNumberValid(PaymentRequest payment) + public IReadOnlyDictionary Validate(PaymentRequest request) { - if (!Regex.IsMatch(payment.CardNumber, @"^\d{14,19}$")) + Dictionary errors = new(); + + // no Luhn check: the bank simulator picks its answer from the last digit, which a Luhn + // check digit would fix, and the issuer validates the number anyway + if (!CardNumberPattern().IsMatch(request.CardNumber)) { - this.logger.LogInformation( - "Validator.IsCardNumberValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); - return false; + errors["cardNumber"] = ["Must be 14 to 19 digits."]; } - return true; - } - - public bool IsExpiryValid(PaymentRequest payment, DateTime now) - { - // most probably not needed, date should be already valid - if (payment.ExpiryMonth < 1 || payment.ExpiryMonth > 12) + if (request.ExpiryMonth is < 1 or > 12) { - this.logger.LogInformation( - "Validator.IsExpiryMonthValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); - - return false; + errors["expiryMonth"] = ["Must be between 1 and 12."]; } - - var expiry = new DateTime(payment.ExpiryYear, payment.ExpiryMonth, 1); - if (expiry < new DateTime(now.Year, now.Month, 1)) + else if (this.IsExpired(request)) { - this.logger.LogInformation( - "Validator.IsExpiryValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); - - return false; + errors["expiryYear"] = ["The card has expired."]; } - - return true; - } - - public bool IsAmountValid(PaymentRequest payment) - { - if (payment.Amount < 1) + else if (request.ExpiryYear > this.timeProvider.GetUtcNow().Year + MaxExpiryYearsAhead) { - this.logger.LogInformation( - "Validator.IsAmountValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + errors["expiryYear"] = [$"Must be at most {MaxExpiryYearsAhead} years ahead."]; + } - return false; + if (!this.supportedCurrencies.Contains(request.Currency)) + { + errors["currency"] = [$"Must be one of {string.Join(", ", this.supportedCurrencies)}."]; } - return true; - } + if (request.Amount <= 0) + { + errors["amount"] = ["Must be a positive amount in the currency's minor unit."]; + } - public bool IsCurrencyCodeValid(PaymentRequest payment) - { - // should be a list of accepted currencies, via database, config - if (payment.Currency.Length != 3) + if (!CvvPattern().IsMatch(request.Cvv)) { - this.logger.LogInformation( - "Validator.IsCurrencyCodeValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + errors["cvv"] = ["Must be 3 or 4 digits."]; + } - return false; + if (errors.Count > 0) + { + string fields = string.Join(",", errors.Keys); + this.LogRejected("Rejected", fields); } - return true; + return errors; } - public bool IsCvvValid(PaymentRequest payment) + // compares year and month directly, building a DateTime would throw for an out of range year; + // UTC, not the server's time zone, and a card is valid through the last day of its expiry month + private bool IsExpired(PaymentRequest request) { - // 3 or 4 digits - if (!Regex.IsMatch(payment.Cvv, @"^\d{3,4}$")) - { - this.logger.LogInformation( - "Validator.IsCvvValid, PaymentId={PaymentId}, IsValid={IsValid}", - payment.PaymentId, false); + DateTimeOffset now = this.timeProvider.GetUtcNow(); + return request.ExpiryYear < now.Year || (request.ExpiryYear == now.Year && request.ExpiryMonth < now.Month); + } - return false; - } + // [0-9], not \d, which also matches other scripts' digits + [GeneratedRegex("^[0-9]{14,19}$")] + private static partial Regex CardNumberPattern(); - return true; - } + [GeneratedRegex("^[0-9]{3,4}$")] + private static partial Regex CvvPattern(); + + [LoggerMessage(Level = LogLevel.Information, Message = "Validate, Status={Status}, Fields={Fields}")] + private partial void LogRejected(string status, string fields); } diff --git a/src/Services/PaymentService.cs b/src/Services/PaymentService.cs new file mode 100644 index 0000000..bce90e7 --- /dev/null +++ b/src/Services/PaymentService.cs @@ -0,0 +1,171 @@ +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; +using PaymentGatewayApi.Models; + +namespace PaymentGatewayApi.Services; + +public interface IPaymentService +{ + // takes a validated request through the bank and returns the payment with the bank's outcome; with + // an idempotency key already used by this merchant, returns what that key's first request produced + Task ProcessPayment( + string merchantId, PaymentRequest request, string? idempotencyKey, CancellationToken cancellationToken); + + // null when the payment does not exist or belongs to another merchant; the caller cannot tell which + Task FindPayment(string merchantId, Guid paymentId, CancellationToken cancellationToken); +} + +public partial class PaymentService : IPaymentService +{ + private readonly ILogger logger; + private readonly IBankClient bankClient; + private readonly IPaymentsRepository paymentRepo; + private readonly TimeProvider timeProvider; + private readonly TimeSpan idempotencyKeyRetention; + + public PaymentService( + ILogger logger, + IBankClient bankClient, + IPaymentsRepository paymentRepo, + TimeProvider timeProvider, + IOptions options) + { + this.logger = logger; + this.bankClient = bankClient; + this.paymentRepo = paymentRepo; + this.timeProvider = timeProvider; + this.idempotencyKeyRetention = options.Value.IdempotencyKeyRetention; + } + + public async Task ProcessPayment( + string merchantId, PaymentRequest request, string? idempotencyKey, CancellationToken cancellationToken) + { + DateTimeOffset now = this.timeProvider.GetUtcNow(); + Payment pending = new() + { + Id = Guid.NewGuid(), + MerchantId = merchantId, + Status = PaymentStatus.Pending, + CardNumberLastFour = request.CardNumber[^4..], + ExpiryMonth = request.ExpiryMonth, + ExpiryYear = request.ExpiryYear, + Currency = request.Currency.ToUpperInvariant(), + Amount = request.Amount, + CreatedAt = now, + UpdatedAt = now + }; + + this.LogProcessPayment(pending.Id, "Started"); + + // recorded before the bank is called, so a card can never be charged without a record of it; + // if this write fails the request fails and the bank is never called + if (idempotencyKey is null) + { + await this.paymentRepo.Add(pending, cancellationToken); + } + else + { + IdempotencyRecord idempotency = new() + { + MerchantId = merchantId, + Key = idempotencyKey, + PaymentId = pending.Id, + RequestFingerprint = IdempotencyRecord.Fingerprint(request), + CreatedAt = now, + ExpiresAt = now + this.idempotencyKeyRetention + }; + + // the key and the payment are written together, so of two concurrent requests with the + // same key exactly one reaches the bank + if (!await this.paymentRepo.Add(pending, idempotency, cancellationToken)) + { + return await this.ReplayOrReject(merchantId, idempotency, cancellationToken); + } + } + + // a disconnected client can still stop the payment here; once the bank is called, cancelling + // would leave its outcome unknown, so the call and the write that records it run to completion + // (bounded by the bank client's resilience timeouts) + cancellationToken.ThrowIfCancellationRequested(); + + return new PaymentAttempt.Processed(await this.Authorize(pending, request)); + } + + public async Task FindPayment(string merchantId, Guid paymentId, CancellationToken cancellationToken) + { + Payment? payment = await this.paymentRepo.Find(paymentId, cancellationToken); + + // another merchant's payment is reported exactly like a missing one, so ids cannot be probed + return payment?.MerchantId == merchantId ? payment : null; + } + + private async Task Authorize(Payment pending, PaymentRequest request) + { + BankOutcome outcome = await this.bankClient.Authorize(pending.Id, request, CancellationToken.None); + + Payment completed = pending with + { + Status = outcome.Status, + AuthorizationCode = outcome.AuthorizationCode, + UpdatedAt = this.timeProvider.GetUtcNow() + }; + + try + { + await this.paymentRepo.Complete(completed, CancellationToken.None); + } + catch (Exception exception) + { + // the bank has answered, so the merchant still gets the real outcome; the stored payment + // stays Pending, and this log line carries what reconciliation needs to complete it + this.LogCompleteFailed(exception, completed.Id, "Failed", completed.Status, completed.AuthorizationCode); + return completed; + } + + this.LogPaymentCompleted(completed.Id, "Completed", completed.Status); + return completed; + } + + private async Task ReplayOrReject(string merchantId, IdempotencyRecord attempted, CancellationToken cancellationToken) + { + // the write lost to an unexpired record, and records are only deleted once expired + IdempotencyRecord existing = await this.paymentRepo.FindIdempotencyRecord(merchantId, attempted.Key, cancellationToken) + ?? throw new InvalidOperationException("The idempotency record that blocked the payment could not be read."); + + if (existing.RequestFingerprint != attempted.RequestFingerprint) + { + this.LogIdempotencyKeyReused(existing.PaymentId, "Rejected"); + return new PaymentAttempt.KeyReused(); + } + + // written in the same transaction as the key, so it exists; Pending until the first request completes it + Payment? payment = await this.paymentRepo.Find(existing.PaymentId, cancellationToken); + if (payment is null || payment.Status == PaymentStatus.Pending) + { + this.LogIdempotentReplay(existing.PaymentId, "InProgress"); + return new PaymentAttempt.InProgress(existing.PaymentId); + } + + this.LogIdempotentReplay(payment.Id, "Replayed"); + return new PaymentAttempt.Replayed(payment); + } + + [LoggerMessage(Level = LogLevel.Information, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}")] + private partial void LogProcessPayment(Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Information, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}, PaymentStatus={PaymentStatus}")] + private partial void LogPaymentCompleted(Guid paymentId, string status, PaymentStatus paymentStatus); + + [LoggerMessage(Level = LogLevel.Information, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}, a repeated Idempotency-Key")] + private partial void LogIdempotentReplay(Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Warning, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}, the Idempotency-Key was used for a different request")] + private partial void LogIdempotencyKeyReused(Guid paymentId, string status); + + [LoggerMessage(Level = LogLevel.Critical, Message = "ProcessPayment, PaymentId={PaymentId}, Status={Status}, the bank's outcome was not stored, PaymentStatus={PaymentStatus}, AuthorizationCode={AuthorizationCode}")] + private partial void LogCompleteFailed(Exception exception, Guid paymentId, string status, PaymentStatus paymentStatus, string? authorizationCode); +} diff --git a/src/Services/PaymentsRepository.cs b/src/Services/PaymentsRepository.cs index 32423d8..7c34f9a 100644 --- a/src/Services/PaymentsRepository.cs +++ b/src/Services/PaymentsRepository.cs @@ -1,96 +1,227 @@ -using System; +using System; using System.Collections.Generic; -using System.Linq; -using System.Net; -using System.Text.Json; +using System.Globalization; +using System.Threading; using System.Threading.Tasks; using Amazon.DynamoDBv2; using Amazon.DynamoDBv2.Model; +using Microsoft.Extensions.Options; +using PaymentGatewayApi.Configuration; using PaymentGatewayApi.Models; -using Microsoft.Extensions.Logging; namespace PaymentGatewayApi.Services; public interface IPaymentsRepository { - Task Add(PostPaymentResponse payment); - Task Get(string paymentId); + // stores a new payment, failing if its id already exists + Task Add(Payment payment, CancellationToken cancellationToken); + + // stores a new payment and its idempotency key atomically; false, storing nothing, when the + // merchant's key is already held by an earlier payment + Task Add(Payment payment, IdempotencyRecord idempotency, CancellationToken cancellationToken); + + Task FindIdempotencyRecord(string merchantId, string key, CancellationToken cancellationToken); + + // records the bank's outcome on a payment that is still pending + Task Complete(Payment payment, CancellationToken cancellationToken); + + Task Find(Guid paymentId, CancellationToken cancellationToken); } +// one item per payment, keyed by PaymentId, with one attribute per field; the item is the storage +// contract, kept apart from the public response so either can change without migrating the other. +// Idempotency keys live in their own table, keyed by "{MerchantId}#{Key}" so merchants never collide. public class PaymentsRepository : IPaymentsRepository { - private readonly ILogger logger; private readonly IAmazonDynamoDB dynamo; + private readonly TimeProvider timeProvider; + private readonly string tableName; + private readonly string idempotencyTableName; - public PaymentsRepository(ILogger logger, IAmazonDynamoDB dynamo) + public PaymentsRepository(IAmazonDynamoDB dynamo, TimeProvider timeProvider, IOptions options) { - this.logger = logger; this.dynamo = dynamo; + this.timeProvider = timeProvider; + this.tableName = options.Value.PaymentsTable; + this.idempotencyTableName = options.Value.IdempotencyKeysTable; + } + + public async Task Add(Payment payment, CancellationToken cancellationToken) + { + PutItemRequest request = new() + { + TableName = this.tableName, + Item = ToItem(payment), + ConditionExpression = "attribute_not_exists(PaymentId)" + }; + + await this.dynamo.PutItemAsync(request, cancellationToken); } - public async Task Add(PostPaymentResponse payment) + public async Task Add(Payment payment, IdempotencyRecord idempotency, CancellationToken cancellationToken) { - var item = new Dictionary + TransactWriteItemsRequest request = new() { - ["PaymentId"] = new AttributeValue { S = payment.PaymentId }, - ["PaymentRaw"] = new AttributeValue { S = JsonSerializer.Serialize(payment) } + TransactItems = + [ + new TransactWriteItem + { + Put = new Put + { + TableName = this.tableName, + Item = ToItem(payment), + ConditionExpression = "attribute_not_exists(PaymentId)" + } + }, + new TransactWriteItem + { + Put = new Put + { + TableName = this.idempotencyTableName, + Item = ToItem(idempotency), + + // an expired key may be taken again even before the TTL has deleted it + ConditionExpression = "attribute_not_exists(IdempotencyKey) OR ExpiresAt < :now", + ExpressionAttributeValues = new Dictionary + { + [":now"] = new AttributeValue { N = this.timeProvider.GetUtcNow().ToUnixTimeSeconds().ToString(CultureInfo.InvariantCulture) } + } + } + } + ] }; try { - await this.dynamo.PutItemAsync("Payments", item); + await this.dynamo.TransactWriteItemsAsync(request, cancellationToken); + return true; } - catch (Exception exception) + catch (TransactionCanceledException exception) when (exception.CancellationReasons is [_, { Code: "ConditionalCheckFailed" }]) { - // the bank has already answered, so failing the request here would hide an authorized payment - // from the merchant; log the raw item instead so it can be reconciled - this.logger.LogError( - exception, - "AddPayment, PaymentId={PaymentId}, Status={Status}, PaymentRaw={PaymentRaw}", - payment.PaymentId, "Failed", item["PaymentRaw"].S); + return false; } } - public async Task Get(string paymentId) + public async Task FindIdempotencyRecord(string merchantId, string key, CancellationToken cancellationToken) { - var key = new Dictionary + GetItemRequest request = new() { - ["PaymentId"] = new AttributeValue { S = paymentId } + TableName = this.idempotencyTableName, + Key = new Dictionary { ["IdempotencyKey"] = new AttributeValue { S = IdempotencyItemKey(merchantId, key) } }, + + // read right after another request's write lost the race for the same key + ConsistentRead = true }; - GetItemRequest request = new() { Key = key, TableName = "Payments" }; + GetItemResponse response = await this.dynamo.GetItemAsync(request, cancellationToken); - PostPaymentResponse? payment; - try - { - GetItemResponse response = await this.dynamo.GetItemAsync(request); + return response.Item is { Count: > 0 } item ? FromIdempotencyItem(item) : null; + } - if (response.Item != null && response.Item.Any()) - { - string raw = response.Item["PaymentRaw"].S; - payment = JsonSerializer.Deserialize(raw); - } - else + public async Task Complete(Payment payment, CancellationToken cancellationToken) + { + // an outcome is recorded once: a payment that already left Pending is never overwritten + PutItemRequest request = new() + { + TableName = this.tableName, + Item = ToItem(payment), + ConditionExpression = "#status = :pending", + ExpressionAttributeNames = new Dictionary { ["#status"] = "Status" }, + ExpressionAttributeValues = new Dictionary { - payment = null; + [":pending"] = new AttributeValue { S = nameof(PaymentStatus.Pending) } } - } - catch (Exception exception) + }; + + await this.dynamo.PutItemAsync(request, cancellationToken); + } + + public async Task Find(Guid paymentId, CancellationToken cancellationToken) + { + GetItemRequest request = new() { - // rethrow so ErrorMiddleware answers 500: returning null would report an unreadable payment as not found - this.logger.LogError(exception, "GetPayment, PaymentId={PaymentId}, Status={Status}", paymentId, "Failed"); - throw; - } + TableName = this.tableName, + Key = new Dictionary { ["PaymentId"] = new AttributeValue { S = paymentId.ToString() } } + }; - if (payment is not null) + GetItemResponse response = await this.dynamo.GetItemAsync(request, cancellationToken); + + return response.Item is { Count: > 0 } item ? FromItem(item) : null; + } + + private static Dictionary ToItem(Payment payment) + { + Dictionary item = new() + { + ["PaymentId"] = new AttributeValue { S = payment.Id.ToString() }, + ["MerchantId"] = new AttributeValue { S = payment.MerchantId }, + ["Status"] = new AttributeValue { S = payment.Status.ToString() }, + ["CardNumberLastFour"] = new AttributeValue { S = payment.CardNumberLastFour }, + ["ExpiryMonth"] = new AttributeValue { N = payment.ExpiryMonth.ToString(CultureInfo.InvariantCulture) }, + ["ExpiryYear"] = new AttributeValue { N = payment.ExpiryYear.ToString(CultureInfo.InvariantCulture) }, + ["Currency"] = new AttributeValue { S = payment.Currency }, + ["Amount"] = new AttributeValue { N = payment.Amount.ToString(CultureInfo.InvariantCulture) }, + ["CreatedAt"] = new AttributeValue { S = payment.CreatedAt.ToString("O", CultureInfo.InvariantCulture) }, + ["UpdatedAt"] = new AttributeValue { S = payment.UpdatedAt.ToString("O", CultureInfo.InvariantCulture) } + }; + + if (payment.AuthorizationCode is not null) { - return payment; + item["AuthorizationCode"] = new AttributeValue { S = payment.AuthorizationCode }; } - else + + return item; + } + + private static string IdempotencyItemKey(string merchantId, string key) + { + return $"{merchantId}#{key}"; + } + + private static Dictionary ToItem(IdempotencyRecord record) + { + return new Dictionary { - this.logger.LogInformation("GetPayment, PaymentId={PaymentId}, Status={Status}", paymentId, "NotFound"); + ["IdempotencyKey"] = new AttributeValue { S = IdempotencyItemKey(record.MerchantId, record.Key) }, + ["MerchantId"] = new AttributeValue { S = record.MerchantId }, + ["Key"] = new AttributeValue { S = record.Key }, + ["PaymentId"] = new AttributeValue { S = record.PaymentId.ToString() }, + ["RequestFingerprint"] = new AttributeValue { S = record.RequestFingerprint }, + ["CreatedAt"] = new AttributeValue { S = record.CreatedAt.ToString("O", CultureInfo.InvariantCulture) }, - return null; - } + // epoch seconds, the format DynamoDB's TTL reads + ["ExpiresAt"] = new AttributeValue { N = record.ExpiresAt.ToUnixTimeSeconds().ToString(CultureInfo.InvariantCulture) } + }; + } + + private static IdempotencyRecord FromIdempotencyItem(Dictionary item) + { + return new IdempotencyRecord + { + MerchantId = item["MerchantId"].S, + Key = item["Key"].S, + PaymentId = Guid.Parse(item["PaymentId"].S), + RequestFingerprint = item["RequestFingerprint"].S, + CreatedAt = DateTimeOffset.Parse(item["CreatedAt"].S, CultureInfo.InvariantCulture), + ExpiresAt = DateTimeOffset.FromUnixTimeSeconds(long.Parse(item["ExpiresAt"].N, CultureInfo.InvariantCulture)) + }; } -} \ No newline at end of file + + private static Payment FromItem(Dictionary item) + { + return new Payment + { + Id = Guid.Parse(item["PaymentId"].S), + MerchantId = item["MerchantId"].S, + Status = Enum.Parse(item["Status"].S), + CardNumberLastFour = item["CardNumberLastFour"].S, + ExpiryMonth = int.Parse(item["ExpiryMonth"].N, CultureInfo.InvariantCulture), + ExpiryYear = int.Parse(item["ExpiryYear"].N, CultureInfo.InvariantCulture), + Currency = item["Currency"].S, + Amount = int.Parse(item["Amount"].N, CultureInfo.InvariantCulture), + AuthorizationCode = item.TryGetValue("AuthorizationCode", out AttributeValue? code) ? code.S : null, + CreatedAt = DateTimeOffset.Parse(item["CreatedAt"].S, CultureInfo.InvariantCulture), + UpdatedAt = DateTimeOffset.Parse(item["UpdatedAt"].S, CultureInfo.InvariantCulture) + }; + } +} diff --git a/src/Simulators/wiremock/mappings/authorized.json b/src/Simulators/wiremock/mappings/authorized.json index a11f16b..ddc83f9 100644 --- a/src/Simulators/wiremock/mappings/authorized.json +++ b/src/Simulators/wiremock/mappings/authorized.json @@ -4,7 +4,7 @@ "url": "/payments", "bodyPatterns": [ { "matchesJsonPath": { "expression": "$.card_number", "matches": "^.*[13579]$" } }, - { "matchesJsonPath": "$.expiry_date" }, + { "matchesJsonPath": { "expression": "$.expiry_date", "matches": "^(0[1-9]|1[0-2])/[0-9]{4}$" } }, { "matchesJsonPath": "$.currency" }, { "matchesJsonPath": "$.amount" }, { "matchesJsonPath": "$.cvv" } diff --git a/src/Simulators/wiremock/mappings/declined.json b/src/Simulators/wiremock/mappings/declined.json index 9b7ccb1..a7aa61f 100644 --- a/src/Simulators/wiremock/mappings/declined.json +++ b/src/Simulators/wiremock/mappings/declined.json @@ -4,7 +4,7 @@ "url": "/payments", "bodyPatterns": [ { "matchesJsonPath": { "expression": "$.card_number", "matches": "^.*[2468]$" } }, - { "matchesJsonPath": "$.expiry_date" }, + { "matchesJsonPath": { "expression": "$.expiry_date", "matches": "^(0[1-9]|1[0-2])/[0-9]{4}$" } }, { "matchesJsonPath": "$.currency" }, { "matchesJsonPath": "$.amount" }, { "matchesJsonPath": "$.cvv" } diff --git a/src/Simulators/wiremock/mappings/unavailable.json b/src/Simulators/wiremock/mappings/unavailable.json index 7fb2f3b..15bf743 100644 --- a/src/Simulators/wiremock/mappings/unavailable.json +++ b/src/Simulators/wiremock/mappings/unavailable.json @@ -4,7 +4,7 @@ "url": "/payments", "bodyPatterns": [ { "matchesJsonPath": { "expression": "$.card_number", "matches": "^.*0$" } }, - { "matchesJsonPath": "$.expiry_date" }, + { "matchesJsonPath": { "expression": "$.expiry_date", "matches": "^(0[1-9]|1[0-2])/[0-9]{4}$" } }, { "matchesJsonPath": "$.currency" }, { "matchesJsonPath": "$.amount" }, { "matchesJsonPath": "$.cvv" } diff --git a/src/appsettings.Development.json b/src/appsettings.Development.json new file mode 100644 index 0000000..d70c3e2 --- /dev/null +++ b/src/appsettings.Development.json @@ -0,0 +1,44 @@ +{ + "Serilog": { + "Using": [ "Serilog.Sinks.Console", "Serilog.Sinks.File" ], + "WriteTo": { + "Console": { + "Args": { + "formatter": { + "type": "Serilog.Formatting.Display.MessageTemplateTextFormatter, Serilog", + "outputTemplate": "[{Timestamp:HH:mm:ss.fff} {Level:u3}] {CorrelationId} {Message:l}{NewLine}{Exception}" + } + } + }, + "InfoFile": { + "Name": "File", + "Args": { + "path": "z-logs/info-.log", + "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff} {Level:u3}] CorrelationId={CorrelationId}, TraceId={TraceId}, Message={Message:l}, Source={SourceContext}{NewLine}{Exception}", + "rollingInterval": "Day" + } + }, + "ErrorFile": { + "Name": "File", + "Args": { + "path": "z-logs/error-.log", + "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff} {Level:u3}] CorrelationId={CorrelationId}, TraceId={TraceId}, Message={Message:l}, Source={SourceContext}{NewLine}{Exception}", + "rollingInterval": "Day", + "restrictedToMinimumLevel": "Warning" + } + } + } + }, + "ApiKeys": { + "Merchants": [ + { "MerchantId": "merchant-a", "KeySha256": "c8c3a5a56d9ba1bff80c60a7d11478525838e0bef3d728fc4185aa3da1b84015" }, + { "MerchantId": "merchant-b", "KeySha256": "eb56dcd8cc40c7452f90e58fb83f526c4985c56cd51633ebe3f40131f0d1dd41" } + ] + }, + "Bank": { + "BaseUrl": "http://localhost:8080/" + }, + "AWS": { + "ServiceURL": "http://localhost:8000" + } +} diff --git a/src/appsettings.json b/src/appsettings.json index 7bfeaa6..331676a 100644 --- a/src/appsettings.json +++ b/src/appsettings.json @@ -1,9 +1,45 @@ { "AllowedHosts": "*", - "SimulatorBankEndpoint": "http://localhost:8080/payments", + "Serilog": { + "Using": [ "Serilog.Sinks.Console" ], + "MinimumLevel": { + "Default": "Information", + "Override": { + "Microsoft.AspNetCore": "Warning", + "System.Net.Http": "Warning", + "Polly": "Warning" + } + }, + "WriteTo": { + "Console": { + "Name": "Console", + "Args": { + "formatter": { "type": "Serilog.Formatting.Compact.RenderedCompactJsonFormatter, Serilog.Formatting.Compact" } + } + } + }, + "Enrich": [ "FromLogContext" ] + }, + "Bank": { + "Resilience": { + "AttemptTimeout": { "Timeout": "00:00:10" }, + "TotalRequestTimeout": { "Timeout": "00:00:30" } + } + }, + "RateLimiting": { + "PermitLimit": 100, + "Window": "00:00:10" + }, + "Payments": { + "SupportedCurrencies": [ "GBP", "EUR", "USD" ], + "IdempotencyKeyRetention": "1.00:00:00" + }, + "Dynamo": { + "PaymentsTable": "Payments", + "IdempotencyKeysTable": "PaymentIdempotencyKeys" + }, "AWS": { "Region": "eu-west-2", - "ServiceURL": "http://localhost:8000", "MaxErrorRetry": 0 } } diff --git a/src/docker-compose.yml b/src/docker-compose.yml index 6cf5efb..1d8a921 100644 --- a/src/docker-compose.yml +++ b/src/docker-compose.yml @@ -31,6 +31,20 @@ services: --provisioned-throughput ReadCapacityUnits=1,WriteCapacityUnits=1 \ --table-class STANDARD > /dev/null 2>&1 + aws dynamodb create-table \ + --endpoint-url http://dynamodb:8000 \ + --table-name PaymentIdempotencyKeys \ + --attribute-definitions \ + AttributeName=IdempotencyKey,AttributeType=S \ + --key-schema AttributeName=IdempotencyKey,KeyType=HASH \ + --provisioned-throughput ReadCapacityUnits=1,WriteCapacityUnits=1 \ + --table-class STANDARD > /dev/null 2>&1 + + aws dynamodb update-time-to-live \ + --endpoint-url http://dynamodb:8000 \ + --table-name PaymentIdempotencyKeys \ + --time-to-live-specification Enabled=true,AttributeName=ExpiresAt > /dev/null 2>&1 + echo "local dynamodb setup completed" # https://github.com/wiremock/wiremock @@ -53,11 +67,18 @@ services: payment-gateway-api: container_name: payment-gateway-api profiles: ["api"] - build: . + build: + context: .. + dockerfile: src/Dockerfile ports: - "5067:8080" environment: - SimulatorBankEndpoint: "http://bank-simulator-wiremock:8080/payments" + Bank__BaseUrl: "http://bank-simulator-wiremock:8080/" + # the local keys "dev-key-merchant-a" / "dev-key-merchant-b", as their SHA-256 + ApiKeys__Merchants__0__MerchantId: "merchant-a" + ApiKeys__Merchants__0__KeySha256: "c8c3a5a56d9ba1bff80c60a7d11478525838e0bef3d728fc4185aa3da1b84015" + ApiKeys__Merchants__1__MerchantId: "merchant-b" + ApiKeys__Merchants__1__KeySha256: "eb56dcd8cc40c7452f90e58fb83f526c4985c56cd51633ebe3f40131f0d1dd41" AWS__ServiceURL: "http://dynamodb:8000" AWS_ACCESS_KEY_ID: "dummy" AWS_SECRET_ACCESS_KEY: "dummy" diff --git a/src/logconfig.json b/src/logconfig.json deleted file mode 100644 index 9b4268c..0000000 --- a/src/logconfig.json +++ /dev/null @@ -1,44 +0,0 @@ -{ - "Serilog": { - "Using": [ - "Serilog.Sinks.Console", - "Serilog.Sinks.File" - ], - "MinimumLevel": { - "Default": "Information", - "Override": { - "Microsoft.AspNetCore": "Warning", - "System.Net.Http": "Warning" - } - }, - "WriteTo": [ - { - "Name": "Console", - "Args": { - "theme": "Serilog.Sinks.SystemConsole.Themes.AnsiConsoleTheme::Sixteen, Serilog.Sinks.Console", - "outputTemplate": "[{Timestamp:HH:mm:ss.fff}] [{Level:u3}], Message={Message}{NewLine}" - } - }, - { - "Name": "File", - "Args": { - "path": "z-logs/info-.log", - "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff}] [{Level:u3}] CallerId={CallerId:l}, CorrelationId={CorrelationId:l}, Message={Message}, Source={SourceContext}{NewLine}", - "rollingInterval": "Day" - } - }, - { - "Name": "File", - "Args": { - "path": "z-logs/error-.log", - "outputTemplate": "[{Timestamp:yyyy-MM-dd HH:mm:ss.fff}] [{Level:u3}] CallerId={CallerId:l}, CorrelationId={CorrelationId:l}, Message={Message},Source={SourceContext}{NewLine}Exception={Exception}{NewLine}", - "rollingInterval": "Day", - "restrictedToMinimumLevel": "Warning" - } - } - ], - "Enrich": [ - "FromLogContext" - ] - } -} \ No newline at end of file diff --git a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs index 4c811da..fd2f02a 100644 --- a/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs +++ b/tests/PaymentGatewayApiEndToEndTests/ApiFixture.cs @@ -8,24 +8,37 @@ namespace PaymentGatewayApiEndToEndTests; -// shared by every test, waits for the api started by docker compose to answer its healthcheck -public class ApiFixture : IAsyncLifetime +// shared by every test, waits for the api started by docker compose to answer its liveness check +public sealed class ApiFixture : IAsyncLifetime { + // the local keys configured in docker-compose.yml and appsettings.Development.json + public const string MerchantAKey = "dev-key-merchant-a"; + public const string MerchantBKey = "dev-key-merchant-b"; + private static readonly TimeSpan StartupTimeout = TimeSpan.FromSeconds(30); - public HttpClient Client { get; } + private readonly Uri baseAddress = new(Environment.GetEnvironmentVariable("E2E_BASE_URL") ?? "http://localhost:5067"); public ApiFixture() { - string baseUrl = Environment.GetEnvironmentVariable("E2E_BASE_URL") ?? "http://localhost:5067"; + this.Client = this.CreateClient(MerchantAKey); + this.Anonymous = this.CreateClient(null); + } + + // authenticated as merchant A + public HttpClient Client { get; } - // long timeout, the api retries the bank's 503 before answering - this.Client = new HttpClient + public HttpClient Anonymous { get; } + + public HttpClient CreateClient(string? apiKey) + { + HttpClient client = new() { BaseAddress = this.baseAddress, Timeout = TimeSpan.FromSeconds(30) }; + if (apiKey is not null) { - BaseAddress = new Uri(baseUrl), - Timeout = TimeSpan.FromSeconds(60) - }; - this.Client.DefaultRequestHeaders.Add("CallerId", "e2e-tests"); + client.DefaultRequestHeaders.Add("X-Api-Key", apiKey); + } + + return client; } public async ValueTask InitializeAsync() @@ -35,7 +48,7 @@ public async ValueTask InitializeAsync() { try { - HttpResponseMessage response = await this.Client.GetAsync("api/payment"); + HttpResponseMessage response = await this.Anonymous.GetAsync(new Uri("health/live", UriKind.Relative)); if (response.IsSuccessStatusCode) { return; @@ -50,13 +63,14 @@ public async ValueTask InitializeAsync() } throw new InvalidOperationException( - $"Payment gateway not reachable at {this.Client.BaseAddress} after {StartupTimeout.TotalSeconds}s. " + + $"Payment gateway not reachable at {this.baseAddress} after {StartupTimeout.TotalSeconds}s. " + "Start the stack from src/ with: docker compose --profile api up -d --build"); } public ValueTask DisposeAsync() { this.Client.Dispose(); + this.Anonymous.Dispose(); return ValueTask.CompletedTask; } } diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs index 66c3c91..7747552 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentEndToEndTests.cs @@ -2,126 +2,263 @@ using System.Net; using System.Net.Http; using System.Net.Http.Json; +using System.Text.Json.Nodes; using System.Threading.Tasks; -using PaymentGatewayApi.Models; using Xunit; namespace PaymentGatewayApiEndToEndTests; public class PaymentEndToEndTests { + private readonly ApiFixture fixture; private readonly HttpClient client; public PaymentEndToEndTests(ApiFixture fixture) { + this.fixture = fixture; this.client = fixture.Client; } - [Fact] - public async Task Healthcheck_ReturnsOk() + private Task Post(PaymentBody payment) + { + return this.client.PostAsJsonAsync("api/payments", payment, TestContext.Current.CancellationToken); + } + + private static async Task Post(HttpClient caller, PaymentBody payment, string idempotencyKey) + { + using HttpRequestMessage request = new(HttpMethod.Post, "api/payments") { Content = JsonContent.Create(payment) }; + request.Headers.Add("Idempotency-Key", idempotencyKey); + + return await caller.SendAsync(request, TestContext.Current.CancellationToken); + } + + private static async Task ReadJson(HttpResponseMessage response) + { + JsonNode? json = JsonNode.Parse(await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + Assert.NotNull(json); + return json; + } + + private static async Task AssertProblem(HttpResponseMessage response, HttpStatusCode expected) + { + Assert.Equal(expected, response.StatusCode); + Assert.Equal("application/problem+json", response.Content.Headers.ContentType?.MediaType); + + JsonNode problem = await ReadJson(response); + Assert.Equal((int)expected, problem["status"]?.GetValue()); + Assert.NotNull(problem["traceId"]); + return problem; + } + + [Theory] + [InlineData("health/live")] + [InlineData("health/ready")] // the stack's DynamoDB table is reachable + public async Task Health_ReturnsHealthy(string path) { - HttpResponseMessage response = await this.client.GetAsync("api/payment", TestContext.Current.CancellationToken); + HttpResponseMessage response = await this.fixture.Anonymous.GetAsync(path, TestContext.Current.CancellationToken); Assert.Equal(HttpStatusCode.OK, response.StatusCode); - Assert.Equal("ok", await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + Assert.Equal("Healthy", await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); } [Fact] - public async Task PostPayment_OddCard_AuthorizedAndRetrievable() + public async Task OpenApi_NotServedOutsideDevelopment() + { + // the container runs in Production; asked anonymously, any unmapped path is a 401 like every other + // endpoint that does not opt out of authentication + HttpResponseMessage response = await this.client.GetAsync("openapi/v1.json", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Theory] + [InlineData(TestData.AuthorizedCard, "Authorized")] + [InlineData(TestData.DeclinedCard, "Declined")] + public async Task PostPayment_BankAnswers_CreatedAndRetrievable(string cardNumber, string expectedStatus) { - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); + PaymentBody payment = TestData.ValidPayment(cardNumber); - HttpResponseMessage postResponse = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + HttpResponseMessage postResponse = await this.Post(payment); - Assert.Equal(HttpStatusCode.OK, postResponse.StatusCode); - string paymentId = await postResponse.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); - Assert.True(Guid.TryParse(paymentId, out _)); + Assert.Equal(HttpStatusCode.Created, postResponse.StatusCode); + JsonNode created = await ReadJson(postResponse); + Guid paymentId = Guid.Parse(created["id"]!.GetValue()); + Assert.Equal(expectedStatus, created["status"]?.GetValue()); + Assert.NotNull(postResponse.Headers.Location); + Assert.EndsWith($"/api/payments/{paymentId}", postResponse.Headers.Location.ToString(), StringComparison.OrdinalIgnoreCase); - HttpResponseMessage getResponse = await this.client.GetAsync($"api/payment/{paymentId}", TestContext.Current.CancellationToken); + HttpResponseMessage getResponse = await this.client.GetAsync(postResponse.Headers.Location, TestContext.Current.CancellationToken); Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); string raw = await getResponse.Content.ReadAsStringAsync(TestContext.Current.CancellationToken); - Assert.DoesNotContain(TestData.AuthorizedCard, raw); + Assert.DoesNotContain(cardNumber, raw); + Assert.DoesNotContain("cvv", raw, StringComparison.OrdinalIgnoreCase); - PostPaymentResponse? stored = await getResponse.Content.ReadFromJsonAsync(TestContext.Current.CancellationToken); - Assert.NotNull(stored); - Assert.Equal(paymentId, stored.PaymentId); - Assert.Equal(PaymentStatus.Authorized, stored.Status); - Assert.Equal("8873", stored.CardNumberLastFour); - Assert.Equal(payment.ExpiryMonth, stored.ExpiryMonth); - Assert.Equal(payment.ExpiryYear, stored.ExpiryYear); - Assert.Equal(payment.Currency, stored.Currency); - Assert.Equal(payment.Amount, stored.Amount); + JsonNode stored = JsonNode.Parse(raw)!; + Assert.Equal(paymentId.ToString(), stored["id"]?.GetValue()); + Assert.Equal(expectedStatus, stored["status"]?.GetValue()); + Assert.Equal(cardNumber[^4..], stored["cardNumberLastFour"]?.GetValue()); + Assert.Equal(payment.ExpiryMonth, stored["expiryMonth"]?.GetValue()); + Assert.Equal(payment.ExpiryYear, stored["expiryYear"]?.GetValue()); + Assert.Equal(payment.Currency, stored["currency"]?.GetValue()); + Assert.Equal(payment.Amount, stored["amount"]?.GetValue()); } [Fact] - public async Task PostPayment_EvenCard_Declined() + public async Task PostPayment_SingleDigitExpiryMonth_Authorized() { - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.DeclinedCard); - - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + // the simulator only answers a "MM/yyyy" expiry, so a month sent as "4/..." would fall through to its 400 + HttpResponseMessage response = await this.Post(TestData.ValidPayment(TestData.AuthorizedCard) with { ExpiryMonth = 4 }); - await AssertBadRequest(response, PaymentStatus.Declined); + Assert.Equal(HttpStatusCode.Created, response.StatusCode); + Assert.Equal("Authorized", (await ReadJson(response))["status"]?.GetValue()); } [Fact] - public async Task PostPayment_CardEndingZero_InternalError() + public async Task PostPayment_BankUnavailable_BadGatewayWithFailedPaymentOnRecord() { - // slow, the resilience handler retries the simulator's 503 before giving up - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.BankUnavailableCard); + HttpResponseMessage response = await this.Post(TestData.ValidPayment(TestData.BankUnavailableCard)); - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + JsonNode problem = await AssertProblem(response, HttpStatusCode.BadGateway); + Assert.Equal("Failed", problem["paymentStatus"]?.GetValue()); - await AssertBadRequest(response, PaymentStatus.InternalError); + string paymentId = problem["paymentId"]!.GetValue(); + HttpResponseMessage getResponse = await this.client.GetAsync($"api/payments/{paymentId}", TestContext.Current.CancellationToken); + Assert.Equal(HttpStatusCode.OK, getResponse.StatusCode); + Assert.Equal("Failed", (await ReadJson(getResponse))["status"]?.GetValue()); } [Theory] - [InlineData("CardNumber")] - [InlineData("Expiry")] - [InlineData("Amount")] - [InlineData("Currency")] - [InlineData("Cvv")] - public async Task PostPayment_InvalidField_Rejected(string invalidField) - { - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); - switch (invalidField) + [InlineData("cardNumber")] + [InlineData("expiryMonth")] + [InlineData("expiryYear")] + [InlineData("amount")] + [InlineData("currency")] + [InlineData("cvv")] + public async Task PostPayment_InvalidField_ValidationProblemNamingTheField(string field) + { + PaymentBody payment = TestData.ValidPayment(TestData.AuthorizedCard); + payment = field switch { - case "CardNumber": payment.CardNumber = "1234"; break; - case "Expiry": payment.ExpiryYear = DateTime.Now.Year - 1; break; - case "Amount": payment.Amount = 0; break; - case "Currency": payment.Currency = "GBPX"; break; - case "Cvv": payment.Cvv = "12"; break; - } + "cardNumber" => payment with { CardNumber = "1234" }, + "expiryMonth" => payment with { ExpiryMonth = 13 }, + "expiryYear" => payment with { ExpiryYear = 0 }, + "amount" => payment with { Amount = 0 }, + "currency" => payment with { Currency = "JPY" }, + _ => payment with { Cvv = "12" } + }; - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + HttpResponseMessage response = await this.Post(payment); - await AssertBadRequest(response, PaymentStatus.Rejected); + JsonNode problem = await AssertProblem(response, HttpStatusCode.BadRequest); + JsonObject errors = problem["errors"]!.AsObject(); + Assert.Single(errors); + Assert.True(errors.ContainsKey(field)); } - [Fact] - public async Task PostPayment_ExpiryYearZero_ReturnsInternalServerError() + [Theory] + [InlineData("""{"expiryMonth":11,"expiryYear":2030,"currency":"GBP","amount":1050,"cvv":"123"}""")] // no card number + [InlineData("""{"cardNumber":null,"expiryMonth":11,"expiryYear":2030,"currency":"GBP","amount":1050,"cvv":"123"}""")] + [InlineData("""{"cardNumber":"2222405343248873","expiryMonth":11,"expiryYear":2030,"currency":"GBP","amount":10.5,"cvv":"123"}""")] + public async Task PostPayment_MalformedBody_ValidationProblem(string body) { - // NOTE: defect - an out of range year is not validated, the validator throws and - // ErrorMiddleware answers 500 instead of the payment being rejected - PaymentRequest payment = TestData.ValidPaymentRequest(TestData.AuthorizedCard); - payment.ExpiryYear = 0; + using StringContent content = new(body, System.Text.Encoding.UTF8, "application/json"); + + HttpResponseMessage response = await this.client.PostAsync("api/payments", content, TestContext.Current.CancellationToken); + + await AssertProblem(response, HttpStatusCode.BadRequest); + } - HttpResponseMessage response = await this.client.PostAsJsonAsync("api/payment", payment, TestContext.Current.CancellationToken); + [Fact] + public async Task GetPayment_UnknownId_NotFoundProblem() + { + HttpResponseMessage response = await this.client.GetAsync($"api/payments/{Guid.NewGuid()}", TestContext.Current.CancellationToken); - Assert.Equal(HttpStatusCode.InternalServerError, response.StatusCode); + await AssertProblem(response, HttpStatusCode.NotFound); } [Fact] - public async Task GetPayment_UnknownId_ReturnsNotFound() + public async Task GetPayment_MalformedId_NotFound() { - HttpResponseMessage response = await this.client.GetAsync($"api/payment/{Guid.NewGuid()}", TestContext.Current.CancellationToken); + HttpResponseMessage response = await this.client.GetAsync("api/payments/not-a-guid", TestContext.Current.CancellationToken); Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); } - private static async Task AssertBadRequest(HttpResponseMessage response, PaymentStatus expected) + [Theory] + [InlineData(null)] + [InlineData("not-a-configured-key")] + public async Task PostPayment_WithoutAValidApiKey_Unauthorized(string? apiKey) + { + using HttpClient caller = this.fixture.CreateClient(apiKey); + + HttpResponseMessage response = await caller.PostAsJsonAsync( + "api/payments", TestData.ValidPayment(TestData.AuthorizedCard), TestContext.Current.CancellationToken); + + await AssertProblem(response, HttpStatusCode.Unauthorized); + Assert.Contains(response.Headers.WwwAuthenticate, header => header.Scheme == "ApiKey"); + } + + [Fact] + public async Task GetPayment_AnotherMerchantsPayment_NotFound() + { + HttpResponseMessage created = await this.Post(TestData.ValidPayment(TestData.AuthorizedCard)); + Assert.Equal(HttpStatusCode.Created, created.StatusCode); + using HttpClient merchantB = this.fixture.CreateClient(ApiFixture.MerchantBKey); + + HttpResponseMessage response = await merchantB.GetAsync(created.Headers.Location, TestContext.Current.CancellationToken); + + // indistinguishable from an id that does not exist, so ids cannot be probed + await AssertProblem(response, HttpStatusCode.NotFound); + } + + [Theory] + [InlineData(TestData.AuthorizedCard, HttpStatusCode.Created)] + [InlineData(TestData.BankUnavailableCard, HttpStatusCode.BadGateway)] + public async Task PostPayment_RepeatedIdempotencyKey_ReplaysTheFirstOutcome(string cardNumber, HttpStatusCode expected) + { + string key = Guid.NewGuid().ToString(); + PaymentBody payment = TestData.ValidPayment(cardNumber); + + HttpResponseMessage first = await Post(this.client, payment, key); + HttpResponseMessage second = await Post(this.client, payment, key); + + Assert.Equal(expected, first.StatusCode); + Assert.Equal(expected, second.StatusCode); + Assert.False(first.Headers.Contains("Idempotent-Replayed")); + Assert.Equal(["true"], second.Headers.GetValues("Idempotent-Replayed")); + + string idProperty = expected == HttpStatusCode.Created ? "id" : "paymentId"; + Assert.Equal( + (await ReadJson(first))[idProperty]!.GetValue(), + (await ReadJson(second))[idProperty]!.GetValue()); + } + + [Fact] + public async Task PostPayment_IdempotencyKeyReusedForADifferentPayment_UnprocessableEntity() { - Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); - Assert.Equal(expected.ToString(), await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)); + string key = Guid.NewGuid().ToString(); + PaymentBody payment = TestData.ValidPayment(TestData.AuthorizedCard); + Assert.Equal(HttpStatusCode.Created, (await Post(this.client, payment, key)).StatusCode); + + HttpResponseMessage response = await Post(this.client, payment with { Amount = payment.Amount + 1 }, key); + + await AssertProblem(response, HttpStatusCode.UnprocessableEntity); + } + + [Fact] + public async Task PostPayment_SameIdempotencyKeyFromAnotherMerchant_IsIndependent() + { + string key = Guid.NewGuid().ToString(); + PaymentBody payment = TestData.ValidPayment(TestData.AuthorizedCard); + using HttpClient merchantB = this.fixture.CreateClient(ApiFixture.MerchantBKey); + + HttpResponseMessage fromA = await Post(this.client, payment, key); + HttpResponseMessage fromB = await Post(merchantB, payment, key); + + Assert.Equal(HttpStatusCode.Created, fromA.StatusCode); + Assert.Equal(HttpStatusCode.Created, fromB.StatusCode); + Assert.False(fromB.Headers.Contains("Idempotent-Replayed")); + Assert.NotEqual((await ReadJson(fromA))["id"]!.GetValue(), (await ReadJson(fromB))["id"]!.GetValue()); } } diff --git a/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj b/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj index 067b8c6..3a15a3f 100644 --- a/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj +++ b/tests/PaymentGatewayApiEndToEndTests/PaymentGatewayApiEndToEndTests.csproj @@ -1,21 +1,15 @@ - - disable - enable - Exe - net10.0 - true - false - - - - - - - - - - + + Exe + true + false + + + + + + diff --git a/tests/PaymentGatewayApiEndToEndTests/TestData.cs b/tests/PaymentGatewayApiEndToEndTests/TestData.cs index a3a859d..0408ab0 100644 --- a/tests/PaymentGatewayApiEndToEndTests/TestData.cs +++ b/tests/PaymentGatewayApiEndToEndTests/TestData.cs @@ -1,8 +1,10 @@ using System; -using PaymentGatewayApi.Models; namespace PaymentGatewayApiEndToEndTests; +// the request as a merchant would send it, independent of the api's own model +public sealed record PaymentBody(string CardNumber, int ExpiryMonth, int ExpiryYear, string Currency, int Amount, string Cvv); + public static class TestData { // the bank simulator picks its response from the last card digit @@ -10,16 +12,8 @@ public static class TestData public const string DeclinedCard = "2222405343248872"; public const string BankUnavailableCard = "2222405343248870"; - public static PaymentRequest ValidPaymentRequest(string cardNumber) + public static PaymentBody ValidPayment(string cardNumber) { - return new PaymentRequest - { - CardNumber = cardNumber, - Cvv = "123", - Currency = "GBP", - ExpiryMonth = 11, - ExpiryYear = DateTime.Now.Year + 1, - Amount = 100 - }; + return new PaymentBody(cardNumber, ExpiryMonth: 11, ExpiryYear: DateTime.UtcNow.Year + 1, Currency: "GBP", Amount: 1050, Cvv: "123"); } } diff --git a/tests/PaymentGatewayApiIntegrationTests/Cards.cs b/tests/PaymentGatewayApiIntegrationTests/Cards.cs new file mode 100644 index 0000000..0117289 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/Cards.cs @@ -0,0 +1,18 @@ +using System; +using System.Globalization; + +namespace PaymentGatewayApiIntegrationTests; + +public static class Cards +{ + // a 16 digit card number no other test uses, ending in the digit that steers the stub bank + public static string Unique(char lastDigit) + { + return "4" + Random.Shared.NextInt64(10_000_000_000_000, 99_999_999_999_999).ToString(CultureInfo.InvariantCulture) + lastDigit; + } + + public static object Payment(string cardNumber, int amount = 1050, string currency = "GBP") + { + return new { cardNumber, expiryMonth = 4, expiryYear = DateTime.UtcNow.Year + 2, currency, amount, cvv = "123" }; + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/CollectingSink.cs b/tests/PaymentGatewayApiIntegrationTests/CollectingSink.cs new file mode 100644 index 0000000..584f73f --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/CollectingSink.cs @@ -0,0 +1,20 @@ +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using Serilog.Core; +using Serilog.Events; + +namespace PaymentGatewayApiIntegrationTests; + +// registered as an ILogEventSink service, which the app's ReadFrom.Services adds to its logger +public sealed class CollectingSink : ILogEventSink +{ + private readonly ConcurrentQueue events = new(); + + public IReadOnlyList Events => this.events.ToList(); + + public void Emit(LogEvent logEvent) + { + this.events.Enqueue(logEvent); + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/GatewayFactory.cs b/tests/PaymentGatewayApiIntegrationTests/GatewayFactory.cs new file mode 100644 index 0000000..d80f132 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/GatewayFactory.cs @@ -0,0 +1,95 @@ +using System.Threading.Tasks; +using Amazon.DynamoDBv2; +using Amazon.DynamoDBv2.Model; +using Amazon.Runtime; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using PaymentGatewayApi.Services; +using PaymentGatewayApiIntegrationTests; +using Serilog.Core; +using Testcontainers.DynamoDb; +using Xunit; + +[assembly: AssemblyFixture(typeof(GatewayFactory))] + +namespace PaymentGatewayApiIntegrationTests; + +// the whole api in process: real DynamoDB Local in a container, the real bank client and its +// resilience pipeline in front of a stub bank, and every log event collected +public sealed class GatewayFactory : WebApplicationFactory, IAsyncLifetime +{ + public const string MerchantAKey = "merchant-a-key"; + public const string MerchantBKey = "merchant-b-key"; + + private readonly DynamoDbContainer dynamoContainer = new DynamoDbBuilder("amazon/dynamodb-local:3.3.1").Build(); + + public StubBank Bank { get; } = new(); + + public CollectingSink Logs { get; } = new(); + + public IAmazonDynamoDB NewDynamoClient() + { + return new AmazonDynamoDBClient( + new BasicAWSCredentials("test", "test"), + new AmazonDynamoDBConfig { ServiceURL = this.dynamoContainer.GetConnectionString(), MaxErrorRetry = 0 }); + } + + public async ValueTask InitializeAsync() + { + await this.dynamoContainer.StartAsync(); + + // the same schema the compose init script creates + using IAmazonDynamoDB dynamo = this.NewDynamoClient(); + await dynamo.CreateTableAsync(Table("Payments", "PaymentId")); + await dynamo.CreateTableAsync(Table("PaymentIdempotencyKeys", "IdempotencyKey")); + } + + public override async ValueTask DisposeAsync() + { + await base.DisposeAsync(); + await this.dynamoContainer.DisposeAsync(); + } + + protected override void ConfigureWebHost(IWebHostBuilder builder) + { + // not Development: no local keys, simulator URL or log files from appsettings.Development.json + builder.UseEnvironment("Testing"); + + builder.UseSetting("Bank:BaseUrl", "http://bank.test/"); + builder.UseSetting("Bank:Resilience:AttemptTimeout:Timeout", "00:00:02"); + builder.UseSetting("Bank:Resilience:TotalRequestTimeout:Timeout", "00:00:04"); + builder.UseSetting("ApiKeys:Merchants:0:MerchantId", "merchant-a"); + builder.UseSetting("ApiKeys:Merchants:0:KeySha256", MerchantApiKeysHash(MerchantAKey)); + builder.UseSetting("ApiKeys:Merchants:1:MerchantId", "merchant-b"); + builder.UseSetting("ApiKeys:Merchants:1:KeySha256", MerchantApiKeysHash(MerchantBKey)); + builder.UseSetting("RateLimiting:PermitLimit", "1000"); + builder.UseSetting("Serilog:MinimumLevel:Default", "Debug"); + + builder.ConfigureTestServices(services => + { + services.AddSingleton(_ => this.NewDynamoClient()); + services.AddSingleton(this.Logs); + + // swaps only the innermost handler: the resilience pipeline in front of it is the production one + services.AddHttpClient().ConfigurePrimaryHttpMessageHandler(() => this.Bank); + }); + } + + private static string MerchantApiKeysHash(string apiKey) + { + return PaymentGatewayApi.Security.MerchantApiKeys.Hash(apiKey); + } + + private static CreateTableRequest Table(string name, string hashKey) + { + return new CreateTableRequest + { + TableName = name, + AttributeDefinitions = [new AttributeDefinition(hashKey, ScalarAttributeType.S)], + KeySchema = [new KeySchemaElement(hashKey, KeyType.HASH)], + BillingMode = BillingMode.PAY_PER_REQUEST + }; + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/HttpClientExtensions.cs b/tests/PaymentGatewayApiIntegrationTests/HttpClientExtensions.cs new file mode 100644 index 0000000..e25a7fd --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/HttpClientExtensions.cs @@ -0,0 +1,41 @@ +using System; +using System.Net.Http; +using System.Net.Http.Json; +using System.Text.Json.Nodes; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.Testing; +using Xunit; + +namespace PaymentGatewayApiIntegrationTests; + +public static class HttpClientExtensions +{ + public static HttpClient CreateMerchantClient(this WebApplicationFactory factory, string? apiKey = GatewayFactory.MerchantAKey) + where TEntryPoint : class + { + HttpClient client = factory.CreateClient(new WebApplicationFactoryClientOptions { AllowAutoRedirect = false }); + if (apiKey is not null) + { + client.DefaultRequestHeaders.Add("X-Api-Key", apiKey); + } + + return client; + } + + public static async Task PostPayment(this HttpClient client, object payment, string? idempotencyKey = null) + { + using HttpRequestMessage request = new(HttpMethod.Post, "api/payments") { Content = JsonContent.Create(payment) }; + if (idempotencyKey is not null) + { + request.Headers.Add("Idempotency-Key", idempotencyKey); + } + + return await client.SendAsync(request, TestContext.Current.CancellationToken); + } + + public static async Task ReadJson(this HttpResponseMessage response) + { + return JsonNode.Parse(await response.Content.ReadAsStringAsync(TestContext.Current.CancellationToken)) + ?? throw new InvalidOperationException("The response body is empty."); + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/IdempotencyTests.cs b/tests/PaymentGatewayApiIntegrationTests/IdempotencyTests.cs new file mode 100644 index 0000000..c2b03d9 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/IdempotencyTests.cs @@ -0,0 +1,56 @@ +using System; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Xunit; + +namespace PaymentGatewayApiIntegrationTests; + +public class IdempotencyTests +{ + private readonly GatewayFactory factory; + private readonly HttpClient client; + + public IdempotencyTests(GatewayFactory factory) + { + this.factory = factory; + this.client = factory.CreateMerchantClient(); + } + + [Fact] + public async Task Post_ConcurrentRequestsWithOneKey_TheBankIsCalledOnce() + { + string card = Cards.Unique('3'); + string key = Guid.NewGuid().ToString(); + this.factory.Bank.Answer(card, async _ => + { + // slow enough that every request arrives while the first is still at the bank + await Task.Delay(TimeSpan.FromMilliseconds(500), CancellationToken.None); + return StubBank.Json(HttpStatusCode.OK, """{"authorized":true,"authorization_code":"code-1"}"""); + }); + + HttpResponseMessage[] responses = await Task.WhenAll( + Enumerable.Range(0, 5).Select(_ => this.client.PostPayment(Cards.Payment(card), key))); + + Assert.Single(this.factory.Bank.RequestsFor(card)); + Assert.Single(responses, response => response.StatusCode == HttpStatusCode.Created && !response.Headers.Contains("Idempotent-Replayed")); + Assert.All(responses, response => Assert.Contains(response.StatusCode, new[] { HttpStatusCode.Created, HttpStatusCode.Conflict })); + } + + [Fact] + public async Task Post_RepeatedKey_ReplaysWithoutCallingTheBankAgain() + { + string card = Cards.Unique('3'); + string key = Guid.NewGuid().ToString(); + + HttpResponseMessage first = await this.client.PostPayment(Cards.Payment(card), key); + HttpResponseMessage second = await this.client.PostPayment(Cards.Payment(card), key); + + Assert.Equal(HttpStatusCode.Created, first.StatusCode); + Assert.Equal(HttpStatusCode.Created, second.StatusCode); + Assert.Equal(first.Headers.Location, second.Headers.Location); + Assert.Single(this.factory.Bank.RequestsFor(card)); + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/PaymentGatewayApiIntegrationTests.csproj b/tests/PaymentGatewayApiIntegrationTests/PaymentGatewayApiIntegrationTests.csproj new file mode 100644 index 0000000..ea0aa16 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/PaymentGatewayApiIntegrationTests.csproj @@ -0,0 +1,20 @@ + + + + Exe + true + false + + + + + + + + + + + + + + diff --git a/tests/PaymentGatewayApiIntegrationTests/PaymentsApiTests.cs b/tests/PaymentGatewayApiIntegrationTests/PaymentsApiTests.cs new file mode 100644 index 0000000..d1a8ca7 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/PaymentsApiTests.cs @@ -0,0 +1,105 @@ +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Text.Json.Nodes; +using System.Threading.Tasks; +using Amazon.DynamoDBv2; +using Amazon.DynamoDBv2.Model; +using Xunit; + +namespace PaymentGatewayApiIntegrationTests; + +public class PaymentsApiTests +{ + private readonly GatewayFactory factory; + private readonly HttpClient client; + + public PaymentsApiTests(GatewayFactory factory) + { + this.factory = factory; + this.client = factory.CreateMerchantClient(); + } + + [Fact] + public async Task Post_Authorized_CreatedAndStoredWithItsAuthorizationCode() + { + string card = Cards.Unique('3'); + + HttpResponseMessage response = await this.client.PostPayment(Cards.Payment(card)); + + Assert.Equal(HttpStatusCode.Created, response.StatusCode); + JsonNode created = await response.ReadJson(); + string id = created["id"]!.GetValue(); + Assert.Equal("Authorized", created["status"]!.GetValue()); + Assert.Equal($"/api/payments/{id}", response.Headers.Location?.AbsolutePath); + + // the storage record, read straight from DynamoDB + using IAmazonDynamoDB dynamo = this.factory.NewDynamoClient(); + GetItemResponse item = await dynamo.GetItemAsync( + "Payments", new Dictionary { ["PaymentId"] = new(id) }, TestContext.Current.CancellationToken); + Assert.Equal("Authorized", item.Item["Status"].S); + Assert.Equal("merchant-a", item.Item["MerchantId"].S); + Assert.Equal(card[^4..], item.Item["CardNumberLastFour"].S); + Assert.True(Guid.TryParse(item.Item["AuthorizationCode"].S, out _)); + Assert.DoesNotContain(item.Item.Values, value => value.S == card); + } + + [Fact] + public async Task Post_SendsTheBankItsContract() + { + string card = Cards.Unique('3'); + + await this.client.PostPayment(Cards.Payment(card, amount: 1, currency: "eur")); + + JsonNode sent = Assert.Single(this.factory.Bank.RequestsFor(card)); + Assert.Equal($"04/{DateTime.UtcNow.Year + 2}", sent["expiry_date"]!.GetValue()); + Assert.Equal("EUR", sent["currency"]!.GetValue()); + Assert.Equal(1, sent["amount"]!.GetValue()); + Assert.Equal("123", sent["cvv"]!.GetValue()); + } + + [Fact] + public async Task Post_Declined_CreatedAndRetrievable() + { + HttpResponseMessage response = await this.client.PostPayment(Cards.Payment(Cards.Unique('2'))); + + Assert.Equal(HttpStatusCode.Created, response.StatusCode); + HttpResponseMessage stored = await this.client.GetAsync(response.Headers.Location, TestContext.Current.CancellationToken); + Assert.Equal("Declined", (await stored.ReadJson())["status"]!.GetValue()); + } + + [Fact] + public async Task Post_Invalid_ValidationProblemAndTheBankIsNeverCalled() + { + string card = Cards.Unique('3'); + + HttpResponseMessage response = await this.client.PostPayment(Cards.Payment(card, amount: 0, currency: "JPY")); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + Assert.Equal("application/problem+json", response.Content.Headers.ContentType?.MediaType); + JsonObject errors = (await response.ReadJson())["errors"]!.AsObject(); + Assert.True(errors.ContainsKey("amount")); + Assert.True(errors.ContainsKey("currency")); + Assert.Empty(this.factory.Bank.RequestsFor(card)); + } + + [Fact] + public async Task Get_UnknownId_NotFoundProblem() + { + HttpResponseMessage response = await this.client.GetAsync($"api/payments/{Guid.NewGuid()}", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + Assert.Equal("application/problem+json", response.Content.Headers.ContentType?.MediaType); + } + + [Fact] + public async Task HealthReady_TablesReachable_Healthy() + { + using HttpClient anonymous = this.factory.CreateMerchantClient(apiKey: null); + + HttpResponseMessage response = await anonymous.GetAsync("health/ready", TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/ResilienceTests.cs b/tests/PaymentGatewayApiIntegrationTests/ResilienceTests.cs new file mode 100644 index 0000000..2d41baf --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/ResilienceTests.cs @@ -0,0 +1,181 @@ +using System; +using System.Net; +using System.Net.Http; +using System.Text.Json.Nodes; +using System.Threading; +using System.Threading.Tasks; +using Amazon.DynamoDBv2; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Services; +using Serilog.Events; +using Xunit; + +namespace PaymentGatewayApiIntegrationTests; + +// the failure modes that decide whether money moves correctly, through the production resilience pipeline +public class ResilienceTests +{ + private readonly GatewayFactory factory; + private readonly HttpClient client; + + public ResilienceTests(GatewayFactory factory) + { + this.factory = factory; + this.client = factory.CreateMerchantClient(); + } + + private async Task AssertBadGateway(HttpResponseMessage response, string paymentStatus) + { + Assert.Equal(HttpStatusCode.BadGateway, response.StatusCode); + JsonNode problem = await response.ReadJson(); + Assert.Equal(paymentStatus, problem["paymentStatus"]!.GetValue()); + + // the payment is on record with the same status + HttpResponseMessage stored = await this.client.GetAsync( + $"api/payments/{problem["paymentId"]!.GetValue()}", TestContext.Current.CancellationToken); + Assert.Equal(paymentStatus, (await stored.ReadJson())["status"]!.GetValue()); + return problem; + } + + [Fact] + public async Task Post_BankUnavailable_CalledExactlyOnceAndFailed() + { + string card = Cards.Unique('0'); + + HttpResponseMessage response = await this.client.PostPayment(Cards.Payment(card)); + + await this.AssertBadGateway(response, "Failed"); + Assert.Single(this.factory.Bank.RequestsFor(card)); + } + + [Fact] + public async Task Post_BankServerError_NotRetriedAndUnknown() + { + string card = Cards.Unique('3'); + this.factory.Bank.Answer(card, _ => Task.FromResult(StubBank.Json(HttpStatusCode.InternalServerError, "{}"))); + + HttpResponseMessage response = await this.client.PostPayment(Cards.Payment(card)); + + await this.AssertBadGateway(response, "Unknown"); + Assert.Single(this.factory.Bank.RequestsFor(card)); + } + + [Fact] + public async Task Post_BankTimesOut_NotRetriedAndUnknown() + { + string card = Cards.Unique('3'); + this.factory.Bank.Answer(card, async cancellationToken => + { + // longer than the 2s attempt timeout the factory configures + await Task.Delay(TimeSpan.FromSeconds(30), cancellationToken); + return StubBank.Json(HttpStatusCode.OK, """{"authorized":true}"""); + }); + + HttpResponseMessage response = await this.client.PostPayment(Cards.Payment(card)); + + await this.AssertBadGateway(response, "Unknown"); + Assert.Single(this.factory.Bank.RequestsFor(card)); + } + + [Fact] + public async Task Post_BankAnswerUnreadable_Unknown() + { + string card = Cards.Unique('3'); + this.factory.Bank.Answer(card, _ => Task.FromResult(StubBank.Json(HttpStatusCode.OK, ""))); + + HttpResponseMessage response = await this.client.PostPayment(Cards.Payment(card)); + + await this.AssertBadGateway(response, "Unknown"); + } + + [Fact] + public async Task Post_OutcomeWriteFails_MerchantGetsTheOutcomeAndTheRecordStaysPending() + { + await using WebApplicationFactory failing = this.factory.WithWebHostBuilder(builder => + builder.ConfigureTestServices(services => services.AddSingleton(provider => + new FailingCompleteRepository(ActivatorUtilities.CreateInstance(provider))))); + using HttpClient failingClient = failing.CreateMerchantClient(); + + HttpResponseMessage response = await failingClient.PostPayment(Cards.Payment(Cards.Unique('3'))); + + // the bank authorized it, so that is what the merchant hears + Assert.Equal(HttpStatusCode.Created, response.StatusCode); + Assert.Equal("Authorized", (await response.ReadJson())["status"]!.GetValue()); + + // the record says Pending, and a Critical log line carries the outcome for reconciliation + HttpResponseMessage stored = await failingClient.GetAsync(response.Headers.Location, TestContext.Current.CancellationToken); + Assert.Equal("Pending", (await stored.ReadJson())["status"]!.GetValue()); + Assert.Contains(this.factory.Logs.Events, logEvent => + logEvent.Level == LogEventLevel.Fatal && logEvent.Properties["AuthorizationCode"].ToString().Length > 2); + } + + [Fact] + public async Task Post_ClientDisconnectsDuringTheBankCall_TheOutcomeIsStillRecorded() + { + string card = Cards.Unique('3'); + string key = Guid.NewGuid().ToString(); + using SemaphoreSlim bankCalled = new(0); + this.factory.Bank.Answer(card, async _ => + { + bankCalled.Release(); + await Task.Delay(TimeSpan.FromMilliseconds(500), CancellationToken.None); + return StubBank.Json(HttpStatusCode.OK, """{"authorized":true,"authorization_code":"code-1"}"""); + }); + + using CancellationTokenSource disconnect = new(); + Task abandoned = this.client.SendAsync( + new HttpRequestMessage(HttpMethod.Post, "api/payments") + { + Content = System.Net.Http.Json.JsonContent.Create(Cards.Payment(card)), + Headers = { { "Idempotency-Key", key } } + }, + disconnect.Token); + await bankCalled.WaitAsync(TestContext.Current.CancellationToken); + await disconnect.CancelAsync(); + await Assert.ThrowsAnyAsync(() => abandoned); + + // replaying the key shows what the abandoned request produced, once the bank has answered + HttpResponseMessage replay = await this.client.PostPayment(Cards.Payment(card), key); + for (int attempt = 0; replay.StatusCode == HttpStatusCode.Conflict && attempt < 20; attempt++) + { + await Task.Delay(TimeSpan.FromMilliseconds(100), TestContext.Current.CancellationToken); + replay = await this.client.PostPayment(Cards.Payment(card), key); + } + + Assert.Equal(HttpStatusCode.Created, replay.StatusCode); + Assert.Equal("Authorized", (await replay.ReadJson())["status"]!.GetValue()); + Assert.Single(this.factory.Bank.RequestsFor(card)); + } + + // the real repository, except that recording the bank's outcome always fails + private sealed class FailingCompleteRepository(IPaymentsRepository inner) : IPaymentsRepository + { + public Task Add(Payment payment, CancellationToken cancellationToken) + { + return inner.Add(payment, cancellationToken); + } + + public Task Add(Payment payment, IdempotencyRecord idempotency, CancellationToken cancellationToken) + { + return inner.Add(payment, idempotency, cancellationToken); + } + + public Task FindIdempotencyRecord(string merchantId, string key, CancellationToken cancellationToken) + { + return inner.FindIdempotencyRecord(merchantId, key, cancellationToken); + } + + public Task Complete(Payment payment, CancellationToken cancellationToken) + { + throw new AmazonDynamoDBException("DynamoDB is unavailable"); + } + + public Task Find(Guid paymentId, CancellationToken cancellationToken) + { + return inner.Find(paymentId, cancellationToken); + } + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/SecurityTests.cs b/tests/PaymentGatewayApiIntegrationTests/SecurityTests.cs new file mode 100644 index 0000000..8c27f13 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/SecurityTests.cs @@ -0,0 +1,88 @@ +using System; +using System.Globalization; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.Testing; +using Serilog.Events; +using Xunit; + +namespace PaymentGatewayApiIntegrationTests; + +public class SecurityTests +{ + private readonly GatewayFactory factory; + + public SecurityTests(GatewayFactory factory) + { + this.factory = factory; + } + + [Theory] + [InlineData(null)] + [InlineData("wrong-key")] + public async Task Post_WithoutAValidApiKey_UnauthorizedAndTheBankIsNeverCalled(string? apiKey) + { + string card = Cards.Unique('3'); + using HttpClient client = this.factory.CreateMerchantClient(apiKey); + + HttpResponseMessage response = await client.PostPayment(Cards.Payment(card)); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + Assert.Empty(this.factory.Bank.RequestsFor(card)); + } + + [Fact] + public async Task Get_AnotherMerchantsPayment_NotFound() + { + using HttpClient merchantA = this.factory.CreateMerchantClient(GatewayFactory.MerchantAKey); + using HttpClient merchantB = this.factory.CreateMerchantClient(GatewayFactory.MerchantBKey); + HttpResponseMessage created = await merchantA.PostPayment(Cards.Payment(Cards.Unique('3'))); + + HttpResponseMessage response = await merchantB.GetAsync(created.Headers.Location, TestContext.Current.CancellationToken); + + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task Post_OverTheMerchantsRateLimit_TooManyRequestsWithRetryAfter() + { + await using WebApplicationFactory limited = this.factory.WithWebHostBuilder(builder => + { + builder.UseSetting("RateLimiting:PermitLimit", "2"); + builder.UseSetting("RateLimiting:Window", "00:01:00"); + }); + using HttpClient merchantA = limited.CreateMerchantClient(GatewayFactory.MerchantAKey); + using HttpClient merchantB = limited.CreateMerchantClient(GatewayFactory.MerchantBKey); + + HttpResponseMessage[] allowed = [await merchantA.PostPayment(Cards.Payment(Cards.Unique('3'))), await merchantA.PostPayment(Cards.Payment(Cards.Unique('3')))]; + HttpResponseMessage limitedResponse = await merchantA.PostPayment(Cards.Payment(Cards.Unique('3'))); + HttpResponseMessage otherMerchant = await merchantB.PostPayment(Cards.Payment(Cards.Unique('3'))); + + Assert.All(allowed, response => Assert.Equal(HttpStatusCode.Created, response.StatusCode)); + Assert.Equal(HttpStatusCode.TooManyRequests, limitedResponse.StatusCode); + Assert.NotNull(limitedResponse.Headers.RetryAfter); + Assert.Equal(HttpStatusCode.Created, otherMerchant.StatusCode); + } + + [Fact] + public async Task Post_TheFullCardNumberNeverReachesALogLine() + { + string card = Cards.Unique('3'); + using HttpClient client = this.factory.CreateMerchantClient(); + + // an accepted payment and a rejected one, which exercise the most logging + await client.PostPayment(Cards.Payment(card)); + await client.PostPayment(Cards.Payment(card, amount: 0)); + + LogEvent[] leaking = this.factory.Logs.Events + .Where(logEvent => logEvent.RenderMessage(CultureInfo.InvariantCulture).Contains(card, StringComparison.Ordinal) + || logEvent.Properties.Values.Any(value => value.ToString().Contains(card, StringComparison.Ordinal)) + || (logEvent.Exception?.ToString().Contains(card, StringComparison.Ordinal) ?? false)) + .ToArray(); + Assert.Empty(leaking); + Assert.Contains(this.factory.Logs.Events, logEvent => logEvent.RenderMessage(CultureInfo.InvariantCulture).Contains(card[^4..], StringComparison.Ordinal) + || logEvent.Properties.ContainsKey("PaymentId")); + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/StartupTests.cs b/tests/PaymentGatewayApiIntegrationTests/StartupTests.cs new file mode 100644 index 0000000..3f161b4 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/StartupTests.cs @@ -0,0 +1,58 @@ +using System; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.Testing; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Serilog.Core; +using Xunit; + +namespace PaymentGatewayApiIntegrationTests; + +// a bad setting stops the app at startup, not at the first request that needs it +public class StartupTests +{ + private readonly GatewayFactory factory; + + public StartupTests(GatewayFactory factory) + { + this.factory = factory; + } + + [Theory] + [InlineData("Bank:BaseUrl", "", "BaseUrl")] + [InlineData("RateLimiting:PermitLimit", "0", "PermitLimit")] + [InlineData("Payments:IdempotencyKeyRetention", "00:00:00", "IdempotencyKeyRetention")] + [InlineData("ApiKeys:Merchants:0:KeySha256", "not-a-sha256", "KeySha256")] + [InlineData("ApiKeys:Merchants:1:MerchantId", "", "MerchantId")] + public async Task Startup_InvalidSetting_FailsNamingIt(string setting, string value, string member) + { + CollectingSink logs = new(); + await using WebApplicationFactory misconfigured = this.factory.WithWebHostBuilder(builder => + { + builder.UseSetting(setting, value); + builder.ConfigureTestServices(services => services.AddSingleton(logs)); + }); + + Assert.ThrowsAny(() => misconfigured.CreateClient()); + + Assert.Contains(logs.Events, logEvent => + logEvent.Exception is OptionsValidationException exception && exception.Message.Contains(member, StringComparison.Ordinal)); + } + + [Fact] + public async Task Startup_TwoMerchantsWithOneKey_Fails() + { + CollectingSink logs = new(); + await using WebApplicationFactory misconfigured = this.factory.WithWebHostBuilder(builder => + { + builder.UseSetting("ApiKeys:Merchants:1:KeySha256", PaymentGatewayApi.Security.MerchantApiKeys.Hash(GatewayFactory.MerchantAKey)); + builder.ConfigureTestServices(services => services.AddSingleton(logs)); + }); + + Assert.ThrowsAny(() => misconfigured.CreateClient()); + + Assert.Contains(logs.Events, logEvent => + logEvent.Exception is OptionsValidationException exception && exception.Message.Contains("unique", StringComparison.Ordinal)); + } +} diff --git a/tests/PaymentGatewayApiIntegrationTests/StubBank.cs b/tests/PaymentGatewayApiIntegrationTests/StubBank.cs new file mode 100644 index 0000000..7c30dc0 --- /dev/null +++ b/tests/PaymentGatewayApiIntegrationTests/StubBank.cs @@ -0,0 +1,56 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Text; +using System.Text.Json.Nodes; +using System.Threading; +using System.Threading.Tasks; + +namespace PaymentGatewayApiIntegrationTests; + +// the acquiring bank, in process: answers like the WireMock simulator (by the card's last digit) +// unless a test sets up its own answer for a card, and records every request per card number. +// Tests run in parallel, so each one uses its own card numbers (Cards.Unique) and never shares state. +public sealed class StubBank : HttpMessageHandler +{ + private readonly ConcurrentDictionary>> answers = new(); + private readonly ConcurrentDictionary> requests = new(); + + public void Answer(string cardNumber, Func> answer) + { + this.answers[cardNumber] = answer; + } + + public IReadOnlyList RequestsFor(string cardNumber) + { + return this.requests.TryGetValue(cardNumber, out ConcurrentQueue? received) ? received.ToList() : []; + } + + public static HttpResponseMessage Json(HttpStatusCode statusCode, string body) + { + return new HttpResponseMessage(statusCode) { Content = new StringContent(body, Encoding.UTF8, "application/json") }; + } + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + JsonNode body = JsonNode.Parse(await request.Content!.ReadAsStringAsync(cancellationToken))!; + string cardNumber = body["card_number"]!.GetValue(); + this.requests.GetOrAdd(cardNumber, _ => new ConcurrentQueue()).Enqueue(body); + + if (this.answers.TryGetValue(cardNumber, out Func>? answer)) + { + return await answer(cancellationToken); + } + + int lastDigit = cardNumber[^1] - '0'; + return lastDigit switch + { + 0 => Json(HttpStatusCode.ServiceUnavailable, "{}"), + _ when lastDigit % 2 == 1 => Json(HttpStatusCode.OK, $$"""{"authorized":true,"authorization_code":"{{Guid.NewGuid()}}"}"""), + _ => Json(HttpStatusCode.OK, """{"authorized":false,"authorization_code":""}""") + }; + } +} diff --git a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs deleted file mode 100644 index 4556e30..0000000 --- a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentControllerTests.cs +++ /dev/null @@ -1,159 +0,0 @@ -using System; -using System.Threading.Tasks; -using Microsoft.AspNetCore.Mvc; -using Microsoft.Extensions.Logging.Abstractions; -using Moq; -using PaymentGatewayApi.Controllers; -using PaymentGatewayApi.Models; -using PaymentGatewayApi.Services; -using Xunit; - -namespace PaymentGatewayApiUnitTests.Controllers; - -public class PaymentControllerTests -{ - private readonly Mock paymentRepo = new(); - private readonly Mock validator = new(); - private readonly Mock bankService = new(); - private readonly PaymentController controller; - - public PaymentControllerTests() - { - this.controller = new PaymentController( - NullLogger.Instance, - this.paymentRepo.Object, - this.validator.Object, - this.bankService.Object); - } - - private void SetupValidator() - { - this.validator.Setup(v => v.IsCardNumberValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsExpiryValid(It.IsAny(), It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsAmountValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsCurrencyCodeValid(It.IsAny())).Returns(true); - this.validator.Setup(v => v.IsCvvValid(It.IsAny())).Returns(true); - } - - [Fact] - public void GetHealthcheck_ReturnsOk() - { - ActionResult result = this.controller.GetHealthcheck(); - - OkObjectResult ok = Assert.IsType(result.Result); - Assert.Equal("ok", ok.Value); - } - - [Fact] - public async Task GetPayment_Found_ReturnsOkWithPayment() - { - PostPaymentResponse payment = new() { PaymentId = TestData.PaymentId, Status = PaymentStatus.Authorized }; - this.paymentRepo.Setup(r => r.Get(TestData.PaymentId)).ReturnsAsync(payment); - - ActionResult result = await this.controller.GetPayment(TestData.PaymentId); - - OkObjectResult ok = Assert.IsType(result.Result); - Assert.Same(payment, ok.Value); - } - - [Fact] - public async Task GetPayment_Missing_ReturnsNotFound() - { - this.paymentRepo.Setup(r => r.Get(It.IsAny())).ReturnsAsync((PostPaymentResponse?)null); - - ActionResult result = await this.controller.GetPayment(TestData.PaymentId); - - Assert.IsType(result.Result); - } - - [Fact] - public async Task PostPayment_Authorized_ReturnsOkWithNewPaymentId() - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(payment)).ReturnsAsync(PaymentStatus.Authorized); - - ActionResult result = await this.controller.PostPayment(payment); - - OkObjectResult ok = Assert.IsType(result.Result); - string paymentId = Assert.IsType(ok.Value); - Assert.True(Guid.TryParse(paymentId, out _)); - Assert.Equal(payment.PaymentId, paymentId); - } - - [Fact] - public async Task PostPayment_ClientSuppliedPaymentId_IsReplaced() - { - PaymentRequest payment = TestData.ValidPaymentRequest(); - this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(payment)).ReturnsAsync(PaymentStatus.Authorized); - - await this.controller.PostPayment(payment); - - Assert.NotEqual(TestData.PaymentId, payment.PaymentId); - this.bankService.Verify(b => b.ExecutePayment(It.Is(p => p.PaymentId != TestData.PaymentId)), Times.Once); - } - - [Theory] - [InlineData(nameof(IPaymentRequestValidator.IsCardNumberValid))] - [InlineData(nameof(IPaymentRequestValidator.IsExpiryValid))] - [InlineData(nameof(IPaymentRequestValidator.IsAmountValid))] - [InlineData(nameof(IPaymentRequestValidator.IsCurrencyCodeValid))] - [InlineData(nameof(IPaymentRequestValidator.IsCvvValid))] - public async Task PostPayment_ValidationFails_ReturnsRejectedWithoutCallingBank(string failingCheck) - { - this.SetupValidator(); - switch (failingCheck) - { - case nameof(IPaymentRequestValidator.IsCardNumberValid): - this.validator.Setup(v => v.IsCardNumberValid(It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsExpiryValid): - this.validator.Setup(v => v.IsExpiryValid(It.IsAny(), It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsAmountValid): - this.validator.Setup(v => v.IsAmountValid(It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsCurrencyCodeValid): - this.validator.Setup(v => v.IsCurrencyCodeValid(It.IsAny())).Returns(false); - break; - case nameof(IPaymentRequestValidator.IsCvvValid): - this.validator.Setup(v => v.IsCvvValid(It.IsAny())).Returns(false); - break; - } - - ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest()); - - BadRequestObjectResult badRequest = Assert.IsType(result.Result); - Assert.Equal(nameof(PaymentStatus.Rejected), badRequest.Value); - this.bankService.Verify(b => b.ExecutePayment(It.IsAny()), Times.Never); - } - - [Fact] - public async Task PostPayment_CardNumberInvalid_SkipsRemainingChecks() - { - // this.SetupValidator(isValid: false); - - await this.controller.PostPayment(TestData.ValidPaymentRequest()); - - this.validator.Verify(v => v.IsCardNumberValid(It.IsAny()), Times.Once); - this.validator.Verify(v => v.IsExpiryValid(It.IsAny(), It.IsAny()), Times.Never); - this.validator.Verify(v => v.IsAmountValid(It.IsAny()), Times.Never); - this.validator.Verify(v => v.IsCurrencyCodeValid(It.IsAny()), Times.Never); - this.validator.Verify(v => v.IsCvvValid(It.IsAny()), Times.Never); - } - - [Theory] - [InlineData(PaymentStatus.Declined)] - [InlineData(PaymentStatus.InternalError)] - public async Task PostPayment_NotAuthorized_ReturnsBadRequestWithStatus(PaymentStatus status) - { - this.SetupValidator(); - this.bankService.Setup(b => b.ExecutePayment(It.IsAny())).ReturnsAsync(status); - - ActionResult result = await this.controller.PostPayment(TestData.ValidPaymentRequest()); - - BadRequestObjectResult badRequest = Assert.IsType(result.Result); - Assert.Equal(status.ToString(), badRequest.Value); - } -} diff --git a/tests/PaymentGatewayApiUnitTests/Controllers/PaymentsControllerTests.cs b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentsControllerTests.cs new file mode 100644 index 0000000..0e66308 --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Controllers/PaymentsControllerTests.cs @@ -0,0 +1,208 @@ +using System; +using System.Collections.Generic; +using System.Security.Claims; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Mvc; +using Microsoft.AspNetCore.Mvc.Infrastructure; +using Microsoft.AspNetCore.Mvc.ModelBinding; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using PaymentGatewayApi.Controllers; +using PaymentGatewayApi.Models; +using PaymentGatewayApi.Security; +using PaymentGatewayApi.Services; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Controllers; + +public class PaymentsControllerTests +{ + private readonly Mock validator = new(); + private readonly Mock paymentService = new(); + private readonly PaymentsController controller; + + public PaymentsControllerTests() + { + this.validator.Setup(v => v.Validate(It.IsAny())).Returns(new Dictionary()); + + this.controller = new PaymentsController( + NullLogger.Instance, this.validator.Object, this.paymentService.Object) + { + ControllerContext = new ControllerContext + { + HttpContext = new DefaultHttpContext + { + User = new ClaimsPrincipal(new ClaimsIdentity([new Claim(MerchantClaims.MerchantId, TestData.MerchantId)], "test")) + } + }, + ProblemDetailsFactory = new SimpleProblemDetailsFactory() + }; + } + + private void SetupAttempt(PaymentAttempt attempt) + { + this.paymentService + .Setup(s => s.ProcessPayment(TestData.MerchantId, It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(attempt); + } + + private void SetupPayment(PaymentStatus status) + { + this.SetupAttempt(new PaymentAttempt.Processed(TestData.Payment(status))); + } + + private Task> Post(string? idempotencyKey = null) + { + return this.controller.PostPayment(TestData.ValidPaymentRequest(), idempotencyKey, TestContext.Current.CancellationToken); + } + + private void VerifyNeverProcessed() + { + this.paymentService.Verify( + s => s.ProcessPayment(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [Fact] + public async Task GetPayment_Found_ReturnsPayment() + { + this.paymentService + .Setup(s => s.FindPayment(TestData.MerchantId, TestData.PaymentId, It.IsAny())) + .ReturnsAsync(TestData.Payment(PaymentStatus.Authorized)); + + ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); + + PaymentResponse response = Assert.IsType(result.Value); + Assert.Equal(TestData.PaymentId, response.Id); + Assert.Equal(PaymentStatus.Authorized, response.Status); + Assert.Equal("8873", response.CardNumberLastFour); + } + + [Fact] + public async Task GetPayment_Missing_ReturnsNotFound() + { + this.paymentService + .Setup(s => s.FindPayment(TestData.MerchantId, It.IsAny(), It.IsAny())) + .ReturnsAsync((Payment?)null); + + ActionResult result = await this.controller.GetPayment(TestData.PaymentId, TestContext.Current.CancellationToken); + + Assert.IsType(result.Result); + } + + [Theory] + [InlineData(PaymentStatus.Authorized)] + [InlineData(PaymentStatus.Declined)] // the issuer said no, the request itself was fine + public async Task PostPayment_BankAnswered_ReturnsCreatedPaymentWithLocation(PaymentStatus status) + { + this.SetupPayment(status); + + ActionResult result = await this.Post(); + + CreatedAtActionResult created = Assert.IsType(result.Result); + Assert.Equal(nameof(PaymentsController.GetPayment), created.ActionName); + Assert.Equal(TestData.PaymentId, created.RouteValues?["paymentId"]); + PaymentResponse response = Assert.IsType(created.Value); + Assert.Equal(TestData.PaymentId, response.Id); + Assert.Equal(status, response.Status); + } + + [Fact] + public async Task PostPayment_Invalid_ReturnsValidationProblemWithoutProcessing() + { + this.validator + .Setup(v => v.Validate(It.IsAny())) + .Returns(new Dictionary { ["cvv"] = ["Must be 3 or 4 digits."] }); + + ActionResult result = await this.Post(); + + BadRequestObjectResult badRequest = Assert.IsType(result.Result); + ValidationProblemDetails problem = Assert.IsType(badRequest.Value); + Assert.Equal(["Must be 3 or 4 digits."], problem.Errors["cvv"]); + this.VerifyNeverProcessed(); + } + + [Theory] + [InlineData("")] + [InlineData("has space")] + [InlineData("naïve")] + public async Task PostPayment_MalformedIdempotencyKey_ReturnsValidationProblemWithoutProcessing(string idempotencyKey) + { + ActionResult result = await this.Post(idempotencyKey); + + BadRequestObjectResult badRequest = Assert.IsType(result.Result); + ValidationProblemDetails problem = Assert.IsType(badRequest.Value); + Assert.True(problem.Errors.ContainsKey("Idempotency-Key")); + this.VerifyNeverProcessed(); + } + + [Fact] + public async Task PostPayment_Replayed_ReturnsTheOriginalOutcomeMarkedAsReplayed() + { + this.SetupAttempt(new PaymentAttempt.Replayed(TestData.Payment(PaymentStatus.Authorized))); + + ActionResult result = await this.Post("order-42"); + + CreatedAtActionResult created = Assert.IsType(result.Result); + Assert.Equal(TestData.PaymentId, Assert.IsType(created.Value).Id); + Assert.Equal("true", this.controller.Response.Headers["Idempotent-Replayed"].ToString()); + } + + [Fact] + public async Task PostPayment_KeyStillInProgress_ReturnsConflictWithPaymentId() + { + this.SetupAttempt(new PaymentAttempt.InProgress(TestData.PaymentId)); + + ActionResult result = await this.Post("order-42"); + + ObjectResult objectResult = Assert.IsType(result.Result); + Assert.Equal(StatusCodes.Status409Conflict, objectResult.StatusCode); + Assert.Equal(TestData.PaymentId, Assert.IsType(objectResult.Value).Extensions["paymentId"]); + } + + [Fact] + public async Task PostPayment_KeyReused_ReturnsUnprocessableEntity() + { + this.SetupAttempt(new PaymentAttempt.KeyReused()); + + ActionResult result = await this.Post("order-42"); + + ObjectResult objectResult = Assert.IsType(result.Result); + Assert.Equal(StatusCodes.Status422UnprocessableEntity, objectResult.StatusCode); + } + + [Theory] + [InlineData(PaymentStatus.Failed)] + [InlineData(PaymentStatus.Unknown)] + public async Task PostPayment_BankCouldNotComplete_ReturnsBadGatewayWithPaymentId(PaymentStatus status) + { + this.SetupPayment(status); + + ActionResult result = await this.Post(); + + ObjectResult objectResult = Assert.IsType(result.Result); + Assert.Equal(StatusCodes.Status502BadGateway, objectResult.StatusCode); + ProblemDetails problem = Assert.IsType(objectResult.Value); + Assert.Equal(TestData.PaymentId, problem.Extensions["paymentId"]); + Assert.Equal(status.ToString(), problem.Extensions["paymentStatus"]); + } + + // stands in for MVC's factory, which needs the full service provider + private sealed class SimpleProblemDetailsFactory : ProblemDetailsFactory + { + public override ProblemDetails CreateProblemDetails( + HttpContext httpContext, int? statusCode = null, string? title = null, string? type = null, string? detail = null, string? instance = null) + { + return new ProblemDetails { Status = statusCode, Title = title, Type = type, Detail = detail, Instance = instance }; + } + + public override ValidationProblemDetails CreateValidationProblemDetails( + HttpContext httpContext, ModelStateDictionary modelStateDictionary, int? statusCode = null, string? title = null, + string? type = null, string? detail = null, string? instance = null) + { + return new ValidationProblemDetails(modelStateDictionary) { Status = statusCode ?? StatusCodes.Status400BadRequest }; + } + } +} diff --git a/tests/PaymentGatewayApiUnitTests/Middleware/CorrelationIdMiddlewareTests.cs b/tests/PaymentGatewayApiUnitTests/Middleware/CorrelationIdMiddlewareTests.cs new file mode 100644 index 0000000..1c9940c --- /dev/null +++ b/tests/PaymentGatewayApiUnitTests/Middleware/CorrelationIdMiddlewareTests.cs @@ -0,0 +1,121 @@ +using System; +using System.Collections.Generic; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Http.Features; +using PaymentGatewayApi.Middleware; +using Serilog; +using Serilog.Core; +using Serilog.Events; +using Xunit; + +namespace PaymentGatewayApiUnitTests.Middleware; + +public class CorrelationIdMiddlewareTests +{ + private readonly CollectingSink sink = new(); + private readonly Logger logger; + + public CorrelationIdMiddlewareTests() + { + this.logger = new LoggerConfiguration() + .Enrich.FromLogContext() + .WriteTo.Sink(this.sink) + .CreateLogger(); + } + + private static DefaultHttpContext NewContext(string? header = null) + { + DefaultHttpContext context = new(); + context.Features.Set(new StartableResponseFeature()); + if (header is not null) + { + context.Request.Headers[CorrelationIdMiddleware.HeaderName] = header; + } + + return context; + } + + // runs the middleware with a next delegate that logs one event and starts the response + private async Task InvokeAndCapture(DefaultHttpContext context) + { + CorrelationIdMiddleware middleware = new(_ => + { + this.logger.Information("next"); + return Task.CompletedTask; + }); + + await middleware.InvokeAsync(context); + await ((StartableResponseFeature)context.Features.Get()!).StartAsync(); + + LogEvent logEvent = Assert.Single(this.sink.Events); + Assert.True(logEvent.Properties.TryGetValue("CorrelationId", out LogEventPropertyValue? value)); + return Assert.IsType(value).Value?.ToString(); + } + + [Theory] + [InlineData("abc")] + [InlineData("16bd4749-d150-48d1-a3ce-257457d74d1f")] + public async Task InvokeAsync_WellFormedHeader_IsLoggedStoredAndEchoed(string header) + { + DefaultHttpContext context = NewContext(header); + + string? logged = await this.InvokeAndCapture(context); + + Assert.Equal(header, logged); + Assert.Equal(header, CorrelationIdMiddleware.Get(context)); + Assert.Equal(header, context.Response.Headers[CorrelationIdMiddleware.HeaderName].ToString()); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("abc\ninjected log line")] + [InlineData("