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

Add limits for action functions v2 #4484

Merged
merged 12 commits into from
Nov 23, 2018
Merged

Add limits for action functions v2 #4484

merged 12 commits into from
Nov 23, 2018

Conversation

davidread
Copy link
Contributor

@davidread davidread commented Sep 28, 2018

Fixes #4480

Limits added

This PR adds limits to the following logic functions, configurable with the given configuration option:

  • package_search - ckan.search.rows_max
  • group/organization_show - number of packages limited by ckan.search.rows_max
  • group/organization_list (and see all_fields issue organization_list / group_list [performance] in CKAN 2.6 #3511) - ckan.group_and_organization_list_max / ckan.group_and_organization_list_all_fields_max
  • recently_changed_packages_activity_list (_html) - ckan.activity_list_limit_max
  • package/user/group/organization_activity_list (_html) - ckan.activity_list_limit_max
  • dashboard_activity_list (_html) - ckan.activity_list_limit_max

I've also done a backport for CKAN 2.7, should anyone want to apply it to their own ckan fork:
2.7...4484-limits-backport-2.7

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Please [X] all the boxes above that apply


params['rows'] = min(
int(params.get('rows', 10)),
int(config.get('ckan.search.rows_max', 1000)))
Copy link
Contributor Author

@davidread davidread Sep 28, 2018

Choose a reason for hiding this comment

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

I needed to add limit here, because it bypasses the logic function and therefore the validator (which now does the limiting when calling package_search)

@davidread davidread changed the title Add limits for action functions v2 [WIP] Add limits for action functions v2 Sep 28, 2018
@davidread
Copy link
Contributor Author

@wardi it would be great to get this merged, if you have a chance to review?

@wardi wardi merged commit 5917388 into master Nov 23, 2018
@wardi
Copy link
Contributor

wardi commented Nov 23, 2018

Well done! This change was sorely needed.

@davidread davidread deleted the 4480-limits-v2 branch November 23, 2018 14:30
jguo144 pushed a commit to OpenGov-OpenData/ckan that referenced this pull request Jan 4, 2019
jguo144 added a commit to OpenGov-OpenData/ckan that referenced this pull request Jan 9, 2019
* Add download dropdown

* [ckan#4497] connect js, route, form

* [ckan#4497] implement download redirect

* [ckan#4497] unicode literal

* use request.params for 2.7 compatibility

* [ckan#4462] _get_types: pass connection instead of context

* [ckan#4462] _get_type: pass connection instead of context

* [ckan#4462] _get_fields: pass connection, resource_id instead of context, data_dict

* [ckan#4462] _get_fields_types: pass connection, resource_id instead of context, data_dict

* [ckan#4462] datastore_search: partial result fields fix

* [ckan#4462] fix reuse of variable name

* [ckan#4462] _textsearch_query: lang, q, plain instead of data_dict

* [ckan#4462] datastore_search: exclude rank columns if not in fields

* [ckan#4462] _result_fields: remove unused parameter

* revert string_types change

* revert string_types change

* Get visible columns

* [ckan#4497] download visible columns

* Remove _id column from filtered download

* [ckan#4462] _textsearch_query: lang, q, plain instead of data_dict

* [ckan#4497] s/const/var for js compatibility

* [ckan#4484] manually backported to ckan 2.7.x (#8)

Add limits for action functions

* Add limits to datastore_search(_sql) (#16)

* Backport of 4561-limit-datastore_search 74d8fb2

* CircleCI update from master

* Nop

* Fix tests for opengov branch

The _id column is removed from datastore dumps in this opengov branch
because:
opengov commit: co0adf16426113394af42bbacc4644f039ebe9e2ec
+    field_list = [f['id'] for f in rec['fields'] if f['id'] != '_id']

also some minor fixes

* Use pop() for efficiency
jguo144 added a commit to OpenGov-OpenData/ckan that referenced this pull request Jan 9, 2019
* Add download dropdown

* [ckan#4497] connect js, route, form

* [ckan#4497] implement download redirect

* [ckan#4497] unicode literal

* use request.params for 2.7 compatibility

* [ckan#4462] _get_types: pass connection instead of context

* [ckan#4462] _get_type: pass connection instead of context

* [ckan#4462] _get_fields: pass connection, resource_id instead of context, data_dict

* [ckan#4462] _get_fields_types: pass connection, resource_id instead of context, data_dict

* [ckan#4462] datastore_search: partial result fields fix

* [ckan#4462] fix reuse of variable name

* [ckan#4462] _textsearch_query: lang, q, plain instead of data_dict

* [ckan#4462] datastore_search: exclude rank columns if not in fields

* [ckan#4462] _result_fields: remove unused parameter

* revert string_types change

* revert string_types change

* Get visible columns

* [ckan#4497] download visible columns

* Remove _id column from filtered download

* [ckan#4462] _textsearch_query: lang, q, plain instead of data_dict

* [ckan#4497] s/const/var for js compatibility

* [ckan#4484] manually backported to ckan 2.7.x (#8)

Add limits for action functions

* Add limits to datastore_search(_sql) (#16)

* Backport of 4561-limit-datastore_search 74d8fb2

* CircleCI update from master

* Nop

* Fix tests for opengov branch

The _id column is removed from datastore dumps in this opengov branch
because:
opengov commit: co0adf16426113394af42bbacc4644f039ebe9e2ec
+    field_list = [f['id'] for f in rec['fields'] if f['id'] != '_id']

also some minor fixes

* Use pop() for efficiency
@davidread
Copy link
Contributor Author

@ThrawnCA I have sympathy for this point of view - it does break functionality. However we were keen to add it as a patch for earlier versions, because it was seen as a DOS vector. The default limits are so low, as to try and make it obvious they are limited.

@ThrawnCA
Copy link
Contributor

The default limits are so low, as to try and make it obvious they are limited.

That's a nice thought, but since it still truncates silently, and still allows enough to eg populate a dropdown list enough to need significant scrolling down, it really isn't obvious unless someone was already specifically looking for it.

@davidread
Copy link
Contributor Author

Agreed, it's problematic for the use case you describe. We had some awareness of this sort of thing when we designed it, balanced against the other concerns mentioned. The questions are whether that balance is wrong and what might be done now.

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.

Add limits for action functions
3 participants