token option vs tokenId

[mattheworiordan]

@paddybyers when implementing the Ably Ruby library I opted to use a token_id option as opposed to token because I understood that by just passing a token ID the library could authenticate with Ably. However, I see in the Ably JS library that it seems to expect an actual token, whereas if you instance the library with a string token, it is actually a tokenId.

Should we not be consistent and require either a token or a tokenId, or should we instead perhaps simply add another option which is tokenId that allows the tokenId string to be passed into the constructor?

FYI, I noticed that in the Ably lib I was using the option api_key instead of key, so that is changed now, see ably/ably-ruby@8b6cc9c

[paddybyers]

There is no such thing as a tokenId, there is only a token.

The TokenDetails response contains the token in its id field, which I agree is potentially confusing but I think that's a documentation issue.

[mattheworiordan]

@paddybyers I find this very confusing and inconsistent unfortunately.

For example:

In Java

• A Auth#requestToken function returns a TokenDetails object, which has details of a token, yet the token itself is called id, not token
• However, when calling Auth#requestToken, one has to pass in TokenParams which again has an id field, which is nothing to do with tokens, but instead is a keyName which developers know nothing of i.e. we mostly refer to it as an opaque ApiKey in every context, yet here we need them to understand how a key works and split it by the : and then set the id field, and then the id field of the object that is returned is the token they requested.
• When setting up a client or issuing #requestToken above, one can authorise using a token, yet now it's called authToken and not token or event tokenDetailsId which would indicate to the user where that came from.

In Ably-js

• The JS library is consistent in that authToken param can be used, yet you can also pass in TokenDetails to the client library constructor with the option token. So token in this instance is not a token, but TokenDetails, yet we call it token, and authToken is actually the token string.

REST API

• When issuing a request to the requestToken end point, as mentioned above, we send an id param in the request that is actually the key name, this should really be named something like keyName (see Change {{API_KEY_ID}} to {{API_KEY_NAME}} docs#12 for a previous discussion on this) for consistency and clarity in the API.
• Instead of simply returning the token in the JSON directly through, we instead put the TokenDetails in a field called access_token which is not really an access token, but instead a combination of token meta data and the token itself.
• the field key within access_token is not the key, but the keyName
• the field id within the access_token is not the ID, but the token itself.

I realise this is potentially a breaking change, at least from a client library perspective, but I find that this is very inconsistent unfortunately.

In an ideal world, if we had a versioned API now, this would be less of an issue as we could easily introduce new fields in the REST API end point describe above without fear of breaking any client libraries. And we could then do releases of the client lib down the line with major version numbers and a CHANGELOG indicating the changes. Unfortunately we're not able to do that now.

I would like to discuss this as my implementation is in fact not consistent with the Java & JS libraries because I misunderstood, and the Spec which we have told everyone to work to, is in fact wrong so we now quite likely have client libraries with different implementations.

@kouno FYI, see above as it effects Go & PHP.

[paddybyers]

Unfortunately this is all the result of us having changed our requirements and terminology since this was originally specified and implemented.

an id param in the request that is actually the key name, this should really be named something like keyName

yes, we should do this if we're ok for things to break at this stage. Of course the system can accept both id and keyName.

Instead of simply returning the token in the JSON directly through, we instead put the TokenDetails in a field called access_token which is not really an access token

yes .. this was originally because we were returning:

{
  access_token: <token details>,
  refresh_token: <token details>
}

We removed the idea of a refresh token, but if we promote the token details to be the entire response body it means we would not be able to extend the contents of the response in future (except by adding fields to TokenDetails itself.

That said, I agree that it looks redundant as it is.

the field key within access_token is not the key, but the keyName

I agree it should be keyName, and we have to decide whether or not we're ok with that as a breaking change.

the field id within the access_token is not the ID, but the token itself

as above; it's a breaking change but I'm open to changing it. I think if the other uses of id (ie in the request) are removed, then id is perhaps ok in the TokenDetails but equally it would be token, or access_token.

yet you can also pass in TokenDetails to the client library constructor with the option token

We have to decide whether or not there is a requirement to be able to do this; the only advantage presently is that the library gets to find out the expiry time for the token. If in future we had other metadata, or a refresh token, then there might be more of an argument for it.

[mattheworiordan]

Ok, so here is my recommendation on what we change based on a quick chat with @kouno. If you have any feedback, I will amend this so that this change can be used as a spec for others too.

Auth#requestToken

Auth#requestToken requests a token from the Ably server and as the name implies, should return a token. We could of course return a Token object that had the metadata that is currently included in TokenDetails, however that would then mean we need to support two types of tokens, token strings and token objects. Whilst we could make this work in a lot of languages, this will inevitably lead to further inconsistencies between client libraries. As such, we should return an object that is suitable named:
TokenDetails which contains the following fields:

• token - String (was id)
• expires - long / Datetime depending on what's most idiomatic for the language. If long is used, documentation comments in code must specify that attribute is ms since epoch.
• issued - long / Datetime (as above for expires). Note that this field was previously issued_at in the JSON response from the API
• capability - String (JSON stringified)
• clientId - String

Auth#createTokenRequest

Currently this method returns a TokenParams object, however this name is confusing because TokenParams is really the token request object. In the Java library it's described as "A class providing parameters of a token request", which would be a whole lot simpler if it was "TokenRequest - A class representing a token request". In the Go library I believe this makes more sense, see https://github.com/ably/ably-go/blob/f6b4df13772a1a1d14112e429140884d54b96a47/rest/auth.go#L53-L86. We agreed that TokenParams should be used only as the TokenParams argument, and TokenRequest will inherit from TokenParams and add in nonce & mac attributes. Therefore the TokenRequest object will have the following fields:

• keyName - String (was id)
• ttl - Long (time to live in milliseconds, this is currently in seconds, @paddybyers to review implications of standardising as milliseconds)
• capability - String (JSON stringified)
• clientId - String
• timestamp - long / DateTime (as above with expires)
• nonce - String
• mac - String

Client library constructor

The following params can be used when constructing a library, and also later with Auth options.

• key - API key String
• token - Token String (with optional support to detect TokenDetails and use it where language permits, this was authToken)
• tokenDetails - so that TokenDetails returned from requestToken can be passed directly to the library. This could be optional.
• authUrl - callback to retrieve token via HTTP, it should accept a token string if content type is text/plain, and will detect if the response is a TokenRequest or a TokenDetails based on the JSON response.
• authCallback - as above, it will detect the type of String (Token), TokenRequest or TokenDetails.
• keyName & keySecret - we should remove this from the library as it's not needed

REST requestToken endpoint

• Request param id that is used for keyName will now be called keyName in the JSON body and in our documentation and client libraries. We will not offer backwards support.
• As above, we need to review why we use ttl in seconds here, but as far as I can tell, in milliseconds everywhere else.
• The response will no longer be contained within access_token as we only ever response with one Token request response. The format will therefore be as follows (note id changed to token, key change to keyName, and issued_at changed to issued):

{
    "token": "xVLyHw.CLchevH3hF....MDh9ZC_Q",
    "keyName": "xVLyHw.mDYnFA",
    "issued": 1428356667,
    "expires": 1428360267,
    "capability": "{\"*\":[\"*\"]}"
}

Finally, when we release this change, we will introduce a change log and bump the major version number to indicate a potentially breaking change for consumers. The Changelog will then start once this change + #28 is implemented.

[mattheworiordan]

@paddybyers and @kouno please see comment above. I believe this is what we agreed, please shout if I missed something.

[mattheworiordan]

@paddybyers and @kouno, one question. The property capability is currently a string in the REST API response, yet I don't see why we should expose it as a string to the developer, but should instead expose it as a JSON object. WDYT, I would prefer to expose it as a Hash/JSON object.

[paddybyers]

I would prefer to expose it as a Hash/JSON object

I'd prefer that we don't change it, as we keep it opaque as far as the client library is concerned.

[mattheworiordan]

@paddybyers but I am not sure why we see this as opaque when this capability is derived from JSON provided by the client in every case where it matters. For example, if you want to issue a token and limit the user to a channel you'd do something like (pseudo code alert)

capabilities = { "channel": ["subscribe","publish"] }
tokenDetails = auth.requestToken(capability: JSON.stringify(capabilities))

// lets check the details match
JSON.parse(tokenDetails.capabilities)["channel"]

In what case would a use be checking the capabilities unless they a) wanted to examine the actual JSON, b) didn't issue the token themselves using a capability Hash that had been converted to a JSON string?

[mattheworiordan]

@paddybyers if I recall correctly, didn't Martin create the .NET library so that capabilities are exposed as a class and I think he had a capability builder as well. Clearly in that case it's far from opaque. I'm not saying that is right btw, just saying I think that is how he did it.

[paddybyers]

Clearly in that case it's far from opaque

Actually I think the opposite. What he did was provide a builder API, meaning that the encoded representation (the JSON string) can be completely opaque. I would prefer that we go this way because then we are free to change the encoded representation if we need to. (More specifically, not break the existing encoding, but add support for new representations.)

[mattheworiordan]

Yes, you are right he makes the actual value opaque which incidentally I am entirely happy with, but from what I understand Martin has done, he has made the capabilities field into something that a developer can work with. As it stands now, we require a developer to know how the JSON works and construct it and then stringify it so we have inherently made it NOT opaque, and then when they want to access the object they have to parse it again in JSON to make any sense of it. So if you ask me, but exposing it as a string, we have in fact made this far less opaque and will encourage developers to work directly with this string.

At least if it's JSON, we can always convert from one format to another should we wish to change. As it stands now, we could not do that.

Given how simple https://github.com/ably/ably-dotnet/blob/master/src/Ably/Capability.cs and https://github.com/ably/ably-dotnet/blob/master/src/Ably/CapabilityResource.cs are, should we not simply expose capabilities as objects?

[mattheworiordan]

When we chatted today you mentioned that all times are represented as longs (with the exception of ttl) and that is generally the standard. However, I'm pretty sure I have found a number of other fields that are in fact seconds since epoch and not milliseconds since epoch.

• #requestToken response returns issued_at and expires as seconds since epoch, see http://docs.ably.io/rest-api/#requesttoken and your Java implementation at https://github.com/ably/ably-java/blob/master/src/io/ably/rest/Auth.java#L138-L143
• #requestToken request timestamp is in seconds since epoch too, see my example at http://docs.ably.io/rest-api/#requesttoken which works and uses seconds. The Java lib does deal with this, see https://github.com/ably/ably-java/blob/master/src/io/ably/rest/Auth.java#L556
• #requestToken request ttl - we've discussed this, but just mentioning again in the context of all time related fields that are in seconds.

@paddybyers should we change these fields as well to ms since epoch?

[mattheworiordan]

One further note that we discussed, for the Realtime endpoint we currently use key_id and key_value for basic auth instead of key_name & key_secret, and we use the argument access_token which we agreed to leave as is as access_token will not be used anywhere else moving forwards once the requestToken end point response is flattened.

[paddybyers]

we currently use key_id and key_value for basic auth instead of key_name & key_secret,

This is deprecated; use key for basic auth. key_id and key_value are only supported temporarily to prevent things breaking. I don't think we need to support key_name + key_secret.

[paddybyers]

should we change these fields as well to ms since epoch?

yes

[paddybyers]

These changes are up in dev now, including all timestamps to be in ms.

[mattheworiordan]

@paddybyers I assume you've made these changes on the realtime end then and not in any of the client libraries? I'll update the Ruby library to see if it passes against dev.

[paddybyers]

I assume you've made these changes on the realtime end then and not in any of the client libraries?

ably-js and ably-java updated with tests passing

[mattheworiordan]

Shiiiiit, that was quick. @paddybyers where do you keep the minions?

[mattheworiordan]

@paddybyers when you are back, can we chat about the final capabilities point?

[mattheworiordan]

@paddybyers where do we stand on the capability attribute. I didn't realise, but you have a capability builder in the Java lib. Should we standardise this on all libraries so that we have both a Capability builder & reader so that we can ensure developers never deal with the capability string directly?

[paddybyers]

We have decided to remove a small remaining format inconsistency, and rename TokenDetails.issued_at to TokenDetails.issued.

This is because we don't use snake case for property names in json/msgpack.

[paddybyers]

Closing