Fail closed on Rock-initiated payments in ApollosGateway - #14
Conversation
The Apollos gateway exists to record gifts Apollos has already processed. Its Charge, Authorize, Credit, AddScheduledPayment, ReactivateScheduledPayment and UpdateScheduledPayment methods were inherited from Rock's Test Gateway and returned fabricated "T"+timestamp successes with no processor call, so any Rock flow that reaches them (event registration with a cost, transaction entry, refunds) records a paid transaction while no money moves. Those paths now return a null/false failure with an explanatory message telling staff to pick their church's real payment gateway. Reporting paths (GetPayments, reference numbers, schedule status, cancellation) are unchanged, and the finance sync job that writes already-processed Apollos gifts does not go through this component. Refs APO-11400 Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01RsU1KnsySNbifFhgUXrP2A
|
Wait till you see what it'll take to prove this PR 😃 |
|
Ha — fair, that's why it's a draft. The proof I'd want is: build the package, install it on a throwaway Rock, point a registration template with a cost at the Apollos gateway, and confirm the registration is refused with the message and no Is there a dev/sandbox Rock we can install a test build on, or does that mean standing one up? Generated by Claude Code |
|
We toyed around with that idea. But no, sadly, you're going to have to build it by hand |
|
Understood — and I can't stand one up from here (Rock needs Windows/IIS/SQL Server; this box is Linux, so I couldn't even compile the change). So the runtime proof has to wait on a Rock someone builds by hand, and this stays a draft until then. What it can be judged on now is reading: six methods stop returning a fabricated A dev Rock is going to be a prerequisite for the real registration gateway anyway, so my suggestion is to fold this verification into that setup rather than build one twice. Happy to leave it parked until then. Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Updates ApollosGateway to reject Rock-initiated payment processing instead of fabricating success.
Changes:
- Fails closed for payment and scheduled-payment mutations.
- Propagates failures through automated charges.
- Removes obsolete card validation.
Suppressed comments (1)
apollosproject.ApollosPlugin/Financial/ApollosGateway.cs:128
- This behavior change makes the current README statement that this gateway lets the giving UI and scheduled gifts create local Rock transactions incorrect (
README.md:91). Please update that documentation in the same change so administrators are not instructed to configure this reporting-only gateway for payment entry; it now rejects those flows.
internal const string NoPaymentProcessingMessage = "The Apollos gateway records gifts already processed by Apollos and cannot process payments. Choose your church's payment gateway instead.";
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
apollosproject.ApollosPlugin/Financial/ApollosGateway.cs:128
- This guard does not cover the Obsidian
CreateCustomerAccountentry point: it still returns a new token with an empty error (lines 139–143), even though no customer account or processor-side payment method is created. Rock can therefore report account setup as successful and persist a fabricated token, leaving the same fail-open behavior for this payment setup path. Make this method return the failure message and no token as well, or explicitly remove/disable the account-creation capability.
internal const string NoPaymentProcessingMessage = "The Apollos gateway records gifts already processed by Apollos and cannot process payments. Choose your church's payment gateway instead.";
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
README.md:91
- This says all “scheduled-payment mutations” are rejected, but
CancelScheduledPaymentstill intentionally returnstrueand deactivates the transaction (seeApollosGateway.cs:375-379) so existing fake schedules can be cleaned up. Please qualify this as rejecting creation, reactivation, and updates, or explicitly exclude cancellation, to keep the administrator guidance accurate.
`Financial/ApollosGateway.cs` is reporting-only for gifts Apollos has already processed. It does not call Stripe or another processor, and it rejects Rock-initiated charges, authorizations, refunds, saved-account setup, and scheduled-payment mutations. Do not select it for the giving UI, paid registration templates, or payment entry; use the church's real payment gateway instead. The Apollos Finance Sync job imports processed gifts independently of this component.
|
Letting @solideo-gloria handle the review |
References APO-11400
Issue
ApollosGatewayis reporting-only:StripeSyncJobwrites gifts that Apollos has already processed, and those imports do not use this gateway's payment methods. However, the component copied Rock Test Gateway behavior: Rock-initiatedCharge,Authorize,Credit, and scheduled-payment creation/reactivation/update could return fabricated success values without contacting a processor.APO-11400 involved Rock's built-in Test Gateway, not this plugin. This plugin carried the same unsafe fake-success behavior, so a paid Rock flow configured to use it could record money that never moved.
Solution
Fail closed with one explanatory message from every Rock-initiated processing path:
CreateCustomerAccount,Charge,Authorize,Credit, andAddScheduledPaymentnow returnnull;ReactivateScheduledPaymentandUpdateScheduledPaymentreturnfalse;AutomatedChargesurfacesCharge's failure. The now-unreachable card validator is removed.Reporting/reference/status behavior is unchanged. Cancellation remains available so an existing fake schedule can still be cleaned up. The README now identifies the gateway as reporting-only and directs administrators to a real payment gateway for paid Rock flows.
Validation
c9b7fd56abe1dfa87b9375a7324132e9ba07b282a5c44feca78749cadb73c112a5590da1bc1f2d34; subsequent changes only document the gateway and fail closed for saved-account creation, leaving the E2EChargepath unchangedfba1633a8989b0e0dc60974e15789c84e284ae97; exact head only narrows README wording and contains the sameApollosGateway.csblob98a5b686db1b1516ae4ff675e1e4a58d6b46e6ac18.2.4.018.2.4Roslyn and assemblies in a disposable Mono containerba1a5855ce64892c68d0ed12195be3b3f55d7c571d4d53ff9f6e397d44c21018eca73b7be7118e735a760f87d71ea52d63e07e8f7654667017f3f5010aff5582c7494721090d2dbf467ba035d0ebe72e65068d0167ab6f9fdb8a3cb4b039b5c7CreateCustomerAccount,Charge,Authorize,Credit,AddScheduledPayment,ReactivateScheduledPayment,UpdateScheduledPayment, andAutomatedCharge; no customer token was returned and failed schedule mutations left their models unchangedgit diff --checkProof
Parent
d6b3039completed the real public Rock registration/card flow for $25 and displayed fabricated referenceT20260820181124343. SQL counts moved from zero to one transaction, transaction detail, registration, and registrant.With only the plugin DLL changed to implementation commit
a5c44fe(whoseChargeimplementation is unchanged at exact headc9b7fd5), the same Rock registration/card flow rendered the explanatory gateway rejection. Transaction, transaction-detail, registration, and registrant counts all remained at the parent baseline of one; no second financial transaction was recorded.Risk
Low and narrowly scoped. The change affects only Rock-initiated processing calls on this reporting gateway; the finance import path and non-processing gateway operations are unchanged. A merged plugin change still requires packaging and installation before it can affect a Rock instance.