Skip to content

Commit

Permalink
[bug 720226] Wiki permissions overhaul.
Browse files Browse the repository at this point in the history
This moves all the permission checks related to documents and
revisions to a mixin in permissions.py.

All permissions are now checked off the document as:
    document.allows(user, 'action')

Includes a migration to remove no longer needed delete_document_*
permissions by locale.
  • Loading branch information
rlr committed May 22, 2013
1 parent 4a20731 commit 8cd1303
Show file tree
Hide file tree
Showing 11 changed files with 262 additions and 84 deletions.
7 changes: 3 additions & 4 deletions apps/wiki/forms.py
Expand Up @@ -257,10 +257,9 @@ def save(self, creator, document, based_on_id=None, base_rev=None,
if based_on_id:
new_rev.based_on_id = based_on_id

# If this revision is in the default locale and the user doesn't
# have edit_keywords permission, keep the old value.
if (base_rev and document.locale == settings.WIKI_DEFAULT_LANGUAGE and
not creator.has_perm('wiki.edit_keywords')):
# If the document doesn't allow the revision creator to edit the
# keywords, keep the old value.
if base_rev and not document.allows(creator, 'edit_keywords'):
new_rev.keywords = base_rev.keywords

new_rev.save()
Expand Down
32 changes: 2 additions & 30 deletions apps/wiki/models.py
Expand Up @@ -32,6 +32,7 @@
MEDIUM_SIGNIFICANCE, MAJOR_SIGNIFICANCE,
REDIRECT_HTML, REDIRECT_CONTENT, REDIRECT_TITLE,
REDIRECT_SLUG)
from wiki.permissions import DocumentPermissionMixin


log = logging.getLogger('k.wiki')
Expand All @@ -50,7 +51,7 @@ class _NotDocumentView(Exception):


class Document(NotificationsMixin, ModelBase, BigVocabTaggableMixin,
SearchMixin):
SearchMixin, DocumentPermissionMixin):
"""A localized knowledgebase document, not revision-specific."""
title = models.CharField(max_length=255, db_index=True)
slug = models.CharField(max_length=255, db_index=True)
Expand Down Expand Up @@ -397,35 +398,6 @@ def redirect_document(self):
def __unicode__(self):
return '[%s] %s' % (self.locale, self.title)

def allows_revision_by(self, user):
"""Return whether `user` is allowed to create new revisions of me.
The motivation behind this method is that templates and other types of
docs may have different permissions.
"""
# TODO: Add tests for templateness or whatever is required.
# Leaving this method signature untouched for now in case we do need
# to use it in the future. ~james
return True

def allows_editing_by(self, user):
"""Return whether `user` is allowed to edit document-level metadata.
If the Document doesn't have a current_revision (nothing approved) then
all the Document fields are still editable. Once there is an approved
Revision, the Document fields can only be edited by privileged users.
"""
return (not self.current_revision or
user.has_perm('wiki.change_document'))

def allows_deleting_by(self, user):
"""Return whether `user` is allowed to delete this document."""
return (user.has_perm('wiki.delete_document') or
user.has_perm('wiki.delete_document_{locale}'.format(
locale=self.locale)))

def allows_vote(self, request):
"""Return whether `user` can vote on this document."""
return (not self.is_archived and self.current_revision and
Expand Down
131 changes: 131 additions & 0 deletions apps/wiki/permissions.py
@@ -0,0 +1,131 @@
import logging

from django.conf import settings


log = logging.getLogger('k.wiki')


# Why is this a mixin if it can only be used for the Document model?
# Good question! My only good reason is to keep the permission related
# code organized and contained in one place.
class DocumentPermissionMixin(object):
"""Adds of permission checking methods to the Document model."""

def allows(self, user, action):
"""Check if the user has the permission on the document."""

# If this is kicking up a KeyError it's probably because you typoed!
return getattr(self, '_allows_%s' % action)(user)

def _allows_create_revision(self, user):
"""Can the user create a revision for the document?"""
# For now (ever?), creating revisions isn't restricted at all.
return True

def _allows_edit(self, user):
"""Can the user edit the document?"""
# Document editing isn't restricted until it has an approved
# revision.
if not self.current_revision:
return True

# Locale leaders and reviewers can edit in their locale.
locale = self.locale
if _is_leader(locale, user) or _is_reviewer(locale, user):
return True

# And finally, fallback to the actual django permission.
return user.has_perm('wiki.change_document')

def _allows_delete(self, user):
"""Can the user delete the document?"""
# Locale leaders can delete documents in their locale.
locale = self.locale
if _is_leader(locale, user):
return True

# Fallback to the django permission.
return user.has_perm('wiki.delete_document')

def _allows_archive(self, user):
"""Can the user archive the document?"""
# Just use the django permission.
return user.has_perm('wiki.archive_document')

def _allows_edit_keywords(self, user):
"""Can the user edit the document's keywords?"""
# If the document is in the default locale, just use the
# django permission.
# Editing keywords isn't restricted in other locales.
return (self.locale != settings.WIKI_DEFAULT_LANGUAGE or
user.has_perm('wiki.edit_keywords'))

def _allows_edit_needs_change(self, user):
"""Can the user edit the needs change fields for the document?"""
# If the document is in the default locale, just use the
# django permission.
# Needs change isn't used for other locales (yet?).
return (self.locale == settings.WIKI_DEFAULT_LANGUAGE and
user.has_perm('wiki.edit_needs_change'))

def _allows_mark_ready_for_l10n(self, user):
""""Can the user mark the document as ready for localization?"""
# If the document is localizable and the user has the django
# permission, then the user can mark as ready for l10n.
return (self.is_localizable and
user.has_perm('wiki.mark_ready_for_l10n'))

def _allows_review_revision(self, user):
"""Can the user review a revision for the document?"""
# Locale leaders and reviewers can review revisions in their
# locale.
locale = self.locale
if _is_leader(locale, user) or _is_reviewer(locale, user):
return True

# Fallback to the django permission.
return user.has_perm('wiki.review_revision')

def _allows_delete_revision(self, user):
"""Can the user delete a document's revisions?"""
# Locale leaders and reviewers can delete revisions in their
# locale.
locale = self.locale
if _is_leader(locale, user) or _is_reviewer(locale, user):
return True

# Fallback to the django permission.
return user.has_perm('wiki.delete_revision')


def _is_leader(locale, user):
"""Checks if the user is a leader for the given locale.
Returns False if the locale doesn't exist. This will should only happen
if we forgot to insert a new locale when enabling it or during testing.
"""
from wiki.models import Locale
try:
locale_team = Locale.objects.get(locale=locale)
except Locale.DoesNotExist:
log.warning('Locale not created for %s' % locale)
return False

return user in locale_team.leaders.all()


def _is_reviewer(locale, user):
"""Checks if the user is a reviewer for the given locale.
Returns False if the locale doesn't exist. This will should only happen
if we forgot to insert a new locale when enabling it or during testing.
"""
from wiki.models import Locale
try:
locale_team = Locale.objects.get(locale=locale)
except Locale.DoesNotExist:
log.warning('Locale not created for %s' % locale)
return False

return user in locale_team.reviewers.all()
2 changes: 1 addition & 1 deletion apps/wiki/templates/wiki/edit.html
Expand Up @@ -61,7 +61,7 @@ <h1>{{ _('<em>Editing</em> {title}')|fe(title=document.title) }}</h1>
{% for field in revision_form.visible_fields() %}
<li>
{% if field.name not in ['comment', 'content'] %}
{% if not field.name == 'keywords' or user.has_perm('wiki.edit_keywords') %}
{% if not field.name == 'keywords' or document.allows(user, 'edit_keywords') %}
{{ field|label_with_help }}{{ field|safe }}
{% endif %}
{% elif field.name == 'content' %}
Expand Down
8 changes: 4 additions & 4 deletions apps/wiki/templates/wiki/history.html
Expand Up @@ -43,7 +43,7 @@ <h1 class="title">{{ _('History of {title}')|fe(title=document.title) }}</h1>
{% include 'wiki/includes/ready_for_l10n_modal.html' %}

<section id="contributors" class="editable">
{% if user.has_perm('wiki.change_document') %}
{% if document.allows(user, 'edit') %}
<a class="edit" href="#contributors">{{ _('Edit contributors') }}</a>
{% endif %}
<h1>{{ _('Document contributors') }}</h1>
Expand All @@ -60,15 +60,15 @@ <h1>{{ _('Document contributors') }}</h1>
{{ display_name(u) }}
</a>
</div>
{% if user.has_perm('wiki.change_document') %}
{% if document.allows(user, 'edit') %}
<div class="remove edit-mode">
<a href="{{ url('wiki.remove_contributor', document.slug, u.id) }}" title="{{ _('Remove user from contributors') }}">&#x2716;</a>
</div>
{% endif %}
</li>
{% endfor %}
</ul>
{% if user.has_perm('wiki.change_document') %}
{% if document.allows(user, 'edit') %}
<form id="add-contributor-form" class="edit-mode" action="{{ url('wiki.add_contributor', document.slug) }}" method="POST">
{{ csrf() }}
{{ errorlist(contributor_form) }}
Expand All @@ -78,7 +78,7 @@ <h1>{{ _('Document contributors') }}</h1>
{% endif %}
</section>

{% if document.allows_deleting_by(user) %}
{% if document.allows(user, 'delete') %}
<div id="delete-doc">
<a href="{{ url('wiki.document_delete', document.slug) }}">Delete this document</a>
</div>
Expand Down
4 changes: 2 additions & 2 deletions apps/wiki/templates/wiki/includes/review_form.html
Expand Up @@ -46,15 +46,15 @@
<textarea id="id-approve-comment" required name="comment">{% if form.initial %}{{ form.initial.comment }}{% else %}{{ form.comment.data }}{% endif %}</textarea>
</div>
{% endif %}
{% if document.is_localizable and user.has_perm('wiki.mark_ready_for_l10n') %}
{% if document.allows(user, 'mark_ready_for_l10n') %}
<div class="ready-for-l10n">
<label>
{{ form.is_ready_for_localization }}
{{ form.is_ready_for_localization.label }}
</label>
</div>
{% endif %}
{% if document.locale == settings.WIKI_DEFAULT_LANGUAGE and user.has_perm('wiki.edit_needs_change') %}
{% if document.allows(user, 'edit_needs_change') %}
<div class="needs-change">
{{ form.needs_change }}
{{ form.needs_change|label_with_help }}
Expand Down
8 changes: 4 additions & 4 deletions apps/wiki/templates/wiki/includes/revision_list.html
Expand Up @@ -29,7 +29,7 @@
{% endif %}
<div class="comment">{{ _('Comment') }}</div>
<div class="edit"></div>
{% if document.current_revision != rev and user.has_perm('wiki.delete_revision') %}
{% if document.current_revision != rev and document.allows(user, 'delete_revision') %}
<div class="delete"></div>
{% endif %}
</li>
Expand All @@ -48,7 +48,7 @@
</div>
<div class="status">
{% if not rev.reviewed %}
{% if not reached_current and user.has_perm('wiki.review_revision') %}
{% if not reached_current and document.allows(user, 'review_revision') %}
<a href="{{ url('wiki.review_revision', document.slug, rev.id, locale=document.locale) }}">{{ _('Review') }}</a>
{% else %}
<span class="unreviewed">{{ _('Unreviewed', 'revision') }}</span>
Expand Down Expand Up @@ -83,7 +83,7 @@
{% set reached_ready_for_l10n = True %}
<a class="yes" title="{{ _('This revision is ready for localization.') }}"></a>
{% elif rev.can_be_readied_for_localization() %}
{% if not reached_ready_for_l10n and user.has_perm('wiki.mark_ready_for_l10n') %}
{% if not reached_ready_for_l10n and document.allows(user, 'mark_ready_for_l10n') %}
<a class="markasready" id="rev-{{ rev.id }}-l10n-no" data-revdate="{{ rev.created }}" data-url="{{ url('wiki.mark_ready_for_l10n_revision', document.slug, rev.id, locale=document.locale) }}" title="{{ _('This revision is not ready for localization.') }}"></a>
{% else %}
<a class="no" title="{{ _('This revision is not ready for localization.') }}"></a>
Expand All @@ -99,7 +99,7 @@
<a href="{{ url('wiki.new_revision_based_on', document.slug, rev.id, locale=document.locale) }}" title="{{ _('Edit article based on this revision') }}"></a>
</div>
{% endif %}
{% if document.current_revision != rev and user.has_perm('wiki.delete_revision') %}
{% if document.current_revision != rev and document.allows(user, 'delete_revision') %}
<div class="delete">
<a href="{{ url('wiki.delete_revision', document.slug, rev.id, locale=document.locale) }}" title="{{ _('Delete this revision') }}"></a>
</div>
Expand Down
4 changes: 2 additions & 2 deletions apps/wiki/templates/wiki/includes/sidebar_modules.html
Expand Up @@ -20,7 +20,7 @@
<a href="{{ url('wiki.discuss.threads', document.slug) }}">{{ _('Discussion') }}</a>
</li>
{% endif %}
{% if document and not document.is_archived and (document.allows_revision_by(user) or document.allows_editing_by(user)) %}
{% if document and not document.is_archived and (document.allows(user, 'create_revision') or document.allows(user, 'edit')) %}
<li class="edit{% if active == 'edit' %} active{% endif %}">
{% if active == 'edit' %}
<span>{{ _('Edit Article') }}</span>
Expand Down Expand Up @@ -84,7 +84,7 @@
<a href="{{ url('wiki.discuss.threads', document.slug) }}">{{ _('Discussion') }}</a>
</li>
{% endif %}
{% if document and not document.is_archived and (document.allows_revision_by(user) or document.allows_editing_by(user)) %}
{% if document and not document.is_archived and (document.allows(user, 'create_revision') or document.allows(user, 'edit')) %}
<li{% if active == 'edit' %} class="selected"{% endif %}>
<a href="{{ url('wiki.edit_document', document.slug) }}">{{ _('Edit Article') }}</a>
</li>
Expand Down

0 comments on commit 8cd1303

Please sign in to comment.