When visiting the SAML Login configuration page /admin/config/people/saml_sp/login PHP throws a Out of Memory error.
This error is caused by a wild User::loadMultiple() which attempts to load all users in the system to provide a selection of users; this works fine for small sites, but when you have hundreds or more this fails an in genera it's not a encouraged approach.
/admin/config/people/saml_sp/loginThe simple solution is to use the entity_autocomplete field type, this provides a unfiltered list of users using an autocomplete functionality; it's not perfect but better than the existing solution.
The better and more advanced alternative is to provide a route which will be used as autocomplete provider.
N/A
The list of checkboxes is changed to be a text field with autocomplete which still allows selecting multiple users by using "tag style" input.
N/A
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | saml_sp_login--not-load-all--3193507-2.patch | 3.95 KB | esolitos |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #2
esolitosComment #3
jrglasgow commentedplease create an issue fork with a merge request
Comment #4
jproctorI used Devel Generate to add 10000 users to my dev instance (Drupal 8.9.13, SAML SP 4.0.1 running in the Pantheon recipe in Lando on my MacBook) and could not replicate the out of memory error. It wasn’t even significantly slower, even when I added the
administer userspermission to a role I’d put about 500 of them in.That said, I agree the code doesn’t scale well, so I rerolled your patch against 4.0.1 and tried it.
It came up with Anonymous user as the default. It was misreading my current config, which is an array with uids for keys (all users that were eligible to be selected the last time the form was saved) and value 0 for false or uid for true.
When I removed that and saved (without selecting anyone else), there was an error with
array_walktrying to operate on an empty array.When I got those worked out and tried again, I noticed that Anonymous user was still included, and the autocomplete experience was better than checkboxes, but still not great with 10000 users in it.
entity_autocompleteprovides a filter mechanism thatUser::LoadMultiple()does not, so I added the rules to exclude Anonymous and only select users in roles withadminister userspermission.I am setting up an issue fork with that work, but no MR yet.
Additional work required
I noticed the new code changes the config schema, so my initial concern was that sending mail would not have worked as intended — we’d need an update hook to fix the schema, then undo my code that reads the previous config and change the code that gets the selected users’ email addresses.
As I dug deeper, though, I cannot find where this configuration is used at all. Part of the changes in #3096499: Account request overhaul switched us to use the standard user registration form, which is governed by the Drupal’s Account settings, not this module.
I strongly suspect this config option, as well as “Send request to site mail account” (
account_request_site_mail) and thesaml_sp_drupal_login_mail()function insaml_sp_drupal_login.moduleare all irrelevant, and we either need to remove all this code or inject a change into the way user registration requests are sent by the system.I favor the former, and have spun that up as issue #3206118: Remove unused config options.
Comment #5
jproctorThis code was removed by #3206118: Remove unused config options.