Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Node saml separation #574

Merged
merged 16 commits into from Apr 5, 2021
Merged

Node saml separation #574

merged 16 commits into from Apr 5, 2021

Conversation

zoellner
Copy link
Contributor

@zoellner zoellner commented Mar 24, 2021

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

@zoellner zoellner requested a review from cjbarth March 24, 2021 18:04
@markstos
Copy link
Contributor

Thanks for this work @zoellner !

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 26, 2021

@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
Copy link
Contributor Author

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)

Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

src/node-saml/saml.ts Outdated Show resolved Hide resolved
src/node-saml/saml.ts Show resolved Hide resolved
src/passport-saml/strategy.ts Show resolved Hide resolved
src/passport-saml/strategy.ts Outdated Show resolved Hide resolved
src/passport-saml/strategy.ts Outdated Show resolved Hide resolved
test/node-saml/tests.spec.ts Show resolved Hide resolved
src/node-saml/saml.ts Show resolved Hide resolved
src/node-saml/saml.ts Outdated Show resolved Hide resolved
src/node-saml/saml.ts Show resolved Hide resolved
src/node-saml/saml.ts Outdated Show resolved Hide resolved
@cjbarth cjbarth added the semver-major This change requires at least a semver-major version bump label Mar 27, 2021
@cjbarth cjbarth added this to the 3.0.0 milestone Apr 4, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

@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
Copy link
Contributor Author

zoellner commented Apr 5, 2021

looks good to me

@cjbarth cjbarth merged commit c668737 into master Apr 5, 2021
3 checks passed
@cjbarth cjbarth deleted the node-saml-separation branch April 5, 2021 17:41
@cjbarth cjbarth mentioned this pull request May 10, 2021
@kriss1897
Copy link
Contributor

@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
Copy link
Collaborator

cjbarth commented May 13, 2021

@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
Copy link
Contributor

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

@cjbarth
Copy link
Collaborator

cjbarth commented May 13, 2021

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
Copy link
Contributor Author

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
Copy link
Collaborator

cjbarth commented May 13, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor

kriss1897 commented May 13, 2021

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
Copy link
Contributor

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
Copy link
Collaborator

cjbarth commented May 13, 2021

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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major This change requires at least a semver-major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants