Move idmode to options

[shunkica]

Right now the constructor takes 2 params. An idmode (which can only be null or wssecurity) and the options object.

I do not see any reason for not moving the idmode parameter in the options object, which would clean up the interface, and avoid the uglynew SignedXml(null, { ... } ) syntax.

It is a breaking change, but the 4.0 release might be a good time to do it.

[cjbarth]

I'm completely on board with that. Feel free to put up a PR for that.

[shunkica]

If I'm going to do that, I think I would also add other properties to the options object. For example the privateKey and the publicCert.
But I don't want to create the PR right now because of your upcoming changes and the merge it will require.
Another thing I have noticed is that the types are currently broken. The utils as an interface doesn't make sense, those are not exported in index.js, also an interface can not have static members, the publicCert property is missing, and keyInfoProvider no longer exists.

[cjbarth]

I'm up for whatever you want to do in that PR. I would suggest de-structuring though.

As for the types, I'm working on that. As soon as #316 lands, I'll make a few more changes to get that all making sense again. Feel free to review the code yourself. I just want a second set of eyes on it; IMO the review doesn't have to be @LoneRifle (unless he disagrees).

[LoneRifle]

I think thus far I'm the only other member with write access in this repo. Should we add the others?

[cjbarth]

@LoneRifle , I trust @markstos quite a bit in these matters. I don't otherwise have strong opinions.

[markstos]

@LoneRifle There are four others besides you that have write access, then it looks @cjbarth and I have access by virtue of being "node-saml" organization owners. I won't name the accounts publicly, but we also have a private Slack server for internal organization chat (rarely used) that we could invite you to. For node-saml, we discussed having a policy about how commit access is granted and revoked. For example, if access is granted and there's not activity for six months or a year, access may revoked and may need to be re-applied for, to reduce our exposure to malicious activity from account takeovers. I have not looked, but it's possible xml-crypto has some inactivity contributor accounts who should have access disabled. If a prior contributor is trusted, it could be a simple process to re-enable their access in the future-- it's just safer not to have inactive accounts with commit access!

But to directly your address your point: I'm in favor of having multiple active committers to share the load, provide redundancy and reduce burnout. Because this is a security-sensitive project, vetting contributors has elevated importance. How we've done that for passport-saml might be something else we could discuss via Slack.

[LoneRifle]

@markstos @cjbarth - sure, I'll defer to you both

[shunkica]

#323