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.

Sign up for GitHub

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

First proposal for auth.setOptions() #260

Closed
wants to merge 1 commit into from
Closed

First proposal for auth.setOptions() #260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 45 additions & 8 deletions common/lib/client/auth.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,23 +40,31 @@ var Auth = (function() {

function Auth(client, options) {
this.client = client;
this.tokenParams = options.defaultTokenParams || {};
this._setOptions(options);
}

Auth.prototype._setOptions = function(options) {
this.tokenParams = this.tokenParams || options.defaultTokenParams || {};

/* RSA7a4: if options.clientId is provided and is not
* null, it overrides defaultTokenParams.clientId */
if(options.clientId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should maybe be if('clientId' in options) -- if someone tries to pass in {clientId: null}, "", or undefined, setClientId should deal with that (whether by throwing an error or unsetting the clientId depending on whether we decide setOptions should be able to let you change your clientId)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, makes sense

this.tokenParams.clientId = options.clientId;
this.clientId = options.clientId
var err = this.setClientId(options.clientId);
if(err) { throw err; }
}

/* decide default auth method */
var key = options.key;
if(key) {
if(!options.clientId && !options.useTokenAuth) {
if(!this.clientId && !options.useTokenAuth) {
/* we have the key and do not need to authenticate the client,
* so default to using basic auth */
Logger.logAction(Logger.LOG_MINOR, 'Auth()', 'anonymous, using basic auth');
if(this.method && this.method !== 'basic') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity, should we not be writing these conditionals as if(this.method && (this.method !== 'basic')) {?

throw new Error('Unable to update auth options with incompatible auth method');
}
this.method = 'basic';
if(this.key && this.key !== key) {
throw new Error('Unable to update auth options with incompatible key');
}
this.key = key;
this.basicKey = toBase64(key);
return;
Expand All @@ -77,6 +85,9 @@ var Auth = (function() {
throw new Error(msg);
}
/* using token auth, but decide the method */
if(this.method && this.method !== 'token') {
throw new Error('Unable to update auth options with incompatible auth method');
}
this.method = 'token';
if(options.token) {
/* options.token may contain a token string or, for convenience, a TokenDetails */
Expand All @@ -97,7 +108,33 @@ var Auth = (function() {
Logger.logAction(Logger.LOG_ERROR, 'Auth()', msg);
throw new Error(msg);
}
}
};

/**
* Update the authentication options for this client;
* this will also update these options on the realtime connection
* so that they take effect immediately
* @param options
*/
Auth.prototype.setOptions = function(options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike this API because I do not feel it is clear what it does and it creates ambiguity in regards to what authorise does. For example, if you are using a REST library and call authorise({ force: true }), it will obtain a new token (if a means to generate one is available) and use that for all new requests. However, in the case of realtime, it will not until the connection is re-established. Then calling setOptions in the case of REST will do the same as authorise({ force: true})

I therefore propose we instead simply change the specification stating that calling authorise({ force: true }) will action the current connection to be upgraded in the case of connected Realtime client libraries. I can't think of many use cases where the user would not want that to happen.

The major issue I foresee with either approach at this stage is that if a user attempts to reauthorise and the new token details are incompatible with the old, then instead of simply rejecting the upgrade attempt, the connection will effectively move to the FAILED state and all state will be lost. I feel this is perhaps something we can live with until we support upgrade authentication in the protocol itself.

Also, @paddybyers can you confirm what happens if the user is assigned less privileges than before, will that be rejected or will the new privileges be applied and if any channels are attached that are not supported, they will be detached. I assume there are cases where privileges may be downgraded and I think we should support probably this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorise() ensures that credentials are available for the current options.

setOptions() changes the options.

If you wanted to set a new token with new capabilities, how would authorise() do that?

I feel this is perhaps something we can live with until we support upgrade authentication in the protocol itself.

I'm not sure that the outcome would be much different in that case ...

can you confirm what happens if the user is assigned less privileges than before, will that be rejected or will the new privileges be applied and if any channels are attached that are not supported, they will be detached. I assume there are cases where privileges may be downgraded and I think we should support probably this.

The key and clientId must be the same. Capabilities may be varied in either direction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you wanted to set a new token with new capabilities, how would authorise() do that?

In a number of ways:

/* If generating the token */
client.auth.authorise({ capability: newCapability }, { force: true })

/* When using an external authUrl */
client.auth.authorise(null, { force: true })

setOptions is very misleading. Is it setting options for future requests, is it setting options for the client library? Why is setOptions communicating to the server, and more importantly, why is it causing the client to disconnect.

authorise is clear, it's telling the client to authorise, and with force: true it's stating force a reauth.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorise() just forces the generation of a new token using current options.

setOptions sets the options on which authorise() acts. What if you are setting a token explicitly in options ? What if you change the token capabilities by specifying different params in the authURL ? You can't change either of those things just by calling authorise().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorise() just forces the generation of a new token using current options.

Not according to the spec. See RSA10b and RSA10g.

setOptions sets the options on which authorise() acts

Why not just call authorise then?

What if you are setting a token explicitly in options ?

Why not do it in authorise, client.auth.authorise({ token: token })?

What if you change the token capabilities by specifying different params in the authURL ?

What's the problem. Calling authorise will issue the request again with any params configured in authorise({}, { authParams: { "newParam": "value" }, force: true })

You can't change either of those things just by calling authorise().

Yes you can

this._setOptions(options);
if(this.client instanceof Realtime) {
this.client.connection.connectionManager.onAuthUpdated();
}
};

Auth.prototype.setClientId = function(clientId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of this method implies it's a public method but should not be. Can we change it to _setClientId?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

if(clientId !== '*') {
if(this.clientId && this.clientId !== clientId) {
var msg = 'Unable to set incompatible clientId';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't we let people change their clientId with a reauth?

Edit: Matt points out that from realtime might not like you resuming a connection with a different clientId, which is fair enough. never mind

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why shouldn't we let people change their clientId with a reauth?

Because it is currently a fixed attribute of a connection - you also can't resume or recover with a different clientId. Bad things happen to presence for example, so we don't support it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a user ever upgrade from no clientId i.e. user not yet logged in on their single page app to a connection with a clientId? That I imagine is a common use case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a user ever upgrade from no clientId i.e. user not yet logged in on their single page app to a connection with a clientId?

I agree we could consider that, but right now it is not supported. We shouldn't do it as part of this change IMO but address it when we do mutable auth where we can properly address the various upgrades that are possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't do it as part of this change IMO but address it when we do mutable auth where we can properly address the various upgrades that are possible.

Ok, but is the client library going to actively prevent this, or will it allow it if our upgrade later allowed it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realtime actively prevents it currently. The library allows it, so you would get a failed connection instead of an exception which means the connection isn't touched.

Given this is a temporary situation I'm not sure it really makes a big difference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library allows it, so you would get a failed connection instead of an exception which means the connection isn't touched.

What do yo mean the connection isn't touched?

Given this is a temporary situation I'm not sure it really makes a big difference.

I agree, but assuming we allow anonymous -> identified clients in realtime in the near future, it would be nice to know it will just work without any further changes here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do yo mean the connection isn't touched?

The transport is not disconnected, and no new transport is created.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The library allows it, so you would get a failed connection instead of an exception which means the connection isn't touched.

But you said the library allows it. So as far as I can tell, the following will happen.

  1. User calls setOptions
  2. New clientId is set
  3. Transport is disconnected
  4. Transport reconnects trying to resume and use new clientId, but Ably rejects it with an error code.
  5. Client library moves to the FAILED state

So not sure why you say it's not touched.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but assuming we allow anonymous -> identified clients in realtime in the near future, it would be nice to know it will just work without any further changes here

BTW. Something to consider, if we did support changing the clientId for this temporary solution, then upon reconnection we need to ensure the clientId for the Auth object is updated to reflect the new clientId provided in the CONNECTED message.

return new ErrorInfo(msg, 40102, 401);
}
/* RSA7a4: if options.clientId is provided and is not
* null, it overrides defaultTokenParams.clientId */
this.clientId = this.tokenParams.clientId = clientId;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone tries to call this with clientId of '*', we should probably throw an error (as we do now if you try and put clientId: '*' in clientOptions) rather than silently ignoring it, or people might thing that that's a valid way of unsetting a clientId.

(Though then connectionManager couldn't use it as-is)

} else {
  return new ErrorInfo('Can’t use "*" as a clientId as that string is reserved. (To change the default token request behaviour to use a wildcard clientId, pass in {defaultTokenParams: {clientId: "*"}})', 40012, 400);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to look at your proposed change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed we should disallow explicitly setting clientId to * as this is invalid, although equally this function should not be public. Saying that, a user could implicitly be calling this function when calling setOptions

return null;
};

/**
* Ensure valid auth credentials are present. This may rely in an already-known
Expand Down Expand Up @@ -157,7 +194,7 @@ var Auth = (function() {
}
callback(null, (self.tokenDetails = tokenResponse));
});
}
};

if(token) {
if(this._tokenClientIdMismatch(token.clientId)) {
Expand Down
22 changes: 13 additions & 9 deletions common/lib/transport/connectionmanager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,18 +458,13 @@ var ConnectionManager = (function() {
this.setConnection(connectionId, connectionKey, connectionSerial);
}

var auth = this.realtime.auth;
if(clientId && !(clientId === '*')) {
if(auth.clientId && auth.clientId != clientId) {
/* Should never happen in normal circumstances as realtime should
* recognise mismatch and return an error */
var msg = 'Unexpected mismatch between expected and received clientId'
var err = new ErrorInfo(msg, 40102, 401);
Logger.logAction(Logger.LOG_ERROR, 'ConnectionManager.activateTransport()', msg);
if(clientId) {
var err = this.realtime.auth.setClientId(clientId);
if(err) {
Logger.logAction(Logger.LOG_ERROR, 'ConnectionManager.activateTransport()', err.message);
transport.abort(err);
return;
}
auth.clientId = clientId;
}

this.emit('transport.active', transport, connectionKey, transport.params);
Expand Down Expand Up @@ -876,6 +871,15 @@ var ConnectionManager = (function() {
this.notifyState({state: 'closed'});
};

ConnectionManager.prototype.onAuthUpdated = function() {
var state = this.state.state;
if(state == 'connected' || state == 'connecting') {
/* in the current protocol version we are not able to update auth params on the fly;
* so disconnect, and the new auth params will be used for subsequent reconnection */
this.disconnectAllTransports();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect by doing this we'll trigger a disconnected state which in turn will:

  • Trigger a disconnected event for users of this lib unexpectedly and they may as a result update their UI to reflect the disconnected state
  • The client library will fallback to an HTTP Comet transport for it's first reconnect attempt.

I think both of these things are bad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trigger a disconnected event

Well, that's because it is disconnected, and isn't connected again until the new transport connects.

The way to avoid this is to implement upgrade in all of the other libraries, which I think we agreed would not happen.

This is a temporary situation until we have in-protocol auth update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, fine, we just need to document that then.

Can you confirm that a WS connection would by default fallback to Comet though? That is not ideal.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that a WS connection would by default fallback to Comet though? That is not ideal.

Yes it will and it will do it for the same reasons that we don't currently cache the last successful transport. You might have a websocket connected over cellular, but by the time the reauth happens you're in the office on the office Wifi, where ws fails because of a proxy.

Copy link
Member

@mattheworiordan mattheworiordan Apr 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might have a websocket connected over cellular, but by the time the reauth happens you're in the office on the office Wifi, where ws fails because of a proxy.

Sure, but why not try the same transport again and then fallback? I am reasonably confident that in most cases the transport that was used last could be used again because it's typically a device restriction as opposed to a network restriction. Whereas assuming it can't in most cases negatively impacts performance, adds more work on both ends, and is unlikely to happen that often.

}
};

ConnectionManager.prototype.disconnectAllTransports = function() {
Logger.logAction(Logger.LOG_MINOR, 'ConnectionManager.disconnectAllTransports()', 'disconnecting all transports');

Expand Down