Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion patchwork/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
from django.contrib.auth.models import User
from django.db.models import Prefetch

from guardian.admin import GuardedModelAdmin

from patchwork.models import Bundle
from patchwork.models import Check
from patchwork.models import Cover
Expand Down Expand Up @@ -45,7 +47,7 @@ class DelegationRuleInline(admin.TabularInline):


@admin.register(Project)
class ProjectAdmin(admin.ModelAdmin):
class ProjectAdmin(GuardedModelAdmin):
list_display = ('name', 'linkname', 'listid', 'listemail')
inlines = [
DelegationRuleInline,
Expand Down
20 changes: 19 additions & 1 deletion patchwork/api/check.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,27 @@ class CheckListCreate(CheckMixin, ListCreateAPIView):
lookup_url_kwarg = 'patch_id'
ordering = 'id'

def is_editable(self, user, patch):
if not user.is_authenticated:
return False

# Only users with add_check permission can do it.
# Notice that this is a global permission: it allows
# adding checks to any project inside Patchwork.
if user.has_perm('patchwork.add_check'):
patch._edited_by = user

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(patch is not defined in this first commit)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll fix on a next rebase (I'm currently in vacations - will likely do it next weekend).

return True

if user.has_perm('patchwork.add_check', patch.project):
patch._edited_by = user
return True

# Being maintainer doesn't grant rights to create checks.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this a behavioural change? If yes, could this be avoided not to make upgrade difficult? Especially on large instances with many users :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, after the change, enabling/disabling checks become an independent permission.
I suspect that, even on large instances, there are typically just one user that do CI check additions on a given project - usually a special account for it; the other maintainers shall not have any business adding CI checks.

You got a point though: it could be better to set it by default during migration. Not sure if is there a way to teach migrations to set it by default for maintainers to preserve existing behaviour.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Or do it in 2 steps: one without a behavioural change that can be backported, then changing the permissions. But yes, ideally setting it by default during the migration would make upgrade smoother.

return False

def create(self, request, patch_id, *args, **kwargs):
p = get_object_or_404(Patch, id=patch_id)
if not p.is_editable(request.user):
if not self.is_editable(request.user, p):
raise PermissionDenied()
request.patch = p
return super(CheckListCreate, self).create(request, *args, **kwargs)
Expand Down
24 changes: 24 additions & 0 deletions patchwork/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ def __str__(self):
return self.email

class Meta:
default_permissions = ()
verbose_name_plural = 'People'


Expand Down Expand Up @@ -120,8 +121,13 @@ def __str__(self):
return self.name

class Meta:
default_permissions = ()
unique_together = (('listid', 'subject_match'),)
ordering = ['linkname']
permissions = [
# Per-project permission to add checks
("add_check", "Can add checks"),
]


class DelegationRule(models.Model):
Expand All @@ -145,6 +151,7 @@ def __str__(self):
return self.path

class Meta:
default_permissions = ()
ordering = ['-priority', 'path']
unique_together = ('path', 'project')

Expand Down Expand Up @@ -253,6 +260,7 @@ def __str__(self):
return self.name

class Meta:
default_permissions = ()
ordering = ['ordering']


Expand Down Expand Up @@ -285,6 +293,7 @@ def __str__(self):
return self.name

class Meta:
default_permissions = ()
ordering = ['abbrev']


Expand All @@ -294,6 +303,7 @@ class PatchTag(models.Model):
count = models.IntegerField(default=1)

class Meta:
default_permissions = ()
unique_together = [('patch', 'tag')]


Expand Down Expand Up @@ -415,6 +425,7 @@ def save(self, *args, **kwargs):
super(EmailMixin, self).save(*args, **kwargs)

class Meta:
default_permissions = ()
abstract = True


Expand Down Expand Up @@ -457,6 +468,7 @@ def __str__(self):
return self.name

class Meta:
default_permissions = ()
abstract = True


Expand All @@ -480,6 +492,7 @@ def get_mbox_url(self):
)

class Meta:
default_permissions = ()
ordering = ['date']
unique_together = [('msgid', 'project')]
indexes = [
Expand Down Expand Up @@ -717,6 +730,7 @@ def __str__(self):
return self.name

class Meta:
default_permissions = ()
verbose_name_plural = 'Patches'
ordering = ['date']
base_manager_name = 'objects'
Expand Down Expand Up @@ -780,6 +794,7 @@ def is_editable(self, user):
return False

class Meta:
default_permissions = ()
ordering = ['date']
unique_together = [('msgid', 'cover')]
indexes = [
Expand Down Expand Up @@ -825,6 +840,7 @@ def is_editable(self, user):
return self.patch.is_editable(user)

class Meta:
default_permissions = ()
ordering = ['date']
unique_together = [('msgid', 'patch')]
indexes = [
Expand Down Expand Up @@ -985,6 +1001,7 @@ def __str__(self):
return self.name if self.name else 'Untitled series #%d' % self.id

class Meta:
default_permissions = ()
verbose_name_plural = 'Series'


Expand All @@ -1010,6 +1027,7 @@ def __str__(self):
return self.msgid

class Meta:
default_permissions = ()
unique_together = [('project', 'msgid')]


Expand Down Expand Up @@ -1072,6 +1090,7 @@ def get_mbox_url(self):
)

class Meta:
default_permissions = ()
unique_together = [('owner', 'name')]


Expand All @@ -1081,6 +1100,7 @@ class BundlePatch(models.Model):
order = models.IntegerField()

class Meta:
default_permissions = ()
unique_together = [('bundle', 'patch')]
ordering = ['order']

Expand Down Expand Up @@ -1150,6 +1170,9 @@ def __repr__(self):
def __str__(self):
return '%s (%s)' % (self.context, self.get_state_display())

class Meta:
default_permissions = ('add',)


class Event(models.Model):
"""An event raised against a patch.
Expand Down Expand Up @@ -1332,6 +1355,7 @@ def __repr__(self):
return "<Event id='%d' category='%s'" % (self.id, self.category)

class Meta:
default_permissions = ()
ordering = ['-date']


Expand Down
6 changes: 6 additions & 0 deletions patchwork/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
'django.contrib.sites',
'django.contrib.admin',
'django.contrib.staticfiles',
'guardian',
'patchwork',
]

Expand Down Expand Up @@ -71,6 +72,11 @@
},
]

AUTHENTICATION_BACKENDS = [
'django.contrib.auth.backends.ModelBackend',
'guardian.backends.ObjectPermissionBackend',
]

FORM_RENDERER = 'patchwork.forms.PatchworkTableRenderer'

# TODO(stephenfin): Consider changing to BigAutoField when we drop support for
Expand Down
1 change: 1 addition & 0 deletions requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,5 @@ djangorestframework~=3.17.1
django-filter~=25.2.0
django-debug-toolbar~=6.3.0
django-dbbackup~=5.3.0
django-guardian~=3.3.1

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could you move it to the test one, please? I guess that's why the CI is no longer passing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok, will do at the next rebase.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry, I think you should keep it there, but add it in the tox.ini file (and in the README one as well I suppose)

-r requirements-test.txt
1 change: 1 addition & 0 deletions requirements-prod.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
Django~=6.0.0
djangorestframework~=3.17.1
django-filter~=25.2.0
django-guardian~=3.3.1
psycopg~=3.3.4
sqlparse~=0.5.5
Loading