Tighten validation for id fields (followup)

[amercader]

Fixes #7604

Followup to the great work by @shashigharti in #7868

This opened several cans of worms, here's a summary of the work so far (still WIP)

Entity	empty_if_not_sysadmin	{entity}_id_does_not_exist	id_validator (Valid UUID)
Dataset	existing	existing	added
Resource	n/a (see #8069 (comment))	existing	existing
Resource View	added	added	added
Groups (org/groups)	added	added	added
User	added	added	added
Extras	added	n/a [1]	added
Activity	n/a [2]	n/a	n/a
Tag	n/a [3]	n/a	n/a

[1] model_save uses the package_id/group_id + extra key to create / update extras rows, besides there's one
single schema for dataset and group extras so it's not easy to check for existing ids in the relevant table

[2] Activity ids can not be set by any user via the API

[3] Tag ids can not be set by any users via the API

Two things to discuss

1. Id validator. The only version of this we have right now is the resource_id_validator which was introduced fairly recently:

def resource_id_validator(value):
    pattern = re.compile("[^0-9a-zA-Z _-]")
    if pattern.search(value):
        raise Invalid(_('Invalid characters in resource id'))
    if len(value) < 7 or len(value) > 100:
        raise Invalid(_('Invalid length for resource id'))
    return value

Do we want to stick with this for 2.11 or enforce true UUIDs going forward? Maybe we are more lenient (current validator) in some entities where people might have used some custom ones?

from uuid import UUID

def valid_uuid(value)
    try:
        UUID(value, version=4)
    except ValueError:
        raise Invalid(_("Invalid id provided"))

    return value

Decision: let's move forward with enforcing UUIDs. Users relying on custom ids can override their schemas to customize the validators used

2. Group and User schemas. The thing is a mess but basically the problem is that groups use the same schema for creation and updates so using a validator like empty_if_not_sysadmin is not going to work. There is an insane really confusing number of methods used to get the schemas, probably inherited from an ancient CKAN time (form_to_db_schema_options(), form_to_db_schema(), form_to_db_schema_api_create()...). Sean already simplified the dataset schemas a very long time ago, and it looks like we could do the same here. I'll keep digging.

Decision: Aim to simplify the methods on the group plugins and align them with the simplified dataset version. Check how that affects scheming. Need to check what's possible for users.

[amercader]

Update on the current status:

• UUIDs are now enforced in id fields (main commit is c48bc71, with some followups just to fix tests)
• Simplified schemas handling in IGroupForm (8922684)
• User schemas just required minor tweaks, I'll leave for a future PR simplifying them and implementing proper IUserForm support
• Resources need a special case, because of the way resources are created and updated (several action paths that all end up in package_update). Package updates will use the update_package_schema but it is difficult (impossible?) to use separate schemas for newly added resources (i.e. a create schema) and for existing ones (i.e. an update one), which might even be present in the same input data_dict. So we can not use the pattern of using a create schema with a "id can not exist" / "empty if not sysadmin" validator and an update schema with "id must exist" validator. So I think that for resources alone, we will need to remove the empty_if_not_sysadmin validator. I think that the safeguards provided by resource_id_does_not_exist should be safe enough (This validator should actually be called resource_id_does_not_exist_in_another_dataset)

[smotornyuk]

UUIDs are case-insensitive. How about returning str(uuid.UUID(value, version=4)) to normalize the value? In this way, we'll avoid creating entries with the same UUID written in different cases (we have a similar problem for user emails)

[wardi]

💯 this. A decade ago we imported an organization list with uppercase UUIDs and now they're mixed with new orgs created with lowercase UUIDs. So untidy.

[amercader]

This makes sense, but if we change the value returned we might change the case of some existing upper case UUIDs when editing objects, wouldn't that cause potential issues?

[amercader]

To add to this, this would only be an issue for resources and extras (both use the same schema for creates and updates for reasons explained above). The rest of entities don't use uuid_validator on updates. So if an old system created resource ids like 13857D04-A35B-4834-93F1-5DD98D401D44, when updating the resource in the new CKAN version it would change to 13857d04-a35b-4834-93f1-5dd98d401d44. Would that be an issue assuming it is mentioned in the changelog?

[amercader]

Confirmed, Bad Things™ happen when running this code in existing upper case resource ids:

ckanapi action package_create name=test_resource owner_org=test

ckanapi action resource_create package_id=test_resource id=C246F35C-7EAE-4F05-B971-99ED3A878B07

# Switch to code running with the uuid_validator version normalizing the value:

ckanapi action resource_update id=C246F35C-7EAE-4F05-B971-99ED3A878B07 description=test

ERROR [ckan.logic] Resource C246F35C-7EAE-4F05-B971-99ED3A878B07 exists but it is not found in the package it should belong to.
[...]
  File "/home/adria/dev/pyenvs/ckan-py3.9/ckan/ckan/logic/action/update.py", line 119, in resource_update                                 
    resource = _get_action('resource_show')(context, {'id': id})     
  File "/home/adria/dev/pyenvs/ckan-py3.9/ckan/ckan/logic/__init__.py", line 578, in wrapped                                              
    result = _action(context, data_dict, **kw)                       
  File "/home/adria/dev/pyenvs/ckan-py3.9/ckan/ckan/logic/action/get.py", line 1098, in resource_show                                     
    raise NotFound(_('Resource was not found.'))                     
ckan.logic.NotFound: Resource was not found.  

Looking at the DB, the existing upper case id resource was deleted and a new resource was created for the lower case version. There's a resource_show call at the end of resource_update which fails because it tries to search for the old upper case id (which is now deleted) in the dataset dict and fails.

So all in all, I don't think we can introduce this change with an extra database migration that lower cases all ids, and even then we might break external systems that rely on the existing upper case ids (although this might be fine if we document it)

[wardi]

So we're not going ahead with normalizing UUIDs for this iteration, right?

[amercader]

I think that if we want to normalize UUIDs we need to introduce a migration that changes all existing UUIDS to lowercase, or at least the resource ones to prevent the error I described, and clearly document it. I don't think its worth doing it given the potential for breakages. But what do you think?

[wardi]

Agreed, that clean up can be part of a future release. Maybe migrate our columns to the proper UUID type at the same time for better performance.

[amercader]

@wardi @smotornyuk any more comments on this?

[amercader]

Not sure why a ton of tests started failing after merging master 😭 I'll investigate

[amercader]

@smotornyuk these typing errors are related to the same changes we discussed in https://github.com/ckan/ckan/pull/7976/files#r1597518095

But in this case the methods like create_group_schema() are expected to be present. Do we need to add these methods to IGroupForm (right now they come from the DefaultGroupForm)? or just type: ignore?

[amercader]

All is green again, this is ready to go

[wardi]

Suggested change

	* 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.
	* 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 extend the relevant schema or validate method to override the validation on the ``id`` field, but are strongly encouraged to use a separate custom field to store the custom id instead.

[wardi]

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

[wardi]

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

[wardi]

does this strip the ids out of the extras on show? If so I love it.

[wardi]

Note that these don't completely protect us. If a user is sending multiple requests to create something with the same id it's possible for one to overwrite the other unless we pass a flag through to the save code to have postgres only create the record, on commit we'll reliably get an error if there was a conflict on one of the id fields. That's going to be too late for adding an error to the validation but at least we could prevent bad things from happening.

OTOH maybe all that's not worth the effort if the only users that can pass ids are sysadmins.