8.x-2.x Development

Hi droath,

The more the merrier, I would guess!

cweagans is super busy previous/this month. As a result of communication in #2753607-4: Redo SamlUserService, I've been trying to get a 'Works For Our Clients' version together that builds upon externalauth, in august. (because 8.x-1.3 is not an option.) cweagans has agreed to just push this to a 2.x-alpha0 version for the time being, after a quick review/approval. And he's not opposed to aggressive refactoring.

I don't have full co-maintainer rights / write rights to the repo. But maybe we can prepare some chunks that are slightly more bite-size than your current commit, and queue them up for final review by cweagans? (In whatever way you want. Commits on (y)our github repo(s), or patch files here... Code discussion on github or here... I think I'm in favor of at least creating d.o issues that at least generally mention what we're working on, even if the rest of the work is on github. Idunno, we can see.) Then we can discuss maintainership after.

So, question 1: do you have time to do some sparring together, wait for the maintainer and present him with some things to further review? (Or are you on a "my code needs to be ready and tagged before date X" deadline like I was? I hope you're not, because your changes are... considerable. Also I don't have heaps of time.)

I gave your code a look-over, and first impressions:

• more structured config/schema is always nice.
• - I'm going to have to have another look at the renaming of config values, because I'll be on the hook for writing update functions. (2.x-alpha0 is actually going to be used by various people.)
• SamlAuth and SamlAuthSettings look nice and clean
• - I'm not immediately 100% sure of the use of moving the user stuff into a new event / SamlAuthUserSubscriber. I'm sure it's nice, but - IMHO this would typically be a case for a separate issue on d.o with at least a few lines of reasoning which will be findable by everyone in the coming years.
• - also I'm not immedately sure that externalauth does all the 'linking of existing users' that I need, yet. (When I was rewriting the module, I thought it didn't - which made me keep some of the code inside SamlAuthService.)
• - in the same vein: does your new more-generic user attribute mapping (which is nice) provide for that linking of existing users by name/email? Not sure. (Then again maybe we shouldn't care about this but extend externalauth instead. That, however, would make me argue for only including your nice clean SamlAuth/SamlAuthUserSubscriber after a new version of externalauth is released.)

It looks like you just created your git repo quickly for the benefit of this d.o issue. (Maybe you have some more 'bite size' changes lying around?)

This brings me to question 2 and 3:

* Are you still working on things? Or done?

* I'm sure there are at least some separate changes you could split out into separate commits/issues. Can you do that please? (Or tell me if I'm wrong about that.) I'm sure you are more suitable than me to judge the dependencies / to see which things should come first.

And I know it sucks that you might have to revert parts of your code, just to be able to present a working module with only parts of your changes applied. But yeah... for an open source module that several people are working on, this is just too big of a change to review/include in one commit. I don't see a good way around that -- and that's also why I crafted some 'in between commits' on the way to what is now 2.0-alpha0, which showed some evolution / separated changes.

Also (paraphrasing cweagans): before an actual 2.0 comes out there will have to be more unit/functional tests included.

Hey, sorry for the slow follow up here. All of the above sounds great. roderik already has write access on d.o. I'm not opposed to adding droath, but I'd like to see more bite-sized commits for some of the changes (which look excellent).

As roderik noted, I'm very busy this month (getting married on the 24th) and I'll be completely offline between Oct 7 and Oct 22 for my honeymoon.

Feel free to run with whatever you think is appropriate. If you want feedback, feel free to ping me via my contact form. I may be a little slow to reply, but I'll definitely get back to you. Please don't tag a release for now - I want to review more fully before doing that.

@cweagans - I'm not sure I have write access to the repo; I tried a 'no-op push' once and d.o gave me an error. I may give you a quick ping in PM if necessary. But I'm not concerned about that now because if we're working on changes this big... it may be better to stick to github anyway. I don't want to completely overhaul the module and then push it while you're away. Or at least not tag it.

@droath:

I'm fine with Github PRs. (Minor thing: there's a manager at my first-line employer who pushes everyone to create d.o issues for any nontrivial contribution they make, so we can score attribution points ;-) But I can ignore him if necessary. Haven't decided yet.)

Let's take the starting position for now that "the two of us will have to convince the maintainer to accept whatever we come up with". We might be in a unique position at this moment, where we can get away with just including your huge change in one go - because we're making big changes anyway. If one concern is cleaned up... and given that you're willing to write tests... IMHO there is a case for just taking your branch and letting you spend time on writing tests which ensure that the module works, rather than spending that time on breaking up the changes in parts, and then discussing them.

The one concern I have is this:

The current module has a feature where if an already-existing Drupal account has the same e-mail address as the SAML user that is being logged in for the first time... the existing account is linked to that user, rather than a new account being created. And I am soon (going to have clients nagging me about) extending that to also include "if an existing account has the same user name as the SAML user, link that account". Just like simplesamlphp_auth module does too.

My question is: does your module still have that 'link existing accounts' functionality? (And if so: is the 'link by name' also possible?) I couldn't see it from trying to parse your code changes in my brain only.

If that is the case: never mind the next paragraph. I'm OK with working on github / using your branch as the main one and doing PRs against it for anything I'd want to change. We can retitle this issue to "8.x-2.x branch" and refer to your repo. I'm personally busy / will do almost nothing until the 18th/19th, but if you want to discuss some strategy / want reassurance that I'm behind this, we can do a hangout. And after the 18th feel free to kick my ass repeatedly about working with you. (I will have to write that config-values migration at the very least.)

If that is not the case: we're going to have to have a discussion about that because this is a serious regression which IMO should block the adoption of your code until it is resolved. We have several ways of addressing this I think, but they're either time consuming or will 'break' the clean-ness of your code. If we need to do a hangout about that, let me know. I'm in the EU timezone so not your evenings.

Thanks! I hope to get to some serious manual-testing by the next weekend, so I can either have feedback or recommend this to customers / write the upgrade path from alpha0.

I've added some patches for the 2.x dev branch in the issue queue. What is the plan for the next alpha/beta? Is the development switching to github? What is the main place where the development is happening at the moment? If the dev version in the project page is not the last version, could we fix this?

Please let me know, I have time to work on the module/test it in a project I'm currently working on! If needed I can update my patches to work with the latest version.

Thanks for your message and the helpful attitude / offer for cooperation. It seems there's a nice party of people who want to work on this now; we just need to get things coordinated better.

In that process, I'm the delay at the moment. Sorry for not responding sooner; I haven't had time to get to this yet (and have not talked to cweagans since the messages in this thread).

Since my upcoming weekend is longer, I will likely get to reviewing/testing droath's branch then. (Have the vague feeling that I will mostly say "this is awesome" but will have some absolute requirements, before an alpha1 can be rolled. For which I'll need to write the proposed patches myself, plus an upgrade path.)

After that, we can
* roll an alpha 1
* have a clear 'authorative version' of the module again / have everything in the drupal.org repo
* decide on the development model, going forward
* find out what we need to do remove any bottlenecks (like unresponsive co-maintainers) if still necessary.

If you have time right now and could port your patches to droath's latest, that would be great. I can't promise 100% that this will be the authorative version without having reviewed it and pinged cweagans to review it, but it seems very likely. (You can either post patches here or PRs against droath's branch if necessary. I'll figure it out by saturday/sunday/monday.)

Thanks for your reply! I have time in the coming weeks so I will wait untill monday. If you need any help let me know, I will check out droath's branch in the mean time as well.

Just installed droath's version. Everything seems to work well, the settings and options look relatively easy to migrate. There are also some nice extra features and tests, so I'm all for using that version.

Some thoughts:

• Not sure if the settings belong under webservices, admin/config/people might be more logical
• Missing the password service
• Redirects after login/logout can only be routes, maybe paths/URLs would be better
• Not sure what the Account Username token field is supposed to do?
• The user linking fieldset should contain a better explanation that you can select the field for linking a user on the mapping tab.
• I like the idea of having an event for the user stuff, it allows a lot more flexibility.

Some extra improvements (for some there are already issues in the issue queue):

• Maybe a extra event could be added after doing the loginRegister where we pass the $account and $attributes to let modules do more complex field mappings? The functions assignAccountRoles and refreshAuthAttributes could be event subscribers for this event and provide nice examples.
• Create a README.txt
• Allow custom cert folder instead of having the private/public key in the database
• Allow debugging

What is the best way forward? Add droath's version as 8.x-2.x-dev and start working from there? Doing work on d.o. and github is a bit confusing? It might help people spot the new version and help test it? The migration path seems to most import task, maybe that should be done first. I probably have time to do it if you agree.

Awesome. I say go ahead with the migration path whenever you want. I'm behind you (still finishing up work from this week), so the first thing I will do this weekend is look at droath's source code and verify your thoughts.

Let's work from droath's version; best would probably to push your work to a temporary github repo for the time being . When all of us are synced up, this should indeed become the 8.x-2.x version and I'll ask cweagans to step in. (Last time I didn't have write access for the d.o repo yet but I think that's a little oversight -- which I didn't mind because I haven't done any significant work on it after my patches from a few months ago.) Then cweagans can tag a new version which we can direct people to.

I do not, though, want to *force* anyone on github. If they want to keep working in the d.o issue queue that is good. Also, any time I have any need for discussion that needs to be documented, I'll open up an issue here on d.o.

And I do agree that the version discrepancy needs to be resolved very soon by pushing recent commits into the d.o repo.

I'm not there yet though. I've been trying to fit the new version source code into my head today (and am doing unimportant little code standard edits that one does during a review)... but not that fast. I'll probably be ready this monday.

I think we should try to push @droath version into 2.x branch here. If we opt for developing forward in github, it needs to be stated really clearly in the module page and the issue tracker closed to avoid confusion.

I can help with some bits once I know where to put the patches in :)

I created a pull request to port existing config to the new way things are saved. https://github.com/droath/samlauth/pull/7
I added the 'change password service' field to not lose any settings, but I haven't added the routing for this yet. That's a seperate request I think?

I think it's safe to update now. Could we get this committed on github and add the latest version into 2.x branch on d.o.? After that we can figure out which changes/features are needed the most.

Sorry for being the delay here, guys. But since I got the co-maintainer job right before you showed up (which we can rectify later)... and since I have a vested interest in getting an upgradable/stable v.2.x series... and since I want to be able to stand behind what I tell the original maintainer of this module before I let him tag a new version... I want to do this properly.

Wrapping my head around the details of some of the code design of this completely new branch, is taking longer. I will continue in the evenings and have a list of questions ready a.s.a.p. (I'm thinking of opening a different issue for code related questions about the new architecture, so we can keep this for discussing the mechanics of the way forward to stable 2.x)

In the meantime I
* have pushed a 'master' branch to the drupal.org repository (which is completely not-ideal, I know, but I want to merge the current 8.x-2.x into it only when more details are clear to me)
* plan to do any work that comes with keeping track of the different versions, until I do this merge.
* will take #2670118 into it as soon as I'm done reviewing because @droath has agreed to the concept - thanks for the reroll @seanB

Hi all,

Well, this is a bit awkward...

There are some open questions in my mind about
* the basic design of the SamlAuth* class(es) and the new event;
* some settings, and the UI paradigm of the user field-mapping feature. (Which, combined with the existing-account-linking feature, is going to need some work before it's ready.)

I know 'alpha0' is alpha too, but I am currently unsure whether it's wise to switch to the droauth branch. I know incorporating the code piece by piece is going to be more work (which is why I initially didn't want to force @droath to do that), but I'm on the fence of doing that instead. Depends on the answers and your opinions.

Obviously I'll invest work in doing that (and credit droath in commits). I should have time this weekend again.

Opening a few other issues with questions; links coming up. (Edit: mainly #2824490: [droath branch] questions on design and #2824505: [droath branch] Entity ID type "url" + using the NameID attribute as unique ID.)

Doing the patches I found that the current alpha0 version of the module is indeed much simpler in it's approach. The main reason for switching to droath's version for me was:

• Mapping fields to attributes
• Setting login/logout URLs
• Available tests

The field mapping would be a much wanted feature. Starting with a event to let custom modules do some custom changes to the user based on attributes would already solve this for me in the short term (and we should have this anyway).

The login/logout redirect URLs should be patched in both versions to allow paths in stead of routes. I wrote patches for both versions so there is no difference there.

Test should be added, but from a functional point of view this is not a priority for me.

Given

• there is clear buy-in from other contributors to the published 2.x branch
• droath has not replied after I finally got time to review the branch (I sent some messages outside this issue queue)
• functionality from his branch is in several stages of being ported

...this issue can be closed.

I'm still in the process of porting the mapping UI to the latest 8.x-2.x-dev version. I believe everything else has been handled.