[FR]: Provide data race prevention for custom App Check providers

[sruncimanraileasy]

Description

For my current app, we have implemented a custom provider for App Check, as the SDK allows. This allows us to work around some bugs in the default implementation (e.g. #10561). Although not explicitly made clear in the guide, the phrase "The App Check SDK handles token caching, so always get a new token in your implementation of getToken(completion:)." meant that in my custom implementation, I enforced no synchronisation around token generation, expecting that the Firebase SDK would only request one token at a time. In production however, we have observed that the Firebase SDK can and will request an App Check token, whilst previous token generation flows are already in progress, leading to data races in our client.

The documentation does not explicitly provide any guarantees around data races here, and of course, by implementing custom provider, the SDK has no way of knowing about how tokens are being generated. However I would suggest that calls to getToken() on an instance conforming to AppCheckProvider are synchronised in some way, so that only one token generation can occur at a time.

I guess there is a philosophical question about whether this prevention should occur within the SDK, or whether this should be entirely the responsibility of the implementing client, which is why I am raising this as a feature request and not a bug. Thankfully, it is trivial to implement locking in the client when using Swift (Thanks Swift Actors!) 😄

In summary:
Asynchronous implementations of getToken() on custom AppCheckProvider instances currently must perform their own locking logic, to guard against multiple tokens being generated concurrently for a given app. I propose the SDK handles this, so that only one getToken() invocation can occur at any one time.

API Proposal

No response

Firebase Product(s)

App Check

[google-oss-bot]

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

[paulb777]

@weixifan Any thoughts or recommendations?

[weixifan]

@sruncimanraileasy Thank you for bringing this to our attention; the issue and question you raised are both interesting. I am curious to know: what kind of a data race did you experience? I am a bit surprised that you needed to record client-side global shared state in order to perform your custom attestation handshake.

If your use case is common enough, then I think it may be worthwhile to make the SDK handle the synchronization. My concern is that attestation is already an expensive operation in general, and I would be somewhat reluctant to introduce yet another thing (synchronization) that slows it down.

[sruncimanraileasy]

For our use case, it was to work around #10561 (which I understand is now fixed, but wasn't at the time we had to implement this), so we don't do anything fundamentally different . We wrap the standard flow from Apple's DeviceCheck framework, and so we needed to keep a hold of the key identifier the framework generated, and whether or not it had been asserted already or not. From memory, trying to attest a key identifier with Apple a second time fails, so it's easier to try do the entire flow as one operation. Our custom flow involves several async calls (worst case scenario 6 async calls, 3 for Device Check, and then 3 of our own API calls in response to each stage of the DeviceCheck flow), and it was during this flow that we found Firebase would ask for new tokens whilst we were still registering previous keys, which led to the client and our API having different ideas as to what key was best to be currently used.

There is probably scope to change our client implementation to allow for multiple key identifiers to be stored to work around this, but the repeated calls caught us out with the process we implemented