Page MenuHomePhabricator

Remove 'ipinfo' right from administrators on the Beta Cluster
Closed, InvalidPublic2 Estimated Story Points

Description

Background

Following T270344: Give 'ipinfo' right on Beta Cluster so AHT can test, administrators on beta have the 'ipinfo' right. Ahead of deploying to production, we should remove the right from beta admins, and test on testwiki instead. This allows more control over who can access the feature.

This is a deployment blocker.

AC
  • Administrators (users in the sysop group) on the Beta Cluster don't have the ipinfo right
  • Users in the ipinfo group on testwiki have the ipinfo right
  • No users have the ipinfo-view-full on testwiki

Event Timeline

Tchanders renamed this task from Remove 'ipinfo' right from administrators, and give to checkusers instead to Remove 'ipinfo' right from administrators on beta.Dec 18 2020, 1:48 PM
Tchanders updated the task description. (Show Details)

A question that arose in our meeting was how we intend to test ipinfo-view-full going forward. Some suggestions were:

  • Test locally
  • Use mock data (@phuedx was there a plan of how to do this?)

This will need a bit of discussion and possibly some more tasks (e.g. if we go the mock data route).

  • Use mock data (@phuedx was there a plan of how to do this?)

We'd update the IPInfoGeoIp2InfoRetriever service to return an instance of MediaWiki\IPInfo\InfoRetriever\StubGeoIp2InfoRetriever if a config variable (say $wgIPInfoEnableStubIPInfoRetriever) is truthy. I can think of two implementations for MediaWiki\IPInfo\InfoRetriever\StubGeoIp2InfoRetriever:

1. Always return data for a well-known IP address
src/InfoRetriever/StubGeoIp2InfoRetriever.php
// We extend GeoIp2InfoRetriever here because we're specialising its behaviour.
class StubGeoIp2InfoRetriever extends GeoIp2InfoRetriever
{
  private const IP = 'xxx.xxx.xxx.xxx';

  public function retrieveFromIP( string $ip ): Info {
    return parent::retrieveFromIP( self::IP );
  }
}

Which IP address should we use? MaxMind do use IP addresses in their test suites, e.g. https://github.com/maxmind/GeoIP2-php/blob/main/tests/GeoIp2/Test/Database/ReaderTest.php. Alternatively, Michael C suggests using the Pingdom probe server IP addresses as they're published and stable.

2. Always return well-known fake data
src/InfoRetriever/StubGeoIp2InfoRetriever.php
// We extend GeoIp2InfoRetriever here because we're specialising its behaviour.
class StubGeoIp2InfoRetriever extends GeoIp2InfoRetriever
{
  public function retrieveFromIP( string $ip ): Info {
    return new Info(
      [], // $coordinates
      64512, // $asn: 64512 - 65534 are 16-bit ASNs reserved for private use
      'Wikimedia Foundation', // $organization

      // ...

    );
  }
}
phuedx renamed this task from Remove 'ipinfo' right from administrators on beta to Remove 'ipinfo' right from administrators on the Beta Cluster.Nov 29 2021, 6:09 PM

Just some thoughts on those two approaches:

  1. Always return data for a well-known IP address

Pro: realistic data, won't need updating if Info structure changes
Con: IP address might stop returning data in future versions

  1. Always return well-known fake data

Inverse of the above

Just some thoughts on those two approaches:

  1. Always return data for a well-known IP address

Pro: realistic data, won't need updating if Info structure changes
Con: IP address might stop returning data in future versions

  1. Always return well-known fake data

Inverse of the above

Unfortunately, we might not be able to do #1, because beta is missing so much IP data. For example, I have yet to find an IP which returns country information on beta.

We might have to do #2, even though we will have to be careful to keep updating its structure.

We might have to do #2, even though we will have to be careful to keep updating its structure.

I'm sure this will be fine - we can make sure it's well-commented. Thanks for looking into this @dom_walden

@Niharika Is this task still valid?

  • Administrators (users in the sysop group) on the Beta Cluster don't have the ipinfo right

Was there a legal reason to do this? It would make testing more difficult, as QA would need to wait for commits to arrive on testwiki, or test locally only.

If it is still needed, we should stall this task until we're no longer testing IPInfo on beta.

  • Users in the ipinfo group on testwiki have the ipinfo right

I think we can remove this, since that group doesn't exist any more

  • No users have the ipinfo-view-full on testwiki

Users do currently have this right on testwiki. Do we really want to remove it?

Thanks for catching this, Thalia. I don't think this task is valid as written anymore. I think we wrote this task when we said we will only give out IP Info to admins etc on testwiki and other wikis. We should be okay with the status quo for now. Maybe eventually when we have IP Info deployed everywhere, we can remove it from the beta cluster. How does that sound to you?

Thanks @Niharika - that sounds fine. I'll close this task if that's OK then, and we can make anew one for updating beta if and when we want to