existdb-saml 2.0.0 - Githubissues

eXist-db / existdb-saml

XQuery module that implements SAML v2 single sign-on

GNU Lesser General Public License v2.1

4 stars 3 forks source link

existdb-saml 2.0.0 #32

Open chakl opened 2 months ago

chakl commented 2 months ago

I am going to release version 2.0 of existdb-saml. This is a long pending maintenance release and also adds an important feature: multi-realm authentication.

Multi-realm authentication allows multiple apps within a single eXist-db instance to use SAML authentication, while allowing each app to define distinct permissions based on group membership. The 1.x versions of this code implicitly assumed there would only be a single app that uses SAML authentication.

The current 2.0-RC1 branch will be moved to main. Since this branch is based on commit 9c43d19, it predates and effectively removes changes to the master branch after Jun 21 2021.

I have not merged these earlier changes for various reasons:

they would not apply cleanly to the already prepared maintenance branch, and be even redundant to the work already done;
I disagreed with implementation details and created a more correct implementation;
they were occasionally plain wrong;
but there were also several good suggestions that I eventually implemented.

I did not authorize those changes to be merged, but unfortunately they were. I will drop them with the version 2.0 release that reimplements the useful bits.

I have stopped communicating with this individual and accepting patches after he was

raising "bugfix" issues on non-bugs;
insisting non-bugs are bugs;
claiming issues but not providing proof;
campaigning FUD (fear, uncertainty, doubt) with these claimed bugs.

adamretter commented 2 months ago

@chakl Please merge https://github.com/eXist-db/existdb-saml/pull/31 and https://github.com/eXist-db/existdb-saml/pull/30 first please

adamretter commented 2 months ago

I did not authorize those changes to be merged, but unfortunately they were. I will drop them with the version 2.0 release that reimplements the useful bits.

This is a community project. The changes were merged by a member of the community and not myself. That is the process we have for almost all eXist-db org projects. I think your response above is unhelpful. This is a collaborative project it is not owned by any one person. We should attempt to collaborate.

adamretter commented 2 months ago

campaigning FUD (fear, uncertainty, doubt) with these claimed bugs.

I have consistently pointed at the relevant parts of the specification which clearly state how it should work. You have not followed up on this. I am not spreading FUD, and I find that claim both misleading and offensive. I am simply following the standards, after all, isn't that the point of standards?

adamretter commented 2 months ago

The current 2.0-RC1 branch will be moved to main. Since this branch is based on commit 9c43d19, it predates and effectively removes changes to the master branch after Jun 21 2021.

Please do not do that. That is not a collaborative approach. @dizzzz Can you comment here please? You are instead throwing away community contributions that are in production at several commercial sites and have been tried and tested and proved.

adamretter commented 2 months ago

I would be very happy to have a proper discussion with you @chakl to collaborate on this (preferably on a teleconf).

chakl commented 2 months ago

I'll be in the community meeting today.

adamretter commented 2 months ago

I'll be in the community meeting today.

I appreciate the sentiment. Unfortunately that only would have given me ~45 minutes notice of that meeting.

I am afraid that it's not possible for me to attend the eXist-db community meetings on Monday evenings. However, I would be open to finding a date and time for a meeting (to specifically discuss this) that suits you, I, and any other interested parties...

chakl commented 1 month ago

@adamretter I believe this should be discussed in the community meetup. I would be available at one of the next 2 meetings (Jul 22, Jul 29). Please let me know when you come by.

adamretter commented 1 month ago

@chakl but as I wrote above:

I am afraid that it's not possible for me to attend the eXist-db community meetings on Monday evenings.

So that won't work! I would be happy to find a time that will work for us both, I think we should also invite @dizzzz and @joewiz as apart from you and me they are the only other contributors to this project, and as contributors they should have a say. We should also invite @marmoure as looking at the stats he has been reviewing PRs on this project too, so he has also been contributing.

I can try and coordinate scheduling a meeting in the calendar between us all - I'll send an email out to you and the others shortly...

sgmlguru commented 1 month ago

@chakl Please don't throw away/ignore the contributions by @adamretter. Those fixes are in use in a production system at my client's Azure AD site and are of vital importance to us. Surely it is in everyone's best interest to ensure that a 2.0.0 version remains useful, including every possible enhancement known?

chakl commented 1 month ago

@sgmlguru I am not throwing away / ignoring the useful contributions. In fact, the useful bits have been added to the new code. We are confident that 2.0 is sound and has been tested in production instances with Azure AD. But it's a major version, after all - and therefore may introduce breaking changes compared to 1.x.

chakl commented 1 month ago

@adamretter I really think this belongs into the community meeting, and I'll be happy to discuss this in one of the next community meetings with anyone who likes to attend.

adamretter commented 1 month ago

@chakl The Monday night community forum won't work, as I have written now twice above - I can't attend it:

I can only conclude that you either, (a) are not reading my messages above, or (b) see your continued insistence on choosing a time that I can't attend as a mechanism for excluding me.

I have sent out a Doodle poll to all stakeholders (including yourself) with a selection of dates, most people except yourself have now replied. I am making every effort to collaborate, and to find a date that works for all. Please respond to the poll with the dates that work for you so we can finally discuss, resolve, and move forward.

adamretter commented 1 month ago

the useful bits have been added to the new code

@chakl Those "useful bits" have already previously been merged to the master branch in a collaborative community manner by me sending PRs, someone else then reviewed them, and merged them. That's the Open Source collaborative GitHub style development approach!

I have looked at your 2.0-RC1 branch. First, you have only taken some of my work and erased the rest, and second, you have labelled my work as your own in that branch. There is not a single commit attributed to me in your branch. I assume that this is a mistake that you hand't noticed yet and not misappropriation.

I am not throwing away / ignoring the useful contributions

That's just not true, and the actual truth can be seen in your 2.0-RC1 branch for anyone who wishes to take a look. You have decided that some of my code is useful or not useful to yourself, you have not considered others in this community. As one person, rather than as a community effort you are removing parts of my code that was previously reviewed and merged in a community collaborative manner. Then the code of mine you have decided is useful (presumably to yourself), you have relabelled in the git history of your 2.0-RC1 branch as being both authored and committed by yourself and erased my contributions from the history in your branch.

I am very open to collaboration with you and receiving code contributions to this and other projects from you. So, why can't you just follow the same process as everyone else (in this project, and 90% of all other GitHub projects), i.e. send a Pull Request with your new changes, which the community can review, collaborate on, and then hopefully merge?

chakl commented 1 month ago

@adamretter

you have labelled my work as your own in that branch. There is not a single commit attributed to me in your branch. I assume that this is a mistake that you hand't noticed yet and not misappropriation.

You're right on that one, that's definitely an oversight that's going to be fixed. Please accept my apologies for that.

I still think this topic belongs in the community meetings. So if it's really impossible for you to attend, ok then, let's discuss it in an off-track meeting. @line-o will attend as well, as he's the managing the customer project that this code was originally created for back in 2017. This particular customer had commissioned an audit of this code at that time, which is why I was reluctant to make changes for quite a while.

Indeed, not all of your changes have been integrated into the new branch, at least not literally, because your diffs would not apply cleanly to the maintenance branch I was already working on. What are the important things you are missing? Certainly not code reformatting or replacing = by eq comparison operators I assume. Granted, I refused to make this a pure library module so far. We're currently reconsidering this one. I also don't think that switching the build system to maven is a reasonable move.

adamretter commented 2 weeks ago

let's discuss it in an off-track meeting

Yes, great. I have contacted you about dates. I am sure we can find a date that works for everyone :-)

created for back in 2017. This particular customer had commissioned an audit of this code at that time, which is why I was reluctant to make changes for quite a while

This is an Open Source project, it is not owned by one of your (or my) customers. You (or I) cannot expect everyone else to wait for such a thing.

Indeed, not all of your changes have been integrated into the new branch, at least not literally, because your diffs would not apply cleanly to the maintenance branch I was already working on

Okay, but that is the wrong direction of travel. You should be taking master and rebasing your changes onto that. That is how Open Source development works.

I also don't think that switching the build system to maven is a reasonable move.

I understand your opinion on that, but you and I are just two people with differing opinions, and this isn't a project owned by any one person; its larger than that. The PR to move the build to Maven hasn't been merged yet, so you don't need to be concerned with it just yet anyway. The developers/stakeholders involved in this project can vote on whether they want that merged or not. From my perspective, the advantage of moving to Maven is that we can easily add XQuery Unit Tests and then enable CI, which have been missing from this project; missing tests have caused problems to date as there is no real independent assurance that the code works or that regressions have not been introduced. I already have changes lined up here to do Integration Testing of this code via CI using BoxyHQ Mock SAML IdP that I want to send in to this project.

BernardDebord commented 2 weeks ago

My purpose is not to had fuel on the fire, but just to emphasis the remarquable work of @adamretter about existdb-saml library. The current code provided in the master branch is just NOT working, and without the various PR suggested by Adam it would have been impossible for me to use saml authentication with existdb:6.2.0 in my production context. So please, be carefull to ensure a usable master branch and above all, take into account the PR which fix the issues that are invariably appearing when writing code.