Page MenuHomePhabricator

Investigation: Checkout all the @TODO, @FIXME, and @HACK comments in LoginNotify
Closed, ResolvedPublic2 Estimated Story Points

Description

Figure out if any of these are high risk and need to be addressed before deployment. This investigation doesn't include actually fixing any of the issues.

Event Timeline

kaldari set the point value for this task to 2.

The only two "hacky" functions are to make the wgLoginNotifyEnableForPriv work (onUserLoadOptions, onUserSaveOptions). I personally think the extension is deliverable without fixing these hacks. The only caveat I am able to deduce is explained here, where if the user loses adminship then they lose the default settings, which they might have become accustomed to. That doesn't sound like a deal-breaker to me, but it's debatable.

I haven't looked very much into the todo's or fixme's.

The to-do's/FIXME's:

  • onLoginAuthenticateAudit – "Doesn't catch captcha or throttle failures", but this is for MediaWiki "pre 1.27 or wikis with auth manager disabled", so I guess not a blocker for us?
  • onAddNewAccount (MediaWiki pre 1.27) and onLocalUserCreated (since 1.27) – "still sets cookies if user creates an account while logged in as someone else". I think this means if I were logged in as MusikAnimal and I created User:MusikPuppet, a cookie is set for MusikPuppet even though I haven't logged in as them. Arguably not a blocker, and perhaps even desirable.
  • checkUserInCheckUserAnyInfo – "Does this behaviour make sense, esp. with cookie check?" This is partly where it ties into CheckUser, and to me it does make sense. Before this function, it uses CheckUser data to see if what they logged in with was a known IP. If it wasn't, it then checks if there is any CheckUser data for the account, for the user's top 10 most-edited wikis, and if not it won't treat it as an unknown IP since user has no known IPs.
  • isFromKnownIP and checkUserInCheckUser – "Does this make sense", same as above. It makes sense to me, assuming what the comments say are true, and that is in fact how it works.
  • checkUserInCookie – "does this really make sense?", this is referring to whether it makes sense to say "no info available" if the user is not in the cookie. Not enough context given, so I can't say if this is an issue, but this and the parent function are used to test if the "current computer is known to be used by the given user", and that much seems to work.
  • generateUserCookieRecord – "FIXME Maybe shorten, e.g. User only half the hash?", referring to the how the hash is created that will be stored in the cookie. I have no idea if the current approach is acceptable, but it seems to work so I wouldn't consider it a blocker.

I am also going to make a patch to change all the "checkUser..." functions to be "isUser...", to avoid confusion with the CheckUser extension, which is also being referenced in this code (e.g. "isUserInCheckUser", "isUserInCookie", etc.)