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.

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

authCallback bug, test coverage & additional type support #72

Closed
mattheworiordan opened this issue Jul 24, 2015 · 3 comments
Closed

authCallback bug, test coverage & additional type support #72

mattheworiordan opened this issue Jul 24, 2015 · 3 comments
Assignees
Labels
bug Something isn't working. It's clear that this does need to be fixed. enhancement New feature or improved functionality.

Comments

@mattheworiordan
Copy link
Member

authCallback currently has no test coverage in the library, we need to add some.

Additionally, we need to add support for the callback to return:

  • a token string
  • a TokenDetails object
  • a TokenRequest object

Based on the docs at

* - authCallback: (optional) a javascript callback to be used, passing a set of token
* request params, to get a signed token request.
, I assume only a TokenRequest is supported.

See http://www.rubydoc.info/gems/ably/0.8.2/Ably%2FAuth%3Ainitialize as an example (under auth_callback),.

In our spec as well, we specify When authCallback option is set, it will execute the callback and will expect either a token string, a TokenDetails object or a TokenRequest object which will in turn be used to request a token from Ably.

@mattheworiordan mattheworiordan added bug Something isn't working. It's clear that this does need to be fixed. enhancement New feature or improved functionality. labels Jul 24, 2015
@mattheworiordan
Copy link
Member Author

I also don't think authCallback works when providing it with a valid signed TokenRequest.

See https://jsbin.ably.io/anadag/3/edit. The error is:

Ably: Auth.requestToken(): token request signing call returned error; err = {"keyName":"xVLyHw.koPkRA","timestamp":1437742607739,"nonce":"3114621597342193","mac":"O33fBmGloowPfOt7PKFpfZ5pspAna3ievCJCF6/qnB8="}
ably.min.js:formatted:1826 Ably: ConnectionManager.connectImpl(): Connection attempt failed with error; err = [c; statusCode=401; code=40170]

@mattheworiordan mattheworiordan changed the title authCallback test coverage + additional type support authCallback bug, test coverage & additional type support Jul 24, 2015
@paddybyers
Copy link
Member

@SimonWoolf : are you ok to have a look at this please?

@SimonWoolf
Copy link
Member

All three methods are supported already, see

/* the response from the callback might be a token string, a signed request or a token details */
if(typeof(tokenRequestOrDetails) === 'string') {
callback(null, {token: tokenRequestOrDetails});
return;
}
if('issued' in tokenRequestOrDetails) {
callback(null, tokenRequestOrDetails);
return;
}
. PR #80 adds a quick test for each and updates the docstring. Re second comment, see #73 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working. It's clear that this does need to be fixed. enhancement New feature or improved functionality.
Development

No branches or pull requests

3 participants