RSA4c #519

ricardopereira · 2016-10-18T10:06:21Z

No description provided.

ricardopereira · 2016-10-18T10:09:14Z

🚦 If the spec is well implemented then I will fix the client code.

mattheworiordan

Good apart from RSA4c3 which I don't think can land now unfortunately without RTC8a

mattheworiordan · 2016-10-18T11:01:21Z

Spec/Auth.swift

+                            }
+                            expect(errorInfo.code) == 80019
+
+                            expect(realtime.connection.state).to(equal(ARTRealtimeConnectionState.Connected))


I don't think this test can pass until http://docs.ably.io/client-lib-development-guide/features/#RTC8a is implemented. Currently the token would expire, Ably would disconnect the connection, and then the connection would become disconnected. Instead what you want is for the connection be made with a token that does not expire, you then inject an AUTH ProtocolMessage so the client thinks the server has requested it to authorise, it authorises fails and then just leaves the connection in the CONNECTED state waiting for the server to disconnect it eventually.

Done 6cff6e8.

mattheworiordan · 2016-10-18T11:02:06Z

Spec/Auth.swift

+                        }
+
+                        // RSA4c3
+                        it("if the connection is CONNECTED, then the connection should remain CONNECTED") {


As above, not sure this will work now, perhaps save for later as part of RTC8a

mattheworiordan · 2016-10-18T11:03:49Z

Spec/Auth.swift

+                            }
+
+                            guard let errorInfo = realtime.connection.errorReason else {
+                                fail("ErrorInfo is empty"); return


Might be nice to check for a reason that indicates a timeout here

Done ab3e321.

tcard · 2016-10-18T14:34:48Z

Spec/Auth.swift

+                                completion(nil, NSError(domain: NSURLErrorDomain, code: -1003, userInfo: [NSLocalizedDescriptionKey: "A server with the specified hostname could not be found."]))
+                            }
+
+                            expect(realtime.connection.errorReason).toEventuallyNot(beNil(), timeout: testTimeout)


As @mattheworiordan says above, this should happen because of an incoming AUTH and not because the token expires, because the token expiring, as per RTN15a, causes a transition to DISCONNECTED right away instead of staying CONNECTED.

tcard · 2016-10-18T14:39:29Z

Spec/Auth.swift

+                            realtime.options.authParams?.append(NSURLQueryItem(name: "type", value: "text"))
+                            realtime.options.authParams?.append(NSURLQueryItem(name: "body", value: invalidToken))
+
+                            expect(realtime.connection.errorReason).toEventuallyNot(beNil(), timeout: testTimeout)


Same here:

As @mattheworiordan says above, this should happen because of an incoming AUTH and not because the token expires, because the token expiring, as per RTN15a, causes a transition to DISCONNECTED right away instead of staying CONNECTED.

tcard · 2016-10-18T14:40:38Z

Spec/Auth.swift

+                        it("if the connection is CONNECTING, then the connection attempt should be treated as unsuccessful") {
+                            let previousRealtimeRequestTimeout = ARTDefault.realtimeRequestTimeout()
+                            defer { ARTDefault.setRealtimeRequestTimeout(previousRealtimeRequestTimeout) }
+                            ARTDefault.setRealtimeRequestTimeout(0.5)


Why 0.5? It could even be 0.0.

tcard · 2016-10-18T14:41:13Z

Spec/Auth.swift

+                                // Ignore `completion` closure to force a time out
+                            }
+
+                            expect(realtime.connection.errorReason).toEventuallyNot(beNil(), timeout: testTimeout)


Same here:

As @mattheworiordan says above, this should happen because of an incoming AUTH and not because the token expires, because the token expiring, as per RTN15a, causes a transition to DISCONNECTED right away instead of staying CONNECTED.

mattheworiordan · 2016-12-02T16:06:53Z

@ricardopereira this looks like there are still outstanding tasks?

ricardopereira · 2016-12-03T00:38:32Z

I need to rebase this because it's missing the RTC8a.

…then authorise

…ction should remain CONNECTED - RSA4c3

ricardopereira · 2016-12-08T01:21:43Z

Finally passing 🙌
@mattheworiordan @tcard Please have a look.

tcard · 2016-12-08T12:09:53Z

Source/ARTRealtime.m

@@ -268,11 +282,12 @@ - (void)transition:(ARTRealtimeConnectionState)state withErrorInfo:(ARTErrorInfo

 - (void)transitionSideEffects:(ARTConnectionStateChange *)stateChange {
    ARTStatus *status = nil;
+    __weak __typeof(self) weakSelf = self;


This kind of things could really use an explanatory comment. I have no idea why this change. I mean, I assume you don't want to increase the reference count in all those callback blocks but I'm not sure if there's some subtle reason for that.

Yes, you're right, I don't want to increase the ref count on those blocks because, for example, the unlessStateChangesBefore is setting a timer and if the ARTRealtime instance is released before that timer, then it could create a retain cycle and then we have a leak. That could also bring some instability in the test suite because we are creating a lot of ARTRealtime instances. I will add a comment about it. Thanks.

Done 6939c90.

tcard · 2016-12-08T12:16:30Z

Source/ARTRealtime.m

+            [self transportReconnectWithRenewedToken];
+            break;
+        default:
+            NSAssert(false, @"Invalid Realtime state: expected Connecting or Connected");


I would reserve those assertions for bugs on the library. If e. g. the server sends an AUTH while the connection is closing, that would be a bug in the server, and we probably should treat those more gracefully than crashing, e. g. log and ignore.

Done b6d6f92.

tcard · 2016-12-08T12:24:36Z

Source/ARTRealtime.m

+        }];
+    }
+    @finally {
+        self.auth.delegate = self;


Also no idea why this.

ARTAuth doesn't know about ARTRealtime so it has a delegate (ARTAuthDelegate). ARTRealtime is using that delegate but in that case it should not use the delegate functionalities.

Actually, your comment above is relevant about this case.

Done efa57ab.

tcard · 2016-12-08T12:25:19Z

Source/ARTRealtime.m

    if (message.channel) {
        [self onChannelMessage:message];
    } else {
        ARTErrorInfo *error = message.error;
        if ([self shouldRenewToken:&error]) {
+            [self.transport close];


Then it's closed again at 621?

I can't precise the problem that was occurring if the close wasn't called explicitly but the main reason I added the close call was because the authorize from transportReconnectWithRenewedToken takes time and the client shouldn't use the current token.

tcard · 2016-12-08T12:29:56Z

Source/ARTRealtime.m

+    [_transport connectWithToken:tokenDetails.token error:error];
+}
+
+- (void)transportReconnectWithRenewedToken {


Unless I'm missing something, all this logic will only apply when transportReconnectWithRenewedToken is called, ie. when an AUTH or ERROR is received, but not when e. g. first connection or manually calling authorize. The spec says "If an attempt by the realtime client library to authenticate is made using...", so every auth attempt should follow this logic. I'd say this logic should be inside Auth.authorize itself, don't you think?

Good point 👍

@ricardopereira have you made the change on this?

@mattheworiordan No, working on it right now.

Done efa57ab.

That one broke some tests, unfortunately...

😔 Yes, I'm on it. Thanks

Fixed 3462531.

tcard · 2016-12-08T12:36:42Z

Spec/RealtimeClientConnection.swift

@@ -3583,6 +3623,51 @@ class RealtimeClientConnection: QuickSpec {
                    }
                }
            }
+
+            it("should abort reconnection with new token if the server has requested it to authorise and after it the connection has been closed") {


Which spec lines is this testing?

None. That was something I notice on the test suite where the client gets connected when it's waiting for the authorize and the client is explicitly closed. It should ignore it and remain closed.

…estTimeout

…eAuth, then authorise

mattheworiordan

LGTM other than @tcard's comments

ricardopereira · 2016-12-08T18:27:54Z

The authorize should be called before the transport connects because of the (RSA4c3) If the connection is CONNECTED, then the connection should remain CONNECTED. So, I removed the ARTAuth dependency from the ARTWebSocketTransport and for now on the ARTRealtime manages the authentication.

@mattheworiordan @tcard Can I squash and merge?

mattheworiordan · 2016-12-09T16:25:01Z

Seeing as though it's passing LGTM

tcard

All this is surely becoming ripe for a good refactor some day but LGTM. Thanks!

@synchronized

* Start 0.9 version * TO3k7 (#513) * Start 0.9 version * Add fallbackHostsUseDefault option * Fallback: add initialiser accepting client options * TO3k7 * Add ARTFallback+Private * Default fallbackHosts as Array of Strings * fixup! TO3k7 * fixup! Add fallbackHostsUseDefault option * fixup! TO3k7 * Update RSC15a for 0.9 (#515) * Start 0.9 version * Update RSC15a: default fallback hosts * Default fallbackHosts as Array of Strings * RSA4a (#517) * Test suite: add TestProxyHTTPExecutor.simulateIncomingServerErrorOnNextRequest * RSA4a * Fix Realtime: indicate an error and not retry the request when the server responds with a token error * Update RSC15e (#514) * Remove specs RSA10c and RSA10d (#522) * RSA10l (#524) * Auth: deprecate `authorise` in favor of `authorize`. * RSA10l * Use `authorize` instead of `authorise` (close #496) * Remove `AuthOptions.force` (#527) * Remove AuthOptions.force * Fix: make a single attempt to reissue the token and resend the request * Fix RSA10a * Fix RSC9 * Remove `prepareAuthorisationHeader` access from test suite * Fix RSA10a * Update RSC15b for 0.9 (#516) * Update RSC15b * Fix: REST fallback should only apply when the default host is used * Update RSA10a for 0.9 (#520) * Update RSA10a * Fix: authorize should change auth method to Token for future requests * Update RSA10j for 0.9 (#521) * Update RSA10j * Fix: ttl when omitted should set the default value * Fix RTN15h * RTC8 (#526) * Add ARTAuthDetails * RTC8a * RTC8a1 (part 1) * RTC8a1 (part 2) * RTC8a1 (part 3) * RTC8a2 * RTC8a3 * RTC8b * RTC8b1 * RTC8c * Send AUTH protocol message on each authorize * Fix RTC8 * Test suite: `splitDone`, when a test fails, get the right location of the failure * RSA4b (#518) * RSA4b * Fix REST: should retry the request once if a token error occurs * Fix Realtime: if the token creation failed then the connection should move to the DISCONNECTED state and reports the error * RSA4c (#519) * RSA4c * Fix: 80019 and description of the underlying failure should be emitted * Fix: if connected and the client receives an ARTProtocolMessageAuth, then authorise * Fix: if authUrl or authCallback fails and is CONNECTED then the connection should remain CONNECTED - RSA4c3 * Add `artDispatchScheduled` and a way to cancel the scheduled block * Fix: authUrl/authCallback attempt times out after realtimeRequestTimeout * Test suite: reset networkConnectEvent after spec * Enhance: debug info * fixup! Fix: authUrl/authCallback attempt times out after realtimeRequestTimeout * fixup! Fix: if connected and the client receives an ARTProtocolMessageAuth, then authorise * Fix: every realtime auth attempt should check if deadline is reached * Rename createWithNSError to createFromNSError * Fix: remove Auth dependency from WebSocketTransport * Fix: should first authorize and then connect the transport * RTN22 (#537) * RTN22 * RTN22a * Fix: realtime transport can be nil * Fix: realtime should renew token by transitioning to CONNECTING * Update RTL3 for 0.9 (#544) * Update RTL3a * Update RTL3b and RTL3c * RTL3d * RTL3e * RTL14 (#550) * UPDATE event (#559) * UPDATE event (replace ERROR event) * Fix: Connection should emit an UPDATE event * Fix RTC8a1 * Fix specs and legacy tests * RTN4h * RTN4f * RTN24 * Update RTL2 for 0.9 (#543) * Add Realtime Channel Suspended state * Add ProtocolMessageActionToStr method * Add ChannelStateChange type * Update RTL2 * RTL2f: pending - functionality hasn't been deployed * Use ChannelStateChange on channel event emitter * Test suite: simulate client suspension with before suspension callback * Update tests using channel events * Fix RTL14 * Fix: Channel on suspended should transition to SUSPENDED state * Remove RTN18 * Fix: set Suspended on all channels when Connection moves to Suspended * RTL2g * Fix RTL12 * Fix RTL3d * Fix: channel should reattach when connection is Connected * Fix: should resume connection when the connection is Suspended * Fix RTN11 * Fix RTL3e * Fix RTC8a1 * Remove testSuspendingDetachesChannel * Fix RTL3d * Fix: channel is SUSPENDED then operation will result in an error * Update RTL13 for 0.9 (#549) * RTL13a * RTL13b * RTL13c * Add ClientOptions.channelRetryTimeout * RealtimeChannel reattach after timeout * Fix: if the channel receives a server initiated DETACHED message and if the channel is in the ATTACHED or SUSPENDED states, then an attempt to reattach the channel should be made immediately * Fix: move to Suspended if attach times out * Fix: if the channel receives a server initiated DETACHED message and the channel is Attaching * Remove test about #454 (replaced by RTL13) * Update RTL4f * Update RTL4 for 0.9 (#545) * RTL4i * RTL4h * Update RTL4e * Fix: attach request should be treated as though it has failed and the channel should transition to the SUSPENDED state * Fix: attach after Detaching * Fix: if it fails to detach then move back to ATTACHED * Fix RTL5f * Update RTL6c for 0.9 (#547) * Update RTL6c2 * Update RTL6c4 * RTL6c3 for Detached * Update RTL6c4 for channel Failed * Better code completion * Add ChannelStateChange.event property (#561) * Update RTN4 for 0.9 (#560) * Update RTN4e * Update RTN4f * Add ConnectionStateChange.event property * Update RTL5 for 0.9 (#546) * RTL5j * Update RTL5a * RTL5i * Fix: if channel is SUSPENDED then the detach request transitions immediately to DETACHED state * Fix: if channel is ATTACHING then do the detach operation after the completion of the attaching * Remove testSkipsFromAttachingToDetaching * Fix: presence sync can fail * Fix RTP11b * Fix RTP9e * RTP5: pending * Fix RTL6c3 * Fix: should check channel state when a queued message is processed * TR4i (#562) * ARTProtocolMessageFlag enum: use NS_OPTIONS to define a bitmask * TR4i * Fix: should indicate that the channel has been resumed or not (RESUMED flag) * Swift performance: speed up code completion (#569) * Speed up code completion - This problem is fixed on Xcode 8 but since we are still using Xcode 7 I decided to change that code. * Test suite: addMembersSequentiallyToChannel should return the realtime client * Test suite: set AsyncDefaults.Timeout with default value * Remove pending tests (#542) * Fix: RTP6c pending test * RSL1g4: remove pending * RTP2: remove pending * Fix RTC8 * Update RTP3 (#566) * Update RTP8d (#572) * RTP17 (#571) * PresenceMap: list of internal members * RTP17 * Add ARTPresenceActionToStr method * RTP17: pending * Update RTP2 for 0.9 (#563) * RTP2: remove pending * RTP2a * RTP2b * Test suite: set Nimble.AsyncDefaults.Timeout - increase the default timeout value from all async expectations * RTP2c * RTP2d * RTP2e * RTP2f * RTP2g * Test suite: ARTPresenceMessage convenience initializer * Test suite: NSDate custom operators - convenience for use of `dateByAddingTimeInterval` * Fix RTP2b1 * Fix RTP2b2 * PresenceMap: compare for newness * Fix RTP2 * Remove warnings * RTP18 (#567) * Test suite: ARTPresenceMessage convenience initializer * RTP18 * RTP18a * RTP18b * RTP18c * Update RSA9h for 0.9 (#574) * Auth: optional arguments * Update RSA9h * Update RSA8e for 0.9 (#573) * Update RSA8e * Auth: optional arguments * Comments * Update RSA10g for 0.9 (#575) * Auth: optional arguments * Update RSA10g * Auth: subsequent authorizations with stored values * Update RTP5 for 0.9 (#570) * Update RTP5 * Update RTP5a * Update RTP5b * RTP5c * Test suite: replaceAcksWithNacks - better code completion * RTP5f * PresenceMap: existing members before Sync * Fix: presence get members should not wait for sync if sync is not in progress * PresenceMap: reenter local member * Test suite: replacing acks with nacks even with Presence action * Fix: should queue messages before the attach operation * Test suite: ARTPresenceAction description * Better debug info * Fix: should continue incrementing msgSerial serially if the connection resumes successfully * Remove warnings * Test suite: timings * RTP19 (#568) * RTP19 * RTP19a * Fix: if channel resumed successfully then do not start sync * Enhance RTP5: presence get should not have Absent members * Fix race condition * Fix #583: update httpRequestTimeout and httpMaxRetryDuration * EventEmitter: use @synchronized because NSMutableArray are not thread safe * Thread safety (#586) * New EventEmitter (using NSNotificationCenter) - In most ways that matter NSNotificationCenter is thread safe. You can add/remove observers from any thread and you can post notifications from any thread. * Events for the EventEmitter * Fix: should cancel timers when connection times out * Fix: new state change can occur before receiving publishing acknowledgement * Test suite: async forced transitions * Test suite: ack order * Test suite: stop when there's no internet * Fix: instance objects released to soon * Performed a static analysis from Xcode * fixup! Test suite: ack order * Memory leak: call session invalidate to dispose of its strong reference to the delegate * fixup! Test suite: ack order * Fix RTN19a: guarantee of a new transport (check transport reference) * Fix: ACK or NACK has not yet been received for a message, the client should consider the delivery of those messages as failed * Enhance RTN14b: better timings * Fix: REST and Realtime, wait for last operation to release the object * fixup! Test suite: ack order * Fix: cancel timers when a connection gets closed * fixup! Test suite: ack order * Test suite: timings * fixup! Enhance RTN14b: better timings * Test suite: close connections * Fix: turn off immediately reachability when close occurs * Fix RTC1d: wait for host is not reachable error * fixup! Test suite: ack order * Travis update * Fix RTN19a

mattheworiordan requested changes Oct 18, 2016

View reviewed changes

tcard suggested changes Oct 18, 2016

View reviewed changes

ricardopereira force-pushed the 0-9-master branch from 0857337 to 5728e01 Compare November 30, 2016 12:03

ricardopereira force-pushed the rsa4c branch from 4a8e703 to e51a6ba Compare December 5, 2016 20:17

ricardopereira added 4 commits December 6, 2016 12:55

RSA4c

e536b3d

Fix: 80019 and description of the underlying failure should be emitted

8d98976

Fix: if connected and the client receives an ARTProtocolMessageAuth, …

ac032fe

…then authorise

Fix: if authUrl or authCallback fails and is CONNECTED then the conne…

3f17d0f

…ction should remain CONNECTED - RSA4c3

ricardopereira force-pushed the rsa4c branch 5 times, most recently from 7ad951c to 6185486 Compare December 7, 2016 19:01

Add artDispatchScheduled and a way to cancel the scheduled block

2c704bc

ricardopereira force-pushed the rsa4c branch from 6185486 to b202c9d Compare December 7, 2016 22:47

ricardopereira added 3 commits December 8, 2016 00:01

Fix: authUrl/authCallback attempt times out after realtimeRequestTimeout

6c4391b

Enhance: debug info

ea0e5e9

Test suite: reset networkConnectEvent after spec

0c86eb8

ricardopereira force-pushed the rsa4c branch from b202c9d to 0c86eb8 Compare December 8, 2016 00:01

tcard suggested changes Dec 8, 2016

View reviewed changes

ricardopereira added 2 commits December 8, 2016 14:34

fixup! Fix: authUrl/authCallback attempt times out after realtimeRequ…

6939c90

…estTimeout

fixup! Fix: if connected and the client receives an ARTProtocolMessag…

b6d6f92

…eAuth, then authorise

mattheworiordan approved these changes Dec 8, 2016

View reviewed changes

ricardopereira added 3 commits December 8, 2016 15:53

Fix: every realtime auth attempt should check if deadline is reached

efa57ab

Rename createWithNSError to createFromNSError

8c81364

Fix: remove Auth dependency from WebSocketTransport

7021e1f

Fix: should first authorize and then connect the transport

3462531

tcard approved these changes Dec 12, 2016

View reviewed changes

ricardopereira merged commit 67bbcce into 0-9-master Dec 12, 2016

ricardopereira deleted the rsa4c branch December 12, 2016 09:48

RSA4c #519

RSA4c #519

Conversation

ricardopereira commented Oct 18, 2016

ricardopereira commented Oct 18, 2016

mattheworiordan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan commented Dec 2, 2016

ricardopereira commented Dec 3, 2016

ricardopereira commented Dec 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattheworiordan left a comment

Choose a reason for hiding this comment

ricardopereira commented Dec 8, 2016

mattheworiordan commented Dec 9, 2016

tcard left a comment

Choose a reason for hiding this comment