Add jwt tests #714

funkyboy · 2018-04-16T13:58:16Z

Fix ably/docs#713

… content type

funkyboy · 2018-06-04T07:10:39Z

Rebased on master.

mattheworiordan

Few small comments, but mostly looks good to me 👍

mattheworiordan · 2018-06-04T15:18:22Z

Spec/Auth.swift

@@ -3619,5 +3619,7 @@ class Auth : QuickSpec {
                }
            }
        }
+            }


What is going on here with the two extra closing squigglies?

mattheworiordan · 2018-06-04T15:19:06Z

Spec/TestUtilities.swift

@@ -43,6 +43,7 @@ let appSetupJson = JSON(parseJSON: try! String(contentsOfFile: pathForTestResour

 let testTimeout: TimeInterval = 10.0
 let testResourcesPath = "ably-common/test-resources/"
+let echoServerAddress = "https://shrouded-plains-50367.herokuapp.com/createJWT" // TODO: change this


You can just deploy your changes to the Heroku echoserver, even if not fully merged, to avoid having to go back and do this.

Just didn't want to disrupt existing stuff that currently depends on the echo server. Happy to change this when the echo server PR is merged.

But your changes aren’t breaking changes? You only added a new endpoint so I don’t see the risk personally

Only potential issue could be https://github.com/ably/echoserver/pull/4/commits/49a3a0f8af59a02a18f0c36451c958cfcf6224d0

@funkyboy why would that be an issue? I appreciate it's an upgrade, but you have run a full test suite against the echoServer running the new Node without any issues?

No. Will do that with the Ruby SDK. If everything passes can I deploy then to echo.ably.io ?

I ran the full test suite of Ruby against the echo server in staging. All green: https://travis-ci.org/ably/ably-ruby/builds/388329095

Will promote it to production if there are no objections.

mattheworiordan · 2018-06-04T15:19:29Z

Spec/TestUtilities.swift

@@ -494,6 +495,52 @@ func getTestTokenDetails(key: String? = nil, clientId: String? = nil, capability
    return tokenDetails
 }

+func encodeString(_ input: String) -> String? {
+    let allowedQueryParamAndKey =  CharacterSet(charactersIn: "{}[]*: ")
+   if let encodedString_ = input.addingPercentEncoding(withAllowedCharacters: allowedQueryParamAndKey) {


Indentation issues again

Actually it was a leftover :) 54c84de

mattheworiordan · 2018-06-04T15:20:21Z

Spec/TestUtilities.swift

+        keySecret = "invalid"
+    }
+
+    var URLCcomponents = URLComponents(string: echoServerAddress)


Typo I think, URLCcomponents?

Good catch, thanks. 903bc3e

mattheworiordan · 2018-06-04T15:21:22Z

Spec/Auth.swift

+
+                    it("pulls stats successfully") {
+                        waitUntil(timeout: testTimeout) { done in
+                            client.stats{ stats, error in


Missing whitespace after stats I believe?

Yes. See a2100f2

mattheworiordan · 2018-06-04T15:23:22Z

Spec/Auth.swift

+
+                    it("fetches a channels and posts a message") {
+                        client.channels.get(channelName).publish(messageName, data: nil, callback: { error in
+                            expect(error).to(beNil())


In the other tests test are wrapped in a waitUntil with a call to done(). How does the test suite know when this test is done?

Good observation. c7efe08

mattheworiordan · 2018-06-04T15:27:09Z

Spec/Auth.swift

+                    it ("receives a 40142 error from the server") {
+                        waitUntil(timeout: testTimeout) { done in
+                            client.connection.once(.connected) { stateChange in
+                                delay(tokenDuration + 1) {


Why do you need this delay? Should the connection once(.disconnected) fire below anyway once the token has expired? In fact, if waits tokenDuration, what's probably happening with this test is that it's being disconnected once here, and once again below.

Done 936955b

mattheworiordan · 2018-06-04T15:32:05Z

Spec/Auth.swift

+
+                    it("fails to connect") {
+                        waitUntil(timeout: testTimeout) { done in
+                            client.connection.once(.failed) { stateChange in


I thought we had agreed that https://docs.ably.io/client-lib-development-guide/features/#RSA4b stated that the connection should not become failed, but instead should become disconnected and retry. I believe @paddybyers updated the spec after chatting with you about this at some point, although oddly I can't find a matching commit.

Add this as a TODO above

@mattheworiordan RSA4b says that a REST request fails if a token error is returned after a retry, which is correct, and is unrelated to this test.

https://docs.ably.io/client-lib-development-guide/features/#RTN14b relates to token failures on connection creation - it says that a connection should not fail in that case, but become disconnected. This test is therefore wrong and, because it passes, there is also a bug in the library (which will have existed before these JWT changes). @funkyboy pls raise a separate issue for that (and find the other existing, non-JWT, invalid test).

The discussion you remembered related was https://github.com/ably/docs/issues/429, relating to RTN15h, and that's to do with failures to reconnect (ie not initial connection failures). I'm guessing that there's going to be an issue for iOS there as well. I will do a docs PR for that.

@paddybyers @mattheworiordan The non-JWT corresponding test is https://github.com/ably/ably-ios/blob/develop/Spec/Auth.swift#L305 and again it tests for FAILED.

Issue raised here: #730

I will do a docs PR for that.

ably/docs#439. @funkyboy can you check that behaviour please in this library and see if complies with the DISCONNECTED or FAILED behaviour?

@paddybyers This seems to test the new RTN15h1 that you wrote.
These all riff around the same "token expired/wrong" conditions but none seems to tests for the "single attempt to renew the token" clause.

Should I open a new issue about this?

cc @mattheworiordan

UPDATE: this seems close to test this https://github.com/ably/docs/pull/439/files#diff-11900b395df266bc2bbfc1d11bdfc7a0R390 but it's not checking for the connection state :(

@mattheworiordan RSA4b says that a REST request fails if a token error is returned after a retry, which is correct, and is unrelated to this test.

Oops, I should have scrolled up to see the context of the test I found. That explains why I could not find the record of the change we made in the spec!

@paddybyers

ably/docs#439. @funkyboy can you check that behaviour please in this library and see if complies with the DISCONNECTED or FAILED behaviour?

Opened issue here #731

mattheworiordan · 2018-06-04T15:32:55Z

Spec/Auth.swift

+                let clientId = "JWTClientId"
+                let options = AblyTests.clientOptions()
+                options.tokenDetails = ARTTokenDetails(token: getJWTToken(clientId: clientId)!)
+                options.autoConnect = false


More autoConnect false although not sure why.

done 4b5ada4

mattheworiordan · 2018-06-04T15:34:04Z

Spec/Auth.swift

+            }
+
+            // RSA4f, RSA8c
+            context("when the JWT token is returned with application/jwt content tye") {


Small typo, type not tye

See 7351d46#diff-b5d84fe9bce26902a7481c2ffec84fffR3911

funkyboy · 2018-06-07T10:16:08Z

@paddybyers @mattheworiordan @ricardopereira ready for another look

paddybyers

LGTM, thanks

funkyboy force-pushed the add-jwt-tests branch 2 times, most recently from 94e08ba to 560ae07 Compare May 30, 2018 20:26

funkyboy added 13 commits June 4, 2018 09:09

Update ref to ably-common

525b231

Fix tests for expired tokens

ca98832

Add utilities to get a JWT token from echo server

ed30e9e

Add tests for JWT and Realtime

e817604

Fix encrypted option for getJWTToken

044f3c6

Add JWT tests for rest and encrypted/embedded tokens

ed681b3

Use clientId instead of client_id

9de3703

Fix issue in passing errors along

5a6b0ee

Fix JWT renew test

53d4eb9

Add test to check reauth without disconnection using JWT

95b51a8

Add support for application/jwt responses from authUrl

13a78dc

Add test for JWT returned with application/jwt content type

5c36c8c

Add test to request a JWT token via requestToken with application/jwt…

4e37a96

… content type

funkyboy force-pushed the add-jwt-tests branch from bc75217 to 4e37a96 Compare June 4, 2018 07:10

Add refs to spec items

81fdbf1

funkyboy force-pushed the add-jwt-tests branch from c402b11 to 81fdbf1 Compare June 4, 2018 08:57

funkyboy requested review from mattheworiordan, ricardopereira and paddybyers June 4, 2018 15:04

mattheworiordan requested changes Jun 4, 2018

View reviewed changes

funkyboy added 7 commits June 5, 2018 09:44

Remove uneeded method

54c84de

Fix typo

903bc3e

Improve publish with JWT test

c7efe08

Improve token expiration test

936955b

Remove unneeded autoConnect setting

4b5ada4

Fix typos in contexts

7351d46

Remove autoConnect false when not needed

5ec0760

Add spaces around closures

a2100f2

This was referenced Jun 5, 2018

Investigate implementation of RTN14b #730

Closed

Check behavior of RTN15h* #731

Closed

funkyboy force-pushed the add-jwt-tests branch 2 times, most recently from 66aaad8 to ab572c4 Compare June 6, 2018 12:02

paddybyers approved these changes Jun 7, 2018

View reviewed changes

ricardopereira approved these changes Jun 7, 2018

View reviewed changes

Update URL of JWT server

221ec69

funkyboy force-pushed the add-jwt-tests branch from ab572c4 to 221ec69 Compare June 11, 2018 07:43

funkyboy merged commit 9634530 into develop Jun 18, 2018

funkyboy deleted the add-jwt-tests branch June 18, 2018 10:55

@@ @@ -3619,5 +3619,7 @@ class Auth : QuickSpec { @@
                               }
                           }
                       }
+                          }

Add jwt tests #714

Add jwt tests #714

Conversation

funkyboy commented Apr 16, 2018 • edited Loading

funkyboy commented Jun 4, 2018

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

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

paddybyers Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

funkyboy Jun 5, 2018 • edited Loading

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

funkyboy commented Jun 7, 2018

paddybyers left a comment

Choose a reason for hiding this comment

funkyboy commented Apr 16, 2018 •

edited

Loading

paddybyers Jun 5, 2018 •

edited

Loading

funkyboy Jun 5, 2018 •

edited

Loading