Consolidate public APIs for generating user action URLs into a service

Have made a start on this, local testing is showing a few fails, waiting for completion to see what they were.

Well yes, but there are challenges with that, for instance, the route parameters aren't constant so I've ended up with this mess:

  /**
   * Get a user action url using the related route.
   *
   * @param string $route
   * @param \Drupal\user\UserInterface|null $user
   * @param array $options
   *
   * @return \Drupal\Core\Url
   */
  public function fromRoute(string $route, UserInterface $user, array $options = []): Url {
    $timestamp = \Drupal::time()->getRequestTime();
    $langcode = isset($options['langcode']) ? $options['langcode'] : $user->getPreferredLangcode();
    $hash = $this->hashUserPassword($user, $timestamp);

    $url_options = [
      'absolute' => TRUE,
      'language' => \Drupal::languageManager()->getLanguage($langcode),
    ];

    // @todo: Standardize parameter names for user action routes.
    return Url::fromRoute($route, [
      'uid' => $user->id(), // for password route
      'user' => $user->id(), // for cancel route
      'timestamp' => $timestamp,
      'hash' => $hash, // for password route
      'hashed_pass' => $hash, // for cancel route
    ], $url_options);
  }

I realised the service as-is would not work with the route described in #85494: Use email verification when changing user email addresses.

Updated with options to include additional route parameters and a payload to add to the hash. Could tightly couple the two but I thought I'd start with the more flexible option. I suppose one could argue for embedding better security practices in the API, I'd prefer to cover that in documentation in the class (yet to be done properly) but I don't know if there's a policy around this kind of thing already.

I've come to the conclusion the leaving \user_cancel_url and \user_pass_rehash alone in this change is the best course of action for a few reasons:

1. The two existing routes use different parameter names and create exceptions to the "rule" that would be introduced by this change.
2. The Controllers on the existing routes use \user_pass_rehash to check if their hash is correct, so it should really be used to generate the hash.
3. I want to remove the login time from the hash (see below), and I really don't want to mess with \user_pass_rehash which drives home the point for me that the two existing routes should use *that* to generate hashes.

Why do I want to remove the user login time from the hash?

I just tried using this new service to create a user email verification link that gets sent in the welcome email (no admin auth required) because I want the following login flow:

• Set username and password
• Get logged in immediately
• Receive welcome email and verify email address from link within

This is quite a common pattern in SaaS offerings.

The issue is that the welcome email is generated before the user is automatically logged in so the hash differs.

Why was user login time specifically used? Can we replace it with anything better? I'm loath to just remove it, it would reduce the randomness of the hash.

They could.

What then of new uses for this service like those surfaced in #85494: Use email verification when changing user email addresses? Do we keep adding new methods? Or do we create a general-use method and treat 'reset' and 'cancel' paths as special cases? I'm not familiar enough with the policy/procedure/standard for handling BC here.

and... if I did put them in as special cases, and assuming I am successful in campaigning for a change to the hash contents, they would need their own special case checkHash methods (or a gate within that method) which really muddies the water if we ever want to standardise away from the current global \user_pass_rehash function which in my opinion should be private/internal only to the service.

boiled down: I think it best to let people use the global functions as pairs in their current state until they are deprecated and removed - and for the new service to implement the best possible logic (with an unchanging API) for core, modules, and builders to swap over to gradually.

Separately:

I did a little testing and I think $account->getChangedTime() might be a suitable replacement for $account->getLastLoginTime(), at least, it doesn't break the two existing core uses of user_pass_rehash, and it changes from day to day or week to week like the lastLoginTime does. Thoughts?

I'm embedding this change into my application to keep me accountable for keeping it moving, and because it's just really useful. So with that in mind I've changed the loginTime element to changedTime just until there's a direction/decision because loginTime doesn't work for my use-case.

Honestly I suspect there will be use-cases where changedTime is similarly unsuitable.

This does still need tests but it's marked 'needs review' to get some direction.

Another issue with the contents of the hash has been reported at comment #347 on #85494: Use email verification when changing user email addresses:

If the user attempts to change their password and email address at the same time, they cannot verify their email address using the verify email link.

Which comes down to the hash being generated before the new password is saved to the account.

Obviously we can't remove all of the changeable values from the hash or it will become static and far easier to use in an attack.

Thinking out loud: perhaps we could pass an ignore parameter to fromRoute and checkHash in this service that asks it to leave one and only one of the values out of the hash? (PHP 8.1 enums would make this that much easier, sigh)

Looking at the service again I realised I was asking for an associative array but not using the keys in the hash, have fixed that.

Thanks so much for the review @dww :) you've raised some great points.

1. The code and issue discussion are now dealing with a whole other question of changing the hashing for the login links. That's not mentioned anywhere in the summary. I think that's pretty major scope creep for what I thought I was going to be looking at. 😉 We probably want to split that off into another issue, perhaps postpone this one on that, and focus on the hashing stuff first.

   You're right, that's a huge scope creep, they should be separate. I would argue however that if we do this issue first, the altering of the hash for the existing links becomes *much* easier because it will be self-contained and will allow us to change the hash method's signature if we need to, since it shouldn't be exposed by the service the way it currently is.

   Thoughts?

2. If we need 3 kinds of links, and they all are a little different, we should have 3 methods on the service.

   @larowlan questioned this too, ok.

   it's easy to add deprecations to the specific ones and call out to the cleanly generalized thing

   I wasn't under this impression but I have no particular reason I can point to so I will recalibrate my thinking based on that and put the three links in.

   What would we prefer?:

   • Public method to generate each URL type
   • Public method to check each URL type

   or

   • Single public method to generate URL keyed by type
   • Single public method to check URL keyed by type

   The latter could farm out the hash generation to a protected method based on the type key.

   I'm not particularly wedded to either idea, but left to my own devices I'd probably go with the latter.

3. I agree a close review is premature.

#20 I don't think it's moot, I'm just no cryptography or cybersecurity expert so the idea of reducing the number of variables in the hash gives me pause.

I wholeheartedly agree with your points.