Page MenuHomePhabricator

Create and implement IP Info viewing rights [L]
Closed, ResolvedPublic

Description

Motivation

IP Info presents some information which may cause some level of risk to certain users depending on their location. To mitigate this we want to show the more sensitive information only to trusted user groups.

Spec
  • There is a need for varying levels of access to ip info. Implement ipinfo-view-basic and ipinfo-view-full rights with the following viewing permissions:
  1. basic
    • Country
    • Version
    • Connection method
    • Connection owner
    • Proxy
    • Static / Dynamic
    • Number of users on this IP
    • Block information
    • Contributions information
  1. full
    • All basic information
    • Complete Location
    • ISP/Domain
    • Organization
  • IP Info should check for and gate the information as necessary.
  • If a user only has the "basic" access, they will not be able to see the other information. It will completely disappear from the display. There is no indication to the end user that they are viewing a limited version of the information available.
Who sees what? (MVP version, subject to change in the future)
  • Basic: can be accessed by all registered users
  • Full: accessible to these user-groups: sysop, bureaucrat, checkuser, oversight, steward
Things to note:
  • The rights may be split based on already assigned user groups. see T292624: Investigate and document necessary IP Info viewing rights for more detail.
  • In the post-IP-Masking world, there will be a new user right which will be assigned to certain users which allows them to view IPs. When this right will be given, the ipinfo-view-full right will be also implicitly given to these users.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This is much of a muchness but I would lean toward something like "basic" and "full" because admin is an overloaded term.

Niharika updated the task description. (Show Details)
Niharika moved this task from Untriaged to Triage/To be Estimated on the Anti-Harassment board.

At the risk of being prescriptive:

In T286661: Create framework for fetching on-wiki information [L], we introduced the DefaultPresenter class, which took the responsibility of converting domain objects to Plain Ol' PHP Arrays from the RESTful API handler classes. Hiding information based on access level fits neatly into this level, the presentation level. I'd approach this by introducing a new presenter class, e.g. SuppressingPresenter, which uses DefaultPresenter under the hood but deletes information based on the user's rights:

class SuppressingPresenter {
    public function __construct( DefaultPresenter $defaultPresenter, PermissionManager $permissionManager ) { /* ... */ }

    public function present( array $info, UserIdentity $user ) {
        $result = $this->defaultPresenter->present( $info );

        if ( !$this->permissionManager->userHasRight( $user, 'ipinfo-view-full' ) ) {
            unset( $result[ /* ... */ ] );
        }

        return $result;
    }
}
ARamirez_WMF renamed this task from Create IP Info viewing rights to Create and implement IP Info viewing rights.Oct 20 2021, 4:30 PM
ARamirez_WMF renamed this task from Create and implement IP Info viewing rights to Create and implement IP Info viewing rights [L].Oct 20 2021, 4:34 PM

Change 735935 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/IPInfo@master] Add getCountry to Info

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

Change 735942 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/IPInfo@master] Add access levels to ip information returned

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

@Prtksxna I just wanted to check in to make sure that

There is no indication to the end user that they are viewing a limited version of the information available.

didn't conflict with our current "show all potential information even if the user doesn't have access to it" implementation. Here's what the popup looks like with basic permissions:

image.png (236×344 px, 13 KB)

And with full access:

image.png (211×334 px, 11 KB)

Does this look okay? Did we want to show country and location at the same time?

Additionally, we don't have the following set up and I think these would require additional tickets:
Country*
Version
Connection owner - Is this different than ISP?
Static / Dynamic - not available w/data we currently have
Number of users on this IP - not available w/data we currently have

*I wrote something quick for this with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/IPInfo/+/735935 to test out country vs location since I wasn't sure if we wanted to show both or one over the other

Does this look okay? Did we want to show country and location at the same time?

Yep, just a single Location heading that would either show the full location or just the country.

Additionally, we don't have the following set up and I think these would require additional tickets

Set up in terms of viewing rights or showing them in the box?

Connection owner - Is this different than ISP?

Yep. This is userType (as seen in T264837). We changed the label after people had trouble understanding it in the usability tests. The figma mock, T269304 and T287647 have more details.

Static / Dynamic - not available w/data we currently have
Number of users on this IP - not available w/data we currently have

Does this mean that we won't have the data for the BetaFeatue, or that we just don't have it at the moment?

Since country and location are separated out here, do we still need to do T267403?

Ignore me, just seen this:

Did we want to show country and location at the same time?

Yep, just a single Location heading that would either show the full location or just the country.

Change 735935 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Add getCountry to Info

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

Change 735942 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Add access levels to ip information returned

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

@STran ipinfo-view-basic rights has been implemented per current view:

Screen Shot 2021-11-29 at 9.06.40 PM.png (1×2 px, 536 KB)

For ipinfo-view-full rights how can I set that up for my user with that rights, will that be implemented on another ticket? I also do not have documentation on the ticket on how to access/setup these rights.
Also for ipinfo-view-full rights; I did not see it listed in https://en.wikipedia.beta.wmflabs.org/wiki/Special:ListGroupRights
Thanks!

For ipinfo-view-full rights how can I set that up for my user with that rights, will that be implemented on another ticket? I also do not have documentation on the ticket on how to access/setup these rights.
Also for ipinfo-view-full rights; I did not see it listed in https://en.wikipedia.beta.wmflabs.org/wiki/Special:ListGroupRights

We're not going to be granting the ipinfo-view-full right on the Beta Cluster (see T270347: Remove 'ipinfo' right from administrators on the Beta Cluster) so this will have to be tested in your local testing environment.

I also do not have documentation on the ticket on how to access/setup these rights.

For a detailed overview see mw:Manual:User_rights. For this ticket, you can add the following to your LocalSettings.php file:

LocalSettings.php
$wgGroupPermissions['sysop']['ipinfo-view-full'] = true;

which will grant the Admin user the right on your local testing environment.

I was able to confirm user with ipinfo-view-basic and ipinfo-view-full rights:

ipinfo-view-basic on Beta, see screenshot below:

Screen Shot 2021-12-09 at 6.51.20 PM.png (1×2 px, 658 KB)

Via my local environment I was able to test the ipinfo-view-full rights, see below

Screen Shot 2021-12-09 at 5.29.08 PM.png (1×2 px, 836 KB)

Change 753720 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/IPInfo@master] Allow ipinfo-view-full to see ASN

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

cc @Prtksxna ASN is in the designs but no one has access to view it atm 😅 I made an assumption but should ipinfo-view-full see ASN? Should ipinfo-view-basic?

cc @Prtksxna ASN is in the designs but no one has access to view it atm 😅 I made an assumption but should ipinfo-view-full see ASN? Should ipinfo-view-basic?

You're right, since ASN can be used to get a better sense of location it should be in ipinfo-view-full.

Change 753720 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Allow ipinfo-view-full to see ASN

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

Below is view of info view full right. ASN field shows but no data is available. Although there is data for ASN. Is this okay? @STran

Screen Shot 2022-01-20 at 9.46.36 PM.png (1×2 px, 382 KB)

Screen Shot 2022-01-20 at 9.47.38 PM.png (1×2 px, 470 KB)

Below is view of info-view-basic. ASN field is also available in this view, is that expected per the description

Screen Shot 2022-01-20 at 8.54.50 PM.png (1×2 px, 622 KB)

Change 756653 had a related patch set uploaded (by STran; author: STran):

[mediawiki/extensions/IPInfo@master] Add asn to viewing rights

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

Change 756653 merged by jenkins-bot:

[mediawiki/extensions/IPInfo@master] Add asn to viewing rights

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

Below is view of info view full right. ASN field shows but no data is available. Although there is data for ASN. Is this okay?

The data should be visible - the latest patch should fix this.

Below is view of info-view-basic. ASN field is also available in this view, is that expected per the description

This is correct.

ASN now shows for IP info, see below screen shot:

Screen Shot 2022-02-08 at 11.30.04 AM.png (1×2 px, 353 KB)

Screen Shot 2022-02-08 at 11.30.30 AM.png (1×2 px, 297 KB)

IP info below does not have ASN, and it shows accordingly per screen shot below:

Screen Shot 2022-02-08 at 11.02.09 AM.png (1×2 px, 360 KB)

Screen Shot 2022-02-08 at 11.27.26 AM.png (1×1 px, 242 KB)