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

TokenDetails only accepts Ably Token, not Ably JWT #49

Open
mattheworiordan opened this issue Nov 5, 2019 · 7 comments
Open

TokenDetails only accepts Ably Token, not Ably JWT #49

mattheworiordan opened this issue Nov 5, 2019 · 7 comments

Comments

@mattheworiordan
Copy link
Member

mattheworiordan commented Nov 5, 2019

A customer recently set up a local server endpoint that issue JWTs. The response looked something like this:

/* Content-type: application/json */
{ "token": "JWT_TOKEN_BASE64_ENCODED" }

The customer then set the authUrl to the server endpoint, and received an error (similar to the one below, generated from the Node.js lib when trying this out):

22:46:31.567 Ably: Auth.requestToken(): Expected token request callback to call back with a token string, token request object, or token details object
22:46:31.568 Ably: ConnectionManager.actOnErrorFromAuthorize: Client configured authentication provider request failed

Changing the response to a plaintext response, and sending just the JWT_TOKEN_BASE64_ENCODED, everything works as expected.

If you refer to the spec, https://docs.ably.io/client-lib-development-guide/features/#RSA4f, it does indeed state that text/plain is allowed, but also allows TokenDetails. The spec for TokenDetails simple states that token is a token string, yet doesn't indicate that it needs to be a native Ably token string as opposed to an Ably JWT or Ably embedded JWT.

Whilst we could correct the documentation to correct this ambiguity, I think it would make more sense to allow a JWT to be included in the token attribute of a TokenDetails object, given it reduces complexity / surprises for end users, and given we're moving toward JWT-compatible tokens more generally, it's a move in the right direction.

┆Issue is synchronized with this Jira Task by Unito

@SimonWoolf
Copy link
Member

SimonWoolf commented Nov 5, 2019

Copying from slack comment:

The reason ably-js is rejecting it is nothing to do with the value of the token property is a jwt not an ably token string -- it's not inspecting that property much at all. It's rejecting it because it's an object that is neither a valid tokenDetails object nor a valid tokenRequest object nor a JWT.

True, it's a subset of a tokenDetails object, but the fields other than token aren't optional -- or at least, the spec doesn't show them as optional. It's lacking expires, issued, and capability. If we want those to be optional fields of a TokenDetails and token to be the only mandatory property then we should change the spec to make that so.

@mattheworiordan
Copy link
Member Author

but the fields other than token aren't optional

Where is that requirement in the spec? TD2 -> TD7 don't specify whether they are optional or not.

@SimonWoolf
Copy link
Member

SimonWoolf commented Nov 5, 2019

TD6 does specify that clientId can be omitted. The others don't, so they might be presumed to be mandatory; a client lib developer looking for confirmation might consult the IDL, which sure enough marks clientId as optional (with a question mark), and the others without.

class TokenDetails:
  +fromJson(String | JsonObject) -> TokenDetails// TD7
  capability: String // TD5
  clientId: String? // TD6
  expires: Time // TD3
  issued: Time // TD4
  token: String // TD2

(the questionmark was added to clientId but not any of the other properties in 6f6715800)

@mattheworiordan
Copy link
Member Author

Ok, thanks for clarifying.

Although I am afraid this unearths a few inconsistencies.

First obvious one, is our documentation for customers does not make that clear. See https://www.ably.io/documentation/realtime/authentication#token-details. None of the attributes are marked required or optional.

screenshot_2019-11-05_11-33-29_pm

Then what is more of an issue, is that the very rules we define in the spec about fields being required, are not followed by the client library itself. Firstly when we instance a library with an invalid TokenDetails object, which is perhaps understandable:

var Ably = require('ably');
var realtime = new Ably.Realtime({ "token": { "token": "xVLyHw.A-pwh7wvdPz91ZQvLj-TW9XdVV6TTKk8gOCStpKtZYt0Atm_6k" }, log: { level: 3 }, autoConnect: false});
realtime.connection.once('connected', function() {
  console.log(" >>>> Token", realtime.auth.tokenDetails);
});
realtime.connect();

/* outputs
 >>>> Token { token: 'xVLyHw.A-pwh7wvdPz91ZQvLj-TW9XdVV6TTKk8gOCStpKtZYt0Atm_6k' }
*/

And far worse when the library obtains a token string from an authUrl, it too constructs a TokenDetails object that too does not have required attributes.

var Ably = require('ably');
var realtime = new Ably.Realtime({ "authUrl": "https://www.ably.io/ably-auth/token/docs", log: { level: 4 }, autoConnect: false });
realtime.connection.once('connected', function() {
  console.log(" >>>> Token", realtime.auth.tokenDetails);
});
realtime.connect();

/* outputs
 >>>> Token { token: 'xVLyHw.A-pwh7wvdPz91ZQvLj-TW9XdVV6TTKk8gOCStpKtZYt0Atm_6k' }
*/

Now of course the client library has no means to obtain the attributes when a token string is provided, either as an invalid TokenDetails object with just a token attribute, or as a literal token string, so perhaps the question then really is why do we insist on a TokenDetails object needing to have attributes that are not actually needed by the library?

@SimonWoolf
Copy link
Member

And far worse when the library obtains a token string from an authUrl, it too constructs a TokenDetails object that too does not have required attributes.

Fair point. Ultimately the answer is that it assumes that any tokenDetails supplied to an authUrl or authCallback was generated by realtime, and so has all the fields (specifically it checks 'issued'); for its own internal purposes it plays a bit more fast and loose.

perhaps the question then really is why do we insist on a TokenDetails object needing to have attributes that are not actually needed by the library?

No particularly good reason, it's just always done it that way. As I said above we could change the spec to make all the fields other than the token optional, and then switch the client lib from checking issued to checking token.

But I guess my question is -- why is anything supplying an object that looks like a TokenDetails that wasn't generated by realtime to an authUrl or authCallback? Sure, yes, we could support {"token":"..."} easily enough, but why, what's the usecase for doing that rather than just supplying a token string as plain text, which all client libs already support?

@mattheworiordan
Copy link
Member Author

why, what's the usecase for doing that rather than just supplying a token string as plain text, which all client libs already support?

None that I can think of for now :)

Interesting though that the customer tried that. If we aren't going to support token objects with just a token string, then we should:

  • Work out why someone thought that was possible
  • Ensure we specify these attributes are required
  • Consider a new error code when a tokendetails/tokenrequest like object is provided, yet doesn't match what is expected, so that we can write a support article on this.
  • Revisit how we approach the currentToken attribute given it violates the requirements or a TokenDetails object.

@QuintinWillison QuintinWillison transferred this issue from ably/docs Oct 3, 2022
@sync-by-unito
Copy link

sync-by-unito bot commented Oct 17, 2022

➤ Automation for Jira commented:

The link to the corresponding Jira issue is https://ably.atlassian.net/browse/SDK-2792

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

No branches or pull requests

3 participants