Skip to content
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

input: Add disable direct web fetch by default #144

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

michaelwood
Copy link
Member

This adds the setting allow_direct_web_fetch which configures whether the functionality to allow using a web direct fetch i.e. not via a form on the frontend but via a direct GET parameter to be fetched from will be allowed.

Re-instate csrf_protect

This adds the setting `allow_direct_web_fetch` which configures whether
the functionality to allow using a web direct fetch i.e. not via
a form on the frontend but via a direct GET parameter to be fetched from
will be allowed.

Re-instate csrf_protect
@michaelwood
Copy link
Member Author

fyi @jpmckinney

@odscjames if there is nothing further needed right now shall I make a minor release with associated changelog/tag etc?

@jpmckinney
Copy link
Contributor

Thanks!

Several CoVE's have a link on the homepage to submit some sample data. For OCDS, we can just remove that link, but not sure if you wanted to develop some way to preserve it (e.g. fill in the URL and submit the form via JS).

@jpmckinney
Copy link
Contributor

Also, curious, why is this set via COVE_CONFIG and not settings at top-level? COVE_CONFIG gets passed to lib-cove-*, which doesn't (need to) understand this option.

@michaelwood
Copy link
Member Author

michaelwood commented Sep 26, 2024

Thanks!

Several CoVE's have a link on the homepage to submit some sample data. For OCDS, we can just remove that link, but not sure if you wanted to develop some way to preserve it (e.g. fill in the URL and submit the form via JS).

Ah right, yeah agree a click handler in JS on the link would be the simplest solution to maintain functionality with the new settings in the default off state, alternatively just expect people to download and upload the sample (which might be better to save some cycles when the site is being web crawled), AFAIK the "preview a sample" is standard specific so I'll check the other users of lib-cove-web.

Also, curious, why is this set via COVE_CONFIG and not settings at top-level? COVE_CONFIG gets passed to lib-cove-*, which doesn't (need to) understand this option.

My only thought was that it sits logically with input_methods (i.e. https://github.com/OpenDataServices/lib-cove-web/blob/main/cove/context_processors.py#L6 ), happy to move config item up a level if it makes more sense. I'm not entirely clear on what the lines of separation in cove are/intended to be.

@odscjames
Copy link
Collaborator

Can we have something in the changelog? Thanks!

@odscjames
Copy link
Collaborator

alternatively just expect people to download and upload the sample (which might be better to save some cycles when the site is being web crawled)

That seems like a worse UI solution that would just put people off. I think we need to keep the link option somehow.

This is way beyond the scope of this PR but if web-cycles and crawl performance is a concern what we really want is not to re-process the data any time a user clicks the link. Instead there could be some special way to mark this data as sample and it's not expired and removed like all the other uploads are. We process the sample data once then every user just sees the same set of cached results. (We'd have to make sure we reprocessed the sample when something in the data processing mechanism changed - I've done this in other code already).

@odscjames
Copy link
Collaborator

fill in the URL and submit the form via JS

Also (Psuedo code):

<form>
{% csrf_token %}
<input type="hidden" name="source_url" value="...">
<input type="submit" value="See a sample with ..."
</form>

There are ways we can style buttons as links - or just leave it as buttons, I don't think that's so bad.

@odscjames
Copy link
Collaborator

I think ideally the changelog should be on the same commit as the code, as that way when someone looks later and uses git blame it's easy to find the code and the intention behind the change too. As this is a small change tho I think it's ok.

The rest looks good to me - I don't have strong views on which level the config should sit so happy to go with this.

@michaelwood michaelwood merged commit 6dfb551 into main Oct 3, 2024
20 checks passed
@michaelwood michaelwood deleted the mw/disable_direct_web_fetch_by_default branch October 3, 2024 15:06
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.

3 participants