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

Tighten validation for id fields (followup) #8069

Merged
merged 40 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
0ff9e93
Add validation and test to Resource
shashigharti Oct 19, 2023
482b796
Fix for failing test
shashigharti Oct 19, 2023
19937f0
Rename resource_id_validator function to id_validator
shashigharti Oct 19, 2023
7669403
Add validation for Resource View
shashigharti Oct 19, 2023
df1e66f
Add validator to check unique id value
shashigharti Oct 19, 2023
fd771ce
Add validation to group
shashigharti Oct 19, 2023
1b580c3
change test name
shashigharti Oct 29, 2023
3308546
Added validation for package extra
shashigharti Oct 30, 2023
7d6c4de
Added id validation for group extras
shashigharti Oct 30, 2023
f08e4d8
Add validation in tag
shashigharti Oct 31, 2023
946a07d
Add id validator for activity
shashigharti Oct 31, 2023
a230be1
Merge branch '7604/tighten-validation-for-id-fields' of https://githu…
amercader Feb 7, 2024
4f85b61
Add id_validator to datasets
amercader Feb 7, 2024
0c9daaa
Tag ids can not be set by users
amercader Feb 7, 2024
dcf84e8
Activity ids can not be set via the API
amercader Feb 7, 2024
393e17f
Consolidate order of the validators
amercader Feb 7, 2024
e3d6200
Separate validator for user id
amercader Feb 9, 2024
0ed63cc
Separate schema for extras show
amercader Feb 9, 2024
bb8e533
Better message for id validator
amercader Feb 9, 2024
9fbd0b9
Prevent further validation on empty_if_not_sysadmin
amercader Feb 9, 2024
18c19f5
No id validation on group show
amercader Feb 9, 2024
09c00c9
User id or name when updating
amercader Feb 9, 2024
6af7cd0
Fix validators for resource update
amercader Feb 13, 2024
ea1d824
Fix id length in test
amercader Feb 13, 2024
c48bc71
Enforce actual UUIDs as ids
amercader Feb 14, 2024
22e4cb5
Fix more uuid tests
amercader Feb 19, 2024
8922684
Simplify schema methods in IGroupForm
amercader Feb 19, 2024
24da1f1
Fix more uuid tests
amercader Feb 20, 2024
3eca61b
Fix schema for user form
amercader Feb 20, 2024
5868a7b
Remove empty_if_not_sysadmin from resource ids
amercader Feb 20, 2024
920a8df
Lint
amercader Feb 20, 2024
dc6bbad
Add changelog
amercader Feb 20, 2024
d9968bb
Merge branch 'master' into shashigharti-7604/tighten-validation-for-i…
amercader May 14, 2024
48cde8e
Re-add schema calls lost during last merge
amercader May 14, 2024
0c7daf9
Add schema methods to IGroupForm
amercader May 15, 2024
3758551
lint
amercader May 15, 2024
a7e16b1
Changelog tweaks
amercader May 17, 2024
bd720fd
Fall back to old IGroupForm methods for compatibility
amercader May 21, 2024
517a8ee
lint
amercader May 21, 2024
dbbf0fa
Better plugin schema methods checks
amercader May 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions changes/8069.migration
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
* Only sysadmins can now set the ``id`` field of Datasets, Groups, Organizations, Users, Resource Views and Extras
* If provided, the value of the ``id`` field needs to be a valid UUID v4 string. Sites using custom ids that are not UUIDs can uextend the relevant schema to override the validation on the ``id`` field, but are strongly encouraged to use a separate custom field to store the custom id instead.
amercader marked this conversation as resolved.
Show resolved Hide resolved
* The ``form_to_db_*`` and ``db_to_form_*`` methods of the ``IGroupForm`` interface are now deprecated, and have been replaced by``create_group_schema()``, ``update_group_schema()`` and ``show_group_schema()``.
41 changes: 32 additions & 9 deletions ckan/lib/plugins.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from flask import Blueprint

import ckan.logic.schema as schema
from ckan.lib.maintain import deprecated
from ckan.common import g
from ckan import logic, model, plugins
import ckan.authz
Expand Down Expand Up @@ -491,10 +492,25 @@ def bulk_process_template(self) -> str:
def group_form(self) -> str:
return 'group/new_group_form.html'

def create_group_schema(self) -> Schema:
return schema.default_create_group_schema()

def update_group_schema(self) -> Schema:
return schema.default_update_group_schema()

def show_group_schema(self) -> Schema:
return schema.default_show_group_schema()

# Deprecated schema methods, to be removed in CKAN 2.12

@deprecated(
"Use either `create_group_schema()` or `update_group_schema()`",
since="2.11.0"
)
def form_to_db_schema_options(self,
options: dict[str, Any]) -> dict[str, Any]:
''' This allows us to select different schemas for different
purpose eg via the web interface or via the api or creation vs
''' [Deprecated] This allows us to select different schemas for
Copy link
Contributor

Choose a reason for hiding this comment

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

Not only deprecated, these won't be called at all any more even as a fall-back, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

(looks like ckanext-scheming uses the validate method of IGroupForm it won't be affected by the change)

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct. I thought of leaving them for a version in case some plugin was using them but that's seems unlikely and maybe we can just remove them?

Copy link
Contributor

Choose a reason for hiding this comment

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

The polite thing would be to have a version released with the methods deprecated and working as a fall-back then remove them next release.

Marking them deprecated but actually broken doesn't make sense. Better to remove them and raise an error when a plugin tries to use them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to make sure @wardi , you are suggesting that in the actions, if an IGroupForm does not have one of the new methods (eg IGroupForm.create_group_schema() it should try to call the old equivalent, eg IGroupForm.form_to_db_schema() right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, with the deprecation warning and remove it the next release

Copy link
Member Author

Choose a reason for hiding this comment

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

different purpose eg via the web interface or via the api or creation vs
updating. It is optional and if not available form_to_db_schema
should be used.
If a context is provided, and it contains a schema, it will be
Expand All @@ -512,25 +528,32 @@ def form_to_db_schema_options(self,
else:
return self.form_to_db_schema()

@deprecated("Use `create_group_schema()`", since="2.11.0")
def form_to_db_schema_api_create(self) -> dict[str, Any]:
return schema.default_group_schema()
""" Deprecated """
return schema.default_create_group_schema()

@deprecated("Use `update_group_schema()`", since="2.11.0")
def form_to_db_schema_api_update(self) -> dict[str, Any]:
""" Deprecated """
return schema.default_update_group_schema()

@deprecated("Use `create_group_schema()`", since="2.11.0")
def form_to_db_schema(self) -> dict[str, Any]:
return schema.group_form_schema()
""" Deprecated """
return schema.default_create_group_schema()

@deprecated("Use `show_group_schema()`", since="2.11.0")
def db_to_form_schema(self) -> dict[str, Any]:
'''This is an interface to manipulate data from the database
into a format suitable for the form (optional)'''
""" Deprecated """
return {}

@deprecated("Use `show_group_schema()`", since="2.11.0")
def db_to_form_schema_options(self,
options: dict[str, Any]) -> dict[str, Any]:
'''This allows the selection of different schemas for different
purposes. It is optional and if not available, ``db_to_form_schema``
should be used.
''' [Deprecated] This allows the selection of different schemas
for different purposes. It is optional and if not available,
``db_to_form_schema`` should be used.
If a context is provided, and it contains a schema, it will be
returned.
'''
Expand Down
9 changes: 3 additions & 6 deletions ckan/logic/action/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -723,12 +723,9 @@ def _group_or_org_create(context: Context,
# get the schema
group_type = data_dict.get('type', 'organization' if is_org else 'group')
group_plugin = lib_plugins.lookup_group_plugin(group_type)
try:
schema: Schema = group_plugin.form_to_db_schema_options({
'type': 'create', 'api': 'api_version' in context,
'context': context})
except AttributeError:
schema = group_plugin.form_to_db_schema()

schema: Schema = context.get(
"schema") or group_plugin.create_group_schema()

data, errors = lib_plugins.plugin_validate(
group_plugin, context, data_dict, schema,
Expand Down
10 changes: 1 addition & 9 deletions ckan/logic/action/get.py
Original file line number Diff line number Diff line change
Expand Up @@ -1215,13 +1215,7 @@ def _group_or_org_show(
item.read(group)

group_plugin = lib_plugins.lookup_group_plugin(group_dict['type'])
try:
schema: Schema = group_plugin.db_to_form_schema_options({
'type': 'show',
'api': 'api_version' in context,
'context': context})
except AttributeError:
schema = group_plugin.db_to_form_schema()
schema: Schema = context.get("schema") or group_plugin.show_group_schema()

if include_followers:
context = plugins.toolkit.fresh_context(context)
Expand All @@ -1231,8 +1225,6 @@ def _group_or_org_show(
else:
group_dict['num_followers'] = 0

if not schema:
schema = ckan.logic.schema.default_show_group_schema()
group_dict, _errors = lib_plugins.plugin_validate(
group_plugin, context, group_dict, schema,
'organization_show' if is_org else 'group_show')
Expand Down
9 changes: 2 additions & 7 deletions ckan/logic/action/update.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import ckan.lib.app_globals as app_globals

from ckan.common import _, config
from ckan.types import Context, DataDict, ErrorDict
from ckan.types import Context, DataDict, ErrorDict, Schema

if TYPE_CHECKING:
import ckan.model as model_
Expand Down Expand Up @@ -662,12 +662,7 @@ def _group_or_org_update(

# get the schema
group_plugin = lib_plugins.lookup_group_plugin(group.type)
try:
schema = group_plugin.form_to_db_schema_options({'type': 'update',
'api': 'api_version' in context,
'context': context})
except AttributeError:
schema = group_plugin.form_to_db_schema()
schema: Schema = context.get("schema") or group_plugin.update_group_schema()

upload = uploader.get_uploader('group')
upload.update_data_dict(data_dict, 'image_url',
Expand Down
115 changes: 73 additions & 42 deletions ckan/logic/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from functools import wraps
from typing import Any, Callable, Iterable, cast

from ckan.lib.maintain import deprecated
import ckan.model
import ckan.plugins as plugins
from ckan.logic import get_validator
Expand Down Expand Up @@ -35,10 +36,10 @@ def default_resource_schema(
if_empty_guess_format: Validator, clean_format: Validator,
isodate: Validator, int_validator: Validator,
extras_valid_json: Validator, keep_extras: Validator,
resource_id_validator: Validator,
uuid_validator: Validator,
resource_id_does_not_exist: Validator) -> Schema:
return {
'id': [ignore_empty, resource_id_validator,
'id': [ignore_empty, uuid_validator,
resource_id_does_not_exist, unicode_safe],
'package_id': [ignore],
'url': [ignore_missing, unicode_safe, remove_whitespace],
Expand Down Expand Up @@ -66,6 +67,7 @@ def default_resource_schema(
@validator_args
def default_update_resource_schema():
schema = default_resource_schema()

return schema


Expand Down Expand Up @@ -123,11 +125,11 @@ def default_create_package_schema(
datasets_with_no_organization_cannot_be_private: Validator,
empty: Validator, tag_string_convert: Validator,
owner_org_validator: Validator, json_object: Validator,
ignore_not_sysadmin: Validator) -> Schema:
ignore_not_sysadmin: Validator, uuid_validator: Validator) -> Schema:
return {
'__before': [duplicate_extras_key, ignore],
'id': [empty_if_not_sysadmin, ignore_missing, unicode_safe,
package_id_does_not_exist],
'id': [ignore_missing, empty_if_not_sysadmin, uuid_validator,
package_id_does_not_exist, unicode_safe],
'name': [
not_empty, unicode_safe, name_validator, package_name_validator],
'title': [if_empty_same_as("name"), unicode_safe],
Expand Down Expand Up @@ -206,9 +208,12 @@ def default_show_package_schema(keep_extras: Validator,
schema.update({
'tags': {'__extras': [keep_extras]}})

schema["extras"] = default_show_extras_schema()

# Add several keys to the 'resources' subschema so they don't get stripped
# from the resource dicts by validation.
cast(Schema, schema['resources']).update({
'id': [not_empty],
'format': [ignore_missing, clean_format, unicode_safe],
'created': [ignore_missing],
'position': [not_empty],
Expand Down Expand Up @@ -265,16 +270,30 @@ def default_show_package_schema(keep_extras: Validator,
return schema


@deprecated(
"Use the relevant `default_{create|update|show}_group_schema` instead",
since="2.11.0"
)
def default_group_schema():
""" Deprecated """
return default_create_group_schema()


@validator_args
def default_group_schema(ignore_missing: Validator, unicode_safe: Validator,
ignore: Validator, not_empty: Validator,
name_validator: Validator,
group_name_validator: Validator,
package_id_or_name_exists: Validator,
no_loops_in_hierarchy: Validator,
ignore_not_group_admin: Validator) -> Schema:
def default_create_group_schema(
ignore_missing: Validator, unicode_safe: Validator,
ignore: Validator, not_empty: Validator,
name_validator: Validator,
group_name_validator: Validator,
package_id_or_name_exists: Validator,
no_loops_in_hierarchy: Validator,
empty_if_not_sysadmin: Validator,
uuid_validator: Validator,
group_id_does_not_exist: Validator,
ignore_not_group_admin: Validator) -> Schema:
return {
'id': [ignore_missing, unicode_safe],
'id': [ignore_missing, empty_if_not_sysadmin, uuid_validator,
group_id_does_not_exist, unicode_safe],
'name': [
not_empty, unicode_safe, name_validator, group_name_validator],
'title': [ignore_missing, unicode_safe],
Expand Down Expand Up @@ -309,38 +328,25 @@ def default_group_schema(ignore_missing: Validator, unicode_safe: Validator,


@validator_args
def group_form_schema(not_empty: Validator, unicode_safe: Validator,
ignore_missing: Validator, ignore: Validator):
schema = default_group_schema()
# schema['extras_validation'] = [duplicate_extras_key, ignore]
schema['packages'] = {
"name": [not_empty, unicode_safe],
"title": [ignore_missing],
"__extras": [ignore]
}
schema['users'] = {
"name": [not_empty, unicode_safe],
"capacity": [ignore_missing],
"__extras": [ignore]
}
return schema


@validator_args
def default_update_group_schema(ignore_missing: Validator,
group_name_validator: Validator,
unicode_safe: Validator):
schema = default_group_schema()
def default_update_group_schema(
ignore_missing: Validator,
not_empty: Validator,
group_id_exists: Validator,
group_name_validator: Validator,
unicode_safe: Validator):
schema = default_create_group_schema()
schema["id"] = [not_empty, group_id_exists, unicode_safe]
schema["name"] = [ignore_missing, group_name_validator, unicode_safe]
return schema


@validator_args
def default_show_group_schema(
keep_extras: Validator, ignore_missing: Validator):
schema = default_group_schema()
schema = default_create_group_schema()

# make default show schema behave like when run with no validation
schema['id'] = []
schema['num_followers'] = []
schema['created'] = []
schema['display_name'] = []
Expand All @@ -358,9 +364,10 @@ def default_show_group_schema(
def default_extras_schema(ignore: Validator, not_empty: Validator,
extra_key_not_in_root_schema: Validator,
unicode_safe: Validator, not_missing: Validator,
ignore_missing: Validator) -> Schema:
ignore_missing: Validator, uuid_validator: Validator,
empty_if_not_sysadmin: Validator) -> Schema:
return {
'id': [ignore],
'id': [ignore_missing, empty_if_not_sysadmin, uuid_validator],
'key': [not_empty, extra_key_not_in_root_schema, unicode_safe],
'value': [not_missing],
'state': [ignore],
Expand All @@ -370,6 +377,16 @@ def default_extras_schema(ignore: Validator, not_empty: Validator,
}


@validator_args
def default_show_extras_schema(ignore: Validator):

schema = default_extras_schema()

schema["id"] = [ignore]
wardi marked this conversation as resolved.
Show resolved Hide resolved

return schema


@validator_args
def default_relationship_schema(ignore_missing: Validator,
unicode_safe: Validator, not_empty: Validator,
Expand Down Expand Up @@ -422,9 +439,12 @@ def default_user_schema(
not_empty: Validator, strip_value: Validator,
email_validator: Validator, user_about_validator: Validator,
ignore: Validator, boolean_validator: Validator,
empty_if_not_sysadmin: Validator,
uuid_validator: Validator, user_id_does_not_exist: Validator,
json_object: Validator) -> Schema:
return {
'id': [ignore_missing, unicode_safe],
'id': [ignore_missing, empty_if_not_sysadmin, uuid_validator,
user_id_does_not_exist, unicode_safe],
'name': [
not_empty, name_validator, user_name_validator, unicode_safe],
'fullname': [ignore_missing, unicode_safe],
Expand Down Expand Up @@ -469,9 +489,11 @@ def user_new_form_schema(
@validator_args
def user_edit_form_schema(
ignore_missing: Validator, unicode_safe: Validator,
not_empty: Validator, user_id_or_name_exists: Validator,
user_password_validator: Validator, user_passwords_match: Validator):
schema = default_user_schema()

schema["id"] = [not_empty, user_id_or_name_exists, unicode_safe]
schema['password'] = [ignore_missing]
schema['password1'] = [ignore_missing, unicode_safe,
user_password_validator, user_passwords_match]
Expand All @@ -482,11 +504,13 @@ def user_edit_form_schema(

@validator_args
def default_update_user_schema(
not_empty: Validator, user_id_or_name_exists: Validator,
ignore_missing: Validator, name_validator: Validator,
user_name_validator: Validator, unicode_safe: Validator,
user_password_validator: Validator):
schema = default_user_schema()

schema["id"] = [not_empty, user_id_or_name_exists, unicode_safe]
schema['name'] = [
ignore_missing, name_validator, user_name_validator, unicode_safe]
schema['password'] = [
Expand Down Expand Up @@ -686,9 +710,15 @@ def default_create_resource_view_schema(resource_view: Any):
@validator_args
def default_create_resource_view_schema_unfiltered(
not_empty: Validator, resource_id_exists: Validator,
unicode_safe: Validator, ignore_missing: Validator, empty: Validator
unicode_safe: Validator, ignore_missing: Validator, empty: Validator,
empty_if_not_sysadmin: Validator,
ignore_empty: Validator,
uuid_validator: Validator,
resource_view_id_does_not_exist: Validator
) -> Schema:
return {
'id': [ignore_empty, empty_if_not_sysadmin, uuid_validator,
resource_view_id_does_not_exist, unicode_safe],
'resource_id': [not_empty, resource_id_exists],
'title': [not_empty, unicode_safe],
'description': [ignore_missing, unicode_safe],
Expand Down Expand Up @@ -724,10 +754,11 @@ def default_update_resource_view_schema_changes(not_missing: Validator,
unicode_safe: Validator,
resource_id_exists: Validator,
ignore: Validator,
ignore_missing: Validator
ignore_missing: Validator,
uuid_validator: Validator,
) -> Schema:
return {
'id': [not_missing, not_empty, unicode_safe],
'id': [not_missing, not_empty, uuid_validator, unicode_safe],
'resource_id': [ignore_missing, resource_id_exists],
'title': [ignore_missing, unicode_safe],
'view_type': [ignore], # cannot change after create
Expand Down