Mika56 / PHP-SPF-Check

Simple library to check an IP address against a domain's SPF record
MIT License
44 stars 25 forks source link

Version 1 -> 2 upgrade: Usage advice for accessing requestCount. #47

Closed coopan closed 1 year ago

coopan commented 1 year ago

Hi there. Thanks for the great lib. We are just upgrading our instance from 1.1.6 to latest and see there's a little bit of architectural change. I've gone through the Upgrade.md and sorted all of that.

We have a slightly unconventional implementation where we want to control our own request limits. So under 1.1.6 we extended your DnsRecordGetter and used $requestCount there to do that.

use Mika56\SPFCheck\DNS\DNSRecordGetter as BaseDNSRecordGetter;
use Mika56\SPFCheck\Exception\DNSLookupLimitReachedException;

class DNSRecordGetter extends BaseDNSRecordGetter
{
    public const LIMIT = 20;

    public function countRequest()
    {
        if (++$this->requestCount > self::LIMIT) {
            throw new DNSLookupLimitReachedException();
        }
    }

    public function getRequestCount(): int
    {
        return $this->requestCount;
    }
}

I see that all of this stuff has moved from old src/DNSRecordGetter.php to the new src/DNS/Session.php class. So this obviously errors due to requestCount not existing.

What's the best way to do this now? It looks like the Result object contains a Session object. And I see on the Readme that getIPStringResult() returns a Result object. Should I just access this property that way? Or is there a better way to do the above entirely?

Thanks again.

Mika56 commented 1 year ago

Hi, Are you looking to get the number of DNS queries, or are you looking to increase it? Your code example increased it to 20. I'm fairly certain 10 is the general recommendation, not an imposed number. As such, I see no objection in making it configurable.

coopan commented 1 year ago

Both, aside from overriding the hard coded limit, we are also calling getResultCount() in implementation code for auditing.

Configurable limit would be good unless there is a major reason for it to always be 10. But being able to requestCount as we did with the old version would cover other bases for us (and solve our limit issue in the meantime).

coopan commented 1 year ago

Looks like I was mistaken about getting result count from Session now that those methods are migrated there. I think there's outdated instructions on your Readme for the latest version:

If you want to get more details about the check, you can use SPFCheck::getIPResult(string $ipAddress, string $domainName): Result which will return a Result object with more details about the check.

That method doesn't exist anymore. There's only getIPStringResult per the Update.md file unless I'm missing something. And that just returns a short string. Am I correct that these methods that moved to Session are pretty much locked away from external use?

Mika56 commented 1 year ago

The README is indeed wrong, getIPResult does not exist, that shoud say getResult. The UPGRADE is however correct, isIPAllowed has been replaced by getIPStringResult, and getDomainSPFRecords and getResult have been added

Mika56 commented 1 year ago

You should be able to set custom lookup limits and properly get the number of lookups in v2.1.0. Please see release notes for details: https://github.com/Mika56/PHP-SPF-Check/releases/tag/2.1.0 Feel free to reopen issue if the new release does not answer to your problem completely