+
Skip to content

ADX-968 Bypass auth when checking if fork is synced #20

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

Merged
merged 2 commits into from
Feb 2, 2023

Conversation

jonathansberry
Copy link
Member

@jonathansberry jonathansberry commented Feb 1, 2023

Sierra Leone dataset was throwing a 404 not found error / authorization error when trying to simply view it. See the Jira ticket for more details of the bug symptoms.

The bug was caused by ckanext-fork logic:

  • A resource had been forked from an upstream resource that the user does not have access to.
  • When calling package_show action, ckanext-fork's chained action was checking if the fork was "synced" with upstream (aka the files match).
  • To do this it was calling resource_show for the upstream resource, to get it's sha256
  • This was throwing an unauthorized error which was uncaught, therefore cascading to the top of the stack.

The proposed solution is to simply bypass auth when checking if a fork is synced with upstream. So that wherever a forked resource exists, it is possible for everyone (not just those with access to the upstream resource) to know if the fork is synced with upstream or not.

Chas and I have discussed and both feel this is ok. But we would value @fulior's thoughts as well. Bypassing auth always feels a little tricksy.

Testing

A regression test is written that simply checks the user can view a resource forked from a private dataset. It breaks without the fix and passes with the fix.

Checklist

Put an x in the boxes that apply to this pull request (you can also fill these out after opening the pull request).
You may not need to check all boxes.

  • The Jira ticket for this issue has been updated to "Ready to Review" or equivalent.
  • I have developed these changes in discussion with the appropriate project manager.
  • My code follows the general Fjelltopp documentation (see Confluence).
  • I have made corresponding changes to the Fjelltopp documentation (see Confluence).
  • I have rebased this branch with master.
  • New dependency changes have been committed.
  • I have added automated tests that prove my fix is effective or that my feature works.
  • New and existing tests pass locally with my changes.
  • My changes generate no new warnings.
  • I have performed a self-review of my own code.
  • I have assigned at least one reviewer.

@jonathansberry jonathansberry requested a review from fulior February 1, 2023 15:43
Copy link
Member

@fulior fulior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case it's safe to ignore authentication, we're only comparing a checksum here.

@fulior
Copy link
Member

fulior commented Feb 2, 2023

btw, shouldn't we fix compatibility matrix? It's currently showing 2.9 as "Not tested"
image

@jonathansberry jonathansberry merged commit 784ef47 into development Feb 2, 2023
@jonathansberry jonathansberry deleted the ADX-968-dataset-not-found branch February 2, 2023 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载