... and will become the SAML implementation for @node-saml/passport-saml. When this is mature, @node-saml/passport-saml will have code removed and replaced by a dependency on this library.

Maybe this part of the paragraph could be removed entirely because @node-saml/passport-saml 4.0.0 is now released with changes mentioned in aforementioned text. I.e. @node-saml/passport-saml has now dependency to @node-saml/node-saml

https://github.com/node-saml/passport-saml/blob/v4.0.0/package.json#L54

  "dependencies": {
    "@node-saml/node-saml": "^4.0.0",

As a random sidenote:

@cjbarth Would it be good thing if this documentation would say something about the fact that if @node-saml/passport-saml is introduced as dependency there is no need to introduce another dependency to @node-saml/node-saml. And similar note to @node-saml/passport-saml's documentation.

For background information:

Some users of passport-saml library do not notice that @node-saml/passport-saml exports also @node-saml/node-saml stuff

https://github.com/node-saml/passport-saml/blob/v4.0.0/src/index.ts#L11

export * from "@node-saml/node-saml";

and are using sometimes potentially "out of sync" / untested combinations of @node-saml/passport-saml and @node-saml/node-saml libraries.

I.e. they introduce dependencies to both libraries and have ended up / are ending up / shall end using potentially versions from unrelated releases (at the future @node-saml/passport-saml and @node-saml/node-saml might have different release schedule for breaking changes so that passport-saml is released much later).

Examples from two existing project:

• ais-one/cookbook:
  • which mixes at the moment passport-saml 3.2.1 and node-saml 4.0.0-beta.2
    1e94c2740a1d8806b211f387fa9b60bc8da57e23/js-node/expressjs/package.json#L41-L43
• perforce/helix-authentication-service which has been mixing various passport-saml versions with unrelated node-saml versions for a while.
  1. 91d28f28dff3a45ccc42e2128ecb04b84c599583/package.json#L59

     "passport-saml": "^3.1.0",

  2. 31609ffdb14837a3a3fd46ec4d7e8bf9e70e35d0/package.json#L57-L60
     Started to use passport-saml ^3.1.0 with zoellner's node-saml 3.1.0 ( see issue #21 )

     "node-saml": "^3.1.0",
     "passport-saml": "^3.1.0",

  3. 8fa352c916184b139054674c5f3e5839559db199/package.json#L57-L60

      "node-saml": "^4.0.0-beta.0",
      "passport-saml": "^3.1.0",

  4. 5fce82f90c1eababe6203f1c5d6181d07c06e660/package.json#L57-L60

      "node-saml": "^4.0.0-beta.0",
      "passport-saml": "^3.1.2",

  5. bfd0805e86c9942b1e606b50b005e7c90bfc1f8f/package.json#L57-L60

      "node-saml": "^4.0.0-beta.0",
      "passport-saml": "^3.2.0",

  6. 7a522b61beac3a8232960cbeedc700f211309ed0/package.json#L36-L37

      "@node-saml/node-saml": "^4.0.0-beta.3",
      "@node-saml/passport-saml": "^4.0.0-beta.1",

@srd90 Some users of passport-saml library do not notice that @node-saml/passport-saml exports also @node-saml/node-saml stuff

Yeah! I noticed that yesterday when I started looking into that public key issue which I have opened into this repository. passport-saml should NOT expose anything from node-saml. My opinion is that passport-saml should provide its own type definitions, at the first version it could be copy-paste from node-saml.

On current implementation export * from "@node-saml/node-saml"; I can se only issues (big or small) in future, whether those are type issues in passport-saml users or those may even limit re-factoring of node-saml.

As Readme of this project states This is a SAML 2.0 authentication provider for Node.js. and in passport-saml This is a SAML 2.0 authentication provider for Passport, the Node.js authentication library.

passport-saml lives its life (mostly) with passport and changes in that library. node-saml interface (type definitions) should not cause side effects to passport-saml interface, now it does.

This will also mean as you mentioned that passport-saml and node-saml releases are tight together, if not, then it's only matter of time when those go out of sync.

If node-saml adds a single required property into SamlOptioninterface https://github.com/node-saml/node-saml/blob/master/src/types.ts#L134 it will break things. Adding a new property is not breaking change in javascript environment and can be seen as patch update, but in typescript environment it can be seen as breaking change.

But this discussion is a little off from the PR topic :D

It is a little off topic, but important. What issues (big or small) do you, @RopoMen , see with passport-saml exporting types from node-saml?

It seems to me that, since passport-saml is going to be calling through to the version of node-saml that it depends on, then we'd want to have the consumer use the types that passport-saml exposes from the version of node-saml that it is going to use. Such a case would help eliminate the problem you mention with required parameters in JS/TS.

Additionally, since both node-saml and passport-saml are provided by the same group, and have an almost identical release process, they are very easy to keep in lock-step, as has been the case for a bit now.

@srd90 I've never taken the time to look at why people would do that, or that they even were. Honestly, though, it shouldn't cause too many problems as I expect (though I'm not 100%), passport-saml will call it's declared dependent node-saml, and not use a node-saml dependency that may otherwise be specified. Confusion on the part of the developer would probably be the biggest problem, and that would be their own doing.

@cjbarth Not sure how clear examples I can give, but here is some thinking based on issues which I have had with other libs.

1. Currently passport-saml is exporting everything which node-saml is exporting, not only the minimum SamlOption (Do not let users of passport-saml use any other exported functionality of node-saml)
2. If passport-saml is exporting SamlOption directly from node-saml, then passport-saml cannot reliably document what the interface of passport-saml is and how it should be used, it is party documented in passport-saml and node-saml.
3. If passport-saml is not exporting SamlOptiondirectly, then there could be case where node-saml introduces some functionality which cannot be used easily from pasport-saml, unless some updates are made.

In sections 2 and 3, on generally what I have seen in npm ecosystem is that it is completely library specific how well they are making semver. Some libs don't care anything about it and breaking changes can be introduced in patch updates. Thereof, it is sometimes best to use fixed version of the package. But I don't think that pasport-saml should do that with node-saml, I think that patch, maybe minior version can change freely.

I have had issues like in case 3 with Multer https://github.com/expressjs/multer it uses Busboy under the hood, but multer is not giving direct access to Busboy configuration. That would be ok if they would do decent maintenance to their lib and introduce new Busboy features to Multer as well.

Another case I have with node-soap https://github.com/vpulim/node-soap they introduced patch update where they replaced request with axios everything worked ok, except Proxy configuration, which lead 1 week extra work for our team because proxy request were broken.

Also, they are using different version of Axios which we have and when Axios changes its Type definitions, then tsc starts complain incompatible types in Axios configuration, well, those can be fixed fairly quickly, but still need some work.

Also in the Axios case, node-soap should have given http client configuration Type definition which is not library specific so that they can make changes under the hood freely.

1. I agree, this shouldn't be the case. We need to figure out what exactly would be needed by a passport-saml consumer and export only that.
2. I think the easiest solution to this is to remove the documentation from passport-saml and make a reference to node-saml. At this point passport-saml is just a convenience wrapper around node-saml.
3. That is potentially true, however, since both projects are maintained by the same group, and we try to obey semver and to release an update to passport-saml to match node-saml releases, I don't see this as being a big problem.

@RopoMen, here is a PR to address some of what you're talking about. I know the README also needs to be updated, but we'll take that in another pass.

@srd90 I cleaned up this paragraph now.
I removed also This was forked from passport-saml at v3.0.0.. because that information is irrelevant.

Also removed The code was originally based on Michael Bosworth's... because Michael is in contributors list in package.json file. AND most importantly express-saml has been updated previously 11 years ago. So this library reference is irrelevant, too many years has passed.