fix: Push Notification corner cases

ttypic commented 5 months ago

After thoroughly examining our Push Notification flow, I have built a diagram, and some of the problems have become clear.

We can't ignore `CalledDeactivate` event when we are in `NotActive` state

Currently, we are ignoring the CalledDeactivate event when we are in the NotActive state, which causes a lot of problems later on.

Example:

We ended up in NotActive with valid device identity token (e.g. haven't received registration update).
User logout from the app and calls deactivate (as we recommend to do), nothing happens.
User login with another clientId calls activate and get error clientId mismatch

We shouldn't use token / key auth during deregistration call

Users potentially can change apps, and we can handle this by relying solely on device token authentication during the deregistration call; otherwise, we might end up in a broken state.

Example:

If we use device token auth together with token or key auth from another Ably app we'll receive Token unrecognised error.

We should handle unauthorized errors or other problems with device identity token

When we encounter 401 status errors or code 40005 errors (invalid credentials), we should treat them as successful deregistrations and clear local storage. Otherwise, the device may remain in a broken state. If device identity token for some reason got invalid, broken or revoked we end up in permanent broken state.

paddybyers commented 5 months ago

We ended up in NotActive with valid device identity token (e.g. haven't received registration update)

If we're NotActive, shouldn't we delete all remnants of aborted registration attempts?

ttypic commented 5 months ago

We ended up in NotActive with valid device identity token (e.g. haven't received registration update)

If we're NotActive, shouldn't we delete all remnants of aborted registration attempts?

@paddybyers no, right now spec says that on NotActive you can have old registration / token update attempts (we clear all remnants only when we get Derigistered event during on WaitForDeregistration state). That's why we try to restore the state on CalledActivate event, but we are ignoring CalledDeactivate and that's the problem.

paddybyers commented 5 months ago

So stale/invalid deviceId etc can exist in the NotActive state, and would only get deleted if there was an explicit request to deregister?

That sounds wrong to me. The whole point of having a state machine is that the state characterises everything to do with what future events should be. Do you think deleting unused device details should happen on any transition to NotActive?

maratal commented 5 months ago

I can't find the code path leading to NotActivated while having saved token details. In ably-cocoa you'll end up in ARTPushActivationStateNotActivated in these four cases:

WaitingForDeviceRegistration and failed (you don't have saved token yet)
WaitingForPushDeviceDetails and failed (you don't have saved token yet)
WaitingForPushDeviceDetails and called deactivate (you don't have saved token yet)
WaitingForDeregistration and device is reset before going to NotActivated.

Should be similar on Android I guess? @ttypic

ttypic commented 5 months ago

@maratal

You can be in WaitingForRegistrationSync or WaitingForDeregistration states and haven't received next event (network issues, you closed the app, you closed ably client, you put app to the background, etc). After you reopen the app you'll have deviceIdentityToken and NotActive state.

Also if you take a look at spec and how NotActive should handle CalledActivate, you'll see that we can have valid (or invalid) deviceIdentityToken and other device details on NotActive state.

@paddybyers

So stale/invalid deviceId etc can exist in the NotActive state, and would only get deleted if there was an explicit request to deregister?

It's very important to understand that the NotActive state can have valid deviceId, deviceIdentityToken. And push notifications can work fine.
Sdk user can't explicit request to deregister. They can only send CalledDeactivate, CalledActivate events by calling ably.push.activate()/ably.push.deactivate().
Right now we are ignoring CalledDeactivate on NotActive state.
If user logged out, then logged in with different clientId, called deactivate() and then activate() they will receive clientId mismatch error. (and still be subscribed to push notifications from another account).

Do you think deleting unused device details should happen on any transition to NotActive?

I don't think so, because NotActive can have valid device details and we can simply delete it from local storage.

maratal commented 5 months ago

You can be in WaitingForRegistrationSync or WaitingForDeregistration

But in these two cases the previous state is not NotActivated, right? (at least in ably-cocoa). @ttypic

ttypic commented 5 months ago

@maratal Yes, but the previous state doesn't matter. We were discussing path leading to NotActivated while having saved deviceIdentityToken. And these are the paths, you can look at https://sdk.ably.com/builds/ably/specification/main/features/#RSH3a2a - this spec point specifically mention this exact situation.

owenpearson commented 5 months ago

Having thought about this PR a bit more broadly, I do agree with @paddybyers that having any device push state while in the NotActive state is wrong, and that this PR addresses that by allowing that state to be deleted rather than preventing an invalid state in the first place. Although, I suppose even if we made a change to prevent the machine from having state while NotActive, there would still be clients which got into that invalid state from an older SDK version, so we do at least need something in place to exit that invalid state, I'm not really sure what the best way to do that would be though.

ably / specification