@peterhashair opened this Pull Request on October 4th 2022 Contributor

Description:

Fixes: #18055

update the status code to 401 when an invalid token in API

Review

@peterhashair commented on October 7th 2022 Contributor

Maybe the CHANGELOG wording needs an update.

@peterhashair commented on October 17th 2022 Contributor

Maybe we don't need this change.

   if ($referrer && $matomoUrl && Url::isValidHost(Url::getHostFromUrl($referrer)) &&
                strpos($referrer, $matomoUrl) === 0
            ) {
                Common::sendResponseCode(440);
@github-actions[bot] commented on October 26th 2022 Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@peterhashair commented on October 31st 2022 Contributor

@sgiehl yeah, removed from access.php. I tried another place that only affects API. I don't believe this is a perfect way, any suggestions?

@sgiehl commented on November 2nd 2022 Member

Checking the trace for a certain method doesn't seem to be something we should consider doing. Isn't the method throwing a certain exception that can be checked for?

In general I guess my approach would have been to convert HttpCodeException to an interface, and let the NoAccessException implement it, so we can simply use the http code check that already exists.

@peterhashair commented on November 3rd 2022 Contributor

@sgiehl it seems like the HttpCodeException used a couple of places, convert to an interface maybe broken some other code. I saw we have used HttpCodeExceptionin a way below, could we NoAccessException extend HttpCodeException but has 401 in the __construct, like the current PR, will that cause any regression?

https://github.com/matomo-org/matomo/blob/6b1fca27df7d96c316e60a2c4b45456cb0544809/core/Http/BadRequestException.php#L12-L18

@sgiehl commented on November 3rd 2022 Member

I described my approach that way on purpose. You can't simply change the NoAccessException to extend from HttpCodeException instead of InvalidRequestParameterException. The problem is, that we are catching InvalidRequestParameterException in some places. Currently that indirectly also catches any NoAccessException, as it inherits from it. This won't work now anymore.

@peterhashair commented on November 3rd 2022 Contributor

@sgiehl right, that makes sense. If we convert HttpCodeException into an interface then, what happened to the place that actually creates the class any suggestions? eg:

$ex = new HttpCodeException(Piwik::translate('Login_LoginNotAllowedBecauseUserLoginBlocked'), 403); 
@peterhashair commented on November 10th 2022 Contributor

ping @sgiehl

@sgiehl commented on November 10th 2022 Member

Guess we need to convert this into another exception that implements the new interface. Guess the NoAccessException might work there.

@peterhashair commented on November 15th 2022 Contributor

@sgiehl just checking instead of interface etc. how about just checking if if ($e instanceof NoAccessException) { then return 403 in API request.

@sgiehl commented on November 21st 2022 Member

That works for sure as well. But we are actually not using exceptions in a useful way. Moving the HttpCodeException to an interface, would have been a first step to improve that mess of exception handling. We should try to improve things when we are already working on it. And changing that exception stuff is a matter of half an hour or so. I'm actually not sure where the problem is, wasn't that almost finished?

Powered by GitHub Issue Mirror