Prevent handling already processed transactions ios-365

[pronebird]

This PR introduces a transaction log to keep track of already processed transactions and prevent handling the same transaction more than once.

It turned out that a call to SKPaymentQueue.finishTransaction() may silently fail and then pump the same transaction back on the queue. When that happens we shouldn't upload the AppStore receipt again, instead we should only attempt to finish such transaction immediately.

Transaction log introduced in this PR stores transactions IDs in the text file as a list of newline separated strings. It is made to be thread safe in anticipation that the payment manager could be taken off the main queue and into background queue to offload the main thread, but I didn't want to go into heavy refactoring, nor introduce more asynchronous code paths. Hopefully that should be satisfactory given that transaction log is going to be relatively slim and do very little I/O.

Besides that I took care of documenting a lot of stuff in the payment manager to make it easier to review code and improve documentation coverage.

---

This change is 

[linear]

IOS-365 Keep payment transaction log

It was raised by marco that finishTransaction() of StoreKit may silently fail and the same transaction may come back again on the payment queue.

Since we send an App Store receipt to our servers each time we process a successful transaction, it would be prudent to keep a transaction log and ignore already processed transactions.

[buggmagnet]

Looking good !
Is there an easy way to test this ?

Reviewed 2 of 6 files at r1, 2 of 2 files at r2.
Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 43 at r2 (raw file):
nit
Super mega tiny nitpick.

I would replace replace this with

Maps each payment to an account token.

A dictionary is not necessarily a hash map.

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 67 at r2 (raw file):

    ) {
        self.backgroundTaskProvider = backgroundTaskProvider
        paymentQueue = queue

Is this missing self intentional ?

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 361 at r2 (raw file):

        // Obtain transaction identifier which must be set on transactions with purchased or restored state.
        guard let transactionIdentifier = transaction.transactionIdentifier else {
            logger.warning("Purhased or restored transaction does not contain a transaction identifier!")

typo:
"Purhased" instead of "Purchased"

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 366 at r2 (raw file):

        // Check if transaction is already processed.
        guard !transactionLog.contains(transactionIdentifier: transactionIdentifier) else {

I wonder if we should have some kind of mechanism that automatically prunes a completed transaction XXX days after it's completed.
Otherwise we have an ever growing list of transactions on the disk, and I'm wondering what's the privacy implication of this.

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 384 at r2 (raw file):

		58F3C0A4249CB069003E76BE /* HeaderBarView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 58F3C0A3249CB069003E76BE /* HeaderBarView.swift */; };
		58F3F36A2AA08E3C00D3B0A4 /* PacketTunnelProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 58F3F3692AA08E3C00D3B0A4 /* PacketTunnelProvider.swift */; };
		58F70FE52AEA707800E6890E /* StoreTransactionLog.swift in Sources */ = {isa = PBXBuildFile; fileRef = 58F70FE42AEA707800E6890E /* StoreTransactionLog.swift */; };

Please sort the StorePaymentManager folder by name

[pronebird]

With a sandbox account on device. But I don't know any way how to reproduce the issue related to the silent failure in SKPaymentQueue.finishTransaction(). It happened a couple of times during testing, usually it looks like two transactions with the same transaction ID appear with the "purchased" status on the payment queue, typically the second (repeat) transaction arrives briefly after a call to finishTransaction().

Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions

[pronebird]

Reviewable status: 4 of 6 files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 67 at r2 (raw file):

Previously, buggmagnet wrote…

Is this missing self intentional ?

I think so. self is only necessary to distinguish between the parameter and a member when both have the same name. In that case the parameter is called queue, but the member is paymentQueue. I can fix it if you have some preference here.

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 361 at r2 (raw file):

Previously, buggmagnet wrote…

typo:
"Purhased" instead of "Purchased"

Fixed. I had to type a lot of things quickly. Hope docs make some sense.

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 366 at r2 (raw file):

Otherwise we have an ever growing list of transactions on the disk

Yeah I thought about that too. I didn't see the problem with an ever growing list of transactions. It would really take a gigantic amount of effort and time to make this slow. Even if you do 100 transactions a year, that's 1000 in 10 years, which would be a few dozen KB of data on disk, a measly amount for the modern device.

and I'm wondering what's the privacy implication of this.

Yeah that's a legit concern, but I think our threat model is beyond someone having direct access to the cache of your sandboxed app.

Technically purging very old transactions over time makes sense but what should be the safe age? Say 1 month?

---

ios/MullvadVPN.xcodeproj/project.pbxproj line 384 at r2 (raw file):

Previously, buggmagnet wrote…

Please sort the StorePaymentManager folder by name

Done.

[pronebird]

Reviewable status: 3 of 6 files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 43 at r2 (raw file):

Previously, buggmagnet wrote…

nit
Super mega tiny nitpick.

I would replace replace this with

Maps each payment to an account token.

A dictionary is not necessarily a hash map.

Done.

[buggmagnet]

Reviewed 1 of 6 files at r1, 1 of 2 files at r3, all commit messages.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 67 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

I think so. self is only necessary to distinguish between the parameter and a member when both have the same name. In that case the parameter is called queue, but the member is paymentQueue. I can fix it if you have some preference here.

It's okay to leave it as is, I just wanted to make sure it was intentional.

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 361 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Fixed. I had to type a lot of things quickly. Hope docs make some sense.

The docs are great !

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 366 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Otherwise we have an ever growing list of transactions on the disk

Yeah I thought about that too. I didn't see the problem with an ever growing list of transactions. It would really take a gigantic amount of effort and time to make this slow. Even if you do 100 transactions a year, that's 1000 in 10 years, which would be a few dozen KB of data on disk, a measly amount for the modern device.

and I'm wondering what's the privacy implication of this.

Yeah that's a legit concern, but I think our threat model is beyond someone having direct access to the cache of your sandboxed app.

Technically purging very old transactions over time makes sense but what should be the safe age? Say 1 month?

This has been discussed offline. I agree with the conclusions that @pronebird came to.

[pronebird]

Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadVPN/StorePaymentManager/StorePaymentManager.swift line 361 at r2 (raw file):

Previously, buggmagnet wrote…

The docs are great !

Amazing! 👍

---

ios/MullvadVPN/StorePaymentManager/StoreTransactionLog.swift line 156 at r4 (raw file):

            var mutableFileURL = fileURL
            try mutableFileURL.setResourceValues(resourceValues)

FYI setResourceValues() is marked mutable so I had to copy fileURL into a var. I could as well make var fileURL: URL but I am not sure it's really necessary as I think it's just semantics and the call itself mutates the file attributes on disk.