refactor(api): consolidate Stripe price resolution + startup validation (#78) - #177
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 26951803 | Triggered | Generic Password | aeab93e | tests/Orbit.IntegrationTests/IntegrationTestWebApplicationFactory.cs | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
There was a problem hiding this comment.
Clean consolidation of the 2×2 price-switch behind a single IPriceResolver seam, with fail-fast startup validation that matches the existing JwtSettings.Validate() idiom. The new PriceResolver correctly maps all four combinations and throws (not silently falls through) on an unknown interval. CapturingBillingService is a well-scoped test double — the AddScoped(_ => BillingService) pattern is correct for sequential tests reading the captured ID back immediately. The GitGuardian alert on the encryption key is a false positive: it's a dedicated test placeholder guarded by SetIfMissing, so any real env var wins. No contract changes, no authorization gaps, test coverage for every new code path including the startup-validation throw. Ship it.
|
Rebased onto Both PRs independently rewrote that factory:
Reconciliation in the factory:
No feature changes in this PR were touched — only the test factory file. Validation: Merge #176 first, then this. |
There was a problem hiding this comment.
Post-rebase re-review (rebased onto #176 to avoid factory conflict). The only delta from my prior approval is removing the now-redundant Jwt__SecretKey env-var from the static constructor — JWT secret injection is correctly delegated to #176's UseSetting path. All prior findings still hold: IPriceResolver DI seam is clean, StripeSettings.ValidatePriceIds() follows the existing JwtSettings.Validate() idiom, CapturingBillingService is correctly wired via AddScoped and safe under Sequential execution, and every new code path has unit and integration test coverage. Ship it after #176 merges.
There was a problem hiding this comment.
Post-rebase re-check on commit 1cb8212 (the Development exemption). The IsDevelopment() guard is the correct shape for a fail-fast startup check — non-dev environments (staging, prod) get the hard boot failure, local dev is exempt exactly as the PR description explains. No new correctness issues, no auth gaps, no contract changes. The rest of the review stands from my prior two approvals: IPriceResolver seam is clean, StripeSettings.ValidatePriceIds() follows the JwtSettings.Validate() idiom, PriceResolver correctly throws on unknown intervals rather than silently returning a wrong price ID, and the Sequential-collection CapturingBillingService pattern is safe. Ship after #176 merges.
…on (#78) Consolidate the duplicated 2x2 (interval x isBrazil) price-ID switches from CreateCheckoutCommandHandler and GetPlansQueryHandler behind a single IPriceResolver seam, so the mapping lives in exactly one place. Add fail-fast startup validation (StripeSettings.ValidatePriceIds) wired into ValidateOrbitSecuritySettings so the API refuses to boot when any of the four BRL/USD monthly/yearly price IDs is blank. Add unit tests for the resolver and the validation, plus BR and non-BR integration tests covering checkout price resolution and plans currency end-to-end with a faked IBillingService. Refs thomasluizon/orbit-ui-mobile#78 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Clean consolidation of the Stripe 2×2 price-switch behind IPriceResolver, startup validation following the JwtSettings.Validate() idiom, and a IsDevelopment() guard that exempts local dev without silently degrading deployed environments. PriceResolver correctly throws on unknown intervals instead of the prior silent fallback to a wrong price ID. CapturingBillingService is correctly wired via AddScoped and safe under Sequential collection execution. The GitGuardian flag is a false positive — the encryption key is a test placeholder behind SetIfMissing, so any real env var wins. All new code paths have unit and integration test coverage. Ship it after #176 merges.
What
Hardens and consolidates the BR→BRL / non-BR→USD Stripe pricing path (the behavior already shipped; this is the best-quality follow-up).
(interval, isBrazil)price-ID switches inCreateCheckoutCommandHandlerandGetPlansQueryHandlerare consolidated behind one deepIPriceResolver(Resolve(interval, isBrazil)). The mapping now lives in exactly one place; both handlers consume the seam.StripeSettings.ValidatePriceIds()(mirrors the existingJwtSettings.Validate()idiom) is wired intoValidateOrbitSecuritySettings, so the API refuses to boot when any of the four BRL/USD monthly/yearly price IDs is blank — naming the missing key(s) — instead of silently degrading users to the wrong currency./api/subscriptions/checkout(asserts the resolved price ID) and/api/subscriptions/plans(asserts currency) end-to-end with a fakedIBillingService(no live Stripe).Config decision
Config stays flat (
MonthlyPriceIdBrl/YearlyPriceIdBrl/MonthlyPriceIdUsd/YearlyPriceIdUsd); the issue's nestedStripe:Prices:BRL:Annualsuggestion was intentionally not taken. The audience collapses to oneisBrazilbool, so the price space is a fixed 2×2 thatPriceResolverfully hides from both callers — nesting would churnStripeSettings, the Render env-var names, and existing tests for zero caller-side gain. A WHY comment records this inPriceResolver.cs.Validation
dotnet build Orbit.slnx— green.CreateCheckoutCommandHandlerTests/GetPlansQueryHandlerTestsand the newPriceResolverTests/StripeSettingsTests).SubscriptionPricingIntegrationTestspass against a real Postgresorbit_test. The integrationWebApplicationFactorynow injects test config (JWT/Encryption/DB/Stripe price IDs) via environment variables — the sanctioned pattern pertests/CLAUDE.md— which also makes the suite hermetic and runnable in a git worktree (where the gitignoredappsettings.Development.jsonis absent).ApiKeyTests(free-user PayGate on list/revoke — also fails onmainbaseline) andUserFactsControllerTests.*AfterChat(require a live AI backend).Deploy note
This turns a previously-silent misconfiguration into a hard boot failure (the intended AC). Render env must have all four
Stripe:*PriceId*set before deploy — the error message names any missing key.Merge note
This edits
ServiceCollectionExtensions.csin the Stripe DI block (~308-326) and theValidateOrbitSecuritySettingsmethod (~35). Issue #79 (in flight) edits a different region of the same file (AI-tool registration ~153-198), so a trivial rebase may be needed at merge — no logical conflict.Refs thomasluizon/orbit-ui-mobile#78