Page MenuHomePhabricator

Watchlist token auto-generator too zealous
Closed, ResolvedPublic

Description

The watchlist token now has an auto-generator, but it's become impossible to clear the field (presumably to disable the feature entirely). This is confusing behavior for the end user, as seen in this thread: http://en.wikipedia.org/w/index.php?oldid=332906857#Watchlist_token

It should be changed so that a blank watchlist token field stays blank.


Version: unspecified
Severity: enhancement
URL: http://en.wikipedia.org/w/index.php?oldid=332906857#Watchlist_token

Details

Reference
bz21912

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:56 PM
bzimport set Reference to bz21912.

Re-opening, just because it was designed that way doesn't mean its the best design. The comments there don't address the concerns here and on the linked discussion, that is, with the current design its impossible to disable it. I don't really have an opinion myself, but if its going to be closed without any change, the reason for closing should at least address the reason for opening.

I'm not able to reproduce the bug both on my local wiki and on Wikipedia.

pdhanda wrote:

werdna, Simmertical,

I think yall discussed this with Brion extensively. Any comments on MZMcBride's inability to clear the token entirely? Seems like it gets auto generated everytime a user visits the Special:Watchlist page.

-p

ayg wrote:

Maybe we should autogenerate only on user creation, and have a maintenance script add the tokens to existing users? I don't see a better way. If not that, we'd have to distinguish between "default, so autogenerate when convenient" and "user has explicitly disabled the feature". I guess we could use a magic token that means the latter, so if the user saves with the preference blank we secretly substitute it or something like that. But autogenerating on creation sounds much saner to me.

I think we should use a proper new form field type instead of a random textbox for this. We could then have a checkbox ("enable watchlist token"), which when disabled adds 'disabled' to that field (the magic-value solution). When the checkbox is enabled, it should have a token and then allow users to regenerate the token.

There shouldn't be any need to allow users to enter their own custom values for watchlist tokens, it's just a token.

ayg wrote:

On reflection, agreed. I ended up doing the token thing as a quick hack to get the feature working with as little work as possible. We should just have a checkbox to enable/disable, that's it.

The question is, what happens if the user wants to change the token because someone has stolen it? If we ignore this case, it all becomes very easy. Just hardcode the token to some hash of user_id + secret fixed global salt. But then you can't change the token for an individual user short of changing their user_id, so if anyone ever finds out what the token is, they can never safely enable watchlist feeds again. This is what prompted me to go with a manually-alterable token approach.

Alternatively, what if the hash depending on user_password instead of user_id? Then all they'd have to do is change their password. This means your RSS feed breaks if you change your password, but maybe that's expected? I imagine most users don't change their password very often either.

So here are a few possibilities:

  1. Have a checkbox in preferences to disable watchlist feeds, and a button to generate a new token. Tokens are autogenerated and stored per-user, but not directly exposed in prefs.
  1. Have a checkbox in preferences to disable watchlist feeds, and just use a hash of user_id + secret global salt. If a user's hash gets revealed, they can decide to either stop using watchlist feeds or let everyone see what's on their watchlist. (If the global salt becomes public somehow, it would have to be regenerated, so maybe random per-user hashes would still make more sense here, but that's an implementation detail.)
  1. Have a checkbox in preferences to disable watchlist feeds, and use a hash of user_password + secret global salt. If a user changes his password, he has to update the link in his feed reader too. (If the global salt becomes public somehow, users' passwords would be vulnerable to much easier brute-forcing, so a per-user hash might be a good idea here too. In that case this is just (1), with changing your password instead of a separate button.)

I guess I'm inclined to say (1) is best here, although it will require a bit more work than (2) or (3).

I guess I was unclear, my suggestion was (1) :)

ayg wrote:

No, you were clear, I was just pondering other possibilities. It would probably be easier for you to make a new preference type than me, so if you want to do it, go ahead.

While you're looking at this, I can't help but cry a little as I think of all the entropy that must be wasted because a new value is generated on each page load.

For example, the text for 1.17 says "Filling in this field with a secret key will generate an RSS feed for your watchlist. Anyone who knows the key in this field will be able to read your watchlist, so choose a secure value. Here's a randomly-generated value you can use:" (and a new MD5 each time).

Meanwhile the textbox already contains a token from (I assume) the time the user was created.

Assigning this to MatmaRex. This bug may no longer be valid.

I read and pondered. Here's what I came up with: change I0bdd2469.

(In reply to comment #7)

  1. Have a checkbox in preferences to disable watchlist feeds, and a button to

generate a new token. Tokens are autogenerated and stored per-user, but not
directly exposed in prefs.

My change redoes the interface:

  • prevents the user from changing the token directly
  • autogenerates it whenever used
  • adds a form where they can reset the token to a randomly generated value
  • keeps using the prefs mechanism and allows for changing the token via API

There is no way to disable the functionality, but it would be trivial to implement if deemed useful (literally maybe 10 lines of code changed).

Change 64565 merged by jenkins-bot:
Refactor watchlist token handling

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