Stop specifying a clientId forcing token authentication

[SimonWoolf]

Per discussion in #312.

(This section is numbered a bit weirdly -- 'Auth#clientId attribute is not null when:' is RSA7b, but its sibling 'Auth#clientId attribute is null when:', also a child or RSA7, is RSA12. But I'm a bit hesitant to renumber more sensibly as that'd make updating the spec spreadsheet and any references in client lib tests even more of a PITA...)

[mattheworiordan]

I must admit I am nervous about this change as I know we previously intended for this behaviour. I know that in the case of REST publishes now, we would (in the current proposal) now have to mutate the messages to now include an explicit clientId when a clientId is specified in ClientOptions if API key auth is used. This feels very wrong.

@paddybyers has suggested we could solve this with a new param or header perhaps. We'd need to implement this though in the spec. I'll wait for him to review :)

[paddybyers]

We don't currently, but we can easily bind a clientId to the AuthParams if basic auth is used and a clientId is specified as a query param

[mattheworiordan]

but we can easily bind a clientId to the AuthParams

Which AuthParams? You mean for realtime connections? Or headers using basic auth?

[SimonWoolf]

but we can easily bind a clientId to the AuthParams

Which AuthParams? You mean for realtime connections?

For realtime connections we already do this, I checked before writing rsa7e1.

$ wscat -c 'wss://sandbox-realtime.ably.io:443/?key=mz3G9w.G3yQww:<redacted>&heartbeats=false&v=1.0&clientId=foo'

< {"action":4,"connectionId":"-hpRl0mPFM","connectionKey":"e02PL5F4gAEDaU!-hpRl0mPFMIsZrKQ-6242e02PL5F4gAEDaU","connectionSerial":-1,"connectionDetails":{"clientId":"foo","connectionKey":"e02PL5F4gAEDaU!-hpRl0mPFMIsZrKQ-6242e02PL5F4gAEDaU","maxMessageSize":65536,"maxInboundRate":999,"maxFrameSize":262144,"serverId":"frontend.7310.2.eu-west-2-A.i-0f710a35b4d4f7fc4","connectionStateTtl":120000,"maxIdleInterval":15000}}
> {"action":10,"channel":"channel"}
< {"action":11,"flags":983040,"channel":"channel","channelSerial":"e02uzpWSQAEDaH91882123:-1"}
> {"action":14,"channel":"channel","presence":[{"action":2}],"msgSerial":0}
< {"action":14,"id":"-hpRl0mPFM:0","connectionId":"-hpRl0mPFM","connectionSerial":0,"channel":"channel","channelSerial":"e02uzpWSQAEDaH91882123:0","timestamp":1511862389988,"presence":[{"id":"-hpRl0mPFM:0:0","clientId":"foo","connectionId":"-hpRl0mPFM","timestamp":1511862389988,"action":2}]}

If you meant for REST publishes, I'm not sure I see the point. The way REST messages can have a clientId is with Message.clientId, what do we gain by adding a second way of specifying that in the querystring, in a stateless protocol?

[mattheworiordan]

If you meant for REST messages, I'm not sure I see the point.

I am only referring to REST as, as you say, realtime WS endpoints support clientId.

The way REST messages can have a clientId is with Message.clientId, what do we gain by adding a second way of specifying that in the querystring, in a stateless protocol?

Currently, if you specify a clientId in the ClientOptions and publish a message like channel.publish(name, payload) over REST, it is received by all others with a clientId in the message because it is implicit in the publish operation.

Now, with your proposal, it would be received with no clientId, so it's a breaking change.

If the client lib then sets the clientId in the messages, it would be mutating the message objects which just feels wrong.

Hence why I was saying over REST we need to set the clientId as part of the REST operation so that an implicit clientId is included in the REST operations.

[SimonWoolf]

If the client lib then sets the clientId in the messages, it would be mutating the message objects which just feels wrong.

The message object delivered to the subscribers will include a clientId field either way. The only difference is whether it gets added to the message by the publishing client lib or by realtime (a difference completely hidden from the client lib user). Why does it feel wrong to you in the former case but not the latter?

Seems odd to me that we're considering adding more complication to the rest api in order to make it so that technically it's realtime that actually adds the field, on the instruction of the client lib, rather than the client lib just adding it itself.

[mattheworiordan]

Seems odd to me that we're considering adding more complication to the rest api in order to make it so that technically it's realtime that actually adds the field, on the instruction of the client lib, rather than the client lib just adding it itself.

Because this:

• Introduces complexity in a client library i.e. this is bad when we have to roll this out multiple ties
• Is inconsistent with how realtime connections work where clientId is implicit, and in REST clientId is not.
• Is trivial to add clientId to the request as a param or header if clientId is set in ClientOptions, and then as a result, the message object provided to the REST API endpoint exactly matches the message object provided to the API. In no other cases, as far as I am aware, other than for encoding purposes, do we ever mutate a customer provided message object before it arrives at Ably. Why would we want do that and then move the business logic and what is / is not allowed into the client libraries thus adding yet more client library logic in all client libs.

[SimonWoolf]

OK, fair enough, I see where you're coming from. Added 3fed1e0 accordingly.

(By analogy with Authorization, I've documented it as a header in the spec, but realtime should also supports it as a qs param for when custom headers aren't supported, e.g. jsonp)

[mattheworiordan]

Thanks, LGTM 👍

[mattheworiordan]

@SimonWoolf I assume you don't think it's necessary to specify how JS handles this? Good suggestion re: Base64 btw.

[paddybyers]

What's the reasoning behind having a header instead of a query param?

[SimonWoolf]

I assume you don't think it's necessary to specify how JS handles this?

as in, for when custom headers aren't supported (i.e. IE<10)? Nah, I don't think it's necessary to put it in the spec, it just complicates things for everyone else. Similar to how we do the Authorization header -- realtime also supports auth by qs param for old IEs, but the spec only mentions the main (header) way.

(I think this is justified because it's only temporary -- at some point, maybe not too far away, non-websocket webbrowsers will drop below ε% usage share, and we can drop all the jsonp, comet, upgrade etc. crap from ably-js entirely)

[SimonWoolf]

What's the reasoning behind having a header instead of a query param?

Consistency with the Authorization: Basic ... header (RSA11). Since that's how auth is already done for REST requests, adding a second header can be done just by adding it to the existing Auth#getAuthHeaders (or equivalent in another client lib) method.

Since there's currently no query param that should be applies to all rest requests, client libs probably don't have an existing mechanism to add one, so doing it that way would be a more work for most client libs.

(Though realtime will still have to support both for jsonp, so I'm trading off realtime ease of implementation for client lib ease of implementation)

[mattheworiordan]

I too prefer headers, although I don't have a good reason why 😉
It just feels a whole load cleaner that the REST URL endpoint is the same regardless of your identity.

[mattheworiordan]

@paddybyers @SimonWoolf happy to go with this if you are. It does mean more functionality for our client 1.1 SDK release, but unavoidable. 👍

[paddybyers]

LGTM, thanks