Suport mapping multi-value fields

Multi-value fields are easily supportable in principle, as you indicate; it just needs some thought and testing. And you are the first one to bring it up in all these years, so that thought/testing hasn't been invested yet.

The challenge for generic support is two fold:

1. On the 'source' side (the SAML message), we get all values as arrays, so there is no way to distinguish between a single value and an array containing a single value. (Feel free to set me straight, if e.g. the attribute data in your SAML responses is somehow marked as "an actual array".)

2. On the 'destination' side, not all fields actually support multi-value. (Case in point: user name, email, language.) And some may be configured/defined to be only single-value, even though the Drupal data model supports multiple. We don't want to just set a multi-value array into these fields, at least not without testing / logging a message.

You're welcome to contribute or sponsor adding this functionality. Even a only-partly-generic patch or extra subscriber helps get it into the general module in time. Failing that: it should be fairly easy to roll your own event subscriber with just a few lines of actual logic. Something like

$new = $event->getAttributes()[YOURATTRIBUTE];
$old = $event->getAccount()->get(YOURFIELD);
if ($new != $old) {
  $event->getAccount()->set(YOURFIELD, $new);
  $event->markAccountChanged();
}

I would also like clarity on why samlauth_user_fields does user linking as well as syncing.

I'm not sure whether you're talking about the boolean map_users config value (which didn't initially exist at all / had its reasons for being introduced, but I'm not sure that's relevant here). Bottom line is: linking is optional.
samlauth_user_fields.field_mapping is a per-field mapping, and no field is linked unless you explicitly set the optional 'link_user_order' sub-value for the specific field.

(From just reviewing the config schema, I'm not sure from the top of my head if saving the samlauth_user_fields.field_mapping value always saves a 'link_user_order' with a default value of 0. It wasn't supposed to. If it does: that's a bug. If it doesn't: I don't see any issue with this.)

Unfortunately, `UserFieldsEventSubscriber` has proven to be obtuse and IMHO perhaps overengineered. So I'll most likely be writing a one-off data sync using the SamlauthEvents::USER_SYNC event for my purposes.

That's fair. If there are any others who have needed this multi-value functionality, they have apparently also done that.

This is true, but isn't it possible to know if the destination supports multiple values?

Very possibly yes. With some hardcoding for field types probably, but that's OK. That is why I said [anyone is] welcome to contribute or sponsor adding this functionality.

So this issue stays open for that.

Despite the "obtuse and perhaps overengineered" comment, I do think it is clear enough where to add code for this.

I've added a nots to the bottom of the project README.

@roderik, the SAML attributes that my institution's IdP returns includes a multi-value attribute that was mapped to a multi-value user field when we used the simplesamlphp_auth module, via the simplesamlphp_custom_attributes module. One of our website maintainers who recently migrated to samlauth noticed the missing data and came upon this issue thread.

I have used Claude Code to help me develop an update for the samlauth module that successfully handles multi-value mapping. I have done a code review and testing, as well as patching a production site running samlauth 3.14, and I have found no problems with the update.

Do you accept AI-developed code in an MR, with clear indication that it was developed with the assistance of AI? If so, I will submit it and be ready to answer questions and make changes, if required. Thanks much.

@kfolsom I am curious to see the MR. I still don't have much experience reviewing AI code, but in principle I'd like to include it. if I can understand it / vouch for it, it can go in. (Which has been the case with the few Claude-assisted MRs that I've tried to read so far.)

Final judgment comes after review. I really want a module like this one (auth related) to keep containing human readable code. On the other hand, if this is going to be a separate (perhaps separately enable-able) subscriber, it may not be a big deal.

Final judgment comes after review. I really want a module like this one (auth related) to keep containing human readable code. On the other hand, if this is going to be a separate (perhaps separately enable-able) subscriber, it may not be a big deal.

Currently, this is not a separate subscriber but modifications made to the 'UserFieldsEventSubscriber'. If you believe that enabling this for everyone is problematic, it could be made an optional feature. One of the consequences for those who already have multi-value user fields mapped to multi-value SAML attributes would be that those fields will be updated with all attribute values, replacing the single value already there.

I'll submit the MR and we can go from there. Thanks much.

Reviewed. All good. Needs Work for Q/A 1.

Added a note on AI to the project page.

Apart from that, I'm going to take a closer look at the linking. I suspect the following remark may not be true:

Only the first value is used when [...] linking users (the "Use for linking" option).

I'm planning to

• First squash the giant @todo in onUserLink() in a separate issue, which I suspect I should have done 2 years ago. Should not cause merge conflicts.
• Then poke at it (write tests)... and hold up merging this MR/issue, until the point that I'm confident that I'm not going to want to change its multi-value related behavior with another config option. (After that, this can be merged and those tests can be added in another issue.)

FWIW:

Apart from that, I'm going to take a closer look at the linking. I suspect the following remark may not be true:

Of course it's true. (I didn't fully read back my own ~6-year-old code, which I never used / noone else commented on.)

EDIT -
Like I said, I'm going to think about how I might evolve the linking behavior in the multi-value case... if there is a logical case to be made for it. The only reason why I might bolt it onto this PR, is because of the config option that it will introduce. If the linking behavior can/should change, based on the same option, I might commit it at the same time.

But it's not your problem.

Thanks for the thorough review, @roderik. I will work on Q/A 1 and update the MR.

Hi, @roderik. I've updated the MR. The opt-in has been added: a per-mapping "Synchronize all values" option, on by default for new mappings and off for existing ones (no update hook needed). I also made the comparison ignore value order, since our IdP sends the same values in a different order between logins; duplicates are kept as agreed. The branch is rebased on current 8.x-3.x and I updated the expected debug message in the new UserFieldsLinkTest, because this MR rewords that message. The pipeline is green. MR description updated with details and two small questions. Setting to Needs review.

Status: Needs work for 1 requested code change + needs review+maybe-work for 1 proposal.

On me:

• I'm still going to add the change to the linking behavior (+ extend my recently-added tests) when multivalue=on. It isn't strictly necessary, but IMO will be more intuitive if it follows the new areInputValuesEqual() logic (ex. cardinality differences) that is also used for syncing.
• I reserve the right to do non-invasive last minute changes to logs/warnings/UI/changelog text, before merging. Because I haven't checked all in detail. Also: made a note-to-self to update the issue text, so that it's useful if someone gets here from the CHANGELOG.md link.
• (Later: I'll un-indent / switch some of my own 6-year-old code in the big loop in the event subscriber. Separately from this MR, because I'm sure it'll get less readable.)

Thanks for the feedback, @roderik. I've made the changes you suggested, described below.

Done: areInputValuesEqual() now receives the untruncated values plus the field's cardinality, and array_slice() happens only when writing. A field holding as many values as it can, all of which are among the attribute values, is now left unchanged (your [1,2,3] → [2,3] example, cardinality 2), so it no longer oscillates when the IdP reorders values. The debug log stays where it was.

Note: I read "should return FALSE" as "should not be updated", so the comparison returns TRUE ("equal") in that case. Correct me if you meant otherwise. I added an optional $cardinality parameter because the field name alone doesn't tell full from incomplete (e.g. [2,3] is complete for cardinality 2 but not 3). Added 7 test cases.

I'm leaving the option-off (single-value) behavior as is, per your comment.

Thank you. One line of feedback, but I can implement that.

I'll move the README section somewhere else, because I don't think this is "incomplete functionality" anymore.

I happen to need the new multi-value-linking code for something else... so... if I manage to finish that code with good test coverage, I'll merge this issue with that code committed to the same MR.

Thanks again for seeing this through.

Merged, so I can build on my linking code for something else (which may be included before a next release). Feedback still welcome, but I'll set Fixed.

Thank you for your feedback on this MR and continuing support for the samlauth package.