Second proposal for connection reauthing: using auth#authorise()

[SimonWoolf]

Alternative to https://github.com/ably/ably-js/pull/260/files, as requested

Also fixes #237

One of the tests added (reauth_tokenDetails) currently fails due to https://github.com/ably/realtime/issues/534

[paddybyers]

why this?

[paddybyers]

A few comments but otherwise lgtm

[mattheworiordan]

Shouldn't this be checking if any token auth params are provided as well. Also clientId could be passed in with defaultTokenParams.clientId which would then be ignored and basic auth assumed.

[SimonWoolf]

Shouldn't this be checking if any token auth params are provided as well.

The logic of when to use basic vs token auth isn't something I changed here - ably-js has always used basic if a key is given as long as useTokenAuth isn't set. That said, looking at RSA4, you're right, the logic not spec-compliant. I'll add a separate commit to fix that.

Also clientId could be passed in with defaultTokenParams.clientId which would then be ignored and basic auth assumed.

I'm not sure I agree with this bit - should merely providing defaultTokenParams really force token auth used if it wouldn't otherwise have been used? That seems surprising. If someone wants an identified client they should pass a clientId in clientOptions directly

[mattheworiordan]

I'm not sure I agree with this bit - should merely providing defaultTokenParams really force token auth used if it wouldn't otherwise have been used?

True, you are right

[SimonWoolf]

Added 998b6a4 for RSA4 compliance

[mattheworiordan]

Few comments, but on the whole I think it's a good solution and sets the groundwork for an improved authentication upgrade process in future.

[mattheworiordan]

So to be clear, are we all in agreement that:

• If you provide null as either or both arguments to authorise the default settings will remain and an authorise request will be made if a valid token does not currently exist
• If you provide an object for either argument, the attributes of the TokenParams or AuthOptions will be merged with the client library initial defaults (not the current configured values of the library) and used for that authorise request and all future authorise requests
• If AuthOptions has all attributes null apart from force: true then it will not change the configured AuthOptions for the library, but instead simply force an authorise. I don't think queryTime should fall into this category personally and should be treated the same as all other AuthOptions as why would someone want to call authorise({}, { queryTime: true }) after a previous authentication had been made. They should do that when instancing the library as if the time is incorrect when creating the library there is no point trying to correct that later with an authorise
• I realise some time ago we moved force: true into the AuthOptions, but given that options are NOT merged, I think this is now a mistake. I propose for spec 0.9, we move force: true out of AuthOptions, and perhaps go back to a simple boolean argument i.e. authorise(TokenParams, AuthOptions, boolean force) or go for something more flexible such as authorise(TokenParams, AuthOptions, AuthoriseOptions) where AuthoriseOptions only supports force: true for now. Whilst this is more flexible, the naming is terribly confusing so we'd need a better name for the class. Thoughts on this please.

[SimonWoolf]

If you provide null as either or both arguments to authorise the default settings will remain and an authorise request will be made if a valid token does not currently exist

s/default/previous, but yes

If you provide an object for either argument, the attributes of the TokenParams or AuthOptions will be merged with the client library initial defaults (not the current configured values of the library)

There are no client library initial defaults though, for either tokenParams or authOptions. If you don't supply any authOptions, the library will just throw an exception; if you don't supply any tokenParams, it won't pass any to realtime, so realtime will use its own defaults. (I can't find it now, but IIRC we had a long discussion about this, and we changed the spec such that the client lib would not supply its own defaults). So this is equivalent to "replaces the current values".

If AuthOptions has all attributes null apart from force: true then it will not change the configured AuthOptions for the library, but instead simply force an authorise. I don't think queryTime should fall into this category personally and should be treated the same as all other AuthOptions

All the other authOptions apart from force and queryTime provide a way to authenticate, that's why they're mutually exclusive. Treating queryTime like token/key/authCallback/etc, just means an authorise({}, { queryTime: true }) call would result in the library raising a "no valid authentication parameters" exception next time it needed a new token, rather than just being pointless-but-harmless. Though I guess it doesn't really matter as you're right, no-one's going to do it.

I realise some time ago we moved force: true into the AuthOptions, but given that options are NOT merged, I think this is now a mistake. I propose for spec 0.9, we move force: true out of AuthOptions, and perhaps go back to a simple boolean argument i.e. authorise(TokenParams, AuthOptions, boolean force) or go for something more flexible such as authorise(TokenParams, AuthOptions, AuthoriseOptions) where AuthoriseOptions only supports force: true for now. Whilst this is more flexible, the naming is terribly confusing so we'd need a better name for the class. Thoughts on this please.

Yeah, I guess that would make sense (since {authUrl, authCallback, token, tokenDetails, key} are special: they're not independent settings, they're alternative ways to authenticate. Cleaner to split out actual toggleable settings). I don't have a good answer to the naming/organisation thing though. I dunno, maybe an auth attribute to authOptions (ie authOptions = {auth: {authUrl: "..."}, force: true}), with single-level merge semantics, so the auth attribute has replace semantics? Bit complicated though, and not very good for java etc. I'll wait & see what Paddy suggests.

[mattheworiordan]

There are no client library initial defaults though

There are, see https://www.ably.io/documentation/realtime/types#auth-options. All I am saying is that the default behaviour of the client lib remains if an option is not provided in the Hash with loosely typed languages.

All the other authOptions apart from force and queryTime provide a way to authenticate, that's why they're mutually exclusive

I am not sure I understand what you're saying here because I don't think either queryTime or force are mutually exclusive, and the other attributes are not mutually exclusive either i.e. you can provide authUrl and authParams or you can provide useTokenAuth and authCallback. I also don't see why we would disallow providing token and authCallback so that the lib could use the token provided but has a means to renew.

I don't have a good answer to the naming/organisation thing though. I dunno, maybe an auth attribute to authOptions (ie authOptions = {auth: {authUrl: "..."}, force: true})

If we were starting from scratch perhaps, but I don't think that big a change is a good idea. Simply removing force and moving it into a new argument would for most users, probably not be breaking, whereas your change would certainly break things for everyone.

[paddybyers]

@SimonWoolf Can you update this please to reflect the latest spec proposal, and then add some tests so I can test my realtime changes?

[SimonWoolf]

Added 83b563b for latest reauth spec compliance.

@paddybyers the new reauth_tokenDetails test in spec/realtime/auth.test.js currently fails and should pass against a realtime that allows changing token capabilities. That currently just tests ability to upgrade capabilities, since iirc that's what we decided was the minimum necessary change. Let me know if you also want tests for downgrading capabilities and/or going from anonymous to identified.

[SimonWoolf]

also 1516466. grumble node vm contexts grumble

[mattheworiordan]

Just a note. Before this lands, we need to ensure the connection recovery documentation states that capabilities can be changed because currently it's not

[paddybyers]

Closing in favour of #269

[SimonWoolf]

Merged as 31f70cb