🇳🇱Netherlands @roderik

Dropping a reference for people ending up here through e.g. text search, or searching for the origin of the code change:

It seems (I haven't verified) that an underlying bugfix will be made in Core in 🐛 Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay class does not correspond to an entity type. Needs work , which makes this change unnecessary. So we can probably back out the code after we depend on 11.[something] minimum.

(Which would be good because, as stated in #4 / the code comments, our fix is not ideal: it relies on implementation details of ModuleInstaller.)

Per 2025, old documentation page (drupal.org node 425990) is replaced by: https://www.drupal.org/docs/getting-started/installing-drupal/using-a-lo... →

(Adding obligatory referenced issue, for people who get here by searching/finding this issue title. Though the original issue + comment #4+5 are not strictly related to that issue.)

Login (after coming back from the IdP) is done during a POST request to /saml/acs / SamlController::acs() This request / method

• is not supposed to build any content at all
• returns a RedirectResponse. After that, it is up to Drupal what happens - outside the scope of samlauth.

I am not saying that samlauth definitely cannot do anything about your issue... Just that it does not seem likely to me, at first sight. It doesn't seem likely to me, to be a race condition on unassigning/reassigning roles, because no blocks should be getting built during the request. Unless you are doing something on e.g. hook_user_login() that should probably be done somewhere else.

If you explore a hook_menu_local_tasks_alter() fix, please check during which request it gets executed. My guess is it should be the GET request to /user/ID, during which any trace of samlauth's actions has already vanished, the user should be newly loaded with its new roles, etc.

Given the forum issue you have linked, it's probably useful to debug this and report on it. I just don't have an idea on how we can help here in this module's queue.

Per #3506242-8: FileValidationEvent is not drop-in replacement for hook_file_validate → , I (just a passer-by trying to understand the related issues) doubt that ClamAV is doing anything wrong. It seems that the thing that is wrong is: the file object passed to the event holds wrong data, per your later comment #7 on that issue.

Implementing a subscriber on this event should be fine. That's what it's for.

Re, #17 - #19: The reason that File Hash module implements a different validator (in addition to implementing the event subscriber that extends the checks from FileValidator)... feels like it must be more specific. Like, apparently it needs more things to do its check on. (At a glance: its own validator has more public methods than FileValidator.)

I'm not exactly sure why hook_file_validate did work, that's strange -- but if my feeling is indeed correct, then it seems that somehow the file object as passed to hook_file_validate() had a correct/updated size, whereas the file object passed to FileValidator isn't. That is a regression and should be fixed, yes.

As just an interested reader, arriving here via the CR ( https://www.drupal.org/node/3363700 → ) -> 🐛 File upload stopped working after updating to 2.1.0 Active :

Looking at this change record - https://www.drupal.org/node/3363700 → - it recommends replacing hook_file_validate with FileValidationEvent. Is the change record wrong in this case?

Looks to me like the change record isn't wrong, and there may be a bit of terminology mix-up going on here. Maybe you are aware of this already, but then I'm just summarizing for other readers:

1)
The event is specifically for altering the behavior of FileValidator. That is, usually: add extra constraints to the validator.

ClamAV does that. And I don't see any indication that ClamAV should be doing anything else.

2)
Re #5: Yes, if the size in the file object passed to FileValidator (and then to the event) is wrong, then that's strange, and that is the part that should be fixed -- at least that's what it seems like to me. The code subscribing to the event (like ClamAV) can't help this.

This actually means (seems to me) that the title of this issue is wrong, and FileValidationEvent is fine. As long as it doesn't get a file object with wrong/outdated info.

And the related 🐛 FileValidationEvent may not report the correct file size Active (file resizing) feels like a plausible candidate for code that's buggy and should fix the file size in the file object. (I can't judge that without diving into that code.) So this issue can be closed "(works as designed)" (perhaps after 🐛 FileValidationEvent may not report the correct file size Active is fixed).

If this drush command can be contributed back (IMHO it's best done to the externalauth module)... I'll be happy to review it and make a case to the externalauth maintainer about why it should be included there.

(And if he doesn't want it: I'll add it to this module.)

For completeness:

I'll consider adding the above line to the module (first as an option to make sure existing sites don't break; then remove the option in v4.x because all sites should work with it). Because -- in short: the SAML PHP Toolkit library and Drupal use different ways of determining the hostname. That may be a problem. And I think we can fix it in a generic easy way.

However, this is the first ever report of such a case. So I'd like some confirmation before changing code, and the question from #2 is still outstanding (with followup questions planned, based on what the answer is).

Thank you! Applying without further testing, because the reasoning is good and I assume you've tested.

I don't know what happened with the @morvaim commit; I can't see any new commits in the forked repo. So I applied and pushed the patch.

@jfurnas thank you for the help.

The answer to the original question is: nothing happened with this feature request. Because there was no response in 5 years and noone else mentioned it until now.

In the meantime I still don't claim to be an expert on configuration best practices and can't tell you which exact helpers (config_* modules) are the most perfect for which circumstances... but I've seen a few deployment mechanisms for different environments in a few companies, and all of them need to do some kind of rewriting of configuration values. So adding the rewriting of this specific configuration value on top of the same mechanism, should not be a lot of overhead.

I can indeed see a case specifically for using [site:base-url] in the SP Entity ID, because it's so common to use an URL in the entity ID, and because we know the base url value really comes from the system (it's not dependent on any user-input/modifiable value)**. For the rest, I'm still wary of doing any token replacement. So I'd add a patch/MR that

• does a specific replacement of '[site:base-url]' in the SP entity ID value
• mentions this in the help/description text for the SP Entity ID input.

I don't want to apply the current patch, because I feel that it's good to be 'overly paranoid' for this module; to fear any kind of overwriting in configuration values that could somehow come from 'user input'. Allowing any token replacement feels too generic (for no proven purpose). And I still can't think of a proper use case other than the base URL.

(Also: Yes, Core replaces tokens in a (email) text which is saved as a configuration value. That's true. But I don't see an email text on the same level as other configuration values that govern how a site works.)

** Well, technically... until #2404601: Figure out how to best deal with RequestContext::setCompleteBaseUrl → is fixed, some other code could modify the value by calling setBaseUrl(). But code execution is not user input; if someone can manipulate code to do this, then your sites has bigger issues already.

@scampbell1 - That patch is not going to be applied, because it goes against the basic purpose of the authmap.

So... it seems like your new IdP has a completely new set of 'unique IDs' for people. That's fine, of course. In other words, it has a completely different 'authmap' (mappings of IdP-ID to Drupal-user that must be registered).

If your old IdP is out of service (as far as your Drupal site is concerned): you don't need the old mappings anymore. If you DELETE FROM authmap WHERE provider='samlauth', your issues are over. (And yes, I guess this needs documenting, but I'm not completely sure how, yet.)

If your old IdP is not out of service, you would need two different authmaps. But in that case, you would also need to have two different IdPs registered - which this module can't do yet. When ✨ Multiple IDP Support Needs work gets integrated, it should get support for separate 'authmaps' per IdP. (In other words: the provider='samlauth' field would need to be not-hardcoded to a single value.)

---

Only if someone gives a practical example of them having a need for two separate users from the same IdP needing to log in as the same Drupal user, will we start considering something like this patch. But I haven't heard of it yet, so I assume it won't come to that.

[ bogus comment: attempt at attribution change ]

(Unassigning because 9 months old)

Agreed. Note that intervals of 0 do not get saved in the database at all. (That's an empty/deleted value. And "no value" gets displayed as 0 in the form.) So, allowing negative numbers while not allowing 0... can IMHO be treated as an oversight. And if anyone has saved negative values, they should just reexamine their use case.

I think we can just forbid negative numbers in the UI, and not check the actual backend save. This means the diff is a single line.

Note #min = 0, not 1... because otherwise, if you open an edit screen with an empty field -> gets displayed as 0 -> press save without changing anything -> immediate error.

(I am not going to think about whether an empty field value should actually be displayed as empty, rather than 0. That seems like a separate issue, that might lead to e.g. tweaking the IntervalItem::getInterval() return value, and also I personally don't need it.)

roderik → made their first commit to this issue’s fork.

This module does nothing with the password field -- except it makes it invisible for some users that only log in through SAML.

If you add "authenticated user" (or one of your other roles) in SAML configuration value "Roles allowed to use Drupal login also when ...", then this module does literally nothing with the user/profile form.

I think you're looking at the wrong module.

As a first hack (if you're in a hurry) you can add the following line to SamlService::processLoginResponse() - e.g. on line 438 of SamlService.php:
SamlUtils::setSelfHost('[YOUR-EXTERNAL-HOSTNAME]');

In /admin/config/people/saml/saml ... what is the "Assertion Consumer Service" URL shown as? (The *.appserviceenvironment.com hostname of your externally-visible hostname?)

That'll inform where we need to go from here.

I think it's because [...] so the user doesn't exist yet so I can't assign them to a group.

I suspect that what you say here is true: you can't assign new accounts to a group. But that's on the Group module, or on you, to work around; it's not on the samlauth module to fix. (And also if the Group module does not specify this fact, then is that either a bug or an omission in their documentation? I don't know the module's details.) See below for suggested "fix/workaround".

This quote also suggests you have not really done point 2 of your "steps to reproduce" -- unless I am misreading you and you are implicitly confirming that samlauth's usersync is in fact being dispatched. I have no reason to believe it is not; see the link to samlauth_user_presave() that you point to. (Also: I don't understand point 3. But that's probably unimportant.)

So I'm guessing that 1 of 2 things is happening:

1. The groups module is not able to correctly assign to a new user account that is not saved yet (i.e. that has no ID and where isNfonew() is true).
2. Your code is somehow buggy. (I quickly looked at your event dispatcher, and I don't think this is the case, so I suspect point 1. I would think you need to call $event->markAccountChanged() if you want to be able to sync/change mappings for non-new accounts, but that's separate from the issue you're reporting.)

[...] I figure I'd ask here to see if there's any more context as to why we're using presave instead of login

1. I want to have the same event, where everyone can execute the exact same code, regardless whether an account is new. So no code duplication is necessary for new vs existing accounts.

2. On login, your new user account is already saved, outside the samlauth module's reach. That means the user account would get saved twice: just before login, and during/after login when you update account data from SAML attributes. If that was really necessary, I would live with that, but.... it also means that if, during USER_SYNC, something goes horribly wrong and/or your business logic decides it has to throw prevent login based on SAML attribute values... Drupal now has an already-saved user with half-baked data saved. This can in theory lead to heaps of crap in the users table.

I don't want to enable that possibility. (And it was a legitimate concern with the application I used to work on.) Noone's convinced me so far, to compromise on this point.

(Using the user_presave() hook for this, is very sub-ideal and makes the samlauth code into unfortunate spaghetti. The solution for this will be reorganizing the code / order of events in the underlying externalauth module, so that I can deprecate the USER_SYNC event and use one of the externalauth events. But when that finally hopefully happens... it won't solve your issue. Because that will be dispatched before user save, for the above reason.)

What I think you should probably do in your case:

• If still needed: Check more thoroughly if you really cannot $group->addMember($account) on a new account, and have it saved.
• If that is the case, then yes you need another event. You should be able to use the ExternalAuthLoginEvent (or hook_user_login but the event probably fits better), and you can inject SamlService and check SamlService->getAttributes(). If that returns any values, you can assume a SAML login just got processed and do your thing. It may feel weirdly disconnected, but it should work without issues.
• (If you want to be pedantic about being sure not to do unnecessary double-saves for existing users: process your new users on ExternalAuthLoginEvent and your existing users on samlauth usersync. But I don't really know why you would.)

It is kind-of strange that ExternalAuthLoginEvent is not mentioned anywhere and I had no earlier conversation thread to refer to, but the topic hasn't come up yet in this way: you could be the first person that has asked here and that IMHO has a documented legitimate reason to use an event listener after the user is already saved. I may point to this thread from the first Q/A in the README.

Hi @liuyuanchao,

I am hesitant to implement this, because

1) This change would affect other parts of Drupal, outside 'external login'. I see the potential for confusion about "why can this specific user (who has nothing to do with SAML) not log in", situations where the 'per role' setting is not good for people, ...

2) It is possible to "associate [new accounts] with a SAML login", even before they actually logged in. Then the current setting gives the behavior that you want.

I will assume that:

• you are an administrator of the Drupal site, who pre-creates Drupal users that can log in, because the "Create users upon SAML login" is somehow is not good for your case.
• at this moment, you (can) also know the "unique ID" that this user has in the IdP.

At the same time you create the user, you should insert a record in the authmap table: uid = Drupal userid, authname = [ that unique ID ], provider = 'samlauth'.

There is no code or documented practice for this yet, and there should be. I will change this to a "task", for the community to create some code and/or examples how you did this. We'll then refer to the comments in this issue from (or copy it into) the samlauth README.md.

What I know is:

• The externalauth module has a 'migration destination' for the authmap table. So if you create a lot of users at the same time, maybe it is easy to import a mapping of [authname / unique ID] -> [name of the new Drupal user] using Migrate Tools. (I have no personal experience with this.)
• If you create single new Drupal accounts one by one, maybe there needs to be a new drush command e.g. drush externalauth:add-map <drupal-username> <authname> <provider>. It should be easy enough to create. I believe this is a good addition for the externalauth module. (A note: my current opinion is that it should NOT be possible to edit auth-mappings through the Drupal UI; there only is an already-existing UI to view and delete them. So I believe that just this drush command without any UI equivalent, is good.)

From what I can tell from reading the code, it's just the two abovementioned things. (Maybe the code has some other effects, but I can't think of any.)

1) There's no message if the config form is not set up with #config_target keys ("the new API"). I doubt Core will want to support that; the response will probably be "modernize your config form's code, and you will get this functionality".

2) It is perfectly allowed to override config entities' values using global $config, but (per last March) standard config entity forms (which one would base on EntityForm) don't warn, because the #config_target related code is in ConfigFormBase, not in a (config)EntityForm base class.

I imagine Core committers would be perfectly happy with committing functionality #2 if they got a merge request for it. I personally won't open a feature request without doing some work on it -- and it's not realistic to work on it for me in volunteer time. I imagine there's going to be some "move code into a trait and then reuse it in a ConfigEntityFormBase", but I couldn't understand enough of the path forward in a few minutes, when I looked at it last March.

I would like to contrib it back in an MR, but I think the customizations I am making would break backwards compatibility.

Could you push your work to a new branch (with or without MR)?

There's no decision yet on a specific solution / set of code, because nothing was really reviewed yet. I admit to not looking at the other MRs yet.

(My work is volunteer time and without any set timeline, but sometime in the future....) I want to review

1. the use of config entities for the per-IDP configuration, and how we can make this optional. (I suspect this will end up in a separate add-on module, and before it gets merged into the main module there should probably be an 'update path' from the plain config... which shouldn't be hard to make. I'm curious whether y'all have made different choices for which config properties to 'diversify'.)
2. your config entities for 'login'. I don't have a mental picture yet of how this module should implement "the decision which IdP to use"; from the comments in this issue, I gather that Dylan has implemented a generic event for this, and your 'login entities' are the first 'complete' solution.

I think, before this module includes with multiple IdP support, it must include a ''complete' solution / 'point 2'. But if it is 'opinionated' / other people would want to implement it differently... then this (e.g. your) solution can become the 'reference implementation' in another optional module... that is e.g. implemented on top of Dylan's event.

After reviewing the code, I'll hopefully be able to form a mental image and a better opinion, and will comment on it here. Of course anyone else can do that too :-)

P.S. thanks for mentioning an actual non-dev use case for multiple IdPs.

Nice. I'll set "needs review" which is actually needs testing... and wait. Then when that's done, merge*... and include in a release later on.

* (For this specific issue, I'm not too concerned about the exact MR flow because the change is small and -in the current module state- does not need automated tests.)

Feature flag is possible, but I think I'm OK with just implementing it and publishing a change record. This feels like an edge case. It's possible that some people experience a behavior change, but the impact should be quite small. And "now actually redirecting to where the POST request from the IdP says", shouldn't cause real breakage / doesn't have security implications.

FWIW:

I just realized this patch, technically, constitutes a behavior change, which I should document.

Now, IF a RelayState is set in the IDP response / post request, then hook code can still override it by setting a 'destination' on the current request. After this change, that will be impossible. (If someone really wants to do this, they'll have to do more work == implement some event subscriber.)

In practice, this translates to; the DESTINATION in /saml/login?destination=DESTINATION will override any hook code that sets a 'destination' on the current request. (Whereas right now, it does not.)

I think we actually want that.

Thanks for the refference code, I could have a quick thought and get over my initial hesitation.

Does this work instead? See patch file (untested code, quickly hacked together). After login, I assume we still get to this code block and can remove the 'destination' that was set by the Dashboard module. I assume this code is still executed with Dashboard module installed.

Gives better separation of concerns / we can keep this in the Controller class, because

• I would rather only set the destination parameter as you suggested, if the SAML login was successful
• But that would mean this would need to be somewhere in the middle of the login code, in SamlService.

Thank you!

That was another mistake introduced while splitting the formerly-single-page settings form into 2 pages...

roderik → made their first commit to this issue’s fork.

I don't get the same behavior (unless when entering urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress as value). Also, "Email address" is not the default value.

So, offhand, I can think of two possibilities:

• The config value urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress is set through your settings.php somewhere
• The value you have been repeatedly trying to enter, was always urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress. This is the pretty-much-official-standard machine name for an email address, so that value shows as "Email address" in the dropdown (while being saved as urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress).

If it's neither of these: sorry, I don't know other questions to ask.

If it's the second point: I realize this is not explicitly spelled out in the UI, but I was kinda hoping it would be implicitly clear that the 'machine name' translates to the 'homan name' that shows up... to whoever tries it, so the values in the dropdown don't have to be super long...

I was not thinking this needs a change-record, since it will only influence behavior when no config is created, what I'd not expect anyone to do,

People are doing this by simply installing/enabling the module, unless that is done through a recipe that includes default config. So, an unknown amount of sites is going to be affected.

(OK. I'm not against merging, and this may well be an overall improvement. I'm just really conservative / hesitant changing behavior when the reason wasn't spelled out / when there is seemingly-unaddressed confusion.)

From https://git.drupalcode.org/project/custom_elements/-/commit/c27b1ff37bbe..., I see that you do not want to give your users help upgrading from v2.

Fine. Your decision.

But this change at the very least needs a change record.

Re #15:

In the beginning you only see "default" and it's obvious that is what will be applied to all of them.

You have alreaddy discovered that this is untrue. In the beginning you only see "default" and this is not what will always be applied, so it is not obvious. See of the example that I gave at the top of #11. Which is this way, as stated, to be v2-compatible.

I took a look at this. Adding message is weird, it's just a workaround to the underlying problem:

Yes, it is weird (and can be solved better than my initial PR). No, it is not a "workaround to a problem", it is an implementation of your own stated requirement that the behavior of custom_elements v3 should be equal to v2, if no CE display is present/saved in config.

I will repeat the example I gave in #11: if you have

• no CE displays (or v2 of the module installed)
• entity view displays enabled for 'default' and 'full' view modes;

Then the output will be based on the fields from the full display mode, not the default one. Per your own stated requirement of v2-compatible behavior, when no CE display exists.

So: MR!118 does not reflect a bugfix. It reflects a change in the specification.

Now...
If you retroactively think your previous specification is "weird", because it makes for worse UX / you do not want to complicate UX to match the spec... I have no firm opinion on changing the spec. But it needs to be done consciously.

I would really prefer that this was done in a dedicated ticket whose description mentions this change in behavior, and which discusses e.g. why this does ( / not) warrant a new major release, and the (new) need to explicitly create CE displays while upgrading from v2, if you want to keep the same behavior.

I'm changing to Needs Work to decide on a way forward, and if you really want to do this, to at least mention/acknowledge these things. If you prefer, I can have a look at changing the README section discussing upgrades.

To get "the sync" out of the way (which this issue is not about, so I guess we can close this off):

> On the sync, in my case, I guess the LOGIN event would work [...] that event doesn't have access to the SAML attributes.

OK. I've been holding off adding such an extra event, until someone gives me a practical case where they need the attributes AND truly need to have the user saved already.

---

> I don't really understand your feedback or the status you set.

TL/DR: I don't want to implement your patch and add a new event, for the reasons outlined / because IMHO the functionality fully overlaps with the existing event.

And I didn't want to set "won't fix" yet until you had an opportunity to respond.

> having to deal with edge cases around that. Exceptions IMHO shouldn't be used for code flow/logic that isn't actually an "exception".

1. The implementer of an event listener doesn't need to deal with edge cases. They currently have a single event to do all the things, which is IMHO simpler/clearer for them. They don't need to e.g. first do validation of attributes in one (your new) event, and then actually sync these attributes in another event.

2. "Exceptions IMHO shouldn't be used for code flow" is a new point (in this thread, for me). And I appreciate the argument.

But I'm still not planning to add the event to address that point. For a combination of these overlapping reasons: I find clarity on where/how to implement things more important than the alleged "far cleaner"-ness of the new event, for the remainder of v3.x. The new event doesn't actually add functionality. It's also not full functionality: there's no way to pass on a message right now. So, (and also because of above point 1) people will end up implementing a mix of the new event and USER_SYNC.

You're of course free to keep using your own patch for the remainder of samlauth v3.

--

In general, about this point 2 (if you care):

To address this point 'cleanly' implies deprecating UserVisibleException, AFAICT.

The world apparently agrees with you - judging by the fact that I have never seen things like UserVisibleExceptions implemented elsewhere.

But for situations (of which I've encountered several) where

• code that needs to stop execution because of an error
• and code that needs to decide what to do with the error (e.g. log or set message? Continue with some 'parent' processing or not?)

...are in completely different places, implemented by different modules... I so far haven't seen a good solution/implementation, that doesn't use exceptions.

The v4 rewrite that is happening in my daydreams** will move this logic into the externalauth module, with a different structure. I plan to ask you and some other people what you think about the structure before merging anything, and will consider above point 2.

** if everything goes perfectly, maybe this summer, but definitely don't hold your breath.

Thanks.

Years ago, I figured I'd introduce strict types in a v4.x. I'm still kind-of stubborn/lazy about it, but I admit that the code is starting to smell, with every year that v4.x gets postponed...

Status change for "please protest now (against not implementing the proposed new event) or ..."

Sorry, but I still either don't understand something or don't agree.

Trying to summarize the current state of things:

#8
>> the fact that a UserVisibleException [...] should be fixed.
> True, but I feel like that would be better resolved in a separate issue. Your call.
I fixed it in this issue, given 1) it isn't hard and 2) I doubt that more is needed. (See commit, if you care.)

#8
> I'm synchronizing group memberships based on attributes, but that's only possible on a saved user, so I need to postpone that part until the user is actually saved.

ExternalAuthEvents::LOGIN can be used for that (assuming you can do it after login). Yeah, I didn't document that properly in this module...

When I initially quickly read the above comment, I didn't respond immediately, because I mistakenly thought your planned patch would be related to that. Now I guess that's a separate issue.

I should have ideally responded earlier, because we're not on the same page yet re. the initial reported issue / your patch... Rewinding back to #3, now hopefully without misunderstandings:

> (...) That [using current USER_SYNC event] really seems like a last resort thing.

Is it, though?

With the committed fix, throwing a UserVisibleException during the USER_SYNC event is functionally nearly equivalent to the new event you're implementing - except the "possibility to add a message" already exists. They're dispatched at nearly identical times. The only things that are done between the new and existing hook (for new Drupal users only) are

1. Starting an SQL transaction (without actually sending any SQL commands yet)
2. Calling User::presave(), which contains some really non-problematic code
3. invoking hook_user_presave()

I agree that the fact that a UserVisibleException cancels an already-active SQL transaction feels icky and ugly. But it doesn't actually do anything dangerous. I don't (yet?) see enough reason to add a new event, just because it feels icky.

Now that I look at things again: I could reorder the user_presave hook implementation so that it runs almost-guaranteed first.

In my view, the eventual solution is for externalauth to invoke an event before the user is saved, and then deprecate samlauth's own event(s). By now, that's part of a larger plan for rewriting the module's logic (and moving some generic stuff from samlauth into externalauth). Although I admit that my desire to set aside volunteer time for this large-ish rewrite isn't super realistic...

Sounds plausible. It's kind-of the opposite of old issue #2848809: Login/logout fails with non-default language being active with the URL prefix negotiation → though.

I'll need to read it carefully again, and check that the proposed change doesn't reintroduce any bug / unwanted behavior.

It may well be possible that path_processing should be TRUE for /saml/login and /saml/logout, while staying FALSE for the other /saml/* routes. (In other words: that that old issue changed too much, without carefully thinking about this situation.)

(1AM typo in commit message. Commit is here.)
.
Thanks for the report. Yes, this is a bug... which I never consciously registered.

There's supposedly-always an exception handler active, which takes care of returning a (redirect) response to an error page. But the 'destination' parameter should always be removed in that case. In other words: the 'destination' wasn't removed early enough.

This makes DOMAIN/saml/login?destination=/:88/favicon.ico at DOMAIN/:88/favicon.ico, instead of the error page... which is a bug.

Bug fixed by removing the 'destination' properly/earlier. "The error page" by default is the homepage (with the standard message saying there was an error which has been logged). If that isn't good for your specific case, you can set an "Error redirect URL" in config.

(I briefly considered (additionally) checking/removing invalid 'destination' values, but that seems to be the RequestSanitizer::checkDestination()'s job, so I won't duplicate that.)

My previous comment was quite wrong.

The module is indeed, since v2.0.0-alpha2, unable to deal with relative paths as $return_to - because I introduced a UrlHelper::isValid(... TRUE) call. There is no documentation that says why, and the documentation of the $return_to parameter (which existed in v1, but I think it never worked at all) has not been updated to reflect this.

I can't remember whether or why I assumed that the RelayState should be an absolute URL. (According to the internet, the SAML spec says) it's an opaque value.

Because I don't like the fact that $return_to value worked differently than the "Redirect URL" config settings, I added the ability to be 'just a path' in addition to a URL. Some testing suggests this should be fine.

(Oh, if only we had functional tests for the actual login-IdP-ACS-redirect path...)

You're right. This shows how low-maintained this module is. 1.2.0 released now.

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

Thank you for the report. This feedback (confirms my thoughts from the previous comment and) makes me think about necessary next steps.

I'll close this issue though, because (though the last comment does correctly answer my previous comment) it's not clear from the initial issue description, what should be done here.

I've added a section to the bottom of the README that describes the issue and current situation / possible followup. If anyone does want to implement the followup, that wlll likely be better off as a new issue.

I'm setting this to postponed:

Still a good idea, and my last large comment is still relevant if anyone ever wants to implement this.

But I'm not going to experiment with it myself, so I also won't add things to the README myself yet. (People who are adventurous enough to look into this, can find this issue.)

Also... if I ever get to do the externalauth rewrite that I'm dreaming about... it would actually be nice to have support for "not logging a user in, but only starting a session" in externalauth. That makes the things to do in samlauth (per my last large comment) easier and less hacky.

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.

Thank you! This kind of 'maintenance from users' is exactly what this module needs.

(Per the giant comment above the code line that you changed: I think that actually, the option should not exist / should be "on with automatic fallback" in the PHP-SAML library, so that Microsoft IdPs work by default. But it seems I won't get to discussing that and also actually convincing the maintainers that this is true...)

roderik → made their first commit to this issue’s fork.

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.)

This seems like an oversight / wrong documentation on my part.

A relative path as RelayState parameter (as a request parameter attached to the POST request that also contains the SAML response) should work fine. **

So I think your IdP strips it (i.e. it does not propagate the RelayState to that POST request) if it's a relative path. Maybe most/all IdPs do that. (I should doublecheck SAML docs.)

I guess part of the reason this has not been reported before, is that the destination parameter (if one visits /saml/login?desination=PATH) is converted to an absolute URL, and not many people pass an argument to the $return_to parameter.

I should at least amend the function parameter docs/comments for $return_to.

---

** Unnecessary detail -- I was checking / wrote the below down, before my conclusion above:

• Is checked for 'being safe' in https://git.drupalcode.org/project/samlauth/-/blob/8.x-3.x/src/Controlle...
• Then is checked for access in https://git.drupalcode.org/project/samlauth/-/blob/8.x-3.x/src/Controlle...
• If the path is ignored in either case, there should be a watchdog warning.

I don't see any other code influencing the RelayState parameter.

I'm going to turn this into a reminder for me to, long-term, add something to the README and/or add a config option to skip user login (that might not be exposed in the UI).

It would be good to know if the current saveSamlSession() code is adequate for implementing such a use case.

We don't, however, map these users to individual Drupal accounts as there's no need (or desire) for the users to be registered locally

Oof! That makes things harder. I haven't heard yet, of a use case like this.

To summarize my previous response: samlauth intrinsically links "Idp authentication" to "Drupal user login". That's easier for the common use case.

But in your case,

• you need to peel those things apart again
• you likely need to store some (extra) status/data in a 'user session' that is separate from a logged-in Drupal user (which simpleSAMLphp did, and which samlauth doesn't do), in order to do this access check on those paths.

I think it's possible, though, to use just the "SAML authentication" part of this module and to remove the "Drupal user login" part, without making things too complicated. (I'm going to assume, for simplicity, that you never want to have a Drupal user login after someone returns from the IdP.)

• You'll need to overwrite the samlauth.saml service with a custom service that extends SamlService
• Your extending class can 'empty out' doLogin() (or change acs(), which is the only caller of doLogin(). But I assume leaving acs() is easier).
• This is assuming that you don't want anything registered in the authmap table. (Also, I am not going into the horribble piece of spaghetti that is the doLogin() execution path... because you won't need it / will remove it.)
• Then I advise experimenting with saveSamlSession():
  • There is a concept of a "Saml session", separate from the Drupal login. But ss you can see from the horribly large comments, it's an afterthought / code that has just stuck around since v1.0 and not (to my knowledge) been used well.
  • Decide what data you need, in an access check that you do on the other page loads.
  • Then store that data somewhere you can get to it. (getAttributes() is available here if you need it)
  • As you can see, currently the 'session data' is stored in PrivateTempStore (and you can just add your data next to it). I'm personally not convinced that this is better than just storing this data in $_SESSION -- because as soon as you store something in PrivateTempStore, you automatically/necessarily get a session anyway. I guess it depends on the nature of your data.

I.... think that works without further complications?

If you discover things that need to be patched along the way, I'm open to it. Although, until other people come along who want the same thing, I can imagine this is not going to turn into a configurable option to "skip user creation/login", but instead just a note in the README that this can be achieved with the above.

isAuthenticated() does not exist, because it addresses an issue that is specific to SimpleSAMLphp / does not exist with SamlAuth.

See the two images at https://www.drupal.org/docs/contributed-modules/saml-authentication/usin... → if necessary:

• with SimpleSAMLphp, SAML auth happens outside of Drupal. Then the user is redirected to Drupal, with a cookie set -- and Drupal still needs to figure out whether the at-that-moment-anonymous user is allowed to be logged in. (using isAuthenticated(), which -from my memory- checks the cookie.)
• with SamlAuth, SAML auth happens inside Drupal, during the POST request to /saml/acs. And the SamlAuth module immediately logs the user in, during the same POST request. Either the user is logged in, or the user is presented with a login error. There is no other 'vague in-between state' like with SimpleSAMLphp auth.

Is it possible that the array is empty due to the IdP configuration? Or, am I perhaps misunderstanding at which point the array is actually populated given the current state of the SAML authentication flow?

getAttributes() only returns data while processing the /saml/acs POST request, not on any other page load -- and only after SAML authentication has already happened and was successful.

Without knowing the specifics of what you are solving, I am guessing:

• you're trying to do something during some event very early in the page load -- which is not a concept that works anymore.
  • you should be doing the same thing immediately after successful SAML authentication. There's an event for that: SamlAuthEvents::USER_SYNC. See the comments there. See UserSyncEventSubscriber for an example implementation.

  Note on this event, SAML authentication has already happened and your user ($event->getAccount()) is going to be logged in (except if you or some other event subscriber throws an exception or something horrible happens while saving the user account). So you don't need to do an isAuthenticated() check.

  Or, alternatively, if you want to do something after authentication and Drupal user login has happened, and you cannot cancel it in any way anymore: check the ExternalAuthEvents::LOGIN event. SamlService->getAttributes() should work there always, if you need the attribute values (though the service isn't injected so it's slightly ugly).

  This means you need to do some rewrite work, for this different concept. But the result will probably be (conceptually) simpler, I'm guessing.

Mustapha did the tracing for this, and made a first change. I picked it up to check how exactly the change should be implemented (and to confuse myself on the way).

I've tried to improve the code so the block provides a proper CE response. When done so the block rendering should pick up the CE wrapped in the render-array. But it seems the views/block rendering pipeline wraps the render array somewhere, so it ends up markup instead of json still.

Yes, the block rendering does pick up the CE wrapped in some render array:

The call tree is:
1. LupusDecoupledBlockRenderer::getBlocks()
2. Drupal\views\Plugin\Block\ViewsBlock::build()
3. Drupal\views\Element\View::prerenderViewElement()
4. Drupal\views\ViewExecutable::executeDisplay()

4. returns the simple '#theme = custom_element' wrapper, and 2 + 3 add all the other stuff around it, to 'complete' the render array for a view.

(...) the block rendering should pick up the CE wrapped in the render-array.

I assume this means that you want to do a combination of two things:

A. pick the CE out of above ViewsBlock::build() structure;

B. set the CE instead of the rendered HTML, into $output[$region], in getBlocks(). (Until now, this is never done.)

About "B": OK, no further remarks.

re. "A": this feels to me like, at least in theory, 1) we could be throwing away part of the rendered output (produced by e.g. theme functions set by the block plugin; 2) we could be breaking compatibility by changing HTML output to something else. So I felt like I should put some thought into when exactly we are 'allowed' to do this:

• If the CE is the only render-element-child, like this specific example? Sure. We can throw away the theme-wrapper function.
• If there are several render-element-children, all CEs? Maybe, but...
• ...what if there are several render-element-children, some of which are not CEs? We can probably renderRoot() the non-CE render-element-children individually, and add them as drupal-markup CEs (like is mentioned in 📌 Improve CustomElement::createFromRenderArray() to better handle entites that render to custom elements Active )?
• ...and what if they're not all first-level children? Do we need to recurse, then?

I've also seen 📌 Improve CustomElement::createFromRenderArray() to better handle entites that render to custom elements Active , which proposes to do something like this inside CustomElement::createFromRenderArray(). But thinking about the above points just confused me.

So, in the end, I added only the first bullet point, directly into getBlocks(). That's what we need right now. And I can't justify the testing time for making this more generic / decide how 'backward compatible' the other cases should be... unless I'm explicitly told that we should address those cases now.

@fago see MR. Is this the code change you want? Or am I misinterpreting some things?

Might be fixed by https://git.drupalcode.org/project/config_override_warn/-/merge_requests... ( 📌 Automated Drupal 11 compatibility fixes for config_override_warn Needs review ), specifically the extra installEntitySchema() call. So apparently that fix is not only for D11?

Note - I didn't check, just leaving a quick note.

Thank you for the detailed report.

First - the less important thing, but important for myself:

Per "2)" in my response #9: I still have no clue why you're getting this ugly error and there is not an earlier 'real' error message about the name being empty. But you confirmed your configuration, so I have something to manually reproduce and test, some time.

This issue is "Needs Work" for at least that point: reproducing and testing, and then seeing whether/where I should add a real error ("Your user name is not being populated from SAML data, your 'User name attribute' setting is likely misconfigured" or something like that).

---

Now, to the details of your setup:

You only sent the <saml2:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified">correct@emailaddress.com</saml2:NameID> part of your response.

I am going to assume that your SAMLResponse contains no attributes at all. Attributes look something like (real example at https://www.samltool.com/generic_sso_res.php) :

      <saml:Attribute Name="uid">
        <saml:AttributeValue xsi:type="xs:string">test</saml:AttributeValue>
      </saml:Attribute>

Therefore I am also going to assume that you want to have Drupal users created whose name == the e-mail address == the NameID that you are getting back in the response.

If my assumption is correct, then this is a combination of
1. Your configuration is wrong
2. The module is not giving you any hints about how your configuration could be "fixed". So the UI is wrong/incomplete.

Per 1: If you indeed have no attributes in your response, then the configured "NameID" attribute (for user name) does not exist, and the configured "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified" attribute (for email) does not exist... because no atrributes exist at all.

Note again that "The NameID" and "SAML Attributes" are completely separate things. And (I assume from your comment) you only get the NameID, and zero SAML attributes, in your response.

Per 2: this module only recently implemented support for "NameID" (as opposed to an atrribute) for its "Unique ID" setting.

I failed to, at the same time, adjust the UI for the "User name attribute" and "Email attribute" fields. (And you're the first to semi-explicitly tell me that your responses have zero attributes.)

Solution: this 'trick' should work: set !nameid for the value of both "User name attribute" and "Email attribute" settings. Note: must be lowercase, starts with !.

This issue is also "Needs work" for fixing the UI to make that clear.

Maybe there's a prior question of whether this module is still needed?

Just for the people only reading this issue:

Yes, it's been established that this is still needed, in #3462541-4: Difference with what is now in core? → and further.

So a merge + release is appreciated.

This module can be useful until the mentioned render array attribute gets widely used in the contrib space.

Yes.
In addition: this module also shows overrides for config entities. (It's perfectly possible to override config entities' properties using $config in settings.php.)

We have somewhat simple entities, in a form that extends EntityForm, and this module correctly reports (I changed the names):

In the config file MODULE-NAME.ENTITY-TYPE.ENTITY-ID
- The value for PROPERTY-NAME has been overridden.

I don't see a plan to support this in Core. (And I don't know enough about overriding config entities, to know if changing the code to support this, would be simple. But at this moment, all the code is in ConfigFormBase, which != EntityForm.)

I came here from 🐛 Expected argument of type "array", "string" given Active and this MR inspired me to make a MR there. (The code is different: this one catches a crash in checkValues(); #3444730 catches a crash in validateValues().)

While I have not reproduced this... I think it you can probably argue that

• this does not need tests at all, because we can/should just assume that
  • checkConfigSchema() can throw any old exception
  • and checkValues() just needs to take care of those and never choke
• ...so all you need to do is check the phpcs error and then set to review without tests?

I admit that this in my validateValues() case, that is clearer to me than for this MR. (Because this calls checkConfigSchema(), which I don't know, whereas my case calls Typed Data stuff, which is "urgh, scary black box, we must be prepared to handle any exception that could throw".)

I was inspired by the recent 🐛 Crashes on leftovover themeconfig Active PR to look at the code again and could make some sense of it (because I now understand/think that I can ignore the impenetrable TypedData code). #3359846 does (almost) the same thing to different code, btw.

Proposed fix included in new MR. And...

• ...do we actually need tests (and steps to reproduce) here?
• Or can we just assume that
  • TypedData stuff can throw any old exception
  • and we just need to deal with it (i.e. catch it and turn it into a violation)?

I think it's the second. I think this just needs a try-catch + return error, and I don't need to bother about writing tests that cause any exception.

Manual testing done

• UI:
  • Before: fatal error
  • After: the row for the offending config item reports "1 error"
  • The actual error is reported in the list/detail screen. This does not change / did not suffer from the fatal error.
• drush config:inspect:
  • the fatal error is never reached, because inspector->validateValues() is called before inspector->checkValues(), which is (I think) therefore never called and doesn't get a chance to choke.
  • however, if it was called, this try/catch gives better output. (I tried. Before the fix, you only get the fatal error reported by drush.)

Reproducing the error (though I'm arguing it's unnecessary)

1) we have #19

2) Putting comments #12 and #20 in context:

If you have a firmatted text field, and you change the default value to

default_value:
  format: restricted_html

and import that: this will cause the fatal error in the issue title.

Random useless info

This is of course a completely bogus value: the actual default value would be:

default_value:
  -
    value: '<p>this is my default text</p>'
    format: restricted_html

...and Core does not allow an empty "value".

Our config did, however, have only a default format in the config, like:

default_value:
  -
    format: restricted_html

...which, I assume, was somehow necessary to work around a bug / select the correct default format, when allowed formats was still implemented in contrib (allowed_formats module). But:

• Core does not allow to save a default format without a value, through the UI
• a drush config:export will re-export this format-without-value in the above format which causes the fatal error when you re-import the exported file.

I rely on other people to specify their 'config sync' policy / best practices. So I'll rebrand this a task.

People must be doing this. But so far, I don't remember anyone really commenting on it.

Are they using config_ignore? If so, what are they ignoring? Just the entity ID (provided the certs are in files)?

Do they want to use multiple IdP configs for this? ✨ Multiple IDP Support Needs work

When there's a somewhat authoritative reply from someone, we can determine if this needs documentation, or other work.

Thanks for the re-explanation, I'm not always fast picking everything up :)

Regardless whether this solution would be suitable for your needs: the fact that a UserVisibleException thrown in a USER_SYNC event subscriber is a bug that should be fixed. (And noone's reported it so far + I somehow failed to spot it + there are no tests for this.)

I "plan to soon" test and extend SamlController::handleExceptionInRenderContext() to also recognize the wrapped EntityStorageException, then. (and/or any exception that wraps a UserVisibleException...)

That's arguably a hack to counter a hacky situation (the fact that the event is dispatched during hook_user_presave). But it is a recognizable specific hack without further issues. And for the moment I'm still sticking to the hacky situation and living in hope I'll tweak externalauth someday.

(Because - better summary: otherwise I'd need to give up on either dispatching the same event for new and re-logging-in users, or having protection against saving half-constructed user entities with bogus data.)

(Aaaaaaaactually re. "done during saving":

There are complications. The gist is: I would like to do separate the user sync out from the eventual user_save(), but that requires a rewrite of the externalauth module if you never want to run the risk of ending up with a saved user with half-incorrect data.

I think. It's been a while since I looked at the details.)

> I somehow assumed that the UserSync event would only run for new users

OK, that's cleared up then.

> It does specifically for new users run quite late though, only when already saving the new user. That will also catch and rethrow the exception in the storage handler. This would also break the ability to display a specific message to the user.

Well yeah, interpreting the SAML message and saving the new user is basically the only thing it does. I never considered that "late". (I guess I never cared about the catch + rethrow you mention? What mattered to me only is that it gets caught again in the controller. Also: apologies for the code spaghetti.)

For displaying a message to the user when login (and/or creation / sync) fails, you can throw a Drupal\samlauth\UserVisibleException. Does that help?

I guess I could throw an exception during the user sync event, but if the attributes change for an existing user, it wouldn't prevent the login.

Not sure I'm interpreting this correctly.

• The login is always prevented if you throw an exception.
• What you can't do (well I guess you can, but not in a recommended way), is prevent the login but still update the Drupal user data with data from the IdP. I wouldn't expect anyone to want to do that... so... just asking for confirmation: is that really what you want to do? I never considered there would be a use case for that.

roderik → created an issue.

Rebased the old MR because the underlying code changed a lot with 🐛 When editing a custom elements page view php errors are produced by renderer Needs work (code was moved from the controller to CustomElementsPage plugin). Tested that output is the same as before.

The MR is not ready, because of now 2 reasons:

1) CustomElementsPage and CustomElementsBlock contain pretty much the same code though they extend different classes. This wants a trait.

2) The originally reported issue still exists. I need more time to properly comment on this, than I have right now.

roderik → changed the visibility of the branch 3363234-subrequests-request-uri-patch-8678 to hidden.

@benellefimostfa

* Tests should be green after you merge/rebase.

* Check phpcs pipeline errors. (It's a bit irritating that static analysis for 3.x is "orange" at the moment; I will fix phpstan to be green soon, so it is better to spot.)

Reviewed.

I was wondering if we should test the HTML normalization standalone. In that case, we should likely add that as a "quasi output format" to some CustomElementBaseRenderTest tests.

I'm not sure whether I care much about this at the moment, I need to take a look again later (after changes are implemented), whether we are exercising all normalize code (e.g. normalizing of some array values); if we aren't, then it likely makes sense to do it.

@pfrilling sorry, I should have added the change in caching the form, to MR!139.

If you merge + push to 8.x-1.x without this change, I guess that's fine and we can wait until upgrading to the 3.x branch, until we see this fixed in practice.

(This change is not closely related to the reported issue, but it's valid, I can confirm that I've seen the caching issue.)

That's MR!139.

(I'm going out on a limb that you're not needing to add tests there.)

roderik → made their first commit to this issue’s fork.

CE release is out; requirement added to composer.json. Build is green.

Note this is a new "require" line because previously, we depended on custom_elements only indirectly through lupus_ce_renderer..

I was tempted for a moment to drop support for CE v2, just because we might introduce some functionality later that depends on CE v3 without realizing it.

But 1) that's not up to this specific issue; 2) I can't really think of such functionality (yet; let's see about CE v3.1)

As an aside: the recipes that ship with configuration all require custom_elements v3. But I assume 1) they would error out when trying to install config onCE v2; 2) no new installation will ever install v2 anyway.

roderik → changed the visibility of the branch 3504718- to active.

roderik → changed the visibility of the branch 3504718- to hidden.

Fix in 🐛 Error enabling layout_builder Active . If you approve that issue + want, I can release custom_elements 3.0.2 tomorrow, and add a MR here that depends on that minimum version.

This needs a review, because the code is 'ugly': I don't see a way to know that "layout_builder is being installed" without hardcoded assumptions. (And that's why the comments are big.)

I think we have no other options / it's fine.

• It errs on the "safe" side, being: only skip when it encounters the exact Layout Builder classname (while Layout Builder is not actually installed).
• This does mean: if the class is somehow renamed, the fatal error returns.

I checked the entity_type_alter behavior manually with xdebug enabled, and it does the expected thing:

• only skips while layout_builder is being installed
• works normally,
  • at other moments during the 'drush en layout_builder' command
  • in normal operations
  • during install/uninstall of other modules, either with or without layout_builder enabled.

I only tested with drush. Since the only 'interaction' is with the ModuleInstaller service, drush vs UI install can't make a difference.

roderik → created an issue.

Minimum modules required to trigger the error on my ddev instance:

Note, no lupus_decoupled modules are involved:

$ drush si
$ drush en -y custom_elements
 [success] Successfully installed: custom_elements
$ drush en -y layout_builder
The following module(s) will be installed: layout_builder, layout_discovery


In EntityTypeRepository.php line 111:

  The Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay class does not correspond to an enti
  ty type.

So this seems a custom_elements bug, and feels related to the fact that we are 'unlinking' the LayoutBuilderEntityViewDisplay class:

function custom_elements_entity_type_alter(array &$entity_types) {
  // Use the right class depending on layout builder being used.
  $class = \Drupal::moduleHandler()->moduleExists('layout_builder') ? CustomElementsLayoutBuilderEntityViewDisplay::class : CustomElementsEntityViewDisplay::class;
  /** @var \Drupal\Core\Entity\EntityTypeInterface[] $entity_types */
  $entity_types['entity_view_display']
    ->setClass($class);
}

I will trace this and add a custom_elements issue today.

The error looks bad but it seems not to do anything. I find nothing in the status report, no strange behavior.

Uploading patch for 8.x-1.x to this issue too, for 'fixed' reference for our build system. It can apply to 3.x too, the code is the same, it's just 18 lines higher up in the .module file.

I admit to only live-testing the 8.x-1.x version on our system, but I don't really see how there would be a difference between the two versions in this respect.

Pipeline for 3.x MR is green, so I assume pipeline for 8.x-1.x MR is not failing harder than usual.

roderik → created an issue.

@bourgeoy I still have no idea what is exactly happening so I need your / anyone's help to debug and provide technical details, so I can judge what is going on (and then perhaps provide better error messages).

The communication was haphazard (and sometimes wrong from my side), so I'll try to re-summarize myzelf:

I guess first we need to know if at this point before externalauth is called, your $account_data['name'] is empty.

If it is empty, then there are two separate things going on:

1) There is probably something wrong with your configuration or the data that your IdP sends. The "Contents of the user name attribute" in the message from the IdP should always be populated.

2) Regardless of "1" likely being your actual problem: I am still expecting the username to be filled with something (specifically "samlauth_THE-UNIQUE-ID"). Details in my comment #4. This is why there is no error message from this module: I can't implement one, until I see where and how the username would be set to NULL.

If it is not empty:

Something outside of the module is surely overwriting the username with NULL, just before saving it. I don't know what or where, so I also don't know where to give a better error (if possible).

Embarrassingly, 3.x-1.10 itself contains an error that triggered the warning and therefore should be fixed. Your issue is probably fixed with 🐛 Cacheability Metadata Leakage Error on SAML Login with Samlauth and r4032 Redirect Module Active . 8.x-3.11 is out now.

I should have seen it, of course. Unfortunately I don't use this module actively anymore, so it takes time until I spot things. And I'm happy that people like you are actively tackllng some issues.

Finally a new 8.x-3.11 release is out with fixes now, and the list with outstanding bugs and clearly-needed additions is cleaner than ever. (Dare I say zero?)

The log that potentially bothers people under some circumstances ( 🐛 Code leaked cacheability metadata Active ) is still present in the module, and me reading up on whether it is needed, is still outstanding.

Thanks for adding tests! (I haven't looked at the exact reason why they fail for next-PHP-version; I think that happened in the main branch too, and it went away somehow. I'm also hoping to clean up the bogus warnings sometime soon.)

I was doubting whether to cram this through right now, because I'm going to do a new release and this is the only outstanding thing. Alas, I feel uneasy about one thing that I don't want to hack now, so it'll have to wait; I'm giving that back as/for feedback.

I don't have a setup with different hostnames, to test this immediately. So I think this will leave you with general support from others in this queue, for now.

Assuming you mean the seckit module (which I am not intimately familiar with):

* Acquia supports seckit, and I also know that several of their hosted sites use the samlauth module. So it's remarkable that I haven't seen this issue before.

* It would certainly deserve documentation, if we find that something needs to be preconfigured.

* https://www.drupal.org/project/seckit → mentions https://www.drupal.org/project/seckit_override → -- is that a way to tell Drupal not to complain about saml/login? (I mean... assuming that you can see/verify this request really is right. But I assume you can see that.)

Thanks for the reminder. I guess I should have not closed this stale issue myself, because of the missing credit that I can't assign.

In principle it seems better to assign here -- 📌 Add logo Needs work was just a throwaway reminder issue (without credit) to add the SVG file. But since I don't really know the maintainer situation here and I don't want to cause more indirect communication... I added the credit in there.

Duplicate with 🐛 Error encountered when accessing SAML authentication mapping configuration page Active (which was open earlier, though it didn't contain any useful code yet when this was opened). See there.