StrategyOptionsCallback shouldn't have to pass all SAML options

[oliverlockwood]

…since some can be derived from the defaults set up during initialisation of the strategy, as per the documentation at http://www.passportjs.org/packages/passport-saml/, which clearly says:

The options passed when the MultiSamlStrategy is initialized are also passed as default values to each provider. e.g. If you provide an issuer on MultiSamlStrategy, this will be also a default value for every provider. You can override these defaults by passing a new value through the getSamlOptions function.

This fixes node-saml/node-saml#246.
(I raised the issue in a different repo, as that's where the SamlConfig class resides, but it turns out the change I thought to propose is in this repo. Sorry.)

[cjbarth]

Can we get some tests to make sure this behavior doesn't break in the future? Ideally a tests that throws a type error (which you should mark as such with @ts-expect-error and then a test that works as you want it to, which is currently broken.

[cjbarth]

Needs tests.

[cjbarth]

@oliverlockwood , I'm preparing a release soon. Would you like to work with me to get this landed for the next release?

[cjbarth]

Needs tests.

[oliverlockwood]

Hi @cjbarth, yes, I'll give this a crack. Thanks for the reminder.

[cjbarth]

This needs a test that actually exercises the fact that issuer isn't required in SamlOptionsCallback.

[github-actions]

This PR is stale because is has been open for 90 days with no activity.

[oliverlockwood]

With the "stale" reminder, I'll be honest - I haven't had time to write more extensive tests for this, and it doesn't feel like that's going to change any time soon. I'm sorry about that, but I'm just being realistic.

@cjbarth I guess you'll have to just make a decision as to whether to accept this trivial correction without UT coverage being provided, or whether you'll reject it and continue to have a mismatch between documentation and type definitions. Your call.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.42%. Comparing base (eacbbbb) to head (6491f35).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #838   +/-   ##
=======================================
  Coverage   64.42%   64.42%           
=======================================
  Files           4        4           
  Lines         149      149           
  Branches       37       37           
=======================================
  Hits           96       96           
  Misses         30       30           
  Partials       23       23           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.