Push Activation State Machine: validate an already-registered device on activation #543

paddybyers · 2020-01-03T08:38:36Z

android/src/main/java/io/ably/lib/push/ActivationStateMachine.java

tcard · 2020-01-04T22:31:51Z

lib/src/main/java/io/ably/lib/rest/Auth.java

@@ -1017,6 +1017,11 @@ private static final String hmac(String text, String key) {
 	 * @throws AblyException
 	 */
 	public void setClientId(String clientId) throws AblyException {
+		if(clientId == null) {
+			/* do nothing - we received a token without a clientId */


If this is possible, shouldn't it be in the spec and thus have an associated test?

android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java

paddybyers · 2020-01-04T23:11:56Z

@tcard see also e70483a.

This implements ably/docs@1b775f3

(Tests to follow.)

tcard · 2020-01-19T09:05:41Z

I just want to link to ably/docs#786 for cross-reference in the GitHub UI.

The base branch was changed.

tcard · 2020-02-09T11:25:12Z

I've extracted #553 from this and rebased the rest on top. Once #553 is merged, base should be changed back to master.

tcard · 2020-02-24T15:48:07Z

To-do:

Test changes to RSH3a2a.
Test changes to RSH8*. (Except RSH8i)
Spec changes derived from 257a6c1 (Push: Fix activate() behavior while syncing device docs#824).
Decide what to do with RSH8i.

tcard · 2020-02-26T11:11:06Z

While talking to @QuintinWillison I noticed that now that we've renamed AfterRegistrationUpdateFailed to AfterRegistrationSyncFailed, if clients update were to update the library where the previous persistent state was AfterRegistrationUpdateFailed, loading the state from persistence would fail, as no such state exists now. We need to make sure we upgrade deprecated states to their new counterparts.

tcard · 2020-02-26T16:08:41Z

tcard · 2020-02-27T13:24:35Z

@paddybyers PTAL

This should be easier to review on a per-commit basis.

…on activation

Co-Authored-By: Toni Cárdenas <toni@tcardenas.me>

Conform to the spec as to which callbacks should be called from WaitingForRegistrationSync.

This isn't in the spec (yet). The previous behavior causes the activation callback to be called right away with no error, while a previous activation is ongoing. The correct behavior is to serialize the calls, so that the second call is processed after the first has finished. At the same time, remove the CalledActivate enqueue from NotActivated, which relied on the old behavior but wasn't doing anything really useful. (This needs to be removed from the spec too.)

Realtime seems to expect it, even if we set the device token header.

Also, fix a test that relied on broken behavior.

So that we can mock it in tests. OnCompleteListeners can only be fired by Android. So now, we need to brigde Android's native OnCompleteListener to our Callback.

It was defined but unused.

This provides more flexibility for further tests, and supersedes the need for TestActivationStateMachine.ignoreRegistrationTokenRequest.

As per RSH8a.

Otherwise, we're resetting it each time, which then breaks client ID updates detection.

For a broken test, take blocks indefinitely.

Will be needed to test RSH8i.

paddybyers

I can't approve this because I was the original author, but LGTM. Thanks @tcard , good work. Rescued my half-baked implementation :)

tcard

Thanks @paddybyers. Approving on your behalf so that this thing lets me merge.

QuintinWillison · 2020-05-15T08:38:46Z

🥳

paddybyers requested a review from tcard January 3, 2020 09:10

paddybyers force-pushed the push-local-device-validate branch from 41392bc to 8f9dfb3 Compare January 3, 2020 10:24

tcard approved these changes Jan 4, 2020

View reviewed changes

tcard previously approved these changes Jan 7, 2020

View reviewed changes

QuintinWillison changed the base branch from develop to master February 7, 2020 16:25

tcard self-assigned this Feb 9, 2020

tcard force-pushed the push-local-device-validate branch from 9e339d7 to af708fc Compare February 9, 2020 08:17

tcard mentioned this pull request Feb 9, 2020

Get AndroidPushTest to pass again #553

Merged

tcard force-pushed the push-local-device-validate branch from af708fc to dac720c Compare February 9, 2020 11:22

tcard changed the base branch from master to push-random-fixes February 9, 2020 11:22

tcard changed the base branch from push-random-fixes to master February 10, 2020 07:00

tcard mentioned this pull request Feb 23, 2020

Push Activation State Machine: add requirement for registration validation ably/docs#786

Merged

paddybyers and others added 10 commits March 9, 2020 16:57

Push Activation State Machine: validate an already-registered device …

3e44d09

…on activation

AndroidPushTest: multiple test fixes to reflect latest spec updates

16fbee9

Push: implement LocalDevice.clientId immutability

0327154

Push activation validation: reinstate missing test assertion

ff30a74

Co-Authored-By: Toni Cárdenas <toni@tcardenas.me>

Pass missing JSON response handler to PUT /push/deviceRegistrations/:id.

3115694

RSH3e2/RSH3e3: From CalledActivate, call activate callback.

954327e

Conform to the spec as to which callbacks should be called from WaitingForRegistrationSync.

Use Ably auth for PUT /push/deviceRegistrations/:id.

fff2512

Realtime seems to expect it, even if we set the device token header.

Ensure we serialize handling of push activation events.

9468308

Also, fix a test that relied on broken behavior.

Test new RSH3a2a flows (client ID validation + device sync).

5576cb5

tcard added 21 commits March 9, 2020 16:57

Add RSH8g reference alongside RSH2c.

cda7993

Make ActivationContext take a Callback instead of an OnCompleteListener.

5e3c598

So that we can mock it in tests. OnCompleteListeners can only be fired by Android. So now, we need to brigde Android's native OnCompleteListener to our Callback.

Use TestActivationContext class in activation tests.

8e49910

It was defined but unused.

Mock registration token requests at the ApplicationContext level.

4e254bc

This provides more flexibility for further tests, and supersedes the need for TestActivationStateMachine.ignoreRegistrationTokenRequest.

RSH2c: Mock at the ActivationContext layer; test two events.

53d59a8

Add test for RSH2D / RSH8H.

aff3884

Add test for RSH8b.

09d89ac

Add test for RSH8c.

4ff85ad

Populate LocalDevice.clientId from persistence even without device ID.

b267934

As per RSH8a.

Memoize ActivationContext.getActivationContext().

65f8b56

Otherwise, we're resetting it each time, which then breaks client ID updates detection.

Add test for RSH8a, merge with RSH8c.

9190b80

Add test for RSH8d.

6a2b16a

Add test for RSH8e.

3e8ac8e

Replace all BlockingQueue.take uses by a poll of 10 seconds.

851f2ae

For a broken test, take blocks indefinitely.

Add test for RSH8f.

155a05f

Migrate persisted AfterRegistrationUpdateFailed to ...SyncFailed.

8914b2e

Repopulate instance on ActivationStateMachine.reset.

ea48b40

Wait for PUSH_REGISTER_DEVICE before checking device ID.

de2c272

Make TestActivation more flexible.

ca53aec

Will be needed to test RSH8i.

Implement RSH8i: on setAndroidContext, verify registration token.

c706c0e

Use assertInstanceOf for better test failure feedback.

5f65aa4

QuintinWillison force-pushed the push-local-device-validate branch from e8b7108 to 5f65aa4 Compare March 9, 2020 16:57

QuintinWillison assigned paddybyers and unassigned tcard Mar 9, 2020

paddybyers commented May 1, 2020

View reviewed changes

tcard approved these changes May 7, 2020

View reviewed changes

tcard merged commit 1399a98 into master May 7, 2020

tcard deleted the push-local-device-validate branch May 7, 2020 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Push Activation State Machine: validate an already-registered device on activation #543

Push Activation State Machine: validate an already-registered device on activation #543

paddybyers commented Jan 3, 2020 •

edited by tcard

Loading

tcard Jan 4, 2020

paddybyers commented Jan 4, 2020

tcard commented Jan 19, 2020

tcard commented Feb 9, 2020

tcard commented Feb 24, 2020 •

edited

Loading

tcard commented Feb 26, 2020

tcard commented Feb 26, 2020

tcard commented Feb 27, 2020 •

edited

Loading

paddybyers left a comment

tcard left a comment

QuintinWillison commented May 15, 2020

Push Activation State Machine: validate an already-registered device on activation #543

Push Activation State Machine: validate an already-registered device on activation #543

Conversation

paddybyers commented Jan 3, 2020 • edited by tcard Loading

tcard Jan 4, 2020

Choose a reason for hiding this comment

paddybyers commented Jan 4, 2020

tcard commented Jan 19, 2020

tcard commented Feb 9, 2020

tcard commented Feb 24, 2020 • edited Loading

tcard commented Feb 26, 2020

tcard commented Feb 26, 2020

tcard commented Feb 27, 2020 • edited Loading

paddybyers left a comment

Choose a reason for hiding this comment

tcard left a comment

Choose a reason for hiding this comment

QuintinWillison commented May 15, 2020

paddybyers commented Jan 3, 2020 •

edited by tcard

Loading

tcard commented Feb 24, 2020 •

edited

Loading

tcard commented Feb 27, 2020 •

edited

Loading