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.

Sign up for GitHub

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

Jump to bottom

Prevent crash when checking permissions of external reviewer without shares #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Prevent crash when checking permissions of external reviewer without shares #29

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28be11c
Check for existence of share before checking share.can_comment to pre…
jacobtoppm Apr 17, 2020
3e1206a
Make logic around shares and comments clearer
jacobtoppm Apr 22, 2020
ee5ef84
Ensure permissions check for superusers returns identical results fro…
jacobtoppm Apr 23, 2020
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
23 changes: 10 additions & 13 deletions wagtail_review/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,13 +187,10 @@ def can_comment(self):
if not self.can_view():
return False

if self.reviewer.external_id and not self.share.can_comment:
# External users can leave comments without a share if they are a reviewer
if self.can_review():
return True

# Reviewer can view but not comment
return False
if self.reviewer.external_id and not (self.share and self.share.can_comment):
# If external users have a Share with comments enabled, they can comment
# otherwise, they can comment if they are a reviewer for the current task
return self.can_review()

return True

Expand Down Expand Up @@ -426,7 +423,7 @@ def get_actions(self, page, user, reviewer=None, **kwargs):
"""Returns the possible actions the user can take. Note that this should
be able to be called with a user or a reviewer instance alone (ie where user=None)
to account for external reviewers"""
if self.is_reviewer_for_task(user, reviewer=reviewer) or (user and user.is_superuser):
if self.is_reviewer_for_task(user, reviewer=reviewer):
return [
('review', _("Review")),
('approve', _("Approve")),
Expand Down Expand Up @@ -465,13 +462,13 @@ class ReviewTask(ReviewMixin, Task):

def is_reviewer_for_task(self, user, reviewer=None):
"""Returns True if the Reviewer instance provided, or linked to the user provided,
is among the reviewers assigned to the task"""
is among the reviewers assigned to the task, or the user is a superuser"""
if not reviewer:
try:
reviewer = Reviewer.objects.get(internal=user)
except Reviewer.DoesNotExist:
return False
return self.reviewers.filter(pk=reviewer.pk).exists()
return (user and user.is_superuser)
return self.reviewers.filter(pk=reviewer.pk).exists() or (reviewer.internal and reviewer.internal.is_superuser)

class Meta:
verbose_name = _('Review task')
Expand All @@ -490,12 +487,12 @@ class GroupReviewTask(ReviewMixin, Task):

def is_reviewer_for_task(self, user, reviewer=None):
"""Returns True if the user provided, or the user linked to the reviewer
provided, is in a group assigned to the task"""
provided, is in a group assigned to the task, or is a superuser"""
if reviewer and (not user or not user.is_authenticated):
if not reviewer.internal:
return False
user = get_user_model().objects.get(pk=reviewer.internal.pk)
return self.groups.all().filter(id__in=user.groups.all()).exists()
return user.is_superuser or self.groups.all().filter(id__in=user.groups.all()).exists()

class Meta:
verbose_name = _('Group review task')
Expand Down