Feature/486 token permissions configuration #497
Conversation
danielmursa-dev
requested review from
stevenbal,
swrichards,
Coperh and
SonnyBA
danielmursa-dev
changed the base branch from
master
to
feature/485-token-permissions-setup-configuration
| ) | ||
| ) | ||
|
|
||
| def test_invalid_permissions_type(self): |
There was a problem hiding this comment.
Although thoroughness is generally a virtue, I don't think it's per se necessary to test validation failures (e.g. that an empty object type does not validate as UUID) or that the steps are not run if an exception is raised. These are basically guaranteed by the library. If you specifically want to guard against a regression (e.g. somebody changing a field's type, or making it required/optional), you could assert against your model definitions directly using the model_fields class attribute (example).
I would focus instead on testing the actual logic of your step (as you do above and also below): does you step result in the expected models? Is it idempotent? Does it handle update/create flows well?
| "fields": permission.fields, | ||
| } | ||
| except ObjectDoesNotExist as exception: | ||
| raise PrerequisiteFailed(step=self, validation_error=exception) |
There was a problem hiding this comment.
You can just use ConfigurationRunFailed here: PrerequisiteFailed is reserved mainly for a malformed YAML (but not per se for a bad values). I would give it a user-friendly error string: f"Object type with {permission.object_type} does not exist"
| raise PrerequisiteFailed(step=self, validation_error=exception) | ||
|
|
||
| permission_instance = Permission(**permission_kwargs) | ||
| self._full_clean(permission_instance) |
There was a problem hiding this comment.
I'm not sure if this is validated by the model, but it should only be possible to use fields based authorization for mode read_only (according to the docs https://objects-and-objecttypes-api.readthedocs.io/en/latest/admin/authorization.html#add-a-permission).
Let's add a test for this as well
|
|
||
| class TokenAuthPermissionConfigurationModel(ConfigurationModel): | ||
| object_type: UUID4 | ||
| fields: dict | None = Field(default=None) |
There was a problem hiding this comment.
Let's add the help text from the model and a more specific type annotation
| fields: dict | None = Field(default=None) | |
| fields: dict[str, list[str]] | None = DjangoModelRef(Permission, "fields", default=None) |
I checked and according to the help text "Fields allowed for this token in relation to objecttype versions. Supports only first level of the record.data properties" the shape of fields is like this:
{'1': ['record__data__leeftijd', 'record__data__kiemjaar']}
| mode: read_only | ||
| use_fields: true | ||
| fields: | ||
| key1: value1 |
There was a problem hiding this comment.
Since the data should look like this:
{'1': ['record__data__leeftijd', 'record__data__kiemjaar']}
Let's add an example like that here as well. Though I'm not sure if you can define an integer like that as a string key in YAML? @swrichards
| ) | ||
| except IntegrityError as exception: | ||
| raise ConfigurationRunFailed( | ||
| ("Failed configuring permission for token %s" % token.identifier) |
There was a problem hiding this comment.
Let's make this a little more specific
| ("Failed configuring permission for token %s" % token.identifier) | |
| ("Failed configuring permission for token %s and object type %s" % (token.identifier, permission.object_type)) |
author Daniel Mursa <daniel@maykinmedia.nl> 1734341658 +0100 committer Daniel Mursa <daniel@maykinmedia.nl> 1734604578 +0100 parent 64fa395 author Daniel Mursa <daniel@maykinmedia.nl> 1734341658 +0100 committer Daniel Mursa <daniel@maykinmedia.nl> 1734604572 +0100 [#485] Merge closed branch [#485] Add SitesConfigurationStep and TokenAuthConfigurationStep in settings [#485] Black and isort [#485] Fix requirements [#485] Update namespace [#486] Update TokenAuthConfigurationStep [#486] Permissions can be empty list [#486] Update tests [#486] New tests [#486] Update tests [#486] Fix old tests [#486] Update data.yaml [#486] Activate ObjectTypesConfigurationStep [#486] Add new test [#486] Update config_cli.rst [#486] Fix config_cli.rst [#486] Fix config_cli.rst [#486] Uniform data.yaml and config_cli.rst
danielmursa-dev
force-pushed
the
feature/486-token-permissions-configuration
branch
from
5d874de to
02426aa
Compare
Fixes #486
This branch is based on this PR: #494