Hide password change for SAML-authed users

(For this specific issue I'm going to use the "Needs work" status to mean "contrib opportunity"; I don't think I'll pick this up myself.)

Agreed. If the "Allow SAML users to log in directly" setting is off, linked users (who are registered in the authmap from the externalauth module, with provider "samlauth") should have the password box hidden / replaced by a link to the "change password" service. Not because it's a risk but because it's confusing / the local password just doesn't do anything.

Copying over some behavior from the LDAP module, where either field (email and password fields) can be:

• removed: they are no longer visible on the form
• disabled: they are visible but disabled
• allowed: basically what happens now to them (nothing)

The attached patch adds two keys to config to store the requested bevavior, and implements a simple hook_form_FORM_ID_alter() to enforce its settings.

Adding the correct patch file (please ignore previous one as it has an inverted if condition for testing).

Thank you for working on this. I like the added idea of influencing whether the e-mail can be changed.[*] It does raise questions, however.

First: why is hiding (or locking) the password an administrative option, rather than just always hide it for SAML users? Since login is disallowed... what are the use cases for being able to change your own password?

Maybe there are some other on-Drupal-site uses for the password - besides being able to change your own password and e-mail address - in the contrib sphere? I don't know any.

I briefly checked the LDAP module for clues on why they implemented it this way, but all I got was a sense that it doesn't work perfectly. I see a few TODOs, and there doesn't seem to be a method for non-LDAP users to change their own password if the 'skipAdministrators' option is set. (Which would be strange).

Second: while thinking about this, I was first hit by the realization about "being able to change your own password and e-mail address".

This is not an issue with your patch, it already exists: users who log in through SAML and have their account created for them, have no password. So they cannot change their e-mail, because they need their current password in order to do that. (Users who log in through SAML and have an existing account linked, can change their e-mail address, though.)

If we're going to introduce some option re. the ability to change e-mail, we should probably take that into account. (What to do? Explicitly make it possible to change e-mail without knowing the password? I guess not, because it lowers security. Always lock the e-mail address and hide 'old password' and 'new password' field for people who have no password?)

(Doing anything about the e-mail field could stilll be split off to a separate issue...)

As for a closer code review/test:

• Your second uploaded patch still has the inverted if condition (both patches are the same)
• The config form uses "remove" for the password field; the form_alter checks "hide". (Not that it matters if we're going to remove this option again.)

[*] Also, if my vague plan ever goes through, of moving generalized functionality into the externalauth module so multiple other modules can use it... this would be a candidate.

As mentioned, I copied the behavior from the LDAP module, which I am familiar with as I have it deployed in a few dozen sites.

The use case for changing email/password in general is that Drupal can push attribute changes back to LDAP (so, when a user changes their email/password on a properly configured website, that change can flow back to LDAP). However it is fidgety to setup, which is why a lot of people report it as being broken.

I am only starting to experiment with SAML in general, so I really do not know if that flow is possible.

For users created on the fly, LDAP generates a random password that is not published to the user, and does not have an option to find out what it is or remove yourself from LDAP.

In the one site that this option was requested, it was implemented in a custom module where users associated with LDAP can request (via profile button) to deregister from LDAP. This triggers removal of LDAP data from user data (effectively severing the existing tie), and triggers the core forgot password routine. Since the particular site allows mixed mode logins (ie local and LDAP, with local taking precedence) and has 2FA enabled, it was deemed to be adequate but it's certainly not a solution that is generalized.

I have attached two patches:
- Fixed the previous patch with your comments (hopefully proper switch case, harmonized names of states)
- New patch that simply hides fields and adds a notice on the account page. Ideally, this should also contain a link to the IdP, to manage your profile, however I do not know if SAML supports this (or if the link can be generated).

Possible options to be included in the admin page:
- Whether to hide or completely remove the email/password fields (one or two controls)
- Whether to show as warning status / inline (as implemented) / not at all a notice to users that they need to go to a different url to manage their credentials. If the URL cannot be deduced by module configuration, it's easy to add a simple textfield for it and toggle visibility with #states.

Oh, right, pushing a changed password back 'upstream'. That makes sense. But I don't think that's possible with SAML.

Re. a link back to the IdP: we have the "Change password service" configuration (idp_change_password_service) which

• predates my involvement (it was present in version 8.x-1.x of this module)
• I've never used, and pretty much ignored until now (besides just making sure it keeps working as it did before)
• I assumed was some standard SAML SP functionality, but I'm confident by now that it's just a custom enhancement. (It is not announced in the metadata.xml.)
• is blindly assumed to be a full absolute URL (by SamlController::changepw()), even though nothing validates the config value.

So we can use that existing setting as a link back to the IdP which can be included in the notice text, if present.

I'm working on the last fix-ups for releasing version 8.x-3.0 which kind-of-must be done within a week. Hiding the password, as per your smaller patch, can be included in it - possibly with that link back to the IdP. (No configuration option needed, because see first line ^.)

Hiding/locking the e-mail (and consequently the old-password text field) would be a regression for existing users who were later linked to a SAML login. So I wouldn't want to commit that yet. I'm not convinced this needs a configuration option; maybe some (unfortunately nonexistent yet) logic to distinguish those users from newly-created-by-SAML-login users, makes more sense. That can be worked out in a followup, post stable release.

Thank you for the feedback!

As per your suggestion, this patch:
- only changes the user form with no additional configuration.
- adds documentation comments, and a backlink to this issue in the function docblock
- hides the change password field, but leaves the current password and email fields untouched.
- displays a notice as a simple #markup at the top of the form, with a link to the IdP change password configured url.

Personally, I would like to have a config option to also affect the email field (since in our case accounts/emails are IT controlled, and not up to the user to change), which would cascade into hiding the current password field as well. This seems to be the dominant use case for corporate enviroments, which are the usual suspects for IdPs to my knowledge. Up to you.

Thank you. One thing for feedback: do we want this to be $form['account']['saml_notice'] instead of $form['saml_notice']? The notice seems kind-of lonely at the moment, for me it's showing down the page below the whole "account" section.

(Also a bunch of PHPCS violations, but the whole module is at this moment basically one big PHPCS violation which I'm taking care of before the 3.0 release, so I can fix that.)

Re. the e-mail option: it's basically an optical (not functional) change, because the users can't change their password anyway. I see the point that even so, it would look better for the majority (>99%) use case... but I'm a slow thinker, and still hesitant about introducing a setting that doesn't need to be a setting / may need to be re-one if we solve the same in pure code/logic, and which can cause confusion for edge-case users. I'll open a followup.

Fixed all PHPCS violations for the samlauth.module file in the attached.

Also, the previous patch ( #9 ) uses $form['saml_notice'] already, are you testing a previous one? Kept it as is.

I agree that if "feels lonely" to have it as it is, so I'm also attaching a variant of the patch that uses the standard drupal message area with a warning.

I'm suggesting that using $form['account']['saml_notice'] instead, puts it up near the top. Which your use of #weight seemed (to me) to imply being what we want.

Thanks again for working on this. And I'm sorry for only seeing this now, but I'm going to add another change to this to prevent a potential regression.

I didn't spot until now, that this doesn't take into account "If the "Allow SAML users to log in directly" setting is off" as mentioned in comment #2. We shouldn't point-blank disallow people who can log into Drupal locally, from changing their password.

So I made a quick change - see interdiff.txt. (I chose a way of changing this that would produce a small interdiff.) I assume that in your situation, the setting is off, so it doesn't matter for your case.

The reason I'm just committing this in one go now without further communication is... I opened a followup where we can discuss further / iterate on things that I'm not seeing clearly, if needed. Combining my dislike for this existing setting with the desire to disable the e-mail field on the profile edit screen... I think this setting should be turned into a permission - and this should be able to satisfy people's needs without introducing extra configuration. See #3201411: Improve access controls for logging in / changing e-mail

(My own current to-do list is
1) get (unrelated) #3155968: Validate existing session before redirecting to RelayState done by moving some pieces of code around, holding off on committing it until end of week, for $reasons
2) fix up various ugliness / PHPCS / config schema errors / comments and commit those already
3) make a few tests, maybe?
4) circle back to #3201411: Improve access controls for logging in / changing e-mail
)

(done. Also, we should not have altered the form when editing a different user from the logged-in one. I rolled that fix into #3201411: Improve access controls for logging in / changing e-mail.)