Server processConnectionSettings callback

[fredericosilva]

Hello,

What is your use case for this callback?

Will you be open for a PR to make callbacks async?

Thank you

[vadimpronin]

Hi! I use this callback to authorise client and change/manipulate with connection parameters. Like in this example:

const callbacks = {
    processConnectionSettings: function (settings) {
        if (settings.expiration < Date.now()) {
            console.error('Token expired');

            throw 'Token expired';
        }

        settings.connection['recording-path'] = '/data/recordings/' + settings.s3.bucket;
        settings.connection['drive-path'] = '/tmp/guacamole_' + settings.vmId;

        return settings;
    }
};

So for my purposes it must stay sync. But you are more than welcome to make a PR with another callback (or another set of callbacks) which will be async.

[fredericosilva]

Don't you think it will be more generic to force all callback to be async?

Your example would be something like:

const callbacks = {
    processConnectionSettings: function (settings, callback) {
        if (settings.expiration < Date.now()) {
            console.error('Token expired');

            return callback(new Error('Token expired'));
        }

        settings.connection['recording-path'] = '/data/recordings/' + settings.s3.bucket;
        settings.connection['drive-path'] = '/tmp/guacamole_' + settings.vmId;

        callback(undefined, settings);
    }
};

[vadimpronin]

Seems ok to me. But I don't see how is it async now?

server.callbacks.processConnectionSettings(this.connectionSettings, (err, settings) => {

looks pretty synchronous to me. Or am I missing something?

[fredericosilva]

On your example is sill sync on an async fashion. On my use case I need to do a http request to get all the information for the connection.

processConnectionSettings: (settings, callback) => {
    request
        .get(`http://example.com/vm/${settings.connection.vm}`)
        .end((err, rsp) => {
            if (err) { return callback(err); }

            settings.connection.hostname = rsp.body.hostname

            callback(err, settings);
        });
}

[vadimpronin]

I see. Seems fine. The only thing that concerns me is err as the first parameter of the callback. Because returning undefined in case when there's no error doesn't seem neat to me. I'm talking about this:

callback(undefined, settings);

What do you think we we'll be throwing errors and returning just settings?

[fredericosilva]

One can't catch threw errors from a async function, it's a good practice to use first argument for errors, all node core function use this. Undefined or null is a matter of personal taste I would say.

We could also use async/await or more "pure" promises to handle such behavior.

[vadimpronin]

Ok, then. I'm merging the PR :)

[vadimpronin]

By the way, I know it's not clear as there is no documentation, but there's no need to make another request to backend to get connection parameters. You can all the parameters when you are opening connection to guacamole-lite server in encrypted token.

I'm writing docs now to make it more clear :)

[vadimpronin]

UPD: I've updated readme.md to have some examples. I hope they are clear enough and show what functionality is implemented in guacamole-lite at the moment

[fredericosilva]

Thanks for the documentation and the example :)