Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I'm currently porting this module to Drupal 8. I've taken my starting point as here: https://github.com/grasmash/simplesamlphp_auth/tree/7.x-3.x and work is being completed here: https://github.com/typhonius/simplesamlphp_auth/tree/8.x-1.x
Comment | File | Size | Author |
---|---|---|---|
#24 | SimplesamlphpDrupalAuth.php-24.patch | 909 bytes | iamjdcollins |
#23 | SimplesamlphpDrupalAuth.php_.patch | 5.28 KB | iamjdcollins |
#16 | simplesamlphp_auth-2367389-16.patch | 12.88 KB | eyilmaz |
#15 | simplesamlphp_auth-2367389-15.patch | 12.91 KB | eyilmaz |
#10 | simplesamlphp_auth-2367389-10.patch | 35.47 KB | eyilmaz |
Comments
Comment #1
snufkin CreditAttribution: snufkin commentedHey Adam, could you instead of working of Matts fork work off the 7.x-3.x-dev version hosted here? This one should be newer than the github one.
Comment #2
adammaloneBalazs, yep no problem. I've pulled in a bunch of the updates made to the d.o branch and modified for D8 currently.
Comment #3
adammaloneAnd here we go. This will likely need updates and changes but it's a good stake in the sand. This patch applies cleanly against the current 7.x-3.x branch of the module hosted on drupal.org.
Comment #4
adammaloneUrgh, small change to the patch. Ignore the one in #3.
Comment #5
adammaloneAdded some more documentation and fixed some things missed in #4.
Comment #6
adammaloneUnassigning myself for review.
Comment #7
kim.pepperHi @typhonius. Great start!
There are probably a few API changes since this patch, I didn't try and run it locally.
I've got a few suggestions on how we could make it more manageable. The manager class is quite big, and deals with quite a few different things. I would recommend trying to break this out into interfaces that deal with one thing only. Also, adding unit tests would make this much more robust.
Can we have an interface for this class?
Can we give this a more meaningful name (e.g $authSource)?
Can we have a 'Storage' interface to hide the SQL part and make it easier for others to use a different storage class?
If we make some changes to avoid calling global static functions, we can unit test this method.
We should be using entity query for this.
This could do with some unit tests too.
Comment #8
adammalone@Kim Pepper - these all sound reasonable. Let's chat at DrupalSouth about some of latter points so I can gain clarity!
Comment #9
snufkin CreditAttribution: snufkin commentedThe initial D8 version is now committed to the 8.x-3.x branch. Thanks for the great work!
Comment #10
eyilmazDeleted unused, Drupal 7 - related files.
+ some changes regarding changed D8 API.
Comment #12
bradjones1Might it be a good idea to publish an 8.x-3.x-dev branch so people know this is as far along as it is, and encourage more development?
That would also open up an 8.x issue category so we can segregate out D8 issues from D7.
And... I forked this to github and created a working Single Log Off implementation... see:
https://github.com/bradjones1/simplesamlphp_auth/commit/233cb9f0a89c1b07...
I'd have opened up a separate ticket for that, but... no 8.x issue category :-)
Thanks for all your work on this... you saved me a ton of time!
Comment #13
snufkin CreditAttribution: snufkin commentedOh, I thought I've opened that release up... I've done that now, if you could, can you open that issue? Thanks.
Comment #14
snufkin CreditAttribution: snufkin commentedComment #15
eyilmazmake synchronization of username and email configurable
bring back api functions (don't know if they are really needed.. but just for backwards compability)..
fixed one bug were select query was not executed
1 api function is still missing..
Comment #16
eyilmazpatch 15, with new line at the end of config/install/simplesamlphp_auth.settings.yml
Comment #17
snufkin CreditAttribution: snufkin commentedSorry to be a nuisance, but could you open up a new issue for this? It would be easier for me to track the task separately, and to give you credits for the commit as well. I'd like to close this particular issue as the initial branch is now open.
Comment #18
eyilmazSure, I will open a new issue..:)
Comment #19
cweagansNote that there is a working SAML authentication module at https://www.drupal.org/project/samlauth if anyone needs this immediately. It doesn't do the rest of what simplesamlphp offers, but SAML works OOTB.
Comment #20
snufkin CreditAttribution: snufkin commentedThanks, I've updated the project page to point to the module.
Comment #21
svendecabooterThe first alpha version of the module has been released.
Feedback & testing welcome - please open specific issues to the 8.x-3.x version in the queue.
Comment #23
iamjdcollins CreditAttribution: iamjdcollins as a volunteer commentedI am submitting a patch for your consideration. The app may work as designed but I am making a few suggestions that may be more flexible and better represent the outline in the interface. The focus of this was make the option to register users and link them to existing accounts separate options. They are independent of each other from the interface perspective. As the code is now the option to link does nothing if you are not allowing the module register accounts. In my case I have a different method to get my users on the site. I actually prefer to have them precreated because the accounts may have meaning even if the person never logs in such as a directory. During the creation they do not automatically get enabled for SAML login even though I would like them to be able to login and get roles if needed. Originally I though the option to automatically link would do this but after working with the interface and code I found it would not (and still maintain an alternate method of populating users). All the code changes are in the externalRegister function.
1. First determine if the user exists through both the authname or external modules. Allows a little deduplication.
2. Have an if statement that if the user exists run things related to linking and if not run things related to creating.
3. Only force true on creation not linking. synchronizeUserAttributes($account, TRUE);
4. After running either link or create paths as a final step execute userLoginFinalize if either path produced an $account variable.
Comment #24
iamjdcollins CreditAttribution: iamjdcollins as a volunteer commentedI also updated the function externalLoginRegister in my second patch to remove roles if we are reevaluating each login. This patch was created off the original code so it could be considered separate.
Comment #25
bradjones1@iamjdcollins - Closed tickets aren't the place to propose changes to code. Open a new ticket. Also, with a fixed status, your patch is likely to languish without getting anyone's attention, since it's not "Needs Review."