Node saml separation

[zoellner]

Description

I’ve separated node-saml form passport-saml in src and tests. passport-saml imports from node-saml but not the other way round. That way node-saml can eventually be published separately.
For now I’ve left everything in the same repo. That way the code changes can be reviewed more easily. I’ve made more smaller commits instead of one large one to help with that as well.

This is still a draft PR.
Remaining is to remove all references to the express requests from saml.ts as those are passport related and not saml related.
It is only used in very few places:

• to extract req.protocol and req.headers.host
• to pass through RelayState
• to pass through req.user
• to pass through the profile ID in a logout requests

I think these references can be removed with little effort but it does require changes to the method signatures in a few places. I think we should be able to contain all changes in passport-saml such that there are not changes required to the public API for passport-saml but would like another set of eyes on the branch before I continue down that path.

Todo list:

• remove references to express requests
• move SamlOptions type into saml or split into node-saml/passport-saml parts
• update/split documentation

[markstos]

Thanks for this work @zoellner !

[cjbarth]

@zoellner I see that you asked for my review, but there are still unchecked items. Is all the code done and ready for review and just the docs are left? I don't want to review if there are still churn in the code-base.

[zoellner]

yes, the code related to the split is done from my end.
I have a few minor things on my list for after the split (e.g. mark sha1 as deprecated) - some of them are probably taken care of in the xml split PR as well. So I'd consider the code changes for this PR complete.
next steps would then be merging with the other pending PRs, then work on getting the packages released separately (but that can happen after the 3.x release as well)

[cjbarth]

Looks pretty good. I've made just a few minor suggestions. Ultimately I see this as a good first step and that we'll need more changes going forward. Do we still think we can get this all disentangled by a 3.0 release? If not, no worries, just trying to step my expectations and budget code review time :)

[cjbarth]

@zoellner What do you think of the changes I've made to your branch? Do they look acceptable and appropriate to you? If so, I think we can get this landed.

[zoellner]

looks good to me

[kriss1897]

@cjbarth @zoellner @markstos Hey guys, This is Great!👏🏻👏🏻
This will be really helpful for people like me who are working on code that was extracted from an earlier version of this library to implement SAML (about 3 years ago). If there is any way I can help, please let me know.

[cjbarth]

@kriss1897 the separation isn't completely. We really want to factor all the SAML-specific code out to its own project and use it as an import to passport-saml. We are looking for PRs to help us along those lines. Once the code is completely separate internally, we'll copy it to a new project and then make a PR against passport-saml to import it as a dependency.

[kriss1897]

@cjbarth Do we have any specific things outlined? I can get started on this separation and testing.

[cjbarth]

It seems that we need to update and split the documentation as a next (final?) step before we move all this to another package. We'll probably only remove code from passport-saml in a 4.0.0 release, but that gives us a little time to start making sure that a node-saml package works well.

[zoellner]

Agree with Chris. The documentation split would be the next step. Then establishing a separate package.json in the node-saml folder.
My idea was to do most of the work in the current split folder structure until we feel ready for a full split into a separate repo.

[cjbarth]

I was planning on forking passport-saml to node-saml when we release v3, then we can start making change like package.json in the new repo. I'd release node-saml to NPM as its own package starting at v3 and then when we're ready we'll bump both to v4 and release a passport-saml that only imports node-saml.

Thoughts?

[zoellner]

sounds good. let me know when that happens and we can manage to transfer the node-saml npm package name over to the org (it's currently my own fork/split)

[kriss1897]

Okay. I saw that we have already created the node-saml repo. So I guess further work regarding this would be done on that repo.
I'll go through the current documentation organization/implementation and share my plan with you.
Thoughts?

[kriss1897]

I can see that the only documentation that we have is in the README and a small part in docs dir.
I'm thinking of integrating the node-saml part in my service and update it's docs as I go through the integration and testing, and also try to cover the tickets tagged as documentation.

[cjbarth]

I hope to release v3 soon and then I'll fork passport-saml as the first commit to node-saml. We want to capture the history of passport-saml in node-saml as it is relevant.

Documentation work will need to be done in both repos because we need to make sure that there is nothing in passport-saml that references an API that is only part of node-saml. I'm sure as we go along we'll want to change the API surface to make it as small as possible for both projects, and that too will cause a documentation change. Really, for passport-saml the only API that should be importable would be the ones that passport specifies.

[kriss1897]

Now that both 3.0.0 is released and node-saml is synced. What's next?
I've opend up an issue, just to track the communication about this on the new repo. node-saml/node-saml#2