Return status code 401 for invalid tokens in API requests #19813

peterhashair · 2022-10-04T22:14:43Z

Description:

Fixes: #18055

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

Review

send respond 401 when invalid token provide

update status code

# Conflicts: # CHANGELOG.md

update status code

peterhashair · 2022-10-07T02:05:00Z

Maybe the CHANGELOG wording needs an update.

add tests

fix phpcs

revert code format changes

add status code test on token invalid

peterhashair · 2022-10-17T01:24:56Z

Maybe we don't need this change.

   if ($referrer && $matomoUrl && Url::isValidHost(Url::getHostFromUrl($referrer)) &&
                strpos($referrer, $matomoUrl) === 0
            ) {
                Common::sendResponseCode(440);

github-actions · 2022-10-26T02:13:00Z

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

sgiehl

Maybe better give this one a new try. It can't / doesn't work the way it's implemented.

On the login page, the CSS fails to load, as it results in a 401 response
The 440 response code is actually a special IIS code, not sure if we should use it, as no one might expect it
Currently any method using one of the Access::check* methods might end up in a 401 response, which also affects the UI and not only API
You should not adjust the response code in Access class, as you can't know if exceptions thrown there, are not caught and ignored.

revert access change Api responseBuillder

update tests

peterhashair · 2022-10-31T22:39:48Z

@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 · 2022-11-02T10:30:48Z

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 · 2022-11-03T00:47:45Z

@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?

matomo/core/Http/BadRequestException.php

Lines 12 to 18 in 6b1fca2

    
           class BadRequestException extends HttpCodeException 
        
           { 
        
               public function __construct($message) 
        
               { 
        
                   parent::__construct($message, $code = 400); 
        
               } 
        
           }

CHANGELOG.md

update from 403 to 401

github-actions · 2022-12-29T01:57:04Z

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

bx80 · 2023-01-16T00:00:27Z

@sgiehl Was the update to the changelog the only outstanding issue you had with this PR?

sgiehl · 2023-01-16T13:35:29Z

I think so. But we might still need to ensure that this change only affects those parts is was meant for.

github-actions · 2023-01-24T01:57:52Z

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

bx80 · 2023-02-01T22:25:54Z

@sgiehl I've fixed the integration test which now correctly checks for a 401 if an invalid token is used. All other access tests are passing. Can you give this a (hopefully) final review when you get a chance?

sgiehl

There are some more changes needed

core/Exception/InvalidRequestParameterException.php

core/NoAccessException.php

core/Http/BadRequestException.php

send respond 401 when invalid token provide

890fa05

send respond 401 when invalid token provide

peterhashair changed the base branch from 4.x-dev to 5.x-dev October 5, 2022 08:19

Peter added 3 commits October 6, 2022 13:51

update status code

6ebea2c

update status code

Merge branch '5.x-dev' into m18055

f69dda4

# Conflicts: # CHANGELOG.md

update status code

bf7e830

update status code

peterhashair added this to the 5.0.0 milestone Oct 7, 2022

peterhashair added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Oct 7, 2022

peterhashair linked an issue Oct 13, 2022 that may be closed by this pull request

Request with invalid token_auth responds 200 OK #18055

Closed

Peter added 4 commits October 14, 2022 12:16

add tests

55320ab

add tests

fix phpcs

4f0d19d

fix phpcs

revert code format changes

86b4a22

revert code format changes

add status code test on token invalid

1dfdab4

add status code test on token invalid

peterhashair added the Needs Review PRs that need a code review label Oct 19, 2022

github-actions bot added the Stale The label used by the Close Stale Issues action label Oct 26, 2022

peterhashair removed the Stale The label used by the Close Stale Issues action label Oct 26, 2022

sgiehl requested changes Oct 28, 2022

View reviewed changes

sgiehl removed the Needs Review PRs that need a code review label Oct 28, 2022

Peter added 2 commits November 1, 2022 10:08

revert access change Api responseBuillder

90f6c53

revert access change Api responseBuillder

update tests

6f58ad4

update tests

update NoAccessException to HttpCodeException

d09b33c

peterhashair added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Nov 3, 2022

github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 19, 2022

Merge branch '5.x-dev' into m18055

1b55687

sgiehl requested changes Dec 19, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

update from 403 to 401

dcc2591

update from 403 to 401

peterhashair removed the Stale The label used by the Close Stale Issues action label Dec 21, 2022

github-actions bot added the Stale The label used by the Close Stale Issues action label Dec 29, 2022

mattab added the 5.0.0 label Jan 4, 2023

Merge branch '5.x-dev' into m18055

0c1693a

bx80 removed the Stale The label used by the Close Stale Issues action label Jan 16, 2023

github-actions bot added the Stale The label used by the Close Stale Issues action label Jan 24, 2023

bx80 added 2 commits February 2, 2023 10:18

Merge branch '5.x-dev' into m18055

e65c93e

Fixed test

dc927e8

bx80 changed the title ~~update status code to 401 when invalid token in API~~ Return a 401 error code for invalid tokens in API requests Feb 1, 2023

bx80 requested a review from sgiehl February 1, 2023 22:26

bx80 removed the Stale The label used by the Close Stale Issues action label Feb 1, 2023

Merge branch '5.x-dev' into m18055

41eb15d

sgiehl requested changes Feb 2, 2023

View reviewed changes

core/Exception/InvalidRequestParameterException.php Outdated Show resolved Hide resolved

core/NoAccessException.php Outdated Show resolved Hide resolved

core/NoAccessException.php Outdated Show resolved Hide resolved

core/Http/BadRequestException.php Outdated Show resolved Hide resolved

bx80 added 2 commits February 3, 2023 13:20

Adjust class interface implementations and constructors

9690e63

Merge remote-tracking branch 'origin/m18055' into m18055

aaac007

bx80 requested a review from sgiehl February 3, 2023 01:55

sgiehl approved these changes Feb 3, 2023

View reviewed changes

sgiehl merged commit 8eb30ef into 5.x-dev Feb 3, 2023

sgiehl deleted the m18055 branch February 3, 2023 08:41

sgiehl changed the title ~~Return a 401 error code for invalid tokens in API requests~~ Return status code 401 for invalid tokens in API requests May 16, 2023

sgiehl mentioned this pull request Sep 25, 2023

Ensure response codes are not set for nested API calls #21300

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return status code 401 for invalid tokens in API requests #19813

Return status code 401 for invalid tokens in API requests #19813

peterhashair commented Oct 4, 2022 •

edited

peterhashair commented Oct 7, 2022 •

edited

peterhashair commented Oct 17, 2022

github-actions bot commented Oct 26, 2022

sgiehl left a comment

peterhashair commented Oct 31, 2022

sgiehl commented Nov 2, 2022

peterhashair commented Nov 3, 2022 •

edited

github-actions bot commented Dec 29, 2022

bx80 commented Jan 16, 2023

sgiehl commented Jan 16, 2023

github-actions bot commented Jan 24, 2023

bx80 commented Feb 1, 2023

sgiehl left a comment

Return status code 401 for invalid tokens in API requests #19813

Return status code 401 for invalid tokens in API requests #19813

Conversation

peterhashair commented Oct 4, 2022 • edited

Description:

Review

peterhashair commented Oct 7, 2022 • edited

peterhashair commented Oct 17, 2022

github-actions bot commented Oct 26, 2022

sgiehl left a comment

Choose a reason for hiding this comment

peterhashair commented Oct 31, 2022

sgiehl commented Nov 2, 2022

peterhashair commented Nov 3, 2022 • edited

github-actions bot commented Dec 29, 2022

bx80 commented Jan 16, 2023

sgiehl commented Jan 16, 2023

github-actions bot commented Jan 24, 2023

bx80 commented Feb 1, 2023

sgiehl left a comment

Choose a reason for hiding this comment

peterhashair commented Oct 4, 2022 •

edited

peterhashair commented Oct 7, 2022 •

edited

peterhashair commented Nov 3, 2022 •

edited