Don't treat permission name as a machine readable field, use slug instead #115

Merged
Oleg-Komarov merged 2 commits from permissions-use-slug into main 2024-05-06 17:40:07 +02:00
4 changed files with 42 additions and 42 deletions

View File

@ -202,8 +202,13 @@ class TagAdmin(admin.ModelAdmin):
return ()
class VersionPermissionAdmin(admin.ModelAdmin):
list_display = ('name', 'slug')
admin.site.register(models.Extension, ExtensionAdmin)
admin.site.register(models.Version, VersionAdmin)
admin.site.register(models.Maintainer, MaintainerAdmin)
admin.site.register(models.License, LicenseAdmin)
admin.site.register(models.Tag, TagAdmin)
admin.site.register(models.VersionPermission, VersionPermissionAdmin)

View File

@ -0,0 +1,23 @@
# Generated by Django 4.2.11 on 2024-05-06 12:10
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('extensions', '0027_unique_preview_files'),
]
operations = [
migrations.AlterField(
model_name='license',
name='slug',
field=models.SlugField(help_text='Should be taken from https://spdx.org/licenses/', unique=True),
),
migrations.AlterField(
model_name='versionpermission',
name='slug',
field=models.SlugField(help_text='Permissions add-ons are expected to need.', unique=True),
),
]

View File

@ -19,8 +19,6 @@ from constants.base import (
EXTENSION_TYPE_SLUGS,
FILE_STATUS_CHOICES,
)
from constants.licenses import ALL_LICENSES
from constants.version_permissions import ALL_VERSION_PERMISSIONS
import common.help_texts
import extensions.fields
@ -90,22 +88,13 @@ class License(CreatedModifiedMixin, models.Model):
blank=False,
null=False,
help_text='Should be taken from https://spdx.org/licenses/',
unique=True,
)
url = models.URLField(blank=False, null=False)
def __str__(self) -> str:
return f'{self.name}'
@classmethod
def generate(cls):
"""Generate License records from constants."""
licenses = [cls(id=li.id, name=li.name, slug=li.slug, url=li.url) for li in ALL_LICENSES]
cls.objects.bulk_create(licenses)
@classmethod
def get_by_name(cls, name: str):
return cls.objects.filter(name__startswith=name).first()
@classmethod
def get_by_slug(cls, slug: str):
return cls.objects.filter(slug__startswith=slug).first()
@ -386,28 +375,16 @@ class VersionPermission(CreatedModifiedMixin, models.Model):
blank=False,
null=False,
help_text='Permissions add-ons are expected to need.',
unique=True,
)
help = models.CharField(max_length=128, null=False, blank=False, unique=True)
def __str__(self) -> str:
return f'{self.name}'
@classmethod
def generate(cls):
"""Generate Permission records from constants."""
permissions = [
cls(id=li.id, name=li.name, slug=li.slug, help=li.help)
for li in ALL_VERSION_PERMISSIONS
]
cls.objects.bulk_create(permissions)
@classmethod
def get_by_name(cls, name: str):
return cls.objects.filter(name__startswith=name).first()
@classmethod
def get_by_slug(cls, slug: str):
return cls.objects.filter(slug__startswith=slug).first()
Oleg-Komarov marked this conversation as resolved Outdated

I'm replacing this with an exact match. Was there any usecase to do a prefix match?

I'm replacing this with an exact match. Was there any usecase to do a prefix match?

I don't recall, if it wasn't explained on the corresponding commit than just assume an exact match should be fine. I think I copied this over from the license and other code snippets

I don't recall, if it wasn't explained on the corresponding commit than just assume an exact match should be fine. I think I copied this over from the license and other code snippets
return cls.objects.get(slug=slug)
class Tag(CreatedModifiedMixin, models.Model):
@ -537,12 +514,7 @@ class Version(CreatedModifiedMixin, RatingMixin, TrackChangesMixin, models.Model
return
for permission_name in _permissions:
permission = VersionPermission.get_by_name(permission_name)
# Just ignore versions that are incompatible.
if not permission:
Oleg-Komarov marked this conversation as resolved Outdated

@dfelinto here we explicitly allow to submit a manifest with incorrect permissions names. is it intentional? shouldn't we return a validation error instead if somebody mistypes a permission slug?

@dfelinto here we explicitly allow to submit a manifest with incorrect permissions names. is it intentional? shouldn't we return a validation error instead if somebody mistypes a permission slug?

Right, it should be an error indeed. Maybe already is (caught early on by the validator)?

Right, it should be an error indeed. Maybe already is (caught early on by the validator)?
continue
permission = VersionPermission.get_by_slug(permission_name)
self.permissions.add(permission)
def set_initial_licenses(self, _licenses):

View File

@ -54,12 +54,11 @@ class FileMIMETypeValidator:
class ExtensionIDManifestValidator:
"""
Make sure the extension id is valid:
* Extension id consists of Unicode letters, numbers or underscores.
* Neither hyphens nor spaces are supported.
* Each extension id most be unique across all extensions.
* All versions of an extension must have the same extension id.
"""Make sure the extension id is valid:
* Extension id consists of Unicode letters, numbers or underscores.
* Neither hyphens nor spaces are supported.
* Each extension id most be unique across all extensions.
* All versions of an extension must have the same extension id.
"""
def __init__(self, manifest, extension_to_be_updated):
@ -307,10 +306,11 @@ class PermissionsValidator:
is_error = True
else:
for permission in value:
if VersionPermission.get_by_slug(permission):
continue
is_error = True
logger.info(f'Permission unavailable: {permission}')
try:
VersionPermission.get_by_slug(permission)
except VersionPermission.DoesNotExist:
is_error = True
logger.info(f'Permission unavailable: {permission}')
if not is_error:
return