Include support for run-time params to be included in the generated URLs

[cjbarth]

In order to include information specific to a request in the URL, for example, a query string parameter to pre-populate the username field on the IdP, the URL needs to be built with parameters available only when the authenticate method is called. Currently only parameters available at application start are available to be added as query string parameters when the URL is built.

[pdspicer]

This is an interesting idea. A more general approach might be appropriate here that would enable override or merge of all available options during a call to authenticate. Thoughts?

[cjbarth]

Generally speaking, that is what is being done here, except, internally, the options object isn't being mutated, but the mutation is happening after all the internal work is already done. This preserves the current structure of the code.

If I understand you correctly, you're saying to pass an options object with an overrides property to Strategy.prototype.authenticate = function (req, options) {} which would get merged with self.options when called. I can see how that might be a more general solution, I'm just concerned that modifying the global options might set us up for some problems with multiple operations all interleaved within the event loop. It might be safer if we replace all the self.options calls downstream with a local options object that is a clone of self.options that has the passed-in overrides merged in. However, that is a bigger refactor. I would like to hear from the community on that one before I try anything with that, and I'm not sure I have the cycles to do that since this solution meets my needs.

@pdspicer are you in a position to do such a refactor?

[pdspicer]

I wouldn't suggest mutating anything either, I am in favor of possibly changing this._saml in each separate authorize call to a local variable, initialized with a combination of the options passed to the strategy constructor and the local options.
The other approach would be unifying all calls to the underlying SAML library so that they all allow option overrides and follow a structure along the lines of fn(req, {fn-specific args}, options, callback), that way future additions of this type need not break the implementation by changing the signature. In this way each call could perform its own logic for overriding/extending options locally.

[cjbarth]

Is this branch still the direction we might go with this, or is there another branch that is favored? I'd like to support my scenario in master if possible as I see there are some other nice upgrades to dependencies I'd like to take on; maybe even get something like this in before the next release.

[cjbarth]

@markstos Do you have any feedback on the direction you'd like to see this PR taken?

[markstos]

@cjbarth I'm in favor if the flexibility you are suggesting. What does the SAML spec have to say about it? Is functionality mentioned in the spec? Is this useful with a standards-compliant IdP? Specific links or section number references to the SAML spec are helpful if they apply.

You mentioned a "query string parameter to pre-populate the username field on the IdP" Is there a particular IdP that you have in mind that that supports that?

[cjbarth]

@markstos I'm federated against ADFS and it supports a query string to pre-populate the user name field.

Here is some documentation:
https://msdn.microsoft.com/en-us/library/cc236491.aspx

[cjbarth]

I'm getting back into this PR to clean it up and get it landed. It seems that there are two ways to approach this:

1. allow modification of query string parameters for each request
2. allow modification of any option for each request

This PR takes the approach that only query string parameters will be mutable on a per-request basis (1). Some of the comments by @pdspicer favor a more general approach to allow modification of all options on a per request basis (2). I can see justification for (2), but I can also see why we might want, as a library, to dictate that once the federation is initialized, the options related to the federation won't change i.e. have the federation-related options be an invariant and only allow mutation of the URL.

Which level of override is preferred?

[markstos]

Level 1. We can add more flexibility later.

Could you rebase this branch onto master as a single and get rid of the merge commit on this branch?

Also, I think the updated docs could be clearer:

• additionalParams: dictionary of additional query params to add to all requests; this can also be part of the options that are passed to authenticate for params to be added on a per-call basis.

Could you update the docs with an example that shows the additional query params being passed in?

Thanks.

[cjbarth]

@markstos I've pushed changed in line with your suggestions. Does that look good?

[markstos]

@cjbarth Looks good. I'm going to merge. Thanks.