Page MenuHomePhabricator

Deprecate IP handling in `User::mailPasswordInternal` hook
Open, Needs TriagePublic

Description

PasswordReset class uses User::getRequest() to retrieve the user IP, that later is used in User::mailPasswordInternal hook, and the in the log statement.
With T353339 we will be able to inject request IP into logs, therefore the second remaining use is the User::mailPasswordInternal hook.

After a quick check of https://codesearch.wmcloud.org/search/?q=User%3A%3AmailPasswordInternal&files=&excludeFiles=&repos= I see only CheckUser extension uses this hook, and what more - doesn't use the $ip. Furthermore, IMHO, if some extension that listens to that hook really needs the IP, they can already get it via RequestContext::getMain()

PasswordReset is used in three places:

  • api/ResetUserPassword where we have a request and we can retrieve the $ip
  • SpecialPasswordReset that can be visited in browser and we can retrieve the $ip of caller
  • maintenance/resetUserEmail where we don't have the IP as this is CLI script

As PasswordReset can be triggered via CLI and at this moment I do not see a valid purpose for providing this variable I vote to drop support for it - keep the $ip as param, but leave it as empty string unless we have a better way to deprecate params in Hooks.

Event Timeline

pmiazga added subscribers: daniel, Tgr.

@daniel @Tgr @Krinkle - do you have any thoughts on this?

Change 982907 had a related patch set uploaded (by Pmiazga; author: Pmiazga):

[mediawiki/core@master] DNM: Pass the client IP to PasswordReset

https://gerrit.wikimedia.org/r/982907

CheckUser just ignores the IP passed by the hook and uses the IP from RequestContext instead. I think that's generally fine - it shouldn't be possible for the two IPs to differ, so it doesn't need to be passed explicitly by the hook. In theory someone could call User::newFromSession( $request ) with a fabricated request, maybe as a part of an internal API request, and then it would have significance, but it seems very unlikely to happen in practice.