Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add option to disable ISP DB even if it exists in the filesystem #20027

Merged
merged 6 commits into from Nov 23, 2022
Merged

Add option to disable ISP DB even if it exists in the filesystem #20027

merged 6 commits into from Nov 23, 2022

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Nov 18, 2022 •
edited

Description:

refs CLOUD-710. @sgiehl do you reckon something like this could be an OK solution to disable the ISP/ASN DB even if it is present in the filesystem? I'm trying to add this for multi tenant install on Cloud where we will have the file present in the filesystem but only want it to be used by very few accounts. I tried playing around by pointing path.geoip2 into a different directory depending on the tenant but this proved very difficult in a cloud setup and ensuring files are being kept up to date etc. This would make it a lot simpler.
I haven't fully tested it but do you reckon that would work and be ok like this?

If that solution would be fine I could try and add some test for it.

Review

@tsteur tsteur requested a review from sgiehl November 18, 2022 02:54
@tsteur tsteur added the Needs Review PRs that need a code review label Nov 18, 2022
@sgiehl
Copy link
Member

sgiehl commented Nov 21, 2022 •
edited

As this is no official config option and for now only being used for cloud, I guess we could go with that solution.
In general I think we should improve that geoip stuff overall. Imho the geoip provider should show all databases that were found for each type (location & isp), with the possibility to choose the ones to use. That way it would be possible to e.g. disable one of them or choose e.g. a country database even though a city database is available.
In theory we could even do it in a way that allows to change the used database for each site separately.
But guess those changes are a bit out of scope for now.

@tsteur
Copy link
Member Author

tsteur commented Nov 22, 2022

Awesome thanks. I've just done some manual testing and it worked. I couldn't really figure out a way to test it though. Do you have any idea? Or maybe it doesn't need one?

@tsteur tsteur added this to the 4.12.5 milestone Nov 22, 2022
@sgiehl
Copy link
Member

sgiehl commented Nov 22, 2022

We could perform such tests with disabled isp and check that there aren't any results:

matomo/plugins/GeoIp2/tests/Integration/LocationProviderTest.php

Lines 94 to 114 in d1e15c2

public function testGeoIP2ASN()
{
$locationProvider = new GeoIp2\Php(['loc' => [], 'isp' => ['GeoLite2-ASN.mmdb']]);
$result = $locationProvider->getLocation(['ip' => '194.57.91.215']);
$this->assertEquals([
'isp' => 'Matomo Internet',
'org' => 'Matomo Internet',
], $result);
}
public function testGeoIP2ISP()
{
$locationProvider = new GeoIp2\Php(['loc' => [], 'isp' => ['GeoIP2-ISP.mmdb']]);
$result = $locationProvider->getLocation(['ip' => '194.57.91.215']);
$this->assertEquals([
'isp' => 'Matomo Internet',
'org' => 'Innocraft'
], $result);
}

@tsteur
Copy link
Member Author

tsteur commented Nov 22, 2022

Awesome, thanks for this @sgiehl . I've added some tests

@sgiehl sgiehl merged commit 533500d into 4.x-dev Nov 23, 2022
@sgiehl sgiehl deleted the geoipispenable branch November 23, 2022 15:00
@justinvelluppillai justinvelluppillai modified the milestones: 4.12.5, 4.13.0 Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
4.13.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tsteur @sgiehl @justinvelluppillai