From 06c816de105e4d4afc73c96de439064b975f8a94 Mon Sep 17 00:00:00 2001 From: spacedandy-agents-simplex <342056592+spacedandy-agents-simplex@users.noreply.github.com> Date: Thu, 15 Oct 2026 15:50:21 +0000 Subject: [PATCH] review: round 1 fixes badge-service: a Play charge whose clipped period start leaves its month already spent is answered as period over, not internal (R1-A1). docs: the per-store answer classes list Play's 410 and the App Store subscription check (R1-A2). tests: a held receipt with no attempt yet blocks an invoice (R1-B2); a renewing report keeps an ended status (R1-B3); a test comment names [google] (run 005 C2-R2-1). ios: a subscription refused because this Apple ID subscribed before shows its sentence, not the error's name (R1-C2); ios, android, desktop: the Check your order footnotes share one helper (R1-C3). Run: 006-subscriptions-branch-review, review round 1 --- apps/ios/Shared/Model/SimpleXAPI.swift | 3 ++ .../Views/Badges/BadgesCheckOrderView.swift | 18 ++----- .../views/badges/BadgesCheckOrderView.kt | 13 ++--- .../src/BadgeService/Service.hs | 13 ++++- docs/protocol/badges-rpc.md | 13 ++--- tests/Bots/BadgeService/BotTests.hs | 50 ++++++++++++++++++- 6 files changed, 78 insertions(+), 32 deletions(-) diff --git a/apps/ios/Shared/Model/SimpleXAPI.swift b/apps/ios/Shared/Model/SimpleXAPI.swift index cd9cfdb8c3..454ec15281 100644 --- a/apps/ios/Shared/Model/SimpleXAPI.swift +++ b/apps/ios/Shared/Model/SimpleXAPI.swift @@ -2257,6 +2257,9 @@ func redeemErrorText(_ error: Error, purchase: Bool) -> String { case .unknownKeyIndex, .credentialNotVerified: return NSLocalizedString("This app version cannot verify this badge. Please update the app.", comment: "alert message") } } + if case .subscribedBefore? = error as? BadgeStoreError { + return NSLocalizedString("This Apple ID subscribed before. Subscribing again is not available in the app yet.", comment: "alert message") + } return String.localizedStringWithFormat(NSLocalizedString("Error: %@", comment: "alert message"), responseError(error)) } diff --git a/apps/ios/Shared/Views/Badges/BadgesCheckOrderView.swift b/apps/ios/Shared/Views/Badges/BadgesCheckOrderView.swift index 656ab5c7a5..a798c1fd85 100644 --- a/apps/ios/Shared/Views/Badges/BadgesCheckOrderView.swift +++ b/apps/ios/Shared/Views/Badges/BadgesCheckOrderView.swift @@ -47,18 +47,14 @@ struct BadgesCheckOrderView: View { if case let .price(p) = store.price(level, period, compact: false) { switch period { - case .monthly: subscriptionDisclosure("Payment is charged to your Apple ID when you confirm the purchase. The subscription renews automatically at \(p) per month unless you cancel it at least 24 hours before the end of the current period. You can manage and cancel it in your App Store account settings.") - case .annual: subscriptionDisclosure("Payment is charged to your Apple ID when you confirm the purchase. The subscription renews automatically at \(p) per year unless you cancel it at least 24 hours before the end of the current period. You can manage and cancel it in your App Store account settings.") + case .monthly: footnote("Payment is charged to your Apple ID when you confirm the purchase. The subscription renews automatically at \(p) per month unless you cancel it at least 24 hours before the end of the current period. You can manage and cancel it in your App Store account settings.") + case .annual: footnote("Payment is charged to your Apple ID when you confirm the purchase. The subscription renews automatically at \(p) per year unless you cancel it at least 24 hours before the end of the current period. You can manage and cancel it in your App Store account settings.") case .oneMonth: EmptyView() } } if !store.forSale(period) && store.subscribedBefore == true { - Text("This Apple ID subscribed before. Subscribing again is not available in the app yet.") - .font(.footnote) - .foregroundColor(theme.colors.secondary) - .multilineTextAlignment(.center) - .fixedSize(horizontal: false, vertical: true) + footnote("This Apple ID subscribed before. Subscribing again is not available in the app yet.") } Spacer(minLength: 20) @@ -74,11 +70,7 @@ struct BadgesCheckOrderView: View { .font(.footnote) RestorePurchasesButton() .padding(.top, 10) - Text("Adds an App Store purchase of this Apple ID that no profile here has yet. A purchase still being added on another device may be added here instead.") - .font(.footnote) - .foregroundColor(theme.colors.secondary) - .multilineTextAlignment(.center) - .fixedSize(horizontal: false, vertical: true) + footnote("Adds an App Store purchase of this Apple ID that no profile here has yet. A purchase still being added on another device may be added here instead.") } .padding(.bottom, g.safeAreaInsets.bottom == 0 ? 20 : 0) } @@ -94,7 +86,7 @@ struct BadgesCheckOrderView: View { .alert(item: $alert) { $0.alert } } - private func subscriptionDisclosure(_ text: LocalizedStringKey) -> some View { + private func footnote(_ text: LocalizedStringKey) -> some View { Text(text) .font(.footnote) .foregroundColor(theme.colors.secondary) diff --git a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/badges/BadgesCheckOrderView.kt b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/badges/BadgesCheckOrderView.kt index 02ef484691..76d3bf7c1b 100644 --- a/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/badges/BadgesCheckOrderView.kt +++ b/apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/badges/BadgesCheckOrderView.kt @@ -61,8 +61,8 @@ fun BadgesCheckOrderView(level: BadgeLevel, period: BadgePeriod, modalManager: M val price = BadgeStore.price(level, period, compact = false) if (price is BadgePrice.Price) { when (period) { - BadgePeriod.Monthly -> SubscriptionDisclosure(String.format(stringResource(MR.strings.badges_subscription_disclosure_monthly), price.price)) - BadgePeriod.Annual -> SubscriptionDisclosure(String.format(stringResource(MR.strings.badges_subscription_disclosure_annual), price.price)) + BadgePeriod.Monthly -> Footnote(String.format(stringResource(MR.strings.badges_subscription_disclosure_monthly), price.price)) + BadgePeriod.Annual -> Footnote(String.format(stringResource(MR.strings.badges_subscription_disclosure_annual), price.price)) BadgePeriod.OneMonth -> {} } } @@ -81,19 +81,14 @@ fun BadgesCheckOrderView(level: BadgeLevel, period: BadgePeriod, modalManager: M ) if (badgeStoreLane) { RestorePurchasesButton(Modifier.padding(top = 10.dp)) - Text( - stringResource(MR.strings.badges_restore_purchases_footnote), - style = MaterialTheme.typography.body2, - color = MaterialTheme.colors.secondary, - textAlign = TextAlign.Center - ) + Footnote(stringResource(MR.strings.badges_restore_purchases_footnote)) } } } } @Composable -private fun SubscriptionDisclosure(text: String) { +private fun Footnote(text: String) { Text( text, style = MaterialTheme.typography.body2, diff --git a/apps/simplex-badge-service/src/BadgeService/Service.hs b/apps/simplex-badge-service/src/BadgeService/Service.hs index cc0ac0bca4..f762544afa 100644 --- a/apps/simplex-badge-service/src/BadgeService/Service.hs +++ b/apps/simplex-badge-service/src/BadgeService/Service.hs @@ -544,7 +544,7 @@ purchaseWithReceipt key cc verifier checks purchaseKey masterKey StoreReceipt {t _ -> answerCredit credit newSubscription checked@SubscriptionCheck {subscriptionState} = do now <- badgeNow cc - case grantableCharge provider Nothing now checked of + case grantableCharge provider Nothing now checked >>= fundsMonthNow now Nothing of Left notGranted -> firstChargeNotGranted provider notGranted Right (charge@StoreCharge {periodStart, chargePaid = paid}, (badgeType, months)) -> acknowledged acknowledgeSubscription $ do @@ -609,6 +609,15 @@ grantableCharge provider tier now SubscriptionCheck {subscriptionState, latestCh | now >= periodEnd -> Left NGPeriodOver | otherwise -> Right (charge, plan) +-- | Play dates a period from its end less the plan's months, clipped at month ends, so the months the ledger grants +-- from that start can end before Play's period does: a charge still in its period may then fund no month now. +fundsMonthNow :: UTCTime -> Maybe StatementEntry -> (StoreCharge, (BadgeType, Int)) -> Either NotGranted (StoreCharge, (BadgeType, Int)) +fundsMonthNow now tip_ grant@(StoreCharge {periodStart}, (badgeType, months)) + | isJust $ issueEntry now "" $ last $ tip : chargeEntries periodStart now months "" ("", "", "") tip = Right grant + | otherwise = Left NGPeriodOver + where + tip = fromMaybe (emptyEntry periodStart badgeType) tip_ + -- | No payment row exists yet to stamp, so an App Store claim's checks are bounded by the purchase checks, a Play one's by each answer's retryAfter. firstChargeNotGranted :: PaymentProvider -> NotGranted -> IO BadgeServiceResponse firstChargeNotGranted provider = \case @@ -708,7 +717,7 @@ issueBadgeCmd key cc verifier purchaseKey BadgeBalance {lastEntry} = do Right () -> checkStoreSubscription verifier subscriptionRef Nothing >>= \case Left refusal -> atLeastCheckInterval <$> storeRefusalResponse refusal - Right checked@SubscriptionCheck {subscriptionState, latestCharge} -> case grantableCharge provider (Just badgeType) now checked of + Right checked@SubscriptionCheck {subscriptionState, latestCharge} -> case grantableCharge provider (Just badgeType) now checked >>= fundsMonthNow now (Just tip) of Left NGMalformedPeriod -> storeRefusalResponse $ SRVerifierFailed "a charge whose period does not end after its start" Left reason -> do forM_ latestCharge $ \_ -> logInfo $ "store subscription charge not granted: " <> tshow reason diff --git a/docs/protocol/badges-rpc.md b/docs/protocol/badges-rpc.md index f73fab9615..fcef858785 100644 --- a/docs/protocol/badges-rpc.md +++ b/docs/protocol/badges-rpc.md @@ -42,17 +42,18 @@ The client never states what is signed. The tier is that of whatever funded the - **Pending**, `payment_pending` — a real purchase the store has not settled. - **Unreachable**, `provider_unavailable` — the store was not asked or did not answer. - **Internal**, `internal` — this service's own failure, never the store's answer. - - **App Store.** Apple is verified offline, so it has no "not yet". - - `receipt_invalid`: a JWS that is not three base64url parts or names no `transactionId`, or, for an auto-renewable subscription, no `originalTransactionId`; an `x5c` certificate that is not base64; a JWS signature that is not 64 bytes; a leaf whose signature does not verify against the intermediate; a JWS signature that does not verify against the leaf; a transaction with a `revocationDate`, refunded or revoked. - - `test_purchase`: refused by the service after the verifier vouched for it, a `Sandbox` transaction. - - `internal`: whatever this service's own configuration decides — an intermediate whose signature verifies against none of the configured roots, and another app's `bundleId`, since a root may be missing or the bundle id wrong as likely as the receipt forged; a transaction from Xcode's StoreKit testing, which Xcode signs and not the App Store, so no configured root verifies it; evidence in a shape this service does not know — a header that does not name `ES256` or carries no `x5c` of exactly three certificates, a certificate that does not decode, a leaf signature the intermediate's key cannot check, an algorithm or key type the library does not support, a chain without Apple's receipt-signing marker extensions, a leaf key that is not P-256, a payload with a missing or mistyped field or an unknown `environment`; the verifier's own failure or overrun; and, refused by the service, a verified transaction other than the one claimed, or a quantity other than 1. + - **App Store.** Apple's receipt is verified offline, so it has no "not yet"; a subscription's status is then read from the App Store Server API, which can fail to answer. + - `receipt_invalid`: a JWS that is not three base64url parts or names no `transactionId`, or, for an auto-renewable subscription, no `originalTransactionId`; an `x5c` certificate that is not base64; a JWS signature that is not 64 bytes; a leaf whose signature does not verify against the intermediate; a JWS signature that does not verify against the leaf; a transaction with a `revocationDate`, refunded or revoked; and a subscription status whose transaction is family-shared. + - `test_purchase`: refused by the service after the verifier vouched for it, a `Sandbox` transaction, and a subscription status from the `Sandbox` environment. + - `provider_unavailable`, for a subscription's status only: network failure or timeout, and every error response but 401 and 403 — a 404 included, which Apple answers for an original transaction id its environment does not hold — 429 and 5xx among them. + - `internal`: whatever this service's own configuration decides — an intermediate whose signature verifies against none of the configured roots, and another app's `bundleId`, since a root may be missing or the bundle id wrong as likely as the receipt forged; a transaction from Xcode's StoreKit testing, which Xcode signs and not the App Store, so no configured root verifies it; evidence in a shape this service does not know — a header that does not name `ES256` or carries no `x5c` of exactly three certificates, a certificate that does not decode, a leaf signature the intermediate's key cannot check, an algorithm or key type the library does not support, a chain without Apple's receipt-signing marker extensions, a leaf key that is not P-256, a payload with a missing or mistyped field or an unknown `environment`; the verifier's own failure or overrun; and, refused by the service, a verified transaction other than the one claimed, or a quantity other than 1. For a subscription's status: Apple refusing this service's API token or key (401, 403); an original transaction id that is not all digits; an answer that cannot be read, is over a mebibyte, or names no such subscription; a signed transaction or renewal info that fails any check above (Apple answered it and no client presented it, so it is never `receipt_invalid`), is not a subscription transaction or renewal info, names another app's bundle id or another subscription, or an unknown environment; and a status this service does not know. - `product_unavailable`: a product this service does not price — the verifier vouched for the transaction either way. - `provider_not_configured`: the deployment has no `[apple]` section. - **Google Play.** Only a purchase record is a verdict. - - `receipt_invalid`: `purchaseState` canceled, which a purchase token never leaves, and a product id outside the characters Play documents, since no product of this app can have one. + - `receipt_invalid`: `purchaseState` canceled, which a purchase token never leaves, and a product id outside the characters Play documents, since no product of this app can have one; and a 410 from `purchases.subscriptionsv2.get`, a subscription that expired too long ago to read, on a first purchase (a renewal check answers it as ended). - `test_purchase`: refused by the service after the verifier vouched for it, `purchaseType` test, a license tester's purchase, and a subscription with `testPurchase`. - `payment_pending`: `purchaseState` pending, and `SUBSCRIPTION_STATE_PENDING`. - - `provider_unavailable`: network failure or timeout, and every error response from `purchases.products.get` or `purchases.subscriptionsv2.get` but 401 and 403 — a 404 included, which Play may answer both for a token it has just issued and not yet recorded and for one that was never a purchase, and a 400 or 410 calling the token invalid; a 5xx or 429 from the token endpoint; and a token this service will not send — whose characters fall outside the grammar it accepts, which is our guess since Play documents none, or which is only dots, which a URL path would resolve away. + - `provider_unavailable`: network failure or timeout, and every error response from `purchases.products.get` or `purchases.subscriptionsv2.get` but 401 and 403 — a 404 included, which Play may answer both for a token it has just issued and not yet recorded and for one that was never a purchase, and a 400, or a 410 from `purchases.products.get`, calling the token invalid; a 5xx or 429 from the token endpoint; and a token this service will not send — whose characters fall outside the grammar it accepts, which is our guess since Play documents none, or which is only dots, which a URL path would resolve away. - `internal`: Google refusing this service's credentials or permissions — a 401 or 403 from `purchases.products.get` or `purchases.subscriptionsv2.get`, and any other refusal from the token endpoint; a response that cannot be read, is too large, or names another product; a `purchaseState` or `subscriptionState` this service does not know; a subscription without exactly one line item, or a charge without its `expiryTime` or base plan; and a `purchaseType` other than test or promo — a promo code, which only this app's developer issues, is credited like a purchase. - `product_unavailable` and `provider_not_configured` as for the App Store, the latter without a `[google]` section. - Before crediting a purchase of a product it prices, the service acknowledges it, unless Play already reports it acknowledged or consumed; a subscription is acknowledged with `purchases.subscriptions.acknowledge` before its first charge is credited. It acknowledges and never consumes, so the app's purchase query still lists the purchase. The app may not be honest, and only acknowledgement stops Play refunding the purchase after three days, so nothing is credited unacknowledged. A failed acknowledgement credits nothing and is answered like a failed read — `provider_unavailable` when Play does not answer, `internal` when it refuses this service, both logged with "acknowledging" in the reason — and the app presents the purchase again. A deployment whose service account lacks the permission to acknowledge therefore answers `internal` to every Play purchase the app has not already acknowledged itself. diff --git a/tests/Bots/BadgeService/BotTests.hs b/tests/Bots/BadgeService/BotTests.hs index 40f0841f63..7a3f09cd36 100644 --- a/tests/Bots/BadgeService/BotTests.hs +++ b/tests/Bots/BadgeService/BotTests.hs @@ -52,7 +52,8 @@ import Data.String (fromString) import System.Timeout (timeout) import Data.Text (Text) import qualified Data.Text as T -import Data.Time.Clock (NominalDiffTime, UTCTime, addUTCTime, diffUTCTime, getCurrentTime, nominalDay) +import Data.Time.Calendar (fromGregorian) +import Data.Time.Clock (NominalDiffTime, UTCTime (..), addUTCTime, diffUTCTime, getCurrentTime, nominalDay) import Data.Time.Format (defaultTimeLocale, formatTime) import Data.Word (Word32) import Simplex.Chat.Badges (BadgeCredential (..), BadgeInfo (..), BadgeMasterKey, BadgeType (..), generateMasterKey) @@ -168,6 +169,7 @@ badgeServiceTests = do it "should credit a purchase to a hidden profile without naming it" testPurchaseDeliveredToHiddenProfile it "should record a listed transaction only into a record it already has, and a Restore hand-over as its own" testListedReceiptNotRecorded it "should refuse a code or an invoice while a held receipt may be credited or is an Apple subscription, and not otherwise" testRedeemRefusedWhileReceiptHeld + it "should refuse an invoice while a held receipt's first attempt has not answered" testInvoiceRefusedWhileFirstAttemptOpen describe "invoice first" $ do it "should credit a receipt to the profile that created its invoice however late it arrives, and answer as that profile" testInvoiceOtherProfile it "should credit a receipt naming an unknown invoice to the presenting profile" testInvoiceUnknown @@ -197,6 +199,7 @@ badgeServiceTests = do it "should grant each store charge once, and answer a repeat inside the issued month without asking the store" testSubscriptionChargeGrantedOnce it "should ask the store only once coverage has ended, and at most once an hour, failed checks included" testSubscriptionStoreChecksBounded it "should record and not grant a revoked, other-tier, offer, unstarted or ended charge" testSubscriptionChargeNotGranted + it "should not grant a Play charge whose month from its clipped period start has already run out" testClippedPlayPeriodNotGranted it "should answer an unreachable store with the check interval, writing no charge or ledger row" testSubscriptionStoreUnreachable it "should renew an App Store subscription by its original id, granting a charge first recorded before its period started" testAppleSubscriptionRenews it "should keep asking at zero months, and present the renewed credential when the old one expires" testSubscriptionRenews @@ -2209,6 +2212,26 @@ testSubscriptionChargeNotGranted ps = rowCount cc "sx_badge_service_subscription_charges" `shouldReturn` 1 + length notGranted ledgerRows cc "sx_badge_service_badge_ledger" `shouldReturn` ledger +testClippedPlayPeriodNotGranted :: HasCallStack => TestParams -> IO () +testClippedPlayPeriodNotGranted ps = + withBadgeServiceEnv ps $ \env@BadgeServiceEnv {bsClock, bsController = cc, bsStore = store} -> do + (purchaseKey, _, purchased) <- subscribedMonthly env + ledger <- ledgerRows cc "sx_badge_service_badge_ledger" + -- Play dates the period ending on 31 March from 28 February, and the ledger's month from then ends on 28 March + let clippedEnd = UTCTime (fromGregorian 2027 3 31) 43200 + clipped = (storeCharge "subscr-badge-supporter-month-02" (addMonths (-1) clippedEnd) 1) {periodEnd = clippedEnd, chargeRef = "GPA.clipped"} + storeReports store $ subscriptionCheck SSRenewing $ Just clipped + setClockAt bsClock $ addUTCTime (-2 * nominalDay) clippedEnd + renewal <- assertBalance env purchaseKey (last purchased) + credentialOf renewal `shouldBe` Nothing + map entryTag (fst $ statementOf renewal) `shouldBe` [] + ledgerRows cc "sx_badge_service_badge_ledger" `shouldReturn` ledger + rowCount cc "sx_badge_service_subscription_charges" `shouldReturn` 2 + (otherKey, otherMasterKey) <- newPurchaseKeys + let otherToken = "fake-play-token-clipped.AO-J1Op9n4cjW9qQl5" + setSubscription store (SubscriptionGoogle otherToken) $ subscriptionCheck SSRenewing $ Just clipped + refusalOf <$> serviceCmd env otherKey (purchaseCmd otherMasterKey $ googlePayment "subscr_badge_supporter_01" otherToken) `shouldReturn` (BSEPaymentPending, Just 86400) + testSubscriptionStoreUnreachable :: HasCallStack => TestParams -> IO () testSubscriptionStoreUnreachable ps = withBadgeServiceEnv ps $ \env@BadgeServiceEnv {bsClock, bsController = cc, bsStore = store} -> do @@ -2420,6 +2443,11 @@ testSubscriptionEnds ps = alice <## "ok" threadDelay 2000000 subscriptionChecks store `shouldReturn` checks + -- the device's store still lists it as renewing, which does not undo the service's word that it ended + subscriptionStatuses (chatController alice) `shouldReturn` [Just "ended"] + alice ##> ("/_badge subscription renewing " <> paymentArg subscriptionPlay) + alice <## "ok" + subscriptionStatuses (chatController alice) `shouldReturn` [Just "ended"] subscriptionStatuses :: ChatController -> IO [Maybe Text] subscriptionStatuses ChatController {chatStore} = @@ -2713,7 +2741,7 @@ testAppleSandboxSubscriptionNotAsked ps = do testAppleSubscriptionWithoutApiKey :: HasCallStack => TestParams -> IO () testAppleSubscriptionWithoutApiKey = appleSubscriptionNotConfigured id --- | With [play] beside [apple], configureStoreVerifier builds a checker that answers App Store refs not configured. +-- | [google] beside an [apple] with no API key: configureStoreVerifier builds a checker that answers App Store refs not configured. testAppleSubscriptionWithPlayChecker :: HasCallStack => TestParams -> IO () testAppleSubscriptionWithPlayChecker = appleSubscriptionNotConfigured $ \v -> v {checkSubscription = Just playOnly} where @@ -3030,6 +3058,24 @@ testRedeemRefusedWhileReceiptHeld ps = issueCode cc BTSupporter 1 >>= \code' -> refused ("/_redeem_badge_code 3 " <> codeArg code') refused "/_badge invoice 3" +testInvoiceRefusedWhileFirstAttemptOpen :: HasCallStack => TestParams -> IO () +testInvoiceRefusedWhileFirstAttemptOpen ps = do + entered <- newEmptyMVar + release <- newEmptyMVar + hook <- newIORef $ putMVar entered () >> takeMVar release + let holding store = (googleVerifierWithHook hook store) {verifyTimeout = 60000000} + withBadgeServiceVerifier ps holding $ \BadgeServiceEnv {bsClientCfg} -> + withNewTestChatCfg ps bsClientCfg "alice" aliceProfile $ \alice -> do + alice ##> ("/_badge purchase 1 " <> paymentArg supporterPlay) + storePurchaseOpen alice "" + timeout 30000000 (takeMVar entered) `shouldReturn` Just () + heldStoreReceipts (chatController alice) `shouldReturn` 1 + heldStoreReceiptErrors (chatController alice) `shouldReturn` [] + alice ##> "/_badge invoice 1" + alice <## "cannot get badge: store purchase open" + putMVar release () + expectLinesWithin 60 alice ["1: supporter"] [] + testInvoiceUnknown :: HasCallStack => TestParams -> IO () testInvoiceUnknown ps = withBadgeServiceEnv ps $ \BadgeServiceEnv {bsClientCfg} ->