Page MenuHomePhabricator

Wikidata edits by temporary accounts are added to watchlist table
Closed, ResolvedPublic

Description

Steps to reproduce:

  • Go to a wiki with temporary accounts enabled, e.g. Beta Wikidata
  • Be logged out (e.g. in a private window)
  • Make a Wikidata edit, e.g. create a new item
  • Look at the watchlist table (for Beta Wikidata: SSH to deployment-deploy04.deployment-prep.eqiad1.wikimedia.cloud, then run mwscript mysql wikidatawiki)
MariaDB [wikidatawiki]> SELECT * FROM watchlist WHERE wl_namespace = 0 AND wl_title = 'Q629218';
+---------+---------+--------------+----------+--------------------------+
| wl_id   | wl_user | wl_namespace | wl_title | wl_notificationtimestamp |
+---------+---------+--------------+----------+--------------------------+
| 3806500 |    7903 |            0 | Q629218  | NULL                     |
+---------+---------+--------------+----------+--------------------------+
1 row in set (0.001 sec)

MariaDB [wikidatawiki]> SELECT user_id, user_name, user_is_temp  FROM user WHERE user_id = 7903;
+---------+-------------+--------------+
| user_id | user_name   | user_is_temp |
+---------+-------------+--------------+
|    7903 | ~2024-15416 |            1 |
+---------+-------------+--------------+
1 row in set (0.001 sec)

Expected result: no watchlist row, as temp accounts don’t have a functional watchlist (going to Special:Watchlist just sends you to the login page)

Actual result: a useless watchlist row

This should probably be filtered out in WatchlistManager and/or WatchedItemStore, just like watchlist entries for unregistered users are already silently dropped.

I haven’t been able to reproduce this for normal wikitext edits, by the way – I created this random talk page, and the temporary account for that (id 7905) has no watchlist rows. So there might be some filter in EditPage(?) that could potentially be removed once WatchlistManager/WatchedItemStore do equivalent filtering.

Event Timeline

There’s quite a few rows like this on Beta Wikidata already:

MariaDB [wikidatawiki]> SELECT COUNT(*) FROM watchlist JOIN user ON wl_user = user_id WHERE user_is_temp = 1;
+----------+
| COUNT(*) |
+----------+
|     5116 |
+----------+
1 row in set (0.016 sec)

Also some on dewiki, so I maybe it’s not limited to Wikidata after all? Not sure.

MariaDB [dewiki]> SELECT COUNT(*) FROM watchlist JOIN user ON wl_user = user_id WHERE user_is_temp = 1;
+----------+
| COUNT(*) |
+----------+
|      214 |
+----------+
1 row in set (0.021 sec)

Hmm, WatchlistManager->addWatch() and friends all seem to correctly call $performer->isAllowed( 'editmywatchlist' ); is that wrongly returning true for temp users?

Hmm, WatchlistManager->addWatch() and friends all seem to correctly call $performer->isAllowed( 'editmywatchlist' ); is that wrongly returning true for temp users?

Doesn’t look like it AFAICT:

$ mwscript shell wikidatawiki
Psy Shell v0.12.3 (PHP 7.4.33 — cli) by Justin Hileman
> MW::srv()->getUserFactory()->newFromId( 7903 )->isAllowed( 'editmywatchlist' )
= false

Ah, this would be why:

WikiPageEntityStore::updateWatchlist()
		if (
			$user->isRegistered() &&
			$title &&
			( $watch != $this->watchlistManager->isWatchedIgnoringRights( $user, $title ) )
		) {
			if ( $watch ) {
				// Allow adding to watchlist even if user('s session) lacks 'editmywatchlist'
				// (e.g. due to bot password or OAuth grants)
				$this->watchlistManager->addWatchIgnoringRights( $user, $title );
			} else {
				$this->watchlistManager->removeWatch( $user, $title );
			}
		}

So this one’s pretty clearly our fault, I guess. (Code was introduced in T217144.)

Whereas in EntitySchema we use setWatch(), which unlike addWatch() and removeWatch() doesn’t check the permission, but doesn’t clearly document this either; so in that case I would put the blame closer to WatchlistManager’s interface being confusing, I think ^^

Change #1057220 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Check isNamed() before changing watchlist

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

Although, hang on, WatchlistManager::setWatch() has almost the same code anyway – I guess that’s where we copied it from in T217144?

		if ( $changingWatchStatus ) {
			// If the user doesn't have 'editmywatchlist', we still want to
			// allow them to add but not remove items via edits and such.
			if ( $watch ) {
				return $this->addWatchIgnoringRights( $performer->getUser(), $target, $expiry );
			} else {
				return $this->removeWatch( $performer, $target );
			}
		}
 			// If the user doesn't have 'editmywatchlist', we still want to
			// allow them to add but not remove items via edits and such.

Hmm. Kunal (in person) says it's for users that have the "add pages I edit to my watchlist" but aren't allowed to edit theirs. We should definitely check that they're isNamed() first, yes.

Change #1057252 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] WatchlistManager::setWatch: Exit if the user is a temp account

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

Change #1057252 merged by jenkins-bot:

[mediawiki/core@master] WatchlistManager::setWatch: Exit if the user is a temp account

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

kostajh claimed this task.
kostajh subscribed.

Thanks for the task & patch, @Lucas_Werkmeister_WMDE!

Change #1057220 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Check isNamed() before changing watchlist

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