Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add JWT tests #511

Merged
merged 19 commits into from
Jun 21, 2018
Merged

Add JWT tests #511

merged 19 commits into from
Jun 21, 2018

Conversation

funkyboy
Copy link
Contributor

@funkyboy funkyboy commented Jun 5, 2018 •
edited
Loading

Fix #505

  • Add tests that use ClientOptions
  • Add tests that use authURL
  • Add tests that use authCallback
  • Add tests with JWT wrapping an Ably native token x-ably-token
  • Add tests with encrypted token
  • Add example when token is returned with application/jwt Content-Type
  • Add tests with JWT that includes client_id
  • Add tests with JWT that includes publish/subscribe capabilities
  • Add tests for automatic reauth with and without disconnection
  • Add refs to spec items
  • Change URL of auth_url when echo server is deployed. See TODOs

@funkyboy funkyboy changed the title Add jwt tests Add JWT tests Jun 5, 2018
@funkyboy
Copy link
Contributor Author

funkyboy commented Jun 5, 2018

So far JWT tests are not failing: https://travis-ci.org/ably/ably-js/builds/388146773

@funkyboy
Copy link
Contributor Author

funkyboy commented Jun 6, 2018

Note: rebased on master now, to include #510

paddybyers
paddybyers requested changes Jun 8, 2018
View reviewed changes
Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

Also, I don't see a test that constructs a Rest or Realtime instance with just a string, and verifies RSC1c - ie a JWT passed to the constructor is determined to be a token.

spec/realtime/auth.test.js Outdated
@@ -9,6 +9,7 @@ define(['ably', 'shared_helper', 'async'], function(Ably, helper, async) {
monitorConnection = helper.monitorConnection,
testOnAllTransports = helper.testOnAllTransports,
mixin = helper.Utils.mixin,
jwtTestChannelName = 'JWT_test' + String(Math.floor(Math.random() * 10000) + 1),
echoServer = "http://echo.ably.io";
//echoServer = "http://localhost:5000";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 46011b6

spec/realtime/auth.test.js Outdated
* Request a JWT token that is about to be renewed, check that the client reauths
* without going through a disconnected state.
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 615b142

spec/rest/auth.test.js Outdated
/* RSA8g
* Tests JWT with authCallback
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 615b142

spec/rest/auth.test.js Outdated
});
};

/* RSA8g
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 615b142

spec/realtime/auth.test.js Outdated
var authUrl = echoServer + '/createJWT' + utils.toQueryString(keys);
var rest = helper.AblyRest({authUrl: authUrl});
var clientId = 'testJWTClientId';
var authCallback = function(tokenParams, callback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I don't think you should be creating an AblyRest instance just for the purpose of calling the authURL. Use http directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 805f12e

spec/realtime/auth.test.js Outdated
var rest = helper.AblyRest({authUrl: authUrl});
var clientId = 'testJWTClientId';
var authCallback = function(tokenParams, callback) {
rest.auth.requestToken({clientId: clientId}, function(err, tokenDetails) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This callback is using the rest instance to create an token, but there's nothing that verifies that you actually obtain a JWT. All of the tested behaviours - allowing/disallowing operations, disconnections, etc - would still occur with a native Ably token. Perhaps there should be a single helper getJWT() function which is used by all the tests, and this can call the echoserver directly using http (ie not via an Ably instance and requestToken()) and then you know that you're getting the token from the server.

Then you should have separate tests that explicitly test setting the authURL to the echoserver, and test with the different content types (ie text/plain vs application/jwt).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 4638d3e

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, there was already a test for application/jwt
I added another one f817d81

spec/realtime/auth.test.js Outdated
var keys = {keyName: currentKey.keyName, keySecret: currentKey.keySecret};
var authUrl = echoServer + '/createJWT' + utils.toQueryString(keys);
var rest = helper.AblyRest({authUrl: authUrl});
var authCallback = function(tokenParams, callback) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has the same issue as the test above, and also the tests that follow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 4638d3e

spec/rest/auth.test.js Outdated
/* RSC1, RSC1a, RSC1c, RSA4f, RSA8c, RSA3d
* Tests the different combinations of authParams declared above, with valid keys
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 615b142

@funkyboy
Copy link
Contributor Author

@paddybyers addressed all your comments above

paddybyers
paddybyers approved these changes Jun 14, 2018
View reviewed changes
Copy link
Member

@paddybyers paddybyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@funkyboy funkyboy merged commit 448d2f8 into master Jun 21, 2018
@funkyboy funkyboy deleted the add-JWT-tests branch June 21, 2018 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paddybyers paddybyers paddybyers approved these changes

@SimonWoolf SimonWoolf Awaiting requested review from SimonWoolf

@mattheworiordan mattheworiordan Awaiting requested review from mattheworiordan

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@funkyboy @paddybyers @withakay