Standaloneusers - split the public and private bits of User.SendPasswordReset into separate actions

[ufundo]

Overview

Refactors User.SendPasswordReset into a public "RequestPasswordResetEmail" and private "SendPasswordResetEmail". Folds some of the helper functions from Civi\Standalone\Security into the classes that use them.

Original motivation for this was step towards enabling an admin to send a password (re)set email for a newly created user. I have been having second thoughts about this*, but I think its a good cleanup refactor any way.

Before

• public API action User.SendPasswordReset which checks for a valid user, prepares and sends them an email, and obscures the time taken.
• several helper functions from Security class

After

• private API action User.SendPasswordResetEmail which sends a password reset email for a selected user
• public API action User.RequestPasswordResetEmail which checks for a valid user, sends using the above action if there is, and obscures the time taken
• helper functions moved from Security to sensible homes on the SendPasswordResetEmail or RequestPasswordResetEmail or PasswordReset classes

Technical Details

• I would like to make some of the helpers that are used in one place protected / private, but they are currently used directly by SecurityTest, so have tagged them @internal

• I removed the explicit addition of extra Api actions in \Civi\Api4\User because they get picked up by the class-scanning, so it seems like unnecessary lines of code. Is there any reason to prefer explicit addition to the Api4 class?

Comments

• second thoughts: for an activiation email, you likely need a longer lived token, and maybe it's always best to invite new users to a page where they can request a fresh, short lived token

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[ufundo]

retest this please

[ufundo]

Note: this would be a step on the way to this: ufundo#8

[artfulrobot]

Looking at the sendPasswordReset action's authorization process, is this right:

• it can never be run while checkPermissions is set. So it can never be accessed over http (ajax/rest).
• it's not stopped at permission level (falls back to default "access CiviCRM" for User)
• it's only stopped at ACL level by the absence of an authorizerecord listener to authorize the event.

Why not add "cms:administer users" to the User::permissions()? That would seem clearer and more assuring to me, given the default for that entity. And shouldn't an admin be able to access this over ajax? e.g. for your now-second-thoughted "an admin sends out links" concept. - see next para.

Re admins triggering reset emails: I can see occasions when being able to en-masse send password resets (with a parameter for longer token life) could be useful. On the other hand, those are pretty darned rare, and this potentially increases an attack surface area. So I agree with you that on balance that it's better practice to ask users to do this and send them the link to the password reset request screen, in a normal email, if they want/need to (re)set.

Is there any reason to prefer explicit addition to the Api4 class?

DX/IDE autocomplete?

good refactor anyway

I don't have strong objections to it, so if you feel it's an improvement, fine. I guess it reduces the amount of code in each file, which is sometimes helpful to someone reasoning about the code, and sometimes it adds another level of abstraction that can be harder to follow. I can see, if you wanted the feature you now don't, that the case for the refactor was stronger.

Edit: I've read through the code and it looks fine (see comments) and have r-run this and it works as designed.

[ufundo]

Thanks for reviewing @artfulrobot

Why not add "cms:administer users" to the User::permissions()

I was working on the basis that it was defaulting to administer CiviCRM if unspecified, but maybe that is wrong and it was just the ACL check failing.. At any rate, this makes sense to specify explicitly, will def add.

Is there any reason to prefer explicit addition to the Api4 class?
DX/IDE autocomplete?

Ah ok. Could a method hint comment achieve the same with less lines of code? Or I can just reinstate as was, IDM.

on balance that it's better practice to ask users to do this and send them the link to the password reset request screen, in a normal email, if they want/need to (re)set.

I can see, if you wanted the feature you now don't, that the case for the refactor was stronger.

I haven't decided whether I want the feature or not. (I want it in the short term until we have the 2-step flow, but not in the long term). On balance I think I want the feature.

[artfulrobot]

@ufundo re permissions - yes see the note in the docblock for User::permissions.

Could a method hint comment achieve the same with less lines of code?

I had the same thought! Probably. There's usually that weird repeated boilerplate that basically just passes on the $checkPermissions. IDM either, but I generally think it's good to stick with what everyone else has done unless there's a reason not to.

Re admin feature to bulk send resets: not sure re the "2 step flow"? If you want it then we're going to need permission (for admins) to do it over ajax right?

[ufundo]

I had the same thought! Probably. There's usually that weird repeated boilerplate that basically just passes on the $checkPermissions. IDM either, but I generally think it's good to stick with what everyone else has done unless there's a reason not to.

Ok, have reverted for now.

Re admin feature to bulk send resets: not sure re the "2 step flow"? If you want it then we're going to need permission (for admins) to do it over ajax right?

Sorry, by 2 step flow I meant a way to trigger an email to a user, that directs them to request a password reset. My main thought is on user creation - ideally you have a "welcome email" template and a "set your password screen / activate your account" screen.

I've pulled through the ACL commit from the other branch which makes it possible to use sendPasswordReset over AJAX here. So then the feature bit is just UI level (https://github.com/ufundo/civicrm-core/pull/8/files) . That also means that could be added through a separate extension rather than available by default, which is maybe good if its an uncertain feature? Though if the reason not to add is attack surface, then the attack surface change is mainly in this PR 🤔

[ufundo]

being able to en-masse send password resets... pretty darned rare

Just to note in https://github.com/ufundo/civicrm-core/pull/8/files I only add the button to the UI on a per row basis, not in the select-multiple-rows action list. You could send in a big batch using Api4 explorer or API generally, but wasn't adding core UI for this.

[artfulrobot]

This looks good to me now 👍 @ufundo

[artfulrobot]

...on r-run testing...

there's one weird ish thing, testing with api4 explorer, if I do it with an admin user, I get [null] returned (not [] and not null), but the debug message says it worked.

If I do it with a non-admin user, I don't get an error, and I also get [null]. I would have expected an ACL error? Sorry I forgot that the "staff" role is basically the same as admistrator role. Removing the admin users permission from the unprivileged role does in deed give an error (not an ACL one, just a generic one with a random hash ref that matches logs).

[ufundo]

Good catch, added a standard return array.

Also noticed how it is updating the token even if the reset email workflow message is disabled. No worse than before, but if we do refactor the template prep, would be good to exit earlier if its not available. Added a TODO comment

[ufundo]

rebasing

[totten]

Merge based on @artfulrobot's review. Seems better to get this in sooner than later so that it takes advantage of the RC period.