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

Deprecate this repo in favour of an active fork #267

Closed
LoneRifle opened this issue Nov 3, 2022 · 27 comments
Closed

Deprecate this repo in favour of an active fork #267

LoneRifle opened this issue Nov 3, 2022 · 27 comments
Assignees

Comments

@LoneRifle
Copy link
Collaborator

LoneRifle commented Nov 3, 2022

The organisation I work for has moved from SAML to OIDC for authentication, and hence I have less motivation to be active on this repo. Since @yaronn is still the admin and owner of this repo, all requests to be given write rights for the repo must go through him, even though he has been inactive on xml-crypto for a while.

This package is still in demand though, as evidenced by recent proposals to add a new KeyInfo implementation, and by its use in node-saml, of which @cjbarth is an active maintainer. We could ask for a transfer of this repo to a new organisation, but @yaronn is inactive and we don't know when he would be able to accede to the request.

Given this, I would like to propose that:

  • a new repo based on this one is created and actively managed by current users of this package;
  • the xml-crypto npm package is published from that new repo, and;
  • a prominent notice is added to the README of this repo indicating in no uncertain terms that this repo is inactive and to use the new repo.

@cjbarth, what do you think?

@yaronn
Copy link
Contributor

yaronn commented Nov 3, 2022

@LoneRifle if you want to create a new organization and add all maintainers there I can make an ownership transfer to that organization.

@LoneRifle
Copy link
Collaborator Author

LoneRifle commented Nov 3, 2022

@LoneRifle if you want to create a new organization and add all maintainers there I can make an ownership transfer to that organization.

With @cjbarth's or @marksto's consent, I would like this transferred to the @node-saml organisation

@bjrmatos
Copy link
Contributor

bjrmatos commented Nov 3, 2022

even though i am no longer involved in the project, if it helps i am all in favor of transferring the repo to new org and it to be managed by the active maintainers/contributors

@cjbarth
Copy link
Contributor

cjbarth commented Nov 13, 2022

I'm not currently in favor of moving this to a new organization because of the types. node-saml and passport-saml are TypeScript projects, so we depend on the types. Currently the community maintains types in the DefinitelyTyped repository and moving this package would break that, requiring that we either move those types into this project, or we republish the types for this package's new home.

What would be the advantage of moving this package to a new home?

Side note, I note that I was invited to be an owner of the NPM package, but the invitation expired while I was out of town.

@cjbarth
Copy link
Contributor

cjbarth commented Nov 13, 2022

As a point of reference, it doesn't seem like there are any forks that are particularly active. However, with over 750,000 weekly downloads, it does seems some direction is needed for this very popular package.

@cjbarth
Copy link
Contributor

cjbarth commented Nov 13, 2022

I see that Auth0 has forked this project (though it is quite behind). Perhaps @shane-tomlinson would have something to say about Auth0 being involved in maintaining this package.

@LoneRifle
Copy link
Collaborator Author

@cjbarth - I'll resend the invite tmr!

@LoneRifle
Copy link
Collaborator Author

LoneRifle commented Nov 14, 2022

I'm not currently in favor of moving this to a new organization because of the types. node-saml and passport-saml are TypeScript projects, so we depend on the types. Currently the community maintains types in the DefinitelyTyped repository and moving this package would break that, requiring that we either move those types into this project, or we republish the types for this package's new home.

The npm package name would remain the same though. The discussion is only for the GitHub repo to be created or transferred to a new organisation (the latter now favoured given that yaronn can facilitate the transfer), so as long as the package name remains the same, I can't see how the DT types will break. Is it node-saml policy that all packages from the org must be prefixed with org name?

What would be the advantage of moving this package to a new home?

More active ownership, so that individuals can join the core dev team for this repo more easily.

@cjbarth
Copy link
Contributor

cjbarth commented Nov 14, 2022

Ah, that might be where I misunderstood, moving just GitHub and not NPM. We moved passport-saml to an organization for just that reason. We actually moved the package to an organization for that same reason as well (which is probably why I made the assumption that you wanted to move both). I think I'm OK with it, but right now, I'm doing most of the work on passport-saml and node-saml and I don't have the bandwidth to take on more. In fact, the way things are going, we too may be moving away from SAML at some point, which would change my priorities.

Ultimately we need more maintainers, not to throw the package at an organization in hopes that will fix it. Has anyone else expressed interested in helping maintain this package?

@LoneRifle
Copy link
Collaborator Author

@cjbarth unfortunately we might be the only two semi-active maintainers at this point. I largely agree with your view; it's unfortunate that SAML on node is a specific niche that will only get narrower over time.

A transfer to @auth0 (along with node-saml?) would be ideal given that this is what they do, and they already maintain xml-encryption. I don't know if they have any desire too though.

@djaqua
Copy link
Contributor

djaqua commented Jan 15, 2023

@yaronn, @LoneRifle , & @cjbarth - I would be interested in helping maintain this repository. As far as I know, this is the only NodeJS library that provides this functionality and it seems to be a very popular. Since I plan to be making fairly extensive use of this library, I'd be glad to give back to the community by contributing bug fixes and maybe even a handful of random improvements. What say ye?

@LoneRifle
Copy link
Collaborator Author

@djaqua - sure. We still need to figure out where the repo should be transferred to though, and I think it would make sense to avoid a repeat of the scenario where an individual owner is no longer active.

@djaqua
Copy link
Contributor

djaqua commented Jan 16, 2023

Interesting. So all we'd need is an organization account and Yaron would transfer ownership?

@LoneRifle
Copy link
Collaborator Author

Correct. I originally proposed that it be moved to @node-saml since passport-saml and node-saml are dependents. @cjbarth was not so sure since that org only has him as a maintainer. If you are willing to step up to be part of the maintainer team, that might help!

@djaqua
Copy link
Contributor

djaqua commented Jan 16, 2023

I saw the comment about Auth0 maintaining xml-encryption ... I took a gander at their repositories and noticed that they have already forked this repository, as well. Would Auth0's ownership of this repository solve the problem?

If not, I just looked into creating an organization and apparently there's a "free plan" or whatever. If Yaron is onboard with this, I don't see how the free plan wouldn't solve the issue. At that point we'd just have to figure out what the organization would be called and take care of updating the package on NPMjs.org 🤷‍♂️

I'm definitely interested in helping maintain this library. It's truly the only thing that comes close to doing what it does and with some TLC, I think it still has a lot to bring to the table.

@LoneRifle
Copy link
Collaborator Author

@auth0's fork is dated, about 200+ commits behind this one. Perhaps the way we can move forward now is as follows:

@cjbarth
Copy link
Contributor

cjbarth commented Jan 16, 2023

Do we think it worth reaching out to someone at @auth0 and seeing what their thoughts are? I perceive that their idea of a fork is to freeze the code at what they need and manage security fixes themselves, but without checking with someone, I don't know.

I find it strange that the SAML package by @auth0 uses both xml-crypto and xml-encryption, just like so many other auth packages. It really would be ideal if all that code could be collected together (I'm not in a position to volunteer).

If what the majority decides is to move it over to @node-saml, so be it. I'd like @markstos opinion on that though.

@markstos
Copy link

I believe I asked Auth0 about maintaining passport-saml in the past and they weren't interested. I doubt that will change for this library.

I agree that moving to some organization model is a good idea. Within an organization, each project beneath it can have a unique manager and maintainer. I think there's some value in having it in the same organization with node-saml because they are closely related. The move doesn't necessarily have to imply that the node-saml organization owners will spend more time maintaining the module, though.

So while it's better than having the module under an inactive personal account, it doesn't automatically solve the problem of improving maintenance.

Also, moving the project to the node-saml organization now doesn't preclude moving to its own organization later.

@cjbarth
Copy link
Contributor

cjbarth commented Jan 17, 2023

In that case, @markstos would you be in a position to facilitate moving xml-crypto, and xml-encryption, if they are interested in to the @node-saml organization? That would bring our dependencies into a state where it is easier for us to maintain.

It would be nice if one of the first orders of business for xml-crypto would be to get it working with release-it to make releasing a 1 minutes process as it currently is with node-saml and passport-saml.

@LoneRifle
Copy link
Collaborator Author

@cjbarth I think only @yaronn can initiate the transfer for this repo as its owner. Happy to work into release-it; this effort can run independent of the transfer, I feel

@markstos
Copy link

Anyone could copy the code over, but only the owner can approve an official transfer that would bring along the issue and pull request history, so it would be best to wait a bit and see if that can happen.

@yaronn
Copy link
Contributor

yaronn commented Jan 18, 2023

I tried to move ownership to node-saml but I get an error "You don’t have the permission to create public repositories on node-saml". @cjbarth should I transfer it to you and you will move it there? Or else can I get permissions to create a repo in node-saml?

@cjbarth
Copy link
Contributor

cjbarth commented Jan 18, 2023

I invited you @yaronn to be an organization member of @node-saml. LMK if that doesn't get you what you need.

@yaronn
Copy link
Contributor

yaronn commented Jan 19, 2023

Done! This repository is now part of the node-saml organization. Thanks to everyone that contributed to it over the years! I agree node-saml is the best long term home for xml-crypto.

@LoneRifle
Copy link
Collaborator Author

Thanks everyone! I'll go ahead and mark this as closed. There's still one more item to be done: for @cjbarth to invite @djaqua to this repo or to @node-saml, but I'm sure this will happen in due course.

Here's looking forward to more active work on this!

@cjbarth
Copy link
Contributor

cjbarth commented Jan 19, 2023

This invitation has been sent.

@djaqua
Copy link
Contributor

djaqua commented Jan 20, 2023

Cool - I just accepted the invitation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants