Bug or feature? - Invalid api token results in access denied to public objects

[landreev]

I can see how one can argue that this is the correct behavior. But at the very least, this appears to have been an undocumented change in behavior. We noticed that sometime between 5.9 and 6.0 /api/access/datafile/...?key=null started to result in a 403 on a public file, so many features in Data Explorer v2 are not working in 6.0 because that's what it sends underneath.

[pdurbin]

this appears to have been an undocumented change in behavior

I propose we start an API changelog for these:

• section in API Guide for changes in behavior and backward incompatibilities  #10060

Here's the related issue on the Data Explorer v2 side:

• bad API key when clicking "view categories" on a public tabular file scholarsportal/dataverse-data-explorer-v2#29

[cmbz]

2023/10/31: as per exchange with @scolapasta I have added the issue to the Global Backlog for sizing. I labeled it "Feature: API" although it's somewhat more of a bug than feature.

[cmbz]

2023/11/6: Group discussion following the standup suggests that we need to decide on a strategy before sizing. Either 1) simply document the behavior so that applications that assume they can send inauthentic tokens for public datasets are alerted, or 2) modify the behavior itself, which requires development. I'm not certain who should decide: @landreev, @pdurbin, or @scolapasta can someone weigh in and decide next steps?

[qqmyers]

+1 to document and fail when bad tokens are sent

[landreev]

I too vote +1 for sticking with the current behavior, while documenting it in the guide and in the next release note.
I'm happy to point any other instance that runs into this problem with the Explorer v2 to the local issue where I documented the apache rewrite line that fixes it. But then it's probably safe to assume that it will be fixed in the Explorer very soon too.

[pdurbin]

+1 to the above. Let's also add a test asserting the current behavior.

[landreev]

... an extra +1 for adding a test. Could still be a 3 (?).

[scolapasta]

+1 let's go with this approach

[jp-tosca]

Are there any other changes to the API that would be good to include on this?

I made this quickly just so we can have some input, I was thinking of adding this at the top of the guide and with a format similar to this:

@scolapasta @pdurbin @cmbz

Didn't want to ping everyone but feel free to let me know if I should ping someone else.

[jp-tosca]

@scolapasta mentioned yesterday it was only documentation but, @pdurbin, @landreev mentioned a test. Should I also work on that?
😃

[pdurbin]

My main feedback is that instead of a date, it should be a version. Note that Leonid said above, "We noticed that sometime between 5.9 and 6.0."

Also, I'd put the changelog lower down, maybe even the last page (after the FAQ).

[jp-tosca]

Hi, @landreev just wanted to check with you since you created this thread. In your initial comment, you mention that is a 403 but with the testing I have done I am getting a 401, just wanted to check I am looking at the right thing.

Also, there is already a test in place so probably will just have to modify AccessIT to add this condition.

[luddaniel]

We are currently preparing the migration to v5.14 from v5.12.1 and we jumped on the same issue, so thank you for all the investigation and work done to resolve it.

Here are some additional information for the changelog.rst of PR #10127 :

This issue is related to the commit : e8163b v5.14

If you understand Java, here is the explanation :

@AuthRequired will fill ContainerRequestFilter via AuthFilter
AuthFilter.filter function will try to find the authentication mechanism if there is one and when ApiKeyAuthMechanism.findUserFromRequest calls ApiKeyAuthMechanism.getRequestApiKey it will retrieve a String "null" for key=null and the code will throw new WrappedAuthErrorResponse(RESPONSE_MESSAGE_BAD_API_KEY); ("Bad API key")

This code is completely okay, I join you all on the idea of keep it that way and fix it in Data Explorer v2 scholarsportal/dataverse-data-explorer-v2#29
Maybe @AuthRequired is too much because checkAuthorization(getRequestUser(crc), df); will give you a granted access as long as the file is released, non restricted, not embargoed.

We decided to implement RewriteRule also but faced a weird case where key=null appears 2 times key=null&variables=v289,v290&apikey=nulleable&key=null
So our RewriteRule uses N flag to repear the removal of key=null

RewriteCond %{QUERY_STRING} ^(.+?&|)key=null(?:&(.*)|)$ [NC]
RewriteRule ^ %{REQUEST_URI}?%1%2 [N,R]

Regards

[stevenferey]

For information, we have adapted the rule to handle a case generating an error:Downloading "Tab-delimited file" from DataExplorer. Generated URL is &key=null

RewriteCond %{QUERY_STRING} ^(.*?&|)key=null(?:&(.*)|)$ [NC]
RewriteRule ^ %{REQUEST_URI}?%1%2 [N,R]