StrategyOptionsCallback shouldn't have to pass all SAML options#838
Conversation
|
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. |
|
@oliverlockwood , I'm preparing a release soon. Would you like to work with me to get this landed for the next release? |
|
Hi @cjbarth, yes, I'll give this a crack. Thanks for the reminder. |
…e some can be derived from the defaults set up during initialisation of the strategy
782d7d7 to
b8d3da8
Compare
cjbarth
left a comment
There was a problem hiding this comment.
This needs a test that actually exercises the fact that issuer isn't required in SamlOptionsCallback.
|
This PR is stale because is has been open for 90 days with no activity. |
|
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 ReportAll modified and coverable lines are covered by tests ✅
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. |
…-saml#838) Co-authored-by: Chris Barth <chrisjbarth@hotmail.com>
…-saml#838) Co-authored-by: Chris Barth <chrisjbarth@hotmail.com>
…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:
This fixes node-saml/node-saml#246.
(I raised the issue in a different repo, as that's where the
SamlConfigclass resides, but it turns out the change I thought to propose is in this repo. Sorry.)