Page MenuHomePhabricator

[Bug] Normalise usernames in no-JS
Closed, ResolvedPublic5 Estimated Story PointsBUG REPORT

Description

From T271275: Log when admins are added to or removed from SecurePoll elections:

With JS disabled, there can be some weird behaviour. For example, if you change the election admin from Other Electionadmin to Other_Electionadmin, we appear to treat them as two different users and you get a misleading log entry:


In the same way, you can also add/remove the same admin more than once, and this gets recorded multiple times in the log.

I don't think this is likely to happen in practice, nor would it be a big problem if it did, but I may raise a bug about normalising usernames without JS.

Reproduction steps
One
  1. Create an election with the election admin: "Election admin"
  2. Disable Javascript, then edit the election and add an underscore to the admin's name (e.g. "Election_admin")

Observed outcome: Two entries in the log table, one add and one remove, but for the same user.

Two
  1. Create an election with any election admin
  2. Disable Javascript, then edit the election and add two admins: "Election admin" and "Election_admin"

Observed outcome: Two entries in the log table which claim to be adding the same user. In the securepoll_properties table you will also see Election admin|Election_admin in the admins row.

Event Timeline

Niharika triaged this task as Medium priority.Feb 11 2021, 4:55 PM
Niharika created this task.
Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptFeb 11 2021, 4:55 PM
Niharika changed the subtype of this task from "Task" to "Bug Report".Feb 11 2021, 4:55 PM
ARamirez_WMF set the point value for this task to 5.Feb 11 2021, 5:30 PM

Hm @Prtksxna could I get your opinion on what the expected outcome should be?

If a user has the username "User A", "User A" is the canonical representation but "User_A" is a valid representation as well. If js is enabled, the poll admin selector will always show and save the canonical representation. For instance, if I try to add "User_A", it will not return it as a valid result. I can only add "User A." If js is not enabled, there is no such check.

So if the user does not have js enabled should they be allowed to add "User_A"? I thought so because it's a valid representation of the username but for comparison, if you try to make an account with username "user_a", it will automatically normalize it to "User A." I also thought it would be jarring to type in "User_A" and when you navigated back to the page, to see "User A" but again, create account does it out of the gate and all of our logs normalize the username as well.

What should happen if in no-js they try to add "User A" and "User_A?" Should the UI throw an error? Should it automatically remove "duplicates?" Something else?


I mostly thought about extension-level fixes but it's possible we might want to fix this at the input level so that the js and no-js versions of the widget behave similarly(ish). I'd appreciate any opinions you may have on this @Tchanders. For context, with js enabled, all normalization happens before the user can be selected as a value. There is a validation step when the values are being saved but they check only for user validity, not for canonicity. To make no-js behave similarly, we would have to do something like auto-normalize usernames alongside that validity check (this would theoretically be redundant for js enabled submits).

Change 663940 had a related patch set uploaded (by STran; owner: STran):
[mediawiki/extensions/SecurePoll@master] [WIP] Normalize admin usernames before logging changes

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

So if the user does not have js enabled should they be allowed to add "User_A"? I thought so because it's a valid representation of the username but for comparison, if you try to make an account with username "user_a", it will automatically normalize it to "User A." I also thought it would be jarring to type in "User_A" and when you navigated back to the page, to see "User A" but again, create account does it out of the gate and all of our logs normalize the username as well.

I think they should be allowed to add "User_A". We should accept the input and clean it up and show it how it would correctly be (even if its is jarring). This way we are not throwing errors when we know what the user intended, and we're also showing them the "right" way.

What should happen if in no-js they try to add "User A" and "User_A?" Should the UI throw an error? Should it automatically remove "duplicates?" Something else?

What happens if they currently try to add "User A" and then "User A" again?
I think throwing an error can be avoided, if we could remove 'duplicates' that'd be ideal.

What happens if they currently try to add "User A" and then "User A" again?

With js enabled, this is not possible:


Since it will not show "Admin A" as an option if I already selected Admin A.

Without js, I can add Admin A repeatedly which is considered valid:

sqlite> select * from securepoll_properties where pr_entity = 24 and pr_key = 'admins';
pr_entity   pr_key      pr_value                             
----------  ----------  -------------------------------------
24          admins      Admin|Admin A|Admin A|Admin A|Admin A

I think throwing an error can be avoided, if we could remove 'duplicates' that'd be ideal.

It looks like the js-enabled version handles this edge case. If I re-enable js after adding duplicate entries, it will clean up the duplicates and be reflected when I save the entry:

sqlite> select * from securepoll_properties where pr_entity = 24 and pr_key = 'admins';
pr_entity   pr_key      pr_value     
----------  ----------  -------------
24          admins      Admin|Admin A

Hm. ๐Ÿค” It does sound like we just want to replicate what the js widget does for cleanup. I am leaning toward this being a widget-level fix.

I think throwing an error can be avoided, if we could remove 'duplicates' that'd be ideal.

Hm. ๐Ÿค” It does sound like we just want to replicate what the js widget does for cleanup. I am leaning toward this being a widget-level fix.

I agree with these.

To make no-js behave similarly, we would have to do something like auto-normalize usernames alongside that validity check (this would theoretically be redundant for js enabled submits).

Sounds sensible to me - this shouldn't break anything that's using the widget correctly.

Change 663940 abandoned by STran:
[mediawiki/extensions/SecurePoll@master] [WIP] Normalize admin usernames before logging changes

Reason:
fixing upstream

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

Change 664679 had a related patch set uploaded (by STran; owner: STran):
[mediawiki/core@master] In no-js, there is no on the fly normalization of usernames in HTMLUsersMultiselectField so both "User A" and "User_A" are valid representations of "User A" (the canonical representation).

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

Change 664679 merged by jenkins-bot:
[mediawiki/core@master] Normalize and de-dupe usernames in HTMLUsersMultiselectField

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

@STran I think this means the widget no longer works when you input IPs rather than usernames. It seems to remove/ignore them when you submit.

This is something you would do for things like Special:Investigate, e.g.:

Change 666120 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] HTMLUsersMultiselectField: Keep IP addresses while normalizing

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

Thanks for spotting this @dom_walden - the new patch should fix it.

Change 666120 merged by jenkins-bot:
[mediawiki/core@master] HTMLUsersMultiselectField: Keep IP addresses while normalizing

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

I can no longer reproduce the bugs in the description.

Moreover, the bug above (T274568#6848623) appears fixed. I can submit usernames, IPs and IP ranges via Special:Investigate correctly. Indeed, usernames are de-duplicated (but not IPs as Thalia points out).

Test environment Local vagrant MediaWiki 1.36.0-alpha (c2890c3), SecurePoll 2.0.0 (aef4fb3).