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.

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

Add JWT tests #137

Merged
merged 35 commits into from Jun 13, 2018

Conversation

3 participants
@funkyboy
Copy link
Contributor

funkyboy commented Apr 18, 2018

Fix #136

  • Remove old test
  • Add tests that use authURL
  • Add tests that use ClientOptions
  • Add tests that use authCallback
  • Add tests for automatic renewal
  • Add tests with JWT wrapping an Ably native token x-ably-token
  • Add tests with encrypted token
  • Add tests with JWT that includes TokenDetails in x-ably-capability
  • Add example when token is returned with application/jwt Content-Type
  • Add refs to spec items
  • Change URL of auth_url when echo server is deployed. See TODOs

@funkyboy funkyboy requested review from mattheworiordan and SimonWoolf Apr 18, 2018

@mattheworiordan
Copy link
Member

mattheworiordan left a comment

This is good, but only tests one scenario i.e. using an explicitly provided token which is in fact not the way we recommend customers use JWT. Additionally, this only tests REST when we need to test Realtime & REST given REST uses headers, Realtime uses a query param. Finally, we need to test that renewing tokens automatically via JWT for Realtime (when it expires and reconnects automatically) and REST works.


context 'when using JWT' do
it 'client pulls non nil stats when authenticating with a JWT encoded token' do
require 'jwt'

This comment has been minimized.

@mattheworiordan

mattheworiordan Apr 19, 2018

Member

It's quite unusual to require this within a block. I know it works, but it's not best practice. Can we include all dependencies at the start of this file please?

This was referenced Apr 19, 2018

@funkyboy funkyboy changed the title Add jwt test Add JWT tests May 18, 2018

@funkyboy funkyboy force-pushed the add-jwt-test branch from 3d2b9d4 to 6236c2d May 21, 2018

@funkyboy

This comment has been minimized.

Copy link
Contributor

funkyboy commented May 21, 2018

Rebased on master now.

@funkyboy funkyboy changed the title Add JWT tests [WIP] Add JWT tests May 22, 2018

@funkyboy funkyboy force-pushed the add-jwt-test branch 2 times, most recently from de1353d to 1494d31 May 22, 2018

@funkyboy funkyboy force-pushed the add-jwt-test branch from 0eb78bb to 93f1382 May 22, 2018

@funkyboy funkyboy removed the request for review from SimonWoolf May 22, 2018

@funkyboy funkyboy force-pushed the add-jwt-test branch from c188b92 to d099cdc May 22, 2018

@funkyboy funkyboy requested a review from SimonWoolf May 28, 2018

@funkyboy

This comment has been minimized.

Copy link
Contributor

funkyboy commented May 30, 2018

@paddybyers

generally I would have had the rest tests cover all of the cases that apply to REST, and then in realtime do the cases that additionally apply (which are checking the clientId in the CONNECTED response, and exercising the in-protocol reauth mechanism;

I think it's probably too late for this PR, but I'll take this into account for other repos.

I don't know if I've missed it, but my reading of the auth-callback test here is that you're generating a regular Ably token, not a JWT;

The /createJWT only returns JWT tokens. Anyway I clarified/simplified some callbacks to make them more explicit: 1ef2b12 and c1a56af

I don't see any code change or test for the application/jwt content type in an authUrl response - in order to get the echoserver to respond with each possible Content-Type, is another parameter required to be supported?

Added a new TODO about this in this PR and the echo server

@funkyboy funkyboy force-pushed the add-jwt-test branch from 18b0d04 to c6577ad Jun 4, 2018

@funkyboy

This comment has been minimized.

Copy link
Contributor

funkyboy commented Jun 4, 2018

@paddybyers @mattheworiordan @SimonWoolf ready for another look

@paddybyers
Copy link
Member

paddybyers left a comment

LGTM, thanks

@funkyboy funkyboy force-pushed the add-jwt-test branch from 301c1bd to 1d03b75 Jun 11, 2018

@funkyboy

This comment has been minimized.

Copy link
Contributor

funkyboy commented Jun 12, 2018

@mattheworiordan If you don't have any further observations I'd merge this

@mattheworiordan

This comment has been minimized.

Copy link
Member

mattheworiordan commented Jun 13, 2018

Merge away

@funkyboy funkyboy merged commit 16845c2 into master Jun 13, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 98.537%
Details

@funkyboy funkyboy deleted the add-jwt-test branch Jun 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment